Description: Fix unsafe extraction by using mkdir() instead of shell command This commit fixes following vulnerabilities: - CVE-2016-1243: stack buffer overflow caused by blindly trusting on pathname lengths of archived files Stack allocated buffer sysbuf was filled with sprintf() without any bounds checking in extracTree() function. - CVE-2016-1244: execution of unsanitized input Shell command used for creating directory paths was constructed by concatenating names of archived files to the end of the command string. So, if the user was tricked to extract a specially crafted .adf file, the attacker was able to execute arbitrary code with privileges of the user. This commit fixes both issues by 1) replacing mkdir shell commands with mkdir() function calls 2) removing redundant sysbuf buffer Author: Tuomas Räsänen Last-Update: 2016-09-20 -- --- a/examples/unadf.c +++ b/examples/unadf.c @@ -24,6 +24,8 @@ #define UNADF_VERSION "1.0" +#include +#include #include #include @@ -31,17 +33,15 @@ #include "adflib.h" -/* The portable way used to create a directory is to call the MKDIR command via the - * system() function. - * It is used to create the 'dir1' directory, like the 'dir1/dir11' directory +/* The portable way used to create a directory is to call mkdir() + * which is defined by following standards: SVr4, BSD, POSIX.1-2001 + * and POSIX.1-2008 */ /* the portable way to check if a directory 'dir1' already exists i'm using is to * do fopen('dir1','rb'). NULL is returned if 'dir1' doesn't exists yet, an handle instead */ -#define MKDIR "mkdir" - #ifdef WIN32 #define DIRSEP '\\' #else @@ -51,6 +51,13 @@ #define EXTBUFL 1024*8 +static void mkdirOrLogErr(const char *const path) +{ + if (mkdir(path, S_IRWXU | S_IRWXG | S_IRWXO)) + fprintf(stderr, "mkdir: cannot create directory '%s': %s\n", + path, strerror(errno)); +} + void help() { puts("unadf [-lrcsp -v n] dumpname.adf [files-with-path] [-d extractdir]"); @@ -152,7 +159,6 @@ void extractTree(struct Volume *vol, str { struct Entry* entry; char *buf; - char sysbuf[200]; while(tree) { entry = (struct Entry*)tree->content; @@ -162,16 +168,14 @@ void extractTree(struct Volume *vol, str buf=(char*)malloc(strlen(path)+1+strlen(entry->name)+1); if (!buf) return; sprintf(buf,"%s%c%s",path,DIRSEP,entry->name); - sprintf(sysbuf,"%s %s",MKDIR,buf); if (!qflag) printf("x - %s%c\n",buf,DIRSEP); + if (!pflag) mkdirOrLogErr(buf); } else { - sprintf(sysbuf,"%s %s",MKDIR,entry->name); if (!qflag) printf("x - %s%c\n",entry->name,DIRSEP); + if (!pflag) mkdirOrLogErr(entry->name); } - if (!pflag) system(sysbuf); - if (tree->subdir!=NULL) { if (adfChangeDir(vol,entry->name)==RC_OK) { if (buf!=NULL) @@ -301,21 +305,20 @@ void processFile(struct Volume *vol, cha extractFile(vol, name, path, extbuf, pflag, qflag); } else { - /* the all-in-one string : to call system(), to find the filename, the convert dir sep char ... */ - bigstr=(char*)malloc(strlen(MKDIR)+1+strlen(path)+1+strlen(name)+1); + bigstr=(char*)malloc(strlen(path)+1+strlen(name)+1); if (!bigstr) { fprintf(stderr,"processFile : malloc"); return; } /* to build to extract path */ if (strlen(path)>0) { - sprintf(bigstr,"%s %s%c%s",MKDIR,path,DIRSEP,name); - cdstr = bigstr+strlen(MKDIR)+1+strlen(path)+1; + sprintf(bigstr,"%s%c%s",path,DIRSEP,name); + cdstr = bigstr+strlen(path)+1; } else { - sprintf(bigstr,"%s %s",MKDIR,name); - cdstr = bigstr+strlen(MKDIR)+1; + sprintf(bigstr,"%s",name); + cdstr = bigstr; } /* the directory in which the file will be extracted */ - fullname = bigstr+strlen(MKDIR)+1; + fullname = bigstr; /* finds the filename, and separates it from the path */ filename = strrchr(bigstr,'/')+1; @@ -333,7 +336,7 @@ void processFile(struct Volume *vol, cha return; tfile = fopen(fullname,"r"); /* the only portable way to test if the dir exists */ if (tfile==NULL) { /* does't exist : create it */ - if (!pflag) system(bigstr); + if (!pflag) mkdirOrLogErr(bigstr); if (!qflag) printf("x - %s%c\n",fullname,DIRSEP); } else @@ -350,7 +353,7 @@ void processFile(struct Volume *vol, cha return; tfile = fopen(fullname,"r"); if (tfile==NULL) { - if (!pflag) system(bigstr); + if (!pflag) mkdirOrLogErr(bigstr); if (!qflag) printf("x - %s%c\n",fullname,DIRSEP); } else