1
\$\begingroup\$

I'm not an experienced Linux user and I wanted an easy way to run shell scripts as root from a PHP script, I came up with this:

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

int main(int argc, char *argv[])
{
    if (access(argv[1], F_OK) != -1)
    {
        struct stat filestat;

        if (stat(argv[1], &filestat) == 0)
        {
            if ((filestat.st_uid == 0) && (filestat.st_gid == 0) && (filestat.st_mode & S_IXUSR) && (!(filestat.st_mode & S_IWOTH)))
            {
                char* match = strrchr(argv[1], '.');

                if ((match != NULL) && (strcasecmp(match, ".sh") == 0))
                {
                    if (setuid(0) != -1)
                    {
                        execl("/usr/bin/sudo", "/usr/bin/sudo", argv[1], (char*) NULL);
                        return 0;
                    }
                }
            }
        }
    }

    return 1;
}

Any potential security issues with this? If so how can I improve it?

Example Usage:

Lets say I have a script located at:

/some/path/script.sh

With the following in it:

#!/bin/bash
echo $USER

Now lets say I compile the above C code to a binary and place it at:

/some/path/run-as

and do:

chown root:root /some/path/run-as
chmod 6755 /some/path/run-as

Now I run this PHP script owned by www-data (via browser / local apache web server):

<?php
    echo exec('/some/path/run-as /some/path/script.sh');
?>

I expect the script to output 'root' when ran.

\$\endgroup\$
  • \$\begingroup\$ @TobySpeight I went ahead and edited the post with example usage to give you an idea of what I'm trying to do, also I'm not too familiar with Linux so I'm not even sure If my idea will work as I haven't tested it yet. \$\endgroup\$ – user177906 Aug 22 '18 at 20:36
  • 3
    \$\begingroup\$ Welcome to Code Review! I rolled back your last edit. After getting an answer you are not allowed to change your code anymore. This is to ensure that answers do not get invalidated and have to hit a moving target. If you have changed your code you can either post it as an answer (if it would constitute a code review) or ask a new question with your changed code (linking back to this one as reference). Refer to this post for more information \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Aug 22 '18 at 22:57
1
\$\begingroup\$

This is an ambitious thing to try, with many pitfalls - especially for environments which are Web-accessible. I urge you to learn how to configure and use sudo instead - that's not foolproof, but it is a good deal safer than rolling your own.

That said, I'll make some observations:

  • We're using argv[1] without checking whether any arguments were supplied (we should test argc >= 2 before using argv[1] - possibly check argc == 2, since we ignore any extra arguments).
  • There's a time-of-check/time-of-use (TOCTOU) race, because we examine the supplied file and later assume the properties haven't changed (we we get to running it).
  • Using system causes the supplied argument to be parsed as a shell command, which will do word splitting, expand variables and globs, and more.
  • Using a shell script from a privileged program is vulnerable to the script being changed between the interpreter being started and it opening the script to read commands (e.g. by switching symlinks).

It's quite easy to exploit this script to execute anything I want on your system, if there's a directory I can write to:

  1. ln -s /bin/true 'ls -l ~root; id; cd; echo rm -rf; true .sh'
    
  2. /some/path/run-as ls*.sh
    

We can avoid using system() (and accept extra arguments if we choose), by using execv() instead:

            if (match && !strcasecmp(match, ".sh"))
            {
                execv(argv[1], argv+1);
                /* if we got here, exec failed */
                /* so fall through to return 1 */
            }

This does still leave us vulnerable to a user creating a symbolic link to a root-owned file and rewriting it to point to the user's file between the access check and the exec(), though.


In short: please use sudo if you really have to run some things as root. Configured properly, it can give tight control on the commands you can execute with elevated privileges. It's not completely attack-proof, but it is much better than writing your own (it has had decades of scrutiny to improve it).

\$\endgroup\$
  • \$\begingroup\$ I've edited my original post with some changes, I've decided to strip all non alphanumeric characters (excluding '/' and '.') since I know the paths I use will never include spaces or any special characters. I wasn't sure how to fix the symlink issue, so I decided to just use lstat instead of stat. \$\endgroup\$ – user177906 Aug 22 '18 at 22:50
  • \$\begingroup\$ isnt it even better if user set up a group that only have access to the things that the php script needs access to? \$\endgroup\$ – baot Aug 23 '18 at 10:01
  • \$\begingroup\$ @baot: Absolutely; much better. I've only scratched at the surface with this answer. \$\endgroup\$ – Toby Speight Aug 23 '18 at 10:10
0
\$\begingroup\$

I know It's better to just use sudo, but I still want to do it this way.

Going by Toby Speight's response, I've modified the code a bit:

  • Ensure argc == 2.
  • Stripping unnecessary characters from input.
  • Using lstat instead of stat.

Hopefully this will be good enough.


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

int main(int argc, char *argv[])
{
    if (argc == 2)
    {
        unsigned long i = 0;
        unsigned long j = 0;
        char c;

        while ((c = argv[1][i++]) != '\0')
        {
            if ((isalnum(c)) || (c == '.') || (c == '/'))
            {
                argv[1][j++] = c;
            }
        }

        argv[1][j] = 0;

        if (access(argv[1], F_OK) != -1)
        {
            struct stat filestat;

            if (lstat(argv[1], &filestat) == 0)
            {
                if ((filestat.st_uid == 0) && (filestat.st_gid == 0) && (filestat.st_mode & S_IXUSR) && (!(filestat.st_mode & S_IWOTH)))
                {
                    char* match = strrchr(argv[1], '.');

                    if ((match != NULL) && (strcasecmp(match, ".sh") == 0))
                    {
                        if (setuid(0) != -1)
                        {
                            execv(argv[1], argv+1);
                            return 0;
                        }
                    }
                }
            }
        }
    }

    return 1;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ You don't want to return zero (success) if exec() returns - that's clearly a failure. I still recommend using sudo instead, which can be configured to permit your specific combination of requesting user, target user and command with much better control and fewer bugs. \$\endgroup\$ – Toby Speight Aug 23 '18 at 7:02
  • \$\begingroup\$ I still want to do it this way. Don't. For example, all the tricks you're going through to seemingly ensure that only a root-owned shell script can be exec'd by your process still has a huge hole in it, based on this. Hopefully this will be good enough. No, it's not. Even if you figure out the vulnerability I just pointed you in the direction of, there are still race conditions that your approach pretty much ignores. Heck, I'd bet a simple LD_PRELOAD could be used to exploit your code. \$\endgroup\$ – Andrew Henle Aug 24 '18 at 16:41

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.