0
\$\begingroup\$

I've written a very short program that is intended to run the program specified in its arguments (argv) as the user named "restrict" (which exists). I want to make sure that there are no subtle bugs in the program, and that the program is as simple as possible.

  • Is this program doing what I intend?
  • Can someone use this program to do anything malicious? Assume that anything running as the user "restrict" is not able to do anything malicious, but anything running as the user "root" obviously can.
  • Can this code be simplified, or (assuming the code is correct) can this code's correctness be made more obvious?

The code (compiled with gcc --std=c11):

#include <unistd.h>
int main (int argc, char **argv) {
    setuid(0); // Switch user to root.
    char *start[] = {
        "/usr/bin/sudo", "-u", "restrict", // Switch user to "restrict".
        "--" // End arguments.
    };
    int start_len = (sizeof start) / (sizeof start[0]);
    char *args[start_len+argc];
    char *env[] = {NULL}; // Delete all environment variables.
    int i;
    for (i = 0; i < start_len; ++i)
        args[i] = start[i];
    // Note that argv is null-terminated, so we can go past the end by one.
    // Conveniently, we also want to discard the first argument, so just loop
    // over the arguments as usual, but with an offset of one.
    for (i = 0; i < argc; ++i)
        args[start_len+i] = argv[i+1];
    execve(args[0], args, env);
}

The final binary will be owned by root, and will have the set-uid bit set, and will be executable by users in a particular group.

The ultimate goal is to allow users in a particular group to run any executable as the user "restrict".

Alternatively, is there a better way to accomplish this goal?


See Set-uid root program that runs a program as the user “restrict” (follow-up) for the next iteration of this code.

\$\endgroup\$
  • 1
    \$\begingroup\$ What's wrong with using sudo -u directly? \$\endgroup\$ – jacwah Jul 7 '15 at 15:19
  • 1
    \$\begingroup\$ What @jacwah says, but also, an entry in /etc/sudoers will go a long way to locking down, or opening up the access to the restricted user in a much cleaner, systematic way. \$\endgroup\$ – rolfl Jul 7 '15 at 15:23
  • \$\begingroup\$ I'm not sure how to accomplish this with the sudoers file. \$\endgroup\$ – Collin Jul 7 '15 at 15:30
  • \$\begingroup\$ I've edited my question so that answers related to the sudoers file will be acceptable. \$\endgroup\$ – Collin Jul 7 '15 at 15:35
4
\$\begingroup\$
  • Stay away from variable length arrays. First, they are optional in C11 (in C99 they were mandatory). Second, there is no way to detect allocation failure. A malicious user may invoke the program with a command line long enough to cause stack overflow.

  • Why sudo? You are root already, so you may set the user and group ID directly.

\$\endgroup\$
  • \$\begingroup\$ With respect to your second point, could you give an example of how I would map the username to the user id and group id securely? \$\endgroup\$ – Collin Jul 7 '15 at 17:24
  • \$\begingroup\$ @Collin See manpage for getpwnam. \$\endgroup\$ – vnp Jul 7 '15 at 17:38
  • \$\begingroup\$ If I want a new revision with your suggestions to be reviewed, should I edit the original question, or create a new one? \$\endgroup\$ – Collin Jul 7 '15 at 18:01
  • \$\begingroup\$ @Collin Editing answered questions is against the CR chapter, because it invalidates answers. You are very welcome to post a new question. \$\endgroup\$ – vnp Jul 7 '15 at 18:03
  • \$\begingroup\$ Thanks. I also found the answer on meta: meta.codereview.stackexchange.com/questions/1763/… \$\endgroup\$ – Collin Jul 7 '15 at 18:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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