From 81dfa6b3e08f6934885ba5c98939587d6850d08e Mon Sep 17 00:00:00 2001 From: Josef Moellers Date: Thu, 4 Oct 2018 14:21:48 +0200 Subject: [PATCH] Fix issue #62: Remove any "../" components from pathnames of extracted files. [CVE-2018-17828] --- bins/unzzipcat-big.c | 57 +++++++++++++++++++++++++++++++++++++++++++- bins/unzzipcat-mem.c | 57 +++++++++++++++++++++++++++++++++++++++++++- bins/unzzipcat-mix.c | 57 +++++++++++++++++++++++++++++++++++++++++++- bins/unzzipcat-zip.c | 57 +++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 224 insertions(+), 4 deletions(-) diff --git a/bins/unzzipcat-big.c b/bins/unzzipcat-big.c index 982d262..88c4d65 100644 --- a/bins/unzzipcat-big.c +++ b/bins/unzzipcat-big.c @@ -53,6 +53,48 @@ static void unzzip_cat_file(FILE* disk, char* name, FILE* out) } } +/* + * NAME: remove_dotdotslash + * PURPOSE: To remove any "../" components from the given pathname + * ARGUMENTS: path: path name with maybe "../" components + * RETURNS: Nothing, "path" is modified in-place + * NOTE: removing "../" from the path ALWAYS shortens the path, never adds to it! + * Also, "path" is not used after creating it. + * So modifying "path" in-place is safe to do. + */ +static inline void +remove_dotdotslash(char *path) +{ + /* Note: removing "../" from the path ALWAYS shortens the path, never adds to it! */ + char *dotdotslash; + int warned = 0; + + dotdotslash = path; + while ((dotdotslash = strstr(dotdotslash, "../")) != NULL) + { + /* + * Remove only if at the beginning of the pathname ("../path/name") + * or when preceded by a slash ("path/../name"), + * otherwise not ("path../name..")! + */ + if (dotdotslash == path || dotdotslash[-1] == '/') + { + char *src, *dst; + if (!warned) + { + /* Note: the first time through the pathname is still intact */ + fprintf(stderr, "Removing \"../\" path component(s) in %s\n", path); + warned = 1; + } + /* We cannot use strcpy(), as there "The strings may not overlap" */ + for (src = dotdotslash+3, dst=dotdotslash; (*dst = *src) != '\0'; src++, dst++) + ; + } + else + dotdotslash +=3; /* skip this instance to prevent infinite loop */ + } +} + static void makedirs(const char* name) { char* p = strrchr(name, '/'); @@ -70,6 +112,16 @@ static void makedirs(const char* name) static FILE* create_fopen(char* name, char* mode, int subdirs) { + char *name_stripped; + FILE *fp; + int mustfree = 0; + + if ((name_stripped = strdup(name)) != NULL) + { + remove_dotdotslash(name_stripped); + name = name_stripped; + mustfree = 1; + } if (subdirs) { char* p = strrchr(name, '/'); @@ -79,7 +131,10 @@ static FILE* create_fopen(char* name, char* mode, int subdirs) free (dir_name); } } - return fopen(name, mode); + fp = fopen(name, mode); + if (mustfree) + free(name_stripped); + return fp; } diff --git a/bins/unzzipcat-mem.c b/bins/unzzipcat-mem.c index 9bc966b..793bde8 100644 --- a/bins/unzzipcat-mem.c +++ b/bins/unzzipcat-mem.c @@ -58,6 +58,48 @@ static void unzzip_mem_disk_cat_file(ZZIP_MEM_DISK* disk, char* name, FILE* out) } } +/* + * NAME: remove_dotdotslash + * PURPOSE: To remove any "../" components from the given pathname + * ARGUMENTS: path: path name with maybe "../" components + * RETURNS: Nothing, "path" is modified in-place + * NOTE: removing "../" from the path ALWAYS shortens the path, never adds to it! + * Also, "path" is not used after creating it. + * So modifying "path" in-place is safe to do. + */ +static inline void +remove_dotdotslash(char *path) +{ + /* Note: removing "../" from the path ALWAYS shortens the path, never adds to it! */ + char *dotdotslash; + int warned = 0; + + dotdotslash = path; + while ((dotdotslash = strstr(dotdotslash, "../")) != NULL) + { + /* + * Remove only if at the beginning of the pathname ("../path/name") + * or when preceded by a slash ("path/../name"), + * otherwise not ("path../name..")! + */ + if (dotdotslash == path || dotdotslash[-1] == '/') + { + char *src, *dst; + if (!warned) + { + /* Note: the first time through the pathname is still intact */ + fprintf(stderr, "Removing \"../\" path component(s) in %s\n", path); + warned = 1; + } + /* We cannot use strcpy(), as there "The strings may not overlap" */ + for (src = dotdotslash+3, dst=dotdotslash; (*dst = *src) != '\0'; src++, dst++) + ; + } + else + dotdotslash +=3; /* skip this instance to prevent infinite loop */ + } +} + static void makedirs(const char* name) { char* p = strrchr(name, '/'); @@ -75,6 +117,16 @@ static void makedirs(const char* name) static FILE* create_fopen(char* name, char* mode, int subdirs) { + char *name_stripped; + FILE *fp; + int mustfree = 0; + + if ((name_stripped = strdup(name)) != NULL) + { + remove_dotdotslash(name_stripped); + name = name_stripped; + mustfree = 1; + } if (subdirs) { char* p = strrchr(name, '/'); @@ -84,7 +136,10 @@ static FILE* create_fopen(char* name, char* mode, int subdirs) free (dir_name); } } - return fopen(name, mode); + fp = fopen(name, mode); + if (mustfree) + free(name_stripped); + return fp; } static int unzzip_cat (int argc, char ** argv, int extract) diff --git a/bins/unzzipcat-mix.c b/bins/unzzipcat-mix.c index 91c2f00..73b6ed6 100644 --- a/bins/unzzipcat-mix.c +++ b/bins/unzzipcat-mix.c @@ -69,6 +69,48 @@ static void unzzip_cat_file(ZZIP_DIR* disk, char* name, FILE* out) } } +/* + * NAME: remove_dotdotslash + * PURPOSE: To remove any "../" components from the given pathname + * ARGUMENTS: path: path name with maybe "../" components + * RETURNS: Nothing, "path" is modified in-place + * NOTE: removing "../" from the path ALWAYS shortens the path, never adds to it! + * Also, "path" is not used after creating it. + * So modifying "path" in-place is safe to do. + */ +static inline void +remove_dotdotslash(char *path) +{ + /* Note: removing "../" from the path ALWAYS shortens the path, never adds to it! */ + char *dotdotslash; + int warned = 0; + + dotdotslash = path; + while ((dotdotslash = strstr(dotdotslash, "../")) != NULL) + { + /* + * Remove only if at the beginning of the pathname ("../path/name") + * or when preceded by a slash ("path/../name"), + * otherwise not ("path../name..")! + */ + if (dotdotslash == path || dotdotslash[-1] == '/') + { + char *src, *dst; + if (!warned) + { + /* Note: the first time through the pathname is still intact */ + fprintf(stderr, "Removing \"../\" path component(s) in %s\n", path); + warned = 1; + } + /* We cannot use strcpy(), as there "The strings may not overlap" */ + for (src = dotdotslash+3, dst=dotdotslash; (*dst = *src) != '\0'; src++, dst++) + ; + } + else + dotdotslash +=3; /* skip this instance to prevent infinite loop */ + } +} + static void makedirs(const char* name) { char* p = strrchr(name, '/'); @@ -86,6 +128,16 @@ static void makedirs(const char* name) static FILE* create_fopen(char* name, char* mode, int subdirs) { + char *name_stripped; + FILE *fp; + int mustfree = 0; + + if ((name_stripped = strdup(name)) != NULL) + { + remove_dotdotslash(name_stripped); + name = name_stripped; + mustfree = 1; + } if (subdirs) { char* p = strrchr(name, '/'); @@ -95,7 +147,10 @@ static FILE* create_fopen(char* name, char* mode, int subdirs) free (dir_name); } } - return fopen(name, mode); + fp = fopen(name, mode); + if (mustfree) + free(name_stripped); + return fp; } static int unzzip_cat (int argc, char ** argv, int extract) diff --git a/bins/unzzipcat-zip.c b/bins/unzzipcat-zip.c index 2810f85..7f7f3fa 100644 --- a/bins/unzzipcat-zip.c +++ b/bins/unzzipcat-zip.c @@ -69,6 +69,48 @@ static void unzzip_cat_file(ZZIP_DIR* disk, char* name, FILE* out) } } +/* + * NAME: remove_dotdotslash + * PURPOSE: To remove any "../" components from the given pathname + * ARGUMENTS: path: path name with maybe "../" components + * RETURNS: Nothing, "path" is modified in-place + * NOTE: removing "../" from the path ALWAYS shortens the path, never adds to it! + * Also, "path" is not used after creating it. + * So modifying "path" in-place is safe to do. + */ +static inline void +remove_dotdotslash(char *path) +{ + /* Note: removing "../" from the path ALWAYS shortens the path, never adds to it! */ + char *dotdotslash; + int warned = 0; + + dotdotslash = path; + while ((dotdotslash = strstr(dotdotslash, "../")) != NULL) + { + /* + * Remove only if at the beginning of the pathname ("../path/name") + * or when preceded by a slash ("path/../name"), + * otherwise not ("path../name..")! + */ + if (dotdotslash == path || dotdotslash[-1] == '/') + { + char *src, *dst; + if (!warned) + { + /* Note: the first time through the pathname is still intact */ + fprintf(stderr, "Removing \"../\" path component(s) in %s\n", path); + warned = 1; + } + /* We cannot use strcpy(), as there "The strings may not overlap" */ + for (src = dotdotslash+3, dst=dotdotslash; (*dst = *src) != '\0'; src++, dst++) + ; + } + else + dotdotslash +=3; /* skip this instance to prevent infinite loop */ + } +} + static void makedirs(const char* name) { char* p = strrchr(name, '/'); @@ -86,6 +128,16 @@ static void makedirs(const char* name) static FILE* create_fopen(char* name, char* mode, int subdirs) { + char *name_stripped; + FILE *fp; + int mustfree = 0; + + if ((name_stripped = strdup(name)) != NULL) + { + remove_dotdotslash(name_stripped); + name = name_stripped; + mustfree = 1; + } if (subdirs) { char* p = strrchr(name, '/'); @@ -95,7 +147,10 @@ static FILE* create_fopen(char* name, char* mode, int subdirs) free (dir_name); } } - return fopen(name, mode); + fp = fopen(name, mode); + if (mustfree) + free(name_stripped); + return fp; } static int unzzip_cat (int argc, char ** argv, int extract) diff --git a/bins/unzip-mem.c b/bins/unzip-mem.c index c45cb72..ff564a5 100644 --- a/bins/unzip-mem.c +++ b/bins/unzip-mem.c @@ -88,10 +88,49 @@ static void zzip_mem_entry_pipe(ZZIP_MEM_DISK* disk, } } +static inline void +remove_dotdotslash(char *path) +{ + /* Note: removing "../" from the path ALWAYS shortens the path, never adds to it! */ + char *dotdotslash; + int warned = 0; + + dotdotslash = path; + while ((dotdotslash = strstr(dotdotslash, "../")) != NULL) + { + /* + * Remove only if at the beginning of the pathname ("../path/name") + * or when preceded by a slash ("path/../name"), + * otherwise not ("path../name..")! + */ + if (dotdotslash == path || dotdotslash[-1] == '/') + { + char *src, *dst; + if (!warned) + { + /* Note: the first time through the pathname is still intact */ + fprintf(stderr, "Removing \"../\" path component(s) in %s\n", path); + warned = 1; + } + /* We cannot use strcpy(), as there "The strings may not overlap" */ + for (src = dotdotslash+3, dst=dotdotslash; (*dst = *src) != '\0'; src++, dst++) + ; + } + else + dotdotslash +=3; /* skip this instance to prevent infinite loop */ + } +} + static void zzip_mem_entry_make(ZZIP_MEM_DISK* disk, ZZIP_MEM_ENTRY* entry) { - FILE* file = fopen (entry->zz_name, "wb"); + char name_stripped[PATH_MAX]; + FILE* file; + + strncpy(name_stripped, entry->zz_name, PATH_MAX); + remove_dotdotslash(name_stripped); + + file = fopen (name_stripped, "wb"); if (file) { zzip_mem_entry_pipe (disk, entry, file); fclose (file); } perror (entry->zz_name); if (status < EXIT_WARNINGS) status = EXIT_WARNINGS;