Sponsored by: Klara, Inc.
Details
- Reviewers
imp jrtc27 - Group Reviewers
Klara - Commits
- rG9f9544fd92e6: tftp: Fix buffer overflow and fd leak in multi-file PUT.
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Wow! This is ugly code before. But cp is always pointing at the end of the targ string which we stomped above to remove some things. But the old cp will point to the end of argv[argc - 1] since targ either points to the start of it, or to the first character past the :.
So in the case where it doesn't have a ':' in it, we're good. I'm 100% sure there's no way this code is different than the old, other than using freshly allocated memory.
When there is a ':' though, I'm less sure:
#define a argv[argc - 1]
So a points to 'host:/some/thing' then targ will point to '/some/thing/' when i enters the loop. The new code will allocate a new targ that points to a string with the contents of a a slash and the next arg, which will be something like 'host/arg/here' not '/some/thing/arg/here' like the old code, no?
usr.bin/tftp/main.c | ||
---|---|---|
518 | Do you need to free targ here? | |
523 | do you need to free targ here? |
Except the original has a trivially reproducible buffer overflow:
$ perl -e '$f="A"x512;print"put $f $f $f $f\n"' | tftp localhost tftp: AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA: Bus error (core dumped)
Yea, I was stating poorly that this was equivalent to the intent of the original code, except it also fixed this buffer overflow
usr.bin/tftp/main.c | ||
---|---|---|
534 | Seemed right according to the prevailing style. If there is a blank line after the asprintf(), there should be one before the free(). |
usr.bin/tftp/main.c | ||
---|---|---|
534 | Why's that relevant? There isn't one after every single function call in the file. The point of the blank line after the asprintf is to separate the configuration part of the block from the "go actually run it" part, and there isn't a blank line *before* it I would guess because it's tied to the preceding fstat. It's all a bit subjective, but I would argue this blank line here makes no sense, whereas the others are somewhat justifiable. If anything, put a blank line before the close here instead to separate cleanup from the rest of the code, but not in the middle of cleanup. |