CVE-2018-18586: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal') (CWE-22)
value: chmextract.c in the chmextract sample program, as distributed with libmspack before 0.8alpha, does not protect against absolute/relative pathnames in CHM files, leading to Directory Traversal. NOTE: the vendor disputes that this is a libmspack vulnerability, because chmextract.c was only intended as a source-code example, not a supported application
CWE description: The product uses external input to construct a pathname that is intended to identify a file or directory that is located underneath a restricted parent directory, but the product does not properly neutralize special elements within the pathname that can cause the pathname to resolve to a location that is outside of the restricted directory.
CVSS v3 base score: 5.3
Published: 2018-10-23T02:29Z
Repository: https://github.com/kyz/libmspack
Language: Other
Files changed: libmspack/ChangeLog, libmspack/src/chmextract.c
Commit date: 2018-10-20 19:06:32 +0100
Commit message: add anti "../" and leading slash protection to chmextract

Diff: diff --git a/libmspack/ChangeLog b/libmspack/ChangeLog
index 848610d..325c160 100644
--- a/libmspack/ChangeLog
+++ b/libmspack/ChangeLog
@@ -1,5 +1,15 @@
+2018-10-20  Stuart Caie <kyzer@cabextract.org.uk>
+
+	* src/chmextract.c: add anti "../" and leading slash protection to
+	chmextract. I'm not pleased about this. All the sample code provided
+	with libmspack is meant to be simple examples of library use, not
+	"productised" binaries. Making the "useful" code samples install
+	as binaries was a mistake. They were never intended to protect you
+	from unpacking archive files with relative/absolute paths, and I
+	would prefer that they never will be.
+
 2018-10-17  Stuart Caie <kyzer@cabextract.org.uk>
 
 	* cab.h: Make the CAB block input buffer one byte larger, to allow
 	a maximum-allowed-size input block and the special extra byte added
 	after the block by cabd_sys_read_block to help Quantum alignment.
diff --git a/libmspack/src/chmextract.c b/libmspack/src/chmextract.c
index 1e03341..b535f0e 100644
--- a/libmspack/src/chmextract.c
+++ b/libmspack/src/chmextract.c
@@ -23,12 +23,10 @@
 # endif
 #endif
 
 mode_t user_umask;
 
-#define FILENAME ".test.chmx"
-
 /**
  * Ensures that all directory components in a filepath exist. New directory
  * components are created, if necessary.
  *
  * @param path the filepath to check
@@ -49,130 +47,26 @@ static int ensure_filepath(char *path) {
     if (!ok) return 0;
   }
   return 1;
 }
 
-/**
- * Creates a UNIX filename from the internal CAB filename and the given
- * parameters.
- *
- * @param fname  the internal CAB filename.
- * @param dir    a directory path to prepend to the output filename.
- * @param lower  if non-zero, filename should be made lower-case.
- * @param isunix if zero, MS-DOS path seperators are used in the internal
- *               CAB filename. If non-zero, UNIX path seperators are used.
- * @param utf8   if non-zero, the internal CAB filename is encoded in UTF8.
- * @return a freshly allocated and created filename, or NULL if there was
- *         not enough memory.
- * @see unix_path_seperators()
- */
-static char *create_output_name(unsigned char *fname, unsigned char *dir,
-			 int lower, int isunix, int utf8)
-{
-  unsigned char *p, *name, c, *fe, sep, slash;
-  unsigned int x;
-
-  sep   = (isunix) ? '/'  : '\\'; /* the path-seperator */
-  slash = (isunix) ? '\\' : '/';  /* the other slash */
-
-  /* length of filename */
-  x = strlen((char *) fname);
-  /* UTF8 worst case scenario: tolower() expands all chars from 1 to 3 bytes */
-  if (utf8) x *= 3;
-  /* length of output directory */
-  if (dir) x += strlen((char *) dir);
-
-  if (!(name = (unsigned char *) malloc(x + 2))) {
-    fprintf(stderr, "out of memory!\n");
-    return NULL;
-  }
-  
-  /* start with blank name */
-  *name = '\0';
-
-  /* add output directory if needed */
-  if (dir) {
-    strcpy((char *) name, (char *) dir);
-    strcat((char *) name, "/");
-  }
-
-  /* remove leading slashes */
-  while (*fname == sep) fname++;
-
-  /* copy from fi->filename to new name, converting MS-DOS slashes to UNIX
-   * slashes as we go. Also lowercases characters if needed.
-   */
-  p = &name[strlen((char *)name)];
-  fe = &fname[strlen((char *)fname)];
-
-  if (utf8) {
-    /* UTF8 translates two-byte unicode characters into 1, 2 or 3 bytes.
-     * %000000000xxxxxxx -> %0xxxxxxx
-     * %00000xxxxxyyyyyy -> %110xxxxx %10yyyyyy
-     * %xxxxyyyyyyzzzzzz -> %1110xxxx %10yyyyyy %10zzzzzz
-     *
-     * Therefore, the inverse is as follows:
-     * First char:
-     *  0x00 - 0x7F = one byte char
-     *  0x80 - 0xBF = invalid
-     *  0xC0 - 0xDF = 2 byte char (next char only 0x80-0xBF is valid)
-     *  0xE0 - 0xEF = 3 byte char (next 2 chars only 0x80-0xBF is valid)
-     *  0xF0 - 0xFF = invalid
-     */
-    do {
-      if (fname >= fe) {
-	free(name);
-	return NULL;
-      }
-
-      /* get next UTF8 char */
-      if ((c = *fname++) < 0x80) x = c;
-      else {
-	if ((c >= 0xC0) && (c < 0xE0)) {
-	  x = (c & 0x1F) << 6;
-	  x |= *fname++ & 0x3F;
-	}
-	else if ((c >= 0xE0) && (c < 0xF0)) {
-	  x = (c & 0xF) << 12;
-	  x |= (*fname++ & 0x3F) << 6;
-	  x |= *fname++ & 0x3F;
-	}
-	else x = '?';
-      }
-
-      /* whatever is the path seperator -> '/'
-       * whatever is the other slash    -> '\\'
-       * otherwise, if lower is set, the lowercase version */
-      if      (x == sep)   x = '/';
-      else if (x == slash) x = '\\';
-      else if (lower)      x = (unsigned int) tolower((int) x);
-
-      /* integer back to UTF8 */
-      if (x < 0x80) {
-	*p++ = (unsigned char) x;
-      }
-      else if (x < 0x800) {
-	*p++ = 0xC0 | (x >> 6);   
-	*p++ = 0x80 | (x & 0x3F);
-      }
-      else {
-	*p++ = 0xE0 | (x >> 12);
-	*p++ = 0x80 | ((x >> 6) & 0x3F);
-	*p++ = 0x80 | (x & 0x3F);
-      }
-    } while (x);
-  }
-  else {
-    /* regular non-utf8 version */
-    do {
-      c = *fname++;
-      if      (c == sep)   c = '/';
-      else if (c == slash) c = '\\';
-      else if (lower)      c = (unsigned char) tolower((int) c);
-    } while ((*p++ = c));
-  }
-  return (char *) name;
+char *create_output_name(char *fname) {
+    char *out, *p;
+    if ((out = malloc(strlen(fname) + 1))) {
+        /* remove leading slashes */
+        while (*fname == '/' || *fname == '\\') fname++;
+        /* if that removes all characters, just call it "x" */
+        strcpy(out, (*fname) ? fname : "x");
+
+        /* change "../" to "xx/" */
+        for (p = out; *p; p++) {
+            if (p[0] == '.' && p[1] == '.' && (p[2] == '/' || p[2] == '\\')) {
+               p[0] = p[1] = 'x';
+            }
+        }
+    }
+    return out;
 }
 
 static int sortfunc(const void *a, const void *b) {
   off_t diff = 
     ((* ((struct mschmd_file **) a))->offset) -
@@ -203,11 +97,11 @@ int main(int argc, char *argv[]) {
 	if ((f = (struct mschmd_file **) calloc(numf, sizeof(struct mschmd_file *)))) {
 	  for (i=0, file=chm->files; file; file = file->next) f[i++] = file;
 	  qsort(f, numf, sizeof(struct mschmd_file *), &sortfunc);
 
 	  for (i = 0; i < numf; i++) {
-	    char *outname = create_output_name((unsigned char *)f[i]->filename,NULL,0,1,0);
+	    char *outname = create_output_name(f[i]->filename);
 	    printf("Extracting %s\n", outname);
 	    ensure_filepath(outname);
 	    if (chmd->extract(chmd, f[i], outname)) {
 	      printf("%s: extract error on \"%s\": %s\n",
 		     *argv, f[i]->filename, ERROR(chmd));

Vulnerable code: #define FILENAME ".test.chmx"

/**
 * Creates a UNIX filename from the internal CAB filename and the given
 * parameters.
 *
 * @param fname  the internal CAB filename.
 * @param dir    a directory path to prepend to the output filename.
 * @param lower  if non-zero, filename should be made lower-case.
 * @param isunix if zero, MS-DOS path seperators are used in the internal
 *               CAB filename. If non-zero, UNIX path seperators are used.
 * @param utf8   if non-zero, the internal CAB filename is encoded in UTF8.
 * @return a freshly allocated and created filename, or NULL if there was
 *         not enough memory.
 * @see unix_path_seperators()
 */
static char *create_output_name(unsigned char *fname, unsigned char *dir,
			 int lower, int isunix, int utf8)
{
  unsigned char *p, *name, c, *fe, sep, slash;
  unsigned int x;

  sep   = (isunix) ? '/'  : '\\'; /* the path-seperator */
  slash = (isunix) ? '\\' : '/';  /* the other slash */

  /* length of filename */
  x = strlen((char *) fname);
  /* UTF8 worst case scenario: tolower() expands all chars from 1 to 3 bytes */
  if (utf8) x *= 3;
  /* length of output directory */
  if (dir) x += strlen((char *) dir);

  if (!(name = (unsigned char *) malloc(x + 2))) {
    fprintf(stderr, "out of memory!\n");
    return NULL;
  }
  
  /* start with blank name */
  *name = '\0';

  /* add output directory if needed */
  if (dir) {
    strcpy((char *) name, (char *) dir);
    strcat((char *) name, "/");
  }

  /* remove leading slashes */
  while (*fname == sep) fname++;

  /* copy from fi->filename to new name, converting MS-DOS slashes to UNIX
   * slashes as we go. Also lowercases characters if needed.
   */
  p = &name[strlen((char *)name)];
  fe = &fname[strlen((char *)fname)];

  if (utf8) {
    /* UTF8 translates two-byte unicode characters into 1, 2 or 3 bytes.
     * %000000000xxxxxxx -> %0xxxxxxx
     * %00000xxxxxyyyyyy -> %110xxxxx %10yyyyyy
     * %xxxxyyyyyyzzzzzz -> %1110xxxx %10yyyyyy %10zzzzzz
     *
     * Therefore, the inverse is as follows:
     * First char:
     *  0x00 - 0x7F = one byte char
     *  0x80 - 0xBF = invalid
     *  0xC0 - 0xDF = 2 byte char (next char only 0x80-0xBF is valid)
     *  0xE0 - 0xEF = 3 byte char (next 2 chars only 0x80-0xBF is valid)
     *  0xF0 - 0xFF = invalid
     */
    do {
      if (fname >= fe) {
	free(name);
	return NULL;
      }

      /* get next UTF8 char */
      if ((c = *fname++) < 0x80) x = c;
      else {
	if ((c >= 0xC0) && (c < 0xE0)) {
	  x = (c & 0x1F) << 6;
	  x |= *fname++ & 0x3F;
	}
	else if ((c >= 0xE0) && (c < 0xF0)) {
	  x = (c & 0xF) << 12;
	  x |= (*fname++ & 0x3F) << 6;
	  x |= *fname++ & 0x3F;
	}
	else x = '?';
      }

      /* whatever is the path seperator -> '/'
       * whatever is the other slash    -> '\\'
       * otherwise, if lower is set, the lowercase version */
      if      (x == sep)   x = '/';
      else if (x == slash) x = '\\';
      else if (lower)      x = (unsigned int) tolower((int) x);

      /* integer back to UTF8 */
      if (x < 0x80) {
	*p++ = (unsigned char) x;
      }
      else if (x < 0x800) {
	*p++ = 0xC0 | (x >> 6);   
	*p++ = 0x80 | (x & 0x3F);
      }
      else {
	*p++ = 0xE0 | (x >> 12);
	*p++ = 0x80 | ((x >> 6) & 0x3F);
	*p++ = 0x80 | (x & 0x3F);
      }
    } while (x);
  }
  else {
    /* regular non-utf8 version */
    do {
      c = *fname++;
      if      (c == sep)   c = '/';
      else if (c == slash) c = '\\';
      else if (lower)      c = (unsigned char) tolower((int) c);
    } while ((*p++ = c));
  }
  return (char *) name;
	    char *outname = create_output_name((unsigned char *)f[i]->filename,NULL,0,1,0);
Fixed code: 2018-10-20  Stuart Caie <kyzer@cabextract.org.uk>

	* src/chmextract.c: add anti "../" and leading slash protection to
	chmextract. I'm not pleased about this. All the sample code provided
	with libmspack is meant to be simple examples of library use, not
	"productised" binaries. Making the "useful" code samples install
	as binaries was a mistake. They were never intended to protect you
	from unpacking archive files with relative/absolute paths, and I
	would prefer that they never will be.

char *create_output_name(char *fname) {
    char *out, *p;
    if ((out = malloc(strlen(fname) + 1))) {
        /* remove leading slashes */
        while (*fname == '/' || *fname == '\\') fname++;
        /* if that removes all characters, just call it "x" */
        strcpy(out, (*fname) ? fname : "x");

        /* change "../" to "xx/" */
        for (p = out; *p; p++) {
            if (p[0] == '.' && p[1] == '.' && (p[2] == '/' || p[2] == '\\')) {
               p[0] = p[1] = 'x';
            }
        }
    }
    return out;
	    char *outname = create_output_name(f[i]->filename);