I am trying to write a little program, which is resistant against buffer overflow and similar vulnerabilities.
Since we cannot trust the user input, I thought it would be a good idea to concatenate all the strings that are inputted into one string and then pass it to a static path, which is a bash script in the same folder and then add all the parameters/flags/arguments (e.g. ./script.sh test1 test2 test3 test4).
My logic on paper is the following:
argv[0] is equal to the program name, we need to skip it. Hence the loop starts at 1 (first argument) and ends at argc-1. So good so far.key in-memory.memcpy all the strings to make it safe to usescript.sh 4argshere to the userI tried freeing the memory like in the comments but it seems to not work or I have errors somewhere else.
My segfaulting Proof of Concept:
#include <stdlib.h>
#include <string.h>
#include <stdio.h>
#define BUFSIZE 1000
char *concatenate(size_t size, char *array[size], const char *joint);
char *concatenate(size_t size, char *array[size], const char *joint){
size_t jlen, lens[size];
size_t i, total_size = (size-1) * (jlen=strlen(joint)) + 1;
char *result, *p;
for(i=0;i<size;++i){
total_size += (lens[i]=strlen(array[i]));
}
p = result = malloc(total_size);
for(i=0;i<size;++i){
memcpy(p, array[i], lens[i]);
p += lens[i];
if(i<size-1){
memcpy(p, joint, jlen);
p += jlen;
}
}
*p = '\0';
return result;
}
int parse_output(char *safeargv[]) {
char safeargs = *concatenate(5, safeargv, " ");
char cmd[BUFSIZE];
snprintf(cmd, BUFSIZE, "./script.sh %s", safeargs);
char buf[BUFSIZE] = {0};
FILE *fp;
if ((fp = popen(cmd, "r")) == NULL) {
printf("Error opening pipe!\n");
//free(safeargs);
return -1;
}
while (fgets(buf, BUFSIZE, fp) != NULL) {
printf("OUTPUT: %s", buf);
}
if (pclose(fp)) {
printf("Command not found or exited with error status\n");
//free(safeargs);
return -1;
}
//free(safeargs);
return 0;
}
int main(int argc, char *argv[]) {
if(argc != 5) {
exit(1);
}
char *safeargv[5][4096];
for (int i = 1; i < argc - 1; i++) {
if (i == 1)
strcat(argv[1], ".key");
for (int x = 0; x < strlen(argv[i]); x++) {
char *unsafe_string = argv[i];
size_t max_len = 4096;
size_t len = strnlen(unsafe_string, max_len) + 1;
char *x = malloc(len * sizeof(char));
if (x != NULL) {
strncpy(x, unsafe_string, len);
x[len-1] = '\0'; // Ensure null-termination
strcpy(safeargv[i], x);
free(x);
}
}
}
parse_output(safeargv);
return 0;
}
The warnings I get while compiling:
chal.c: In function 'main':
chal.c:78:32: warning: passing argument 1 of 'strcpy' from incompatible pointer type [-Wincompatible-pointer-types]
78 | strcpy(safeargv[i], x);
| ~~~~~~~~^~~
| |
| char **
In file included from chal.c:2:
/usr/include/string.h:141:39: note: expected 'char * restrict' but argument is of type 'char **'
141 | extern char *strcpy (char *__restrict __dest, const char *__restrict __src)
| ~~~~~~~~~~~~~~~~~^~~~~~
chal.c:83:18: warning: passing argument 1 of 'parse_output' from incompatible pointer type [-Wincompatible-pointer-types]
83 | parse_output(safeargv);
| ^~~~~~~~
| |
| char * (*)[4096]
chal.c:32:24: note: expected 'char **' but argument is of type 'char * (*)[4096]'
32 | int parse_output(char *safeargv[]) {
| ~~~~~~^~~~~~~~~~
It seems only my argc check works, because if I call ./programname abc abc abc abc it segfaults. Also what's the proper way to detect an error, if for case there's a typo in the argument for the script?
What mistake(s) did I make?
My last ltrace lines:
strlen(" ") = 1
strlen(nil <no return ...>
It looks as if it would try to detect the length of a null character or similar.
main(): strcat(argv[1], ".key") is buffer overflow. You write 4 bytes to a string that has no spare space.
(performance) main(): for (int x = 0; x < strlen(argv[i]); x++) move the strlen() before the loop so you don't have to evaluate it per iteration.
main(): char *x = malloc(len * sizeof(char)); is confusing when the enclosing loop uses the variable int x = 0.
(performance) main(): for (int x = 0; x < strlen(argv[i]); x++) does the same thing strlen(argv[i]) times. Eliminate the loop and just do it once.
main(): strcpy(safeargv[i], x) is incorrect as strcpy() expects a char * but safeargv[i] is of type char *[4096]. I suspect you want to fix the type to be for example char *safeargv[4].
main(): With the type fixed strcpy(safeargv[i], x) is still problematic as you start i=1 so safeargv[0] is not initialized.
main(): parse_output(safeargv) is an incompatible type.
parse_output(): char safeargs = *concatenate(5, safeargv, " "); means safeargs hold the 1st character of whatever concatenate() returns. You want a char *. Subsequently passing a char when a char * is expected in the following snprintf(cmd, BUFSIZE, "./script.sh %s", safeargs) will probably trigger a segfault.
parse_output(): snprintf(cmd, BUFSIZE, "./script.sh %s", safeargs); each argument presumably could be up to 4k but BUFSIZE is 1k. You might as well limit each argument to 1k.
parse_output(): Eliminate concatenate() and just use snprintf(). In the program below I check if the command is being truncated. You may or may not care about this.
concatenate() is called with array[5] = { NULL, NULL, NULL, NULL, NULL} and strlen(NULL) cause your original segfault. With the other issues resolved this becomes a non-issue.
(Not fixed) popen() passes the command to shell so if your input is untrusted then you already lost. If you want to use popen() pass the input via a file (for example a temporary file and give the filename as the argument). Or use fork() + execl() instead of popen().
#define _POSIX_C_SOURCE 2
#include <stdio.h>
#define BUFSIZE 1000
int main(int argc, char *argv[]) {
if(argc != 5) {
printf("usage: programname STR STR STR STR\n");
return 1;
}
char cmd[BUFSIZE];
int n = snprintf(cmd, 0, "./script.sh %s.key %s %s %s", argv[1], argv[1], argv[2], argv[3]);
if(n + 1 > BUFSIZE) {
printf("cmd was truncated\n");
return 1;
}
snprintf(cmd, BUFSIZE, "./script.sh %s.key %s %s %s", argv[1], argv[1], argv[2], argv[3]);
FILE *fp = popen(cmd, "r");
if (!fp) {
printf("Error opening pipe!\n");
return 1;
}
char buf[BUFSIZE];
while (fgets(buf, BUFSIZE, fp))
printf("OUTPUT: %s", buf);
if (pclose(fp)) {
printf("Command not found or exited with error status\n");
return 1;
}
}
With script.sh:
#!/bin/bash
echo "$*"
here is an example run:
$ ./a.out abc abc abc abc
OUTPUT: abc.key abc abc abc