Page MenuHomeFreeBSD

tftp: Fix buffer overflow and fd leak in multi-file PUT.
ClosedPublic

Authored by des on Thu, Nov 17, 3:19 PM.

Details

Summary

Sponsored by: Klara, Inc.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

des requested review of this revision.Thu, Nov 17, 3:19 PM
des created this revision.

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?

In D37422#850782, @imp wrote:

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.

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)

Now that looks much better.

This revision is now accepted and ready to land.Thu, Nov 17, 4:28 PM
In D37422#850788, @des wrote:
In D37422#850782, @imp wrote:

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.

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

jrtc27 added a subscriber: jrtc27.

Logic looks right to me, just one nit

usr.bin/tftp/main.c
527

I guess this presumably-leaky line is why you started looking at tsize?

534

Any reason for this blank line?

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.