# Set-uid root program that runs a program as the user “restrict” (follow-up)

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):

#define _GNU_SOURCE
#include <sys/types.h>
#include <unistd.h>
#include <pwd.h>
#include <grp.h>
int main (int argc, char **argv) {
if (argc < 2) return 1;
struct passwd *r = getpwnam("restrict");
gid_t supp[] = {};
// De-escalate privileges.
if (setgroups(0, supp)) return 1; // Remove supplementary groups.
if (setgid(r->pw_gid)) return 1; // Switch group to "restrict".
if (setuid(r->pw_uid)) return 1; // Switch user to "restrict".
char *env[] = {NULL}; // Delete all environment variables.
// Finally, attempt to replace this process.
execvpe(argv[1], &argv[1], env);
return 1; // Getting here means we failed.
}


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?

• You've still not explained why you can't just use sudo -u... – jacwah Jul 7 '15 at 18:27
• I can't use sudo -u restrict ... if the user doesn't have privileges to use sudo, and I'm not familiar enough with sudoers to give all users in a particular group permission to sudo -u restrict .... Sorry for not clarifying that in the other thread. – Collin Jul 7 '15 at 19:01

You should really check the return value of getpwnam(). If, for whatever reason, the user "restrict" is not found, your program will crash. I'm not sure whether a null dereference can be exploited (I think not), but it's a "bad thing" anyways.

As already mentioned, giving a descriptive error message when execv fails is a good idea, too. In fact, do this on every possible error; no need for extra verbosity where you don't exepect an error -- in this case a perror with the failed function name should be enough.

You probably don't want the user name configurable in any way for security reasons, but just to make it easier to "configure" at compile time, I'd suggest to #define it.

All in all somehow like this:

#define _GNU_SOURCE
#include <stdio.h>
#include <string.h>
#include <errno.h>
#include <sys/types.h>
#include <unistd.h>
#include <pwd.h>
#include <grp.h>

int main (int argc, char **argv)
{
if (argc < 2) return 1;
if (!r)
{
return 1;
}

gid_t supp[] = {};
if (setgroups(0, supp))
{
perror("setgroups");
return 1;
}

if (setgid(r->pw_gid))
{
perror("setgid");
return 1;
}

if (setuid(r->pw_uid))
{
perror("setuid");
return 1;
}

char *env[] = {NULL};
execvpe(argv[1], &argv[1], env);
/* the following is only reached on error */

fprintf(stderr, "Cannot execute %s': %s\n", argv[1],
strerror(errno));
return 1;
}

• Very helpful! I'm going to hold off on selecting an answer for a few days, though, to see if anyone brings up any other security concerns. – Collin Jul 8 '15 at 15:20
• By the way, you missed #include <stdio.h>. I tried to edit your post to fix it, but it still has not been reviewed after a day. – Collin Jul 9 '15 at 13:03
• Fixed. Well, I just edited the code on here, not trying to compile it. And sure, it's easy to prove a security flaw once you found it, the opposite is not so trivial. This code is short enough to be quite sure, though. Still I think solving this issue with sudo alone might be the better approach. – Felix Palmen Jul 9 '15 at 23:50
• I also agree that sudo alone might be a better approach, but there is also a benefit to the fact that subprocess.Popen(['/path/to/restrict/binary', 'sleep', '10']).pid is the actual pid of the process I intended to create. – Collin Jul 10 '15 at 13:16

Have one statement per line. This way, you can add a breakpoint at return 1. It's also easier to read since one can just scan the left edge of each line for statements, instead of jumping back and forth. It might not be a big issue in such a small program, but it's good to have as a habit.

Your code has comments that describe almost every statement. They don't add anything, it's just visual noise. Write comments that explain why, not what. For instance, why do you delete all environment variables?

You might want to print an error message if execve fails. Otherwise you're going to leave the user guessing. errno will be set if it returns. There are a lot of reasons for it failing: file doesn't exist, user doesn't have permission etc. See the man page for a full reference.

execvpe(argv[1], &argv[1], env);
perror("Failed to execute command");
return 1;


PS. You've forgot a semicolon after execve! Did you try to compile this code before posting? (OP edited post)

• missing semicolon is a copy/paste error, not related to the question – Collin Jul 7 '15 at 19:59
• These are all very nice suggestions, but they don't address any of the things I'm asking about. – Collin Jul 7 '15 at 20:02
• @Collin Feel free to call attention to specific areas you are concerned about (performance, formatting, etc). However, any aspect of the code posted is fair game for feedback and criticism.` codereview.stackexchange.com/help/on-topic – jacwah Jul 7 '15 at 20:04
• @Collin The easier the code is to read, the easier it is to spot bugs. This leads to better security in the long run. I'm okay with you not marking it as accepted answer though. – jacwah Jul 7 '15 at 20:09
• Absolutely. Thank you for your suggestions :) – Collin Jul 7 '15 at 20:10