New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: prevent extracting archived files outside of target path #374

Merged
merged 1 commit into from May 2, 2018

Conversation

Projects
None yet
4 participants
@odinn1984
Copy link
Contributor

odinn1984 commented May 2, 2018

This PR is meant to fix an arbitrary file write vulnerability, that can be
achieved using a specially crafted zip archive, that holds path traversal
filenames. When the filename gets concatenated to the target extraction
directory, the final path ends up outside of the target folder.

A sample malicious zip file named Zip.Evil.zip was used,
and when running the code below, resulted in the creation of C:/Temp/evil.txt
outside of the intended target directory.

There are various possible ways to avoid this issue, some include checking
for .. (dot dot) characters in the filename, but the best solution in our
opinion is to check if the final target filename, starts with the target
folder (after both are resolved to their absolute path).

Stay secure,
Snyk Team

fix: prevent extracting archived files outside of target path
This PR is meant to fix an arbitrary file write vulnerability, that can be
achieved using a specially crafted zip archive, that holds path traversal
filenames. When the filename gets concatenated to the target extraction
directory, the final path ends up outside of the target folder.

A sample malicious zip file named Zip.Evil.zip was used,
and when running the code below, resulted in the creation of C:/Temp/evil.txt
outside of the intended target directory.

There are various possible ways to avoid this issue, some include checking
for .. (dot dot) characters in the filename, but the best solution in our
opinion is to check if the final target filename, starts with the target
folder (after both are resolved to their absolute path).

Stay secure,
Snyk Team

@adamhathcock adamhathcock merged commit 42b1205 into adamhathcock:master May 2, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@diaconesq

This comment has been minimized.

Copy link

diaconesq commented Jun 22, 2018

I have decompiled version 0.21.1 from Nuget.org (the .net 4.5 dll) and the fix is not there.
image

@majoreffort

This comment has been minimized.

Copy link
Contributor

majoreffort commented Nov 2, 2018

Hi! Don't know if this is the right place, but I ran into a bug related to this fix, or more precisely, the behaviour of the method Path.GetFullPath used here.

I had an edge case scenario where some of the files inside of an archive would have tildes in their filename, e.g. "test~". Somehow, this has an odd influence on the path normalisation on Windows, if the target folder is not given with the proper letter case.

Take the following code snippet as an example:

string dir = "c:/test"; // <- exists in the real filesystem as "C:\Test"
string file = "testfile~";
dir = Path.GetFullPath(dir);
// dir = "c:\\test"
string filepath = Path.Combine(dir, file);
// filepath = "c:\\test\\testfile~"
filepath = Path.GetFullPath(filepath);
// filepath = "c:\\Test\\testfile~" <- note the capital 'T'

The tilde seems to trigger the short-to-long path extension, which accesses the file system and in return "corrects" the case.

Now the check filepath.StartsWith(dir) fails, because the case doesn't match (see

if (!destinationFileName.StartsWith(fullDestinationDirectoryPath))
) and the exception ("Entry is trying to write a file outside of the destination directory.") gets thrown erroneously.

Any idea how to fix this properly without losing cross-platform case sensivity?

@adamhathcock

This comment has been minimized.

Copy link
Owner

adamhathcock commented Nov 2, 2018

Sounds like a possible bug in the Path.GetFullPath code. Is that full framework on windows doing that? Does .NET Core have the same behavior?

It could be trivial to do a case-sensitive filesystem check then do the right thing but it feels wrong to build that if this is a bug.

@majoreffort

This comment has been minimized.

Copy link
Contributor

majoreffort commented Nov 2, 2018

I reproduced it both on .NET Core (2.1) and the full CLR on Windows 10.

CoreCLR just delegates to the Windows API function GetLongPathNameW(https://github.com/dotnet/coreclr/blob/72ee1dacaa5ce68546da9fb9365936ab5a8899b6/src/System.Private.CoreLib/shared/System/IO/PathHelper.Windows.cs#L72). I don't know if we can do anything about that...it may even be accepted behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment