Skip to content
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

arbitrary file over write #453

Closed
turingH opened this issue May 12, 2018 · 12 comments
Closed

arbitrary file over write #453

turingH opened this issue May 12, 2018 · 12 comments

Comments

@turingH
Copy link

@turingH turingH commented May 12, 2018

I'm a security researcher from pwnzen at Shanghai,China.
First of all, thank you for your sharing .

We found that

+ (BOOL)unzipFileAtPath:(NSString *)path
          toDestination:(NSString *)destination
     preserveAttributes:(BOOL)preserveAttributes
              overwrite:(BOOL)overwrite
         nestedZipLevel:(NSInteger)nestedZipLevel
               password:(nullable NSString *)password
                  error:(NSError **)error
               delegate:(nullable id<SSZipArchiveDelegate>)delegate
        progressHandler:(void (^_Nullable)(NSString *entry, unz_file_info zipInfo, long entryNumber, long total))progressHandler
      completionHandler:(void (^_Nullable)(NSString *path, BOOL succeeded, NSError * _Nullable error))completionHandler
{
[...]
if ([strPath rangeOfCharacterFromSet:[NSCharacterSet characterSetWithCharactersInString:@"/\\"]].location != NSNotFound) {
    strPath = [strPath stringByReplacingOccurrencesOfString:@"\\" withString:@"/"];
}          
    NSString *fullPath = [destination stringByAppendingPathComponent:strPath];
[...]
}

function may lead to arbitrary file over write

In case of holding path traversal filenames in the archive, strPath should be checked for whether it contains "../../../".

@ChiChou
Copy link

@ChiChou ChiChou commented May 15, 2018

And this library also supports symbolic link creation, which may cause security issue under certain circumstances

@dustturtle
Copy link

@dustturtle dustturtle commented May 16, 2018

Any update?

@Proteas Proteas mentioned this issue May 16, 2018
@fangli
Copy link

@fangli fangli commented May 16, 2018

AFAIK, supporting unrestricted parent path is a well-known feature in zip, tar or 7z protocol.

don't understand why a zip library should handle this but indeed, lack of knowing this leads to unexpected things, thus arbitrary code execution.

@ganvinalix
Copy link

@ganvinalix ganvinalix commented May 16, 2018

盘古的大神,你们应该是 宣传自己的安全工具吧 @turingH

@zhangyinglong
Copy link

@zhangyinglong zhangyinglong commented May 16, 2018

@fangli
https://zipperdown.org/
I guess this case is because the unzipped file replaces the original legal javascript script, and the script runs without a security check, so runs the attacker's script.
so this code execution needs to be fixed.

The best solution is checking script of security before runs it.

@Naville
Copy link

@Naville Naville commented May 16, 2018

fangli has a valid point. In my humble opinion I'd say we keep the original implementation as-is without extra filtering so as not to break backward compatibility, however we should probably implement new methods that explicitly handle the situation

@bingri
Copy link

@bingri bingri commented May 16, 2018

Agree with fangli. This is a feature, not a bug.
In that case, safety problem was caused by the hot-fix service or the way of downloading remote file.
Not caused by this Zip library.

@maxfong
Copy link

@maxfong maxfong commented May 16, 2018

@Naville 哪都能看见你 ^. ^

@SilverMoonSecurity
Copy link

@SilverMoonSecurity SilverMoonSecurity commented May 18, 2018

@turingH
This would be feature by design.
Or path traversal abuse should be filtered or restricted by develop caller instead of the third party tool.

@ChiChou
Copy link

@ChiChou ChiChou commented May 18, 2018

@SilverMoonSecurity but obviously this module doesn't provide such api, it just extract everything as is.

@nabla-c0d3
Copy link

@nabla-c0d3 nabla-c0d3 commented May 19, 2018

The proposed fix in #456 is the right approach, I think.

Overwriting files outside of the folder where the archive is getting unzipped is very unexpected (even if "valid" when it comes to what the zip format allows) and will surprise most users and developers, resulting in security issues.

This is why most (if not all) popular archiving tool, such as the built-in tools on MacOS and Windows, do not support this behavior and never create files outside of the selected folder.

The fix should make the library safe by default, with an option to override the security feature and restore the original behavior (which allows file overwrite). If you do it the other way around, none of the vulnerable apps will make the code change needed to enable security.

@jhudsonWA
Copy link
Member

@jhudsonWA jhudsonWA commented May 30, 2018

2.1.3 released

@jhudsonWA jhudsonWA closed this May 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet