6

On a Linux platform, I have C++ code that goes like this:

// ...
std::string myDir;
myDir = argv[1]; // myDir is initialized using user input from the command line.
std::string command;
command = "mkdir " + myDir;
if (system(command.c_str()) != 0) {
   return 1;
}
// continue....
  • Is passing user input to a system() call safe at all?
  • Should the user input be escaped / sanitized?
  • How?
  • How could the above code be exploited for malicious purposes?

Thanks.

augustin
  • 14,373
  • 13
  • 66
  • 79

4 Answers4

12

Just don't use system. Prefer execl.

execl ("/bin/mkdir", "mkdir", myDir, (char *)0);

That way, myDir is always passed as a single argument to mkdir, and the shell isn't involved. Note that you need to fork if you use this method.

But if this is not just an example, you should use the mkdir C function:

mkdir(myDir, someMode);
Matthew Flaschen
  • 278,309
  • 50
  • 514
  • 539
  • 1
    Note that `execl` does NOT return if no error occurs (unlike `system`). You will have to `fork` first, wait in the parent process for the child to complete and eventually check the return value of `execl` from the parent process. – Andre Holzner Aug 29 '10 at 11:33
  • Thanks. if I use mkdir(myDir, someMode), myDir does not need to be sanitized nor escaped? Is it safe whatever the user input? – augustin Aug 29 '10 at 11:34
  • Yes, the `mkdir` C function simply takes a directory name. No matter what, it will attempt to create a single directory. – Matthew Flaschen Aug 29 '10 at 11:36
  • 1
    augustin: probably it's not safe. If your program is running as root users could e.g. make your program create top level directories etc. You should probably check if there are any slashes in the path name etc. Or even be as restrictive as: if the directory name does not consist only of characters, letters, underscore and a few other characters you consider safe, refuse to run mkdir. – Andre Holzner Aug 29 '10 at 11:37
  • 1
    @Andre, or minimize what the program does as root. Even if part of the program needs to be root, the directory creation code may not. – Matthew Flaschen Aug 29 '10 at 11:41
  • the exec family of commands replaces the current process. system() doesnt. forking is an option in some cases but forking is really badly defined on some platforms. use system() on linux and CreateProcessW on windows. – Stephen Apr 17 '14 at 13:48
  • @Stephen, the question is about Linux, where forking behaves as specified (unless he's using a Linux system with unusual issues, which we have no reason to think). – Matthew Flaschen Apr 20 '14 at 21:45
2

Using system() call with command line parameters without sanitizing the input can be highly insecure.

The potential security threat could be a user passing the following as directory name

somedir ; rm -rf /

To prevent this , use a mixture of the following

  • use getopt to ensure your input is sanitized
  • sanitize the input
  • use execl instead of system to execute the command

The best option would be to use all three

rajeshnair
  • 1,587
  • 16
  • 32
  • Thanks for the exploit example. That's clear! ;) Actually, myDir comes for getopt, though I didn't show it in the code above. I'll check how to use execl: apparently, a fork is needed (according to another comment here). – augustin Aug 29 '10 at 11:39
  • 2
    Do not try to escape strings AND use execl - this will cause the wrong parameters to be passed. – MarkR Aug 29 '10 at 12:41
0

Further to Matthew's answer, don't spawn a shell process unless you absolutely need it. If you use a fork/execl combination, individual parameters will never be parsed so don't need to be escaped. Beware of null characters however which will still prematurely terminate the parameter (this is not a security problem in some cases).

I assume mkdir is just an example, as mkdir can trivially be called from C++ much more easily than these subprocess suggestions.

MarkR
  • 62,604
  • 14
  • 116
  • 151
0

Reviving this ancient question as I ran into the same problem and the top answers, based on fork() + execl(), weren't working for me. (They create a separate process, whereas I wanted to use async to launch the command in a thread and have the system call stay in-process to share state more easily.) So I'll give an alternative solution.

It's not usually safe to pass user input as-is, especially if the utility is designed to be sudo'd; in order to sanitize it, instead of composing the string to be executed yourself, use environment variables, which the shell has built-in escape mechanisms for.

For your example:

// ...
std::string myDir;
myDir = argv[1]; // myDir is initialized using user input from the command line.
setenv("MY_DIR", myDir, 1);
if (system("mkdir \"${MY_DIR}\"") != 0) {
   return 1;
}
// continue....
qoba
  • 251
  • 1
  • 6