5

We have a legacy C code used to allow less privileged user to run custom scripts with escalated privilege. This has SUID bit set. This code restricts the PATH env to a specific folder and then uses system() api to execute the script with restricted shell :

/bin/bash -r -c "script <arg>"

As the path is restricted, it can execute only scripts from that specific folder.

Now knowing all the pitfalls for command injection with system() api, what measures can be taken to avoid command injection? This is used in many places in various scripts etc, so don't want to do a completely new implementation to avoid any regression.

closed as unclear what you're asking by Rui F Ribeiro, muru, schily, Romeo Ninov, andcoz Aug 27 '18 at 15:21

Please clarify your specific problem or add additional details to highlight exactly what you need. As it's currently written, it’s hard to tell exactly what you're asking. See the How to Ask page for help clarifying this question. If this question can be reworded to fit the rules in the help center, please edit the question.

  • Whit the lack of data you present this question, the only sound advice we can provide is avoiding abusing suid. Otherwise, we cannot guess what your script does. – Rui F Ribeiro Aug 27 '18 at 7:47
  • 1
    An application that does not filter input has to be modified to filter input, no way to get around it with magical Unix solutions. – Rui F Ribeiro Aug 27 '18 at 8:50
  • 1
    As others have warned, it's easy to make mistakes in your own suid code. What about LD_PRELOAD and LD_LIBRARY_PATH, for example? – Edheldil Aug 27 '18 at 9:06
  • Are you sure you need system (which runs a command in a shell) instead of exec (which runs a command)? Often it can be more useful and secure not to use system, even when losing some flexibility. – allo Aug 27 '18 at 11:13
6

Because its hard to get right, I'd suggest removing the SUID on your code. Change your C code to use sudo. By using sudo the harder aspects getting secure system programming are done.

Then you can carefully construct a sudo configuration, using visudo, that does the bare minimum required to perform the task and constrain this to the required users/groups. After configuring sudo, get someone other than you to test it and try to break the intended constrains.

  • 2
    I do agree with not abusing SUID. However, sudo does neither corrects automagically layer 7 problems, nor is a magical bullet to sloppy coding pratices. I actually find your answer just giving a dangerous sense of security. – Rui F Ribeiro Aug 27 '18 at 7:45
  • happy to make changes. General premise is that sudo is better audited than code you write yourself. I think its easier to write sudoers config files correctly than code that calls system and is suid. I agree mistakes still can be made. I'll reword so it doesn't sound so flippant. – danblack Aug 27 '18 at 8:48
  • 1
    I am not saying it is entirely a bad idea, do not get me wrong. It just it is not enough to make it automatically secure. An application that does not filter input has to be modified to filter input, no way to get around it with magical Unix solutions. The OP is expecting a miracle that won't happen. – Rui F Ribeiro Aug 27 '18 at 8:49
  • @danblack I think this would work except that I may not know all the user names upfront. Let me see if I can find a way to map all these users to few groups and sudo policy accordingly. – Rajesh Aug 27 '18 at 9:21
  • Scripts in the restricted folders are properly validating and sanitizing the input. The issue that I was trying to address is, avoiding injection to what is passed to system() call. With sudo conf changes, I can change the code from /bin/bash -r -c <name-of-restricted-script> <arg> to - /bin/sudo -u admin <path-to-restricted-script> <arg>. Even if some one inject a command/script, it would either be executed with the original user permission or won't execute at all depending on permission of that script/command. – Rajesh Aug 27 '18 at 9:29
2

Code injection requires the ability of the user to pass arbitrary strings as parameters to the system() call. This is pretty similar to SQL injections and should be avoided in a similar way: don't pass any user-defined string directly to the call:

  • numerical parameters should be converted to integers, and then converted back to strings at the time of the call

  • parameters which belong to a fixed dictionary should be converted to "enum" values or similar, then back to strings at the time of the call

  • free text input should be restricted to inoffensive character set where possible (e.g. [a-zA-Z0-9]*). Where problematic characters (including space) are required, proper escaping should be applied (that is, a b should be replaced by a\ b, etc.)

  • 2
    backslash should be avoided for quoting as its encoding is part of some characters in some locale. Only single quotes are relatively safe. Note system() runs sh and the OP seems to want to run bash on top of that, so there may need to be two levels of quoting. Some sh implementations drop privileges when run setuid, so it may not work anyway. In any case, the environment would have to be sanitized as well. – Stéphane Chazelas Aug 27 '18 at 10:46
  • DOn't forget to also run these checks on environment variables, and check all of them, not only PATH, also add a whitelist – Ferrybig Aug 27 '18 at 10:58
  • 1
    @Ferrybig, the only reasonable approach for the environment is to start with an empty one, and white list the ones that have harmless names and values and that may be needed like sudo does. Even then, there's harm that can be done with all the LC* ones or TZ for instance. – Stéphane Chazelas Aug 27 '18 at 11:03

Not the answer you're looking for? Browse other questions tagged or ask your own question.