HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Restrictive stupid shell

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
stupidshellrestrictive

Problem

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 / fopen et al /
#include / strcmp, strlen /
#include / access /
#include / 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
fse

Solution

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;

Code Snippets

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

Context

StackExchange Code Review Q#80667, answer score: 5

Revisions (0)

No revisions yet.