Critical Security Issues

[shawnanastasio]

Hi I saw your post on /r/linux and took a look at the code. I've noticed a number of glaring security issues:

• Raw user input is strcat()'d and run with system(). This is a command injection vulnerability. Since the binary is setuid root, this means that anybody on the system can escalate privileges by entering an argument like '; bash.
• There is no bounds checking when modifying the cmd buffer. You could overflow this with a sufficiently long argument list and get the ability to arbitrarily write to the stack.

I would highly suggest removing the setuid requirement and instead require users to run the program with sudo or another proven system authentication manager. There is a large burden placed on setuid applications on UNIX systems because a single bug could mean the entire system's security model is compromised.

[wolftune]

For reference, I posted the Reddit thread, but I am not the author of the software. For reference, link to context (which mentions other code suggestions: https://www.reddit.com/r/linux/comments/6iubb0/nosuspend_is_a_systemdcompatible_tool_to_block/dj9ap5e/

Note that a forum thread about this from a while back mentioned (by the author here, not me): "there is a request/patch on systemd's github page to allow the set root UID for systemd-inhibit with a configuration option in login.conf, but it is a Year old now, so…"

Also does this relate to the issue I just opened? #13

[shawnanastasio]

@wolftune I don't think any of the issues I discovered have to do with #13

[brummer10]

Hi

@shawnanastasio

nosuspend run systemd-inhibit --why='User application run' with root rights, therefor the setuid bit is needed. But the users input goes into a shell with only user rights su -c getlogin_r,
You cant get root rights in this shell.
Sudo (or similar login shells) wont do it here, because only systemd-inhibit needs to run with root rights, the following arg needs to run on user level.

You are right about the bound check, and I'll add it, other then that I've set the size with 5000 far, far more then enough.

regards
hermann

[shawnanastasio]

The user's input is concatenated in between su <username> -c ' and '. If the user enters something non-malicious like whoami, the string will become su <username> -c 'whoami' and all is fine. However, if the user enters a malicious string like whoami' && touch /i_have_root && echo ', the final command string will become su <username> -c 'whoami' && touch /i_have_root && echo ''. This is certainly a command injection vulnerability and has no easy solution, hence my recommendation for dropping the setuid bit.

[shawnanastasio]

Also, 715ac12 doesn't fix the overflow. It is possible by supplying enough arguments of sufficient length to overflow bz and therefore bypass the check.

[shawnanastasio]

Also, argument 1 must be checked to be less than 100 - len("which |xargs ldd | grep -E 'libgtk*|libqt*|fltk'") otherwise there is an overflow for the cmd buffer at line 35.

Again, I must stress that writing a secure setuid application is hard and you shouldn't be doing it at all unless you are experienced in writing C and absolutely need to. If you removed the setuid requirement and ran with sudo, you could keep all your existing code (like su to deescalate privileges) and it wouldn't necessarily matter if it was exploitable, because the user would have to have sudo permissions to run it in the first place.

[brummer10]

Thanks for pointing out the buffer size issues, I've fixed them now.
Just, I ain't get your command injection issue, even with the examples you gave above, you are still in the user shell, without root rights.

[brummer10]

however, I've add a check for prohibited signs in user input now.

[shawnanastasio]

Here's an article on command injection.

You're right that if the user enters normal input, it will only be run as the current user because you're inserting their input between su -c ' and '. However, if the user enters their own quote, they can terminate the su -c command early and inject commands. You should be able to mitigate it by using execv() or something but I still would highly recommend removing the setuid requirement.

[brummer10]

Nice article, thanks for the link.
But try it out with nosuspend, run
nosuspend ls' && whoami && echo '

You'll find that even the second command run only as user. The command run still in the user shell without root rights. That's because the shell to use is explicit set to $USER. Even if you add a new command on the end you'll stay in that shell without higher priority.

However, as wrote above I've add now a user_input_check and prohibit the signs & and ;
which should avoid adding commands.. Surely I could add a test if strings are Unterminated given, and prohibit that as well.