Tests fail with 'possible traversal attack'

[craftyguy]

I'm trying to build/test unshield on Alpine Linux (with musl libc), and all of the bundled tests fail. I've included the output from running the following manually (as found in the baldur gate test sh script):

./build/src/unshield -D 3 -d extract1 x data1.cab

Output:

unshield_fail.log

[twogood]

Seems we use some GNU extension to realpath that probably is not available in Alpine:

  /* use GNU extension to return non-existing files to real_output_directory */
  realpath(output_directory, real_output_directory);
  realpath(filename, real_filename);
  if (real_filename == NULL || strncmp(real_filename,
                                       real_output_directory,
                                       strlen(real_output_directory)) != 0)
  {
    fprintf(stderr, "\n\nExtraction failed.\n");
    fprintf(stderr, "Possible directory traversal attack for: %s\n", filename);
    fprintf(stderr, "To be placed at: %s\n\n", real_filename);
    success = false;
    goto exit;
  }

GNU extensions If the call fails with either EACCES or ENOENT and resolved_path is not NULL, then the prefix of path that is not readable or does not exist is returned in resolved_path.

[twogood]

Please try with the latest master @craftyguy !

[craftyguy]

@twogood works great, thanks for the quick fix!

I'm packaging this for Alpine Linux, is there any chance you could tag a new release so I can package a specific release rather than 'master'?

[craftyguy]

Oof, spoke too soon. One test is failing and referencing #42

/home/pmos/build/src/twogood-unshield-f097b07 # UNSHIELD=$(pwd)/build/src/unshield bash run-tests.sh
Running test ./test/v0/wireplay.sh...succeeded
Running test ./test/v0/the-feeble-files-spanish.sh...succeeded
Running test ./test/v0/baldurs_gate_patch_v1_1_4315_international.sh...succeeded
Running test ./test/v0/avigomanager.sh...succeeded
Running test ./test/v5/CVE-2015-1386/CVE-2015-1386.sh...Cabinet: /home/pmos/build/src/twogood-unshield-f097b07/test/v5/CVE-2015-1386/data1.cab
  extracting: extract1/Bovine_Files/../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../tmp/moo
 --------  -------
          1 files
unshield vulnerable to CVE-2015-1386
See https://github.com/twogood/unshield/issues/42
FAILED with code 2

That issue seems to indicate this was resolved for debian on some earlier version of unshield, but since it has to do with some traversal stuff maybe it's not fully resolved for Alpine/musl libc?

[craftyguy]

FWIW, realpath does appear to be in POSIX (and therefore supported by musl): https://pubs.opengroup.org/onlinepubs/9699919799/

A simple 'hello world' compiles/runs with musl:

#include <limits.h>
#include <stdio.h>
#include <stdlib.h>
int main(void) {
    char b[PATH_MAX];
    char *r = realpath("test.c", b);
    if (r) {
        printf("%s.\n", b);
    } else {
        perror("realpath");
        exit(EXIT_FAILURE);
    }
    return 0;
}

~ $ gcc test.c -o test
~ $ ldd test
        /lib/ld-musl-aarch64.so.1 (0)
        libc.musl-aarch64.so.1 => /lib/ld-musl-aarch64.so.1 (0)
~ $ ./test
/home/pmos/test.c.

[twogood]

Yes, realpath() is POSIX but there is a GNU extension to support files that do not exist, see bottom of my comment https://github.com/twogood/unshield/issues/99#issuecomment-584502506

[craftyguy]

@twogood oops, I missed that somehow when I first read your comment, sorry about that!

So there must be something else causing that test to fail when built with musl?

[twogood]

Well on master I disabled the path traversal check unless you are on GNU libc. "Damned if you, damned if you don't..." ;)

[twogood]

I will create a proper release when we have sorted this out. I want a path traversal check but I don't want to reimplement realpath :)

[craftyguy]

I see. There was a patch submitted for #42 that doesn't appear to rely on GNU extensions to realpath (https://github.com/twogood/unshield/pull/49/commits/10279229a660e2e1d984ab2aae6a26b664d228ba), but comment in that merge request seems to suggest that approach doesn't work. I don't know enough about this stuff to be able to help much, and definitely don't want to re-introduce a CVE into this project by recommending the wrong thing :(

[twogood]

Unfortunately it still seems to rely on the GNU extension to realpath but this must have been an issue on Alpine for much bigger projects than unshield...

[craftyguy]

@twogood I thought about this some more, and talked with some folks on the #musl IRC channel to see what they thought such a check would look like ¹

Are there any cases where having a '..' in the path for an object in a .cab is valid? If there's no case for that, then it seems like 1) checking that the directory to extract into is writable for the current user, and 2) that the file path of the file to extract does not contain any '..'

¹ A (somewhat biased) response was along the lines of "using realpath this way is clearly not the intended purpose of realpath, in glibc or otherwise" (paraphrasing is mine), but I'd rather focus on solving the issue and not derailing the discussion to focus on that bit :)

[twogood]

I don't know if .. is valid but of course, it's pretty easy to add a check for that.

[wdlkmpx]

I just came across this issue, I cross compiled a musl static unshield

The problem with musl is that there's no way to identify it, no macros, the musl devs are insane.

[wdlkmpx]

Guys let's collect .cab files, all the InstallShield files I've checked so far do not contain / or \..\. / is an invalid char in Windows, can't be present in paths.

The paths seem to follow a fixed pattern, no special commands for relative paths, I mean: no \..\or ..\ (at the start)

Cabinet: data1.cab
     6332  Xp64Driver-C1_SWE\brimb3ex.cat
    31173  Xp64Driver-C1_SWE\brprbh3e.cat
     6332  Xp64Driver-C1_NOR\brimb3ex.cat
    31173  Xp64Driver-C1_NOR\brprbh3e.cat
    47672  PCFAX_DOC_FRE\PC_FAX32.chm
    12313  PCFAX_DOC_FRE\BrotherAtYourLogo.jpg
    19746  PCFAX_DOC_FRE\PCFAXsetup.jpg
    24028  PCFAX_DOC_FRE\Phone.jpg
    15942  PCFAX_DOC_FRE\Phonesmall.jpg
    21238  PCFAX_DOC_FRE\driver.jpg
    18056  PCFAX_DOC_FRE\enablePCfax.jpg
     3947  PCFAX_DOC_FRE\howtousebrotherpc.htm
    11498  PCFAX_DOC_FRE\simplesmall.jpg
    31326  <Support>Swedish String Tables\StringTable-001d-Swedish.ips
    25570  <Support>Portuguese (Standard) String Tables\StringTable-0816-Portuguese (Standard).ips
   143111  Twainx64_UK\BRS06LNG.chm
    81920  Twainx64_UK\BrTwdLNG.dll

The only surprise I've seem so far is <Support>....

Please share your .cab files with me, I'll implement a fix without realpath, just simple logic with strings. Remember these .cab files are created by a Windows application for MS Windows, and the paths don't contain special 'commands'.

Need a repository to store .cab files for testing

[wdlkmpx]

I produced an extra workaround for CVE-2015-1386 that will fix a test failure with everything other than gcc+linux See commit diff & message https://github.com/wdlkmpx/unshield/commit/46a91730086f8da69750356b4522d40aca0b6102 My repo has interesting stuff

unshield with a static musl:

# file src/unshield
src/unshield: ELF 32-bit LSB executable, Intel 80386, version 1 (SYSV), statically linked, not stripped

# ./run-tests.sh 
------------------------------------------------
UNSHIELD=/mnt/sda3/pkg2/unshield_static/unshield/src/unshield
------------------------------------------------
Running test ./test/v0/avigomanager.sh...succeeded
Running test ./test/v0/baldurs_gate_patch_v1_1_4315_international.sh...succeeded
Running test ./test/v0/the-feeble-files-spanish.sh...succeeded
Running test ./test/v0/wireplay.sh...succeeded
Running test ./test/v5-CVE-2015-1386/CVE-2015-1386.sh...succeeded

[twogood]

Good workaround!

[wdlkmpx]

I optmized the change I made to extract_file() in the commit linked above, Buit I don't want to trigger a new Travis CI build in the Pull Request until I have new important changes to commit, so I'll paste the diff here so you can analyze it

The app(s) that create InstallShield CAB files, will never insert something like \..\ or /../ in the path, so it's pretty safe to assume it's malicious. It's not a TAR file that really contains 'commands' to relative paths. I'll inspect as many cab files as I can to verify my claim... the realpath stuff might not be necessary...

diff --git a/src/unshield.c b/src/unshield.c
index ebbc4a3..77c19ea 100644
--- a/src/unshield.c
+++ b/src/unshield.c
@@ -400,15 +400,15 @@ static int is_traversal_attack (const char *name)
 static bool extract_file(Unshield* unshield, const char* prefix, int index)
 {
   bool success = false;
-  char* dirname;
-  char* filename;
+  char* dirname = NULL;
+  char* filename = NULL;
   const char* origdir  = NULL;
   const char* basename = NULL;
-  char* p;
+  char* p = NULL;
   int directory = unshield_file_directory(unshield, index);
   size_t path_max;
-  char* real_output_directory;
-  char* real_filename;
+  char* real_output_directory = NULL;
+  char* real_filename = NULL;
   int traversal_attack = 0;
 
   #ifdef PATH_MAX
@@ -419,17 +419,6 @@ static bool extract_file(Unshield* unshield, const char* prefix, int index)
       path_max = 4096;
   #endif
 
-  real_output_directory = malloc(path_max);
-  real_filename = malloc(path_max);
-  dirname = malloc(path_max);
-  filename = malloc(path_max);
-  if (real_output_directory == NULL || real_filename == NULL)
-  {
-    fprintf(stderr,"Unable to allocate memory.");
-    success=false;
-    goto exit;
-  }
-
   /*
      filename: dir2extract1/Language_Independant/Override/xan6.CRE
             -d output_dir           prefix       origdir  basename
@@ -448,6 +437,17 @@ static bool extract_file(Unshield* unshield, const char* prefix, int index)
      goto exit;
   }
 
+  real_output_directory = malloc(path_max);
+  real_filename = malloc(path_max);
+  dirname = malloc(path_max);
+  filename = malloc(path_max);
+  if (real_output_directory == NULL || real_filename == NULL)
+  {
+    fprintf(stderr,"Unable to allocate memory.");
+    success=false;
+    goto exit;
+  }
+
   if(strlen(output_directory) < path_max-1)
   {
     strncpy(dirname, output_directory,path_max-1);
@@ -589,23 +589,25 @@ exit:
   if (traversal_attack)
   {
     fprintf(stderr, "\n\nExtraction failed.\n");
-    fprintf(stderr, "Error: %s (%d).\n", strerror(errno), errno);
-    fprintf(stderr, "Possible directory traversal attack for: %s\n", filename);
-    fprintf(stderr, "To be placed at: %s\n\n", real_filename);
+    fprintf(stderr, "Possible directory traversal attack ");
+    if (filename) {
+       fprintf(stderr, " for: %s\n", filename);
+       fprintf(stderr, "Error: %s (%d).\n", strerror(errno), errno);
+       fprintf(stderr, "To be placed at: %s\n\n", real_filename);
+    }
     success = false;
   }
   if (!success)
   {
-    fprintf(stderr, "Failed to extract file '%s'.%s\n",
-        unshield_file_name(unshield, index),
-        (log_level < 3) ? "Run unshield again with -D 3 for more information." : "");
+    fprintf(stderr, "Failed to extract file '%s'.%s\n", basename,
+            (log_level < 3) ? "Run unshield again with -D 3 for more information." : "");
     unlink(filename);
     exit_status = 1;
   }
-  free(real_filename);
-  free(real_output_directory);
-  free(dirname);
-  free(filename);
+  FREE (real_filename);
+  FREE (real_output_directory);
+  FREE (dirname);
+  FREE (filename);
   return success;
 }
 

[wdlkmpx]

My assumptions were correct, the realpath fix should be removed, that fix itself can be considered a bug.

The string to check is the full path not including the output directory (-d dir), I see the fix for traversal attacks in unace, it just checks the dots and the dir separator, a simplified version:

static int is_directory_traversal(char *str)
{
  if (*str == '.' && (!strncmp(str,"../",3) || !strncmp(str,"..\\",3))) {
      return 1;
  }
  if (strstr(str, "/../")  || strstr(str, "\\..\\")) {
      return 1;
  }
  return 0;
}

Just like ace/WinAce, InstallShield is a closed source Windows app, so the same logic applies, and the app should print and work with \ separator, it should only be changed to / when producing output files (extracted files/dirs). Because there is a special dir traversal attack that is hard to address, but this is the fix I came up with:

   if (extract)
   {
      // '/': UNIX-specific attack
      //  - ACE32.EXE creates:
      //       man\man8\e2mmpstatus.8
      //  - tests/dirtraversal2.ace:
      //       /tmp/unace-dir-traversal
      //       ('/' must not be present in the string)
     if (is_directory_traversal(s) || strchr(s,'/')) {
        f_err = ERR_WRITE;
        printf("\n    Directory traversal attempt:  %s\n", s);
        return NULL;
     }
   }