Restrictive stupid shell

Question

I'm attempting to create a replacement shell for /bin/bash, that only permits commands defined by a read only config file. These commands will be run by absolute paths, and take no arguments. I'm not an experienced C programmer, so I kindly ask for your comments and feedback. I'm especially concerned with buffer overflows etc. Please note that the "re read" config file is by design, if the rules change I would like the sessions already running to adhere to these changes.

#include <stdio.h> /* fopen et al */
#include <string.h> /* strcmp, strlen */
#include <unistd.h> /* access */
#include <stdlib.h> /* system */
#define RULEFILE "/home/ole/wannabeshell/rules.conf"
#define PROMPT "$ "
#define BUFFER 512

// not really a shell - just executes commands
// thats given in the config file by absolute
// paths..

int main (int argc, char **argv) {
    // buffer size + 1 for \0 ending
    char sbuffer[BUFFER + 1], fbuffer[BUFFER + 1];

    // open the rulefile
    FILE *fh = fopen(RULEFILE, "r");

    // give up if unable to open it
    if (fh == NULL) {
        perror("unable to open rulefile");
        return(EXIT_FAILURE);
    }

    while (1) {
        // print prompt character
        printf(PROMPT);

        // force flush
        fflush(NULL);

        // make sure buffers does not contain data
        // from previous runs of the loop
        memset(sbuffer, 0, sizeof(sbuffer));
        memset(fbuffer, 0, sizeof(fbuffer));

        // read up to sizeof buffer, or \n
        if (fgets(sbuffer, sizeof(sbuffer), stdin) == NULL) {
            break;
        }

        // check for data up to NULL, must be atleast one !NULL
        // char for this test to be successful
        if (strlen(sbuffer) < 1) {
            break;
        }

        // find and replace \r and \n if exists
        sbuffer[strcspn(sbuffer, "\r\n")] = 0;

        // time to exit?
        if (strcmp(sbuffer, "exit") == 0 || strcmp(sbuffer, "quit") == 0) {
            break;
        }

        // seek to start of rulefile
        fseek(fh, 0, SEEK_SET);

        // read one line from rule file
        while (fgets(fbuffer, sizeof(fbuffer), fh) != NULL) {
            // strip \n and \r from line
            fbuffer[strcspn(fbuffer, "\r\n")] = 0;

            // if requested command equals rulefile line
            if (strlen(fbuffer) == strlen(sbuffer) && strcmp(fbuffer, sbuffer) == 0) {

                // make sure the command e.g. file exists
                // and is actually executable
                if (access(fbuffer, X_OK) == 0) {
                    // fork and execute fbuffer with /bin/sh -c
                    // system blocks till finished
                    system(fbuffer);
                    break;
                } else {
                    // not executable or does not exist
                    break;
                }
            }
        }
    }

    // close the rulefile
    fclose(fh);

    // indicate success
    return(0);
}

An initial attempt to overflow by perl -e 'print "\0"x50000;' did not reveal anything apparent. I've also tested that it runs as it should.

Example run

[ole@playground ~]$ ./strictshell
$ foo
$ \0
$ bar
$ /usr/bin/uptime
 15:14:44 up 157 days,  1:04, 23 users,  load average: 0.13, 0.15, 0.10
$ exit

Answer 1

Security

It's not safe to execute commands using system. Try to use one of the execv functions instead (see man execv for the flavors that exist in your system).

Although you wrote that only absolute paths can be executed, there's nothing in this code to ensure that. If a malicious user gains access to the rule file, they can enter a relative path, and your program will be happy to execute that. To make this safer using system, the program should ensure that the paths are absolute paths.

The bottom line is, don't use system, use execv. The big difference is that the execv commands don't use the shell, but invoke the specified executables directly, which frees you from potential vulnerabilities of shells, such as environment variable command injection, and who knows what else.

Efficiency

After every validated user input, the program re-reads the rules file. It would be better to read and parse the rules file once at the start into an array, and reuse it in the infinite loop of handling user input.

Simplify

This can be written simpler:

if (access(fbuffer, X_OK) == 0) {
    // ...
    // system blocks till finished
    system(fbuffer);
    break;
} else {
    // not executable or does not exist
    break;
}

Since both branches of the if-else will break, you can move the break outside, and simply drop the else:

if (access(fbuffer, X_OK) == 0) {
    // ...
    // system blocks till finished
    system(fbuffer);
}
// not executable or does not exist
break;