Need help improving a small C program

Question

I wanted to launch a bash script (read: bash not sh script) as a root not as the user calling it, however bash ignore setuid on scripts, so I chose to write a very small script that takes a script/arguments and call it with setuid set.

This worked well and I went even further to verify that the script has setuid set on, executable and setuid() called on the owner of the file and not as root, to avoid any misuse of the program and I ended up with the program below..

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/types.h>
#include <sys/stat.h>

int main(int argc, char **argv)
{
  char *command;
  int i, file_owner, size = 0;
  struct stat status_buf;
  ushort file_mode;

  // Check argc
  if (argc < 2) {
    printf("Usage: %s <script> [arguments]\n", argv[0]);
    return 1;
  }

  // Make sure the script does exist
  if(fopen(argv[1], "r") == NULL) {
    printf("The file %s does not exist.\n", argv[1]);
    return 1;
  }

  // Get the attributes of the file
  stat(argv[1], &status_buf);

  // Get the permissions of the file
  file_mode = status_buf.st_mode;

  // Make sure it's executable and it's setuid
  if(file_mode >> 6 != 567) {
    printf("The file %s should be executable and should have setuid set, please chmod it 0106755.\n", argv[1]);
    return 1;
  }

  // Get the owner of the script
  file_owner = status_buf.st_uid;

  // setuid
  setuid(file_owner);

  // Generate the command
  for (i = 1; i < argc; i++) {
    size += strlen(argv[i]);
  }
  command = (char *) malloc( (size + argc + 11) * sizeof(char) );
  sprintf(command, "/bin/bash %s", argv[1]);
  if (argc > 2) {
    for (i = 2; i < argc; i++) {
      sprintf(command, "%s %s", command, argv[i]);
    }
  }

  // Execute the command
  system(command);

  // free memory
  free(command);

  return 0;
}

The exercise was not only to solve my problem, but it was also a way to get more into C, so what do you guys suggest? Is there anything I should improve ?

Thank you..

P.S: This is an exact replicate of my post on stackoverflow, They suggested I should post here, sorry for the double post

As suggested by Ignacio Vazquez-Abrams on the original post here's the modified version of the script, any other sugesttions ?

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/types.h>
#include <sys/stat.h>

int main(int argc, char **argv)
{
  int    file_owner, status;
  struct stat status_buf;
  ushort file_mode;

  // Check argc
  if (argc < 2) {
    printf("Usage: %s <script> [arguments]\n", argv[0]);
    return 1;
  }

  // Make sure the script does exist
  if(fopen(argv[1], "r") == NULL) {
    printf("The file %s does not exist.\n", argv[1]);
    return 1;
  }

  // Get the attributes of the file
  stat(argv[1], &status_buf);

  // Get the permissions of the file
  file_mode = status_buf.st_mode;

  // Make sure it's executable and it's setuid
  if( !( (file_mode & S_ISUID) || (file_mode & S_IXUSR) ) ) {
    printf("The file %s should be executable and should have setuid set.\n", argv[1]);
    return 1;
  }

  // Get the owner of the script
  file_owner = status_buf.st_uid;

  // setuid
  setuid(file_owner);

  // Execute the command in a child
  if(fork() == 0) {
    execvp(argv[1], argv + 1);
  }

  // Wait for the child to finish
  wait(&status);

  return 0;
}

edit V3

/**
 * vim:ft=c:fenc=UTF-8:ts=2:sts=2:sw=2:expandtab
 *
 * Copyright 2011 TechnoGate <contact@technogate.fr>
 *
 * This program is free software: you can redistribute it and/or modify it under the
 * terms of the GNU General Public License as published by the Free Software
 * Foundation, either version 3 of the License, or (at your option) any later
 * version.
 *
 * This program is distributed in the hope that it will be useful, but WITHOUT ANY
 * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A
 * PARTICULAR PURPOSE. See the GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License along with
 * This program. If not, see http://www.gnu.org/licenses/
 */

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <pwd.h>

int main(int argc, char **argv)
{
  int    file_owner, status;
  char   *newenviron[] = { NULL };
  uid_t  current_uid;
  pid_t  child_pid, wpid;
  ushort file_mode;
  struct stat status_buf;
  struct passwd * passent;

  // Check that we are root!
  current_uid = geteuid();
  if(current_uid != 0) {
    fprintf(stderr, "The wrapper should be ran as root, you should maybe set the owner to root and set setuid.\n");
    return 1;
  }

  // Check argc
  if (argc < 2) {
    fprintf(stderr, "Usage: %s <script> [arguments]\n", argv[0]);
    return 1;
  }

  // Make sure the script does exist
  if(fopen(argv[1], "r") == NULL) {
    fprintf(stderr, "The file %s does not exist.\n", argv[1]);
    return 1;
  }

  // Get the attributes of the file
  if(stat(argv[1], &status_buf) < 0) {
    fprintf(stderr, "Can't stat %s\n", argv[1]);
    return 1;
  }

  // Get the permissions of the file
  file_mode = status_buf.st_mode;

  // Make sure it's executable and it's setuid
  if( !( (file_mode & S_ISUID) || (file_mode & S_IXUSR) ) ) {
    fprintf(stderr, "The file %s should be executable and should have setuid set.\n", argv[1]);
    return 1;
  }

  // Get the owner of the script
  file_owner = status_buf.st_uid;

  // Get the name of the owner
  passent = getpwuid(file_owner);

  // setuid
  if(setuid(file_owner)) {
    if(passent != NULL)
      fprintf(stderr, "Can't change the persona to %s\n", passent -> pw_name);
    else
      fprintf(stderr, "Can't change the persona to %d\n", file_owner);
    return 1;
  }

  // Fork
  child_pid = fork();
  if(child_pid == -1) {
    perror("fork");
    exit(EXIT_FAILURE);
  }

  // Execute the command in a child
  if(child_pid == 0) {
    execve(argv[1], argv + 1, newenviron);
    perror("Failed to execute the script");
    exit(EXIT_FAILURE);
  } else {
    do {
      wpid = waitpid(child_pid, &status, WUNTRACED
    #ifdef WCONTINUED       /* Not all implementations support this */
        | WCONTINUED
    #endif
        );

      // Seems the child is not running
      if (wpid == -1) {
        perror("waitpid");
        exit(EXIT_FAILURE);
      }

      if (WIFEXITED(status)) { /* Child exited ? */
        return WEXITSTATUS(status);


      } else if (WIFSIGNALED(status)) { /* Child signaled ? */
        return WTERMSIG(status);

      } else if (WIFSTOPPED(status)) { /* Child stopped */
        return WSTOPSIG(status);


    #ifdef WIFCONTINUED     /* Not all implementations support this */
      } else if (WIFCONTINUED(status)) {
        // The child is still running...
    #endif
      } else {    /* Non-standard case -- may never happen */
        printf("Unexpected status (0x%x)\n", status);
      }
    } while (!WIFEXITED(status) && !WIFSIGNALED(status));
  }

  return EXIT_FAILURE;
}

Answer 1

1. Check for errors in stat, setuid, fork, and execvp, to name a few. If the exec fails in the child you should call exit.

2. Do you really want the p in execvp? This is not guaranteed to be the same as the argv[1] you just stat-ed. If argv[1] is ls your stat will look for a file at ./ls and it will likely find the program in /bin. I would use execve and either do that PATH lookup yourself or simply omit that part and require the user to specify a full path for something in PATH (eg. /bin/ls instead of just ls).

3. The stat + observe state + exec thing is a race condition. Another process can change the attributes on the file in that timing window. This may or may not be important to you. Given that this is a security-ish program I would say it may very well be.

4. Instead of returning 0, you might want to return the child process's exit code (which you can get with waitpid.) You might also want to return nonzero when the functions I mention in #1 fail. This way a shell script or something calling you programmatically can determine success or failure.

Answer 2

If I understand you correctly you would install your program (edit V3) like this

$ gcc -o edit_v3 edit_v3.c
$ ls -l edit_v3
-rwxrwxr-x 1 erik erik 7698 2012-04-15 12:22 edit_v3
$ sudo chown root.root edit_v3
$ sudo chmod 4755 edit_v3
$ ls -l edit_v3
-rwsr-xr-x 1 root root 7698 2012-04-15 12:22 edit_v3

But a user could then easily get root access

$ ls -l /bin/su
-rwsr-xr-x 1 root root 31116 2011-06-24 11:37 /bin/su
$ ./edit_v3 /bin/su
# id
uid=0(root) gid=0(root) grupper=0(root),116(pulse),117(pulse-access)

It is difficult to write secure setuid programs. Over the years a lot of security vulnerabilities have been found in such program.

Instead of writing your own setuid program, you could also use sudo. You would then have to edit the configuration file /etc/sudoers

Answer 3

One thing I'd do is

change

sprintf(command, "%s %s", command, argv[i]);

to use strcat

while this does work on a number of implementations, it's not considered "safe"

refer http://stackoverflow.com/questions/1283354/is-sprintfbuffer-s-buffer-safe