Tests fail with 'possible traversal attack' #99
Comments
|
Seems we use some GNU extension to
|
|
Please try with the latest master @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'? |
|
Oof, spoke too soon. One test is failing and referencing #42 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? |
|
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: |
|
Yes, |
|
@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? |
|
Well on master I disabled the path traversal check unless you are on GNU libc. "Damned if you, damned if you don't..." ;) |
|
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 :) |
|
I see. There was a patch submitted for #42 that doesn't appear to rely on GNU extensions to realpath (1027922), 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 :( |
|
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... |
|
@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 :) |
|
I don't know if |
|
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. |
|
Guys let's collect .cab files, all the InstallShield files I've checked so far do not contain The paths seem to follow a fixed pattern, no special commands for relative paths, I mean: no The only surprise I've seem so far is Please share your .cab files with me, I'll implement a fix without Need a repository to store .cab files for testing |
|
I produced an extra workaround for CVE-2015-1386 that will fix a test failure with everything other than gcc+linux unshield with a static musl: |
|
Good workaround! |
|
I optmized the change I made to extract_file() in the commit linked above, The app(s) that create InstallShield CAB files, will never insert something like 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;
}
|
|
My assumptions were correct, the The string to check is the full path not including the output directory (-d dir), I see the fix for traversal attacks in 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 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;
}
} |
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.cabOutput:
unshield_fail.log
The text was updated successfully, but these errors were encountered: