Hacker News new | ask | show | jobs
by TheDong 451 days ago
No one else seems to have run 'grep system(', so I will:

https://github.com/Atoptool/atop/blob/037a6d3e4ace6c7be6c5dc...

> system ("gunzip -c %s > %s", tmpname1, tmpname2")

tmpname2 is hardcoded as "/tmp/atopwrkXXXXXX", so that's fine. tmpname1 is '$irawname.gz'. '$irawname' is set by the '-r' flag.

So, presumably if you can get the rest of the code to play nice and get you there, you can escalate from having shell access to run atop, to having shell access. Oh, I guess that's nothing.

Anyway, still a really bad use of system + user-controlled input, don't do that.
2 comments
lnxdork 451 days ago
Agree as a basic example. tmpname1 = "/tmp/file.txt; rm -rf /"; becomes gunzip -c /tmp/file.txt; rm -rf / > /tmp/atopwrkXXXXXX

Also tmpname2 could be symlinked to /etc/passwd before it is unlinked..

link
TheDong 450 days ago
> Also tmpname2 could be symlinked to /etc/passwd before it is unlinked..

Yeah, sure, but only if you run atop as root, otherwise it'll just get a "permission denied", and if you can run atop as root with whatever flags you like, you might as well just run 'rm' instead.

It's not a suid binary, so while it's bad code and a smell, I don't think the TOCTOU is a security issue in how it's commonly run (i.e. as an interactive CLI running as your user).

link
pledess 450 days ago
The TOCTOU is relevant (without suid) if someone can quickly make the right prediction of the tmpname2 value that's generated by the PRNG used by mkstemp, and create a symlink with that value before gunzip is executed. After calling mkstemp, the code should use the returned file descriptor, and thereby eliminate all TOCTOU risk. However, on (perhaps?) most devices that would realistically use atop, the PRNG works well enough that that prediction would fail.
link
isotopp 451 days ago
Eh? Calling system() for a binary without a path? And why system() using execl() in the first place, when you could do something using execve() without a sh inbetween instead?

Even w/o an exploit this can be prettier and more secure.

link
TheDong 451 days ago
We're not disagreeing. Even if there's no 'sploit there, people have spaces in their directory or file names, and it's kinda nice for your tool to work with those, so obviously you should be using an execve variant to pass arguments properly.

I assume the reason for the incorrect system call is that doing a shell redirect ('>') does actually look prettier though.

Doing the actual right code is definitely less pretty looking IMO: https://github.com/luvit/zlib/blob/8de57bce969eb9dafc1f1f5c2...

link