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

APK with relative path entries can't be extracted #1498

Closed
mkilling opened this Issue May 9, 2017 · 8 comments

Comments

Projects
None yet
3 participants
@mkilling
Contributor

mkilling commented May 9, 2017

I'm having a problem extracting an APK that contains entries with relative paths. One entry in the APK/Zip file is ../assets/0/logo.png. This actually seems to be a legit thing to do for zip archives.

I want to provide a pull request that will just ignore entries like these when extracting the APK archive, but I want to make sure beforehand that the maintainers agree that this is a case that should be handled within Apktool.

Information

  1. Apktool Version (apktool -version) - 2.2.2, but also happens in current master, commit ea16f3f
  2. Operating System (Mac, Linux, Windows) - macOS Sierra 10.12.4
  3. APK From? (Playstore, ROM, Other) - Other

Stacktrace/Logcat

Exception in thread "main" brut.androlib.AndrolibException: brut.directory.DirectoryException: Error copying file: ../assets/0/logo.png
	at brut.androlib.Androlib.decodeUnknownFiles(Androlib.java:217)
	at brut.androlib.ApkDecoder.decode(ApkDecoder.java:158)
	at brut.apktool.Main.cmdDecode(Main.java:166)
	at brut.apktool.Main.main(Main.java:81)
Caused by: brut.directory.DirectoryException: Error copying file: ../assets/0/logo.png
	at brut.directory.DirUtil.copyToDir(DirUtil.java:88)
	at brut.directory.AbstractDirectory.copyToDir(AbstractDirectory.java:207)
	at brut.androlib.Androlib.decodeUnknownFiles(Androlib.java:210)
	... 3 more
Caused by: java.io.FileNotFoundException: apk_with_relative_paths/unknown/../assets/0/logo.png (No such file or directory)
	at java.io.FileOutputStream.open0(Native Method)
	at java.io.FileOutputStream.open(FileOutputStream.java:270)
	at java.io.FileOutputStream.<init>(FileOutputStream.java:213)
	at java.io.FileOutputStream.<init>(FileOutputStream.java:162)
	at brut.directory.DirUtil.copyToDir(DirUtil.java:84)
	... 5 more

Steps to Reproduce

  1. apktool d apk_with_relative_paths.apk

Frameworks

If this APK is from an OEM ROM (Samsung, HTC, LG). Please attach framework files
(.apks that live in /system/framework or /system/priv-app)
No

APK

Unfortunately I can't share the original APK with the problem, but I crafted an APK illustrating the problem. I think that this APK cannot be installed on real devices because I didn't codesign it: https://www.dropbox.com/s/fgx2gelpewflfwy/apk_with_relative_paths.apk?dl=1

Questions to ask before submission

  1. Have you tried apktool d, apktool b without changing anything?
    yes
  2. If you are trying to install a modified apk, did you resign it?
    n/a
  3. Are you using the latest apktool version?
    yes
@iBotPeaches

This comment has been minimized.

Show comment
Hide comment
@iBotPeaches

iBotPeaches May 9, 2017

Owner

Interesting. Definitely the first time I've heard about this. Let me get home and test this out on a real device and go from there. Thanks for the report and example application to make this easier. Pending that, I'm not sure what our course of action would be so can't advise anything just yet.

The only two options off the top of my head.

  1. Follow the relative path and keep file at the parsed relative path? IE ../assets/0/logo.png, will just go back to / and put the resource in assets/0/logo.png.
  2. Ignore the resource completely during decode.
Owner

iBotPeaches commented May 9, 2017

Interesting. Definitely the first time I've heard about this. Let me get home and test this out on a real device and go from there. Thanks for the report and example application to make this easier. Pending that, I'm not sure what our course of action would be so can't advise anything just yet.

The only two options off the top of my head.

  1. Follow the relative path and keep file at the parsed relative path? IE ../assets/0/logo.png, will just go back to / and put the resource in assets/0/logo.png.
  2. Ignore the resource completely during decode.
@mkilling

This comment has been minimized.

Show comment
Hide comment
@mkilling

mkilling May 9, 2017

Contributor

I think the most desirable solution would be one that unzips these resources safely and then stores them in the APK again in subsequent apktool b executions.

However, just ignoring the resources will be easier to implement, and I don‘t expect it to cause practical problems.

Contributor

mkilling commented May 9, 2017

I think the most desirable solution would be one that unzips these resources safely and then stores them in the APK again in subsequent apktool b executions.

However, just ignoring the resources will be easier to implement, and I don‘t expect it to cause practical problems.

mkilling added a commit to playtestcloud/Apktool that referenced this issue May 9, 2017

@peret

This comment has been minimized.

Show comment
Hide comment
@peret

peret May 12, 2017

Maybe this is interesting: I think copying the assets file actually works. But then it also tries to copy the same asset file as an "unknown" file. (See the path in the stack trace).
apktool probably does that because of the way it checks for the default APK files:
https://github.com/iBotPeaches/Apktool/blob/v2.2.2/brut.apktool/apktool-lib/src/main/java/brut/androlib/Androlib.java#L188

The path "assets/0/logo.png" would be recognized as being inside the standard assets folder, but the relative path "../assets/0/logo.png" isn't, because it fails the second check in that loop.

Maybe this offers an even easier way to fix it, without discarding the files?

peret commented May 12, 2017

Maybe this is interesting: I think copying the assets file actually works. But then it also tries to copy the same asset file as an "unknown" file. (See the path in the stack trace).
apktool probably does that because of the way it checks for the default APK files:
https://github.com/iBotPeaches/Apktool/blob/v2.2.2/brut.apktool/apktool-lib/src/main/java/brut/androlib/Androlib.java#L188

The path "assets/0/logo.png" would be recognized as being inside the standard assets folder, but the relative path "../assets/0/logo.png" isn't, because it fails the second check in that loop.

Maybe this offers an even easier way to fix it, without discarding the files?

@iBotPeaches

This comment has been minimized.

Show comment
Hide comment
@iBotPeaches

iBotPeaches May 15, 2017

Owner

Took a look this morning. Interesting failure, because of the relative ../, it never creates the unknown folder. It attempts to do so at outFile.getParentFile().mkdirs(), which then makes app/unknown/../assets/0/logo.png into app/assets/0/logo.png, which isn't right.

I feel like if we ignore these resources, it will become an obvious solution for authors to create applications dumping resources into relative path directories knowing apktool will ignore them.

I'm also not aware of a make directories command that treats each directory depth literally, ie makes unknown, then a folder called .. in unknown, then a folder called assets inside .., and so on. So more research when I have more time.

Owner

iBotPeaches commented May 15, 2017

Took a look this morning. Interesting failure, because of the relative ../, it never creates the unknown folder. It attempts to do so at outFile.getParentFile().mkdirs(), which then makes app/unknown/../assets/0/logo.png into app/assets/0/logo.png, which isn't right.

I feel like if we ignore these resources, it will become an obvious solution for authors to create applications dumping resources into relative path directories knowing apktool will ignore them.

I'm also not aware of a make directories command that treats each directory depth literally, ie makes unknown, then a folder called .. in unknown, then a folder called assets inside .., and so on. So more research when I have more time.

@mkilling

This comment has been minimized.

Show comment
Hide comment
@mkilling

mkilling May 15, 2017

Contributor

I think there are two separate issues here:

(A) assets/../assets/0/logo.png - relative path specs that stay inside the directory hierarchy. You can probably change ZipRODirectory to support this. I however don‘t know if it is actually legal for zip files to contain path specs like this.

However, the APK file I attached is highlighting a different problem:

(B) ../assets/0/logo.png - a relative path spec that points outside of the directory hierarchy in the zip file. As far as I understand, Android will just ignore these files when installing the APK (I don't have proof though). The unzip tool on macOS by default ignores these files as well.

I feel that a valid approach going forward is to focus on scenario (B) for now and to just ignore files if they point outside of the directory hierarchy. We should continue to fail when encountering scenario (A). This is my proposal to implement this: playtestcloud@693f592

Contributor

mkilling commented May 15, 2017

I think there are two separate issues here:

(A) assets/../assets/0/logo.png - relative path specs that stay inside the directory hierarchy. You can probably change ZipRODirectory to support this. I however don‘t know if it is actually legal for zip files to contain path specs like this.

However, the APK file I attached is highlighting a different problem:

(B) ../assets/0/logo.png - a relative path spec that points outside of the directory hierarchy in the zip file. As far as I understand, Android will just ignore these files when installing the APK (I don't have proof though). The unzip tool on macOS by default ignores these files as well.

I feel that a valid approach going forward is to focus on scenario (B) for now and to just ignore files if they point outside of the directory hierarchy. We should continue to fail when encountering scenario (A). This is my proposal to implement this: playtestcloud@693f592

@iBotPeaches

This comment has been minimized.

Show comment
Hide comment
@iBotPeaches

iBotPeaches Jun 9, 2017

Owner

Sorry for the delay. (A) looks like some research I need to do whether its legal in the zip file to do such a thing.

As for (B), throw up a PR with this commit playtestcloud@693f592 and I'll get it merged. I'll add a test apk to prevent regression and we can get this merged.

Owner

iBotPeaches commented Jun 9, 2017

Sorry for the delay. (A) looks like some research I need to do whether its legal in the zip file to do such a thing.

As for (B), throw up a PR with this commit playtestcloud@693f592 and I'll get it merged. I'll add a test apk to prevent regression and we can get this merged.

@mkilling

This comment has been minimized.

Show comment
Hide comment
@mkilling

mkilling Jun 9, 2017

Contributor

Pull request here: #1530

I already attached a test APK, but feel free to replace it with yours if you think that‘s better.

Contributor

mkilling commented Jun 9, 2017

Pull request here: #1530

I already attached a test APK, but feel free to replace it with yours if you think that‘s better.

@iBotPeaches

This comment has been minimized.

Show comment
Hide comment
@iBotPeaches

iBotPeaches Jun 9, 2017

Owner

I'll just check the apk to make sure there isn't anything copyrighted that I can't include and get this merged. This will make it into the 2.2.3 release I'm planning for Mon/Tue.

Thanks for quick response.

Owner

iBotPeaches commented Jun 9, 2017

I'll just check the apk to make sure there isn't anything copyrighted that I can't include and get this merged. This will make it into the 2.2.3 release I'm planning for Mon/Tue.

Thanks for quick response.

iBotPeaches added a commit that referenced this issue Jun 11, 2017

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