patterncMinor
Restrictive stupid shell
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
```
#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
Try to use one of the
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
the program should ensure that the paths are absolute paths.
The bottom line is, don't use
The big difference is that the
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:
Since both branches of the
you can move the
and simply drop the
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.