Conversation
fix path traversal bug
|
Resolves #171 |
|
@kratz00 What do you think? |
| /* use GNU extension to return non-existing files to real_output_directory */ | ||
| realpath(output_directory, real_output_directory); | ||
| realpath(filename, real_filename); | ||
| strncat(real_output_directory, "/", PATH_MAX); |
There was a problem hiding this comment.
I think this could overflow real_output_directory as strncat will add a terminating null-character.
If I see it correctly this PR is intended to fix CVE-2015-1386, therefor it might make sense to clean-up/review the existing code which supposedly should already address it and most importantly double check the test in https://github.com/twogood/unshield/tree/main/test/v5/CVE-2015-1386
This is just shooting from the hip, I will have a deeper look as soon as possible.
There was a problem hiding this comment.
So it's the same CVE? I didn't notice. Strange that the old test case passes.
There was a problem hiding this comment.
Saw this and throwing in my two cents.
Looks like it should be strncat(..., PATH_MAX - 1) to avoid the null character causing an off-by-one.
Regarding CVE-2015-1386, #171 looks like a corner case that the original fix missed. In the PoC @eqawasm provided, they appear to be exploiting a common prefix between the real paths: /tm. Suffixing / to real_output_directory prevents the directory name from being misinterpreted as a file name.
There was a problem hiding this comment.
@carter-yagemann summed it up quite nicely 👍
I think the proposed patch for this corner case is not sufficient, even when fixing the off-by-one issue, here is why.
The reproductions steps in #171 can be simplified and do not even require root permissions:
git clone git@github.com:twogood/unshield.git
cd unshield
cmake -B build
cmake --build build
./build/src/unshield -d /tmp x test/v5/CVE-2015-1386/data1.cab
Cabinet: test/v5/CVE-2015-1386/data1.cab
extracting: /tmp/Bovine_Files/../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../../tmp/moo
-------- -------
1 files
real_filename will be always /tmp/moo, in case of this manipulated cabinet file.
For the strncmp call (see https://github.com/twogood/unshield/blob/main/src/unshield.c#L496), we only need to make sure to match the first four bytes at most by influencing the real_output_directory, this can be easily done by using the -d command line option.
In the /tn case adding the slash would work to solve the issue but not for the /tmp case.
4 participants
fix path traversal bug