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

Strings inside xml resource files are truncated #681

Closed
iBotPeaches opened this Issue Mar 18, 2015 · 8 comments

Comments

Projects
None yet
1 participant
@iBotPeaches
Owner

iBotPeaches commented Mar 18, 2015

Original issue 571 created by jtmuhone on 2013-12-17T11:16:55.000Z:

What steps will reproduce the problem?

  1. Download an apk, e.g. https://www.dropbox.com/s/2rnd5a7fbx1ztbo/com.camelgames.fantasyland_1.17.1.apk
  2. Use apktool to decompire: apktool d com.camelgames.fantasyland_1.17.1.apk -o com.camelgames.fantasyland_1.17.1
  3. Check a generated file, e.g. res/xml/rockman.xml

What is the expected output? What do you see instead?

The JSON strings inside file are truncated. This file ends with '["rot",[[0,0],</string>'. The xml is valid, but the JSON inside the XML is not. If you build the application, there are no errors, but the application does not work.

What version of the product are you using? On what operating system?
2.0.0-Beta8 on Ubuntu 13.10

Please provide any additional information below.

@iBotPeaches

This comment has been minimized.

Show comment
Hide comment
@iBotPeaches

iBotPeaches Mar 18, 2015

Owner

Comment #1 originally posted by connor.tumbleson on 2013-12-17T12:45:15.000Z:

Okay,

I'll add a unit-test for this and see why our parser is stripping it.

Owner

iBotPeaches commented Mar 18, 2015

Comment #1 originally posted by connor.tumbleson on 2013-12-17T12:45:15.000Z:

Okay,

I'll add a unit-test for this and see why our parser is stripping it.

@iBotPeaches

This comment has been minimized.

Show comment
Hide comment
@iBotPeaches

iBotPeaches Mar 18, 2015

Owner

Comment #2 originally posted by jtmuhone on 2013-12-17T13:32:17.000Z:

I think the error might be related to the StringBlock string length calculation.

Owner

iBotPeaches commented Mar 18, 2015

Comment #2 originally posted by jtmuhone on 2013-12-17T13:32:17.000Z:

I think the error might be related to the StringBlock string length calculation.

@iBotPeaches

This comment has been minimized.

Show comment
Hide comment
@iBotPeaches

iBotPeaches Mar 18, 2015

Owner

Comment #3 originally posted by connor.tumbleson on 2013-12-17T22:00:41.000Z:

So the JSON was valid prior to apktool? Apktool then malformed it? I put a small valid JSON test-case into a string and it ran through fine.

Owner

iBotPeaches commented Mar 18, 2015

Comment #3 originally posted by connor.tumbleson on 2013-12-17T22:00:41.000Z:

So the JSON was valid prior to apktool? Apktool then malformed it? I put a small valid JSON test-case into a string and it ran through fine.

@iBotPeaches

This comment has been minimized.

Show comment
Hide comment
@iBotPeaches

iBotPeaches Mar 18, 2015

Owner

Comment #4 originally posted by jtmuhone on 2013-12-17T22:29:31.000Z:

It is. I downloaded the apk, installed it via adb and the app works.
Then I decompiled it using apktool, built it again without any modification and the app crashes. From logcat I could see an JSON parse exception and the error message says that just that JSON string is not valid. I also unzipped the apk, and the string inside the binary xml file is ok.

Could there be something special with this apk? A new string format? Even the broken string is quite long, ~16000 bytes.

Owner

iBotPeaches commented Mar 18, 2015

Comment #4 originally posted by jtmuhone on 2013-12-17T22:29:31.000Z:

It is. I downloaded the apk, installed it via adb and the app works.
Then I decompiled it using apktool, built it again without any modification and the app crashes. From logcat I could see an JSON parse exception and the error message says that just that JSON string is not valid. I also unzipped the apk, and the string inside the binary xml file is ok.

Could there be something special with this apk? A new string format? Even the broken string is quite long, ~16000 bytes.

@iBotPeaches

This comment has been minimized.

Show comment
Hide comment
@iBotPeaches

iBotPeaches Mar 18, 2015

Owner

Comment #5 originally posted by connor.tumbleson on 2013-12-17T22:50:06.000Z:

Using aapt d strings apk.apk

I found tons of strings in the form of

as by aapt
{id:65538,v:2,tid:20003,mst:1,x:-1,y:-1,a:6000,b:3000,lm:{chp:1000,rep:0,bt:0,mp:[[101,0,1,0],[101,0,1,1],[101,0,1,2],[101,0,1,3],[101,0,1,4],[101,0,1,5],[100,0,0,0],[100,0,0,1],[100,0,0,2],[100,0,0,3],[100,0,0,4],[100,0,0,5]]},rm:{chp:1000,rep:0,bt:0,mp:[[100,0,1,0],[100,0,1,1],[100,0,1,2],[100,0,1,3],[100,0,1,4],[100,0,1,5],[101,0,0,0],[101,0,0,1],[101,0,0,2],[101,0,0,3],[101,0,0,4],[101,0,0,5]]}}

post decode
{id:65538,v:2,tid:20003,mst:1,x:-1,y:-1,a:6000,b:3000,lm:{chp:1000,rep:0,bt:0,mp:[[101,0,1,0],[101,0,1,1],[101,0,1,2],[101,0,1,3],[101,0,1,4],[101,0,1,5],[100,0,0,0],[100,0,0,1],[100,0,0,2],[100,0,0,3],[100,0,0,4],[100,0,0,5]]},rm:{chp:1000,rep:0,bt:0,mp:[[100,0,1,0],[100,0,1,1],[100,0,1,2],[100,0,1,3],[100,0,1,4],[100,0,1,5],[101,0,0,0],[101,0,0,1],[101,0,0,2],[101,0,0,3],[101,0,0,4],[101,0,0,5]]}}

The JSON input is valid according to RFC 4627 (JSON specfication). I ran it through and got it right back. This was pulled directly from that APK. You original report shows a JSON segment with double quotes. Double quotes and strings don't play nice with Android without escaping.

(Note: I haven't had time to do any real device testing yet)

Owner

iBotPeaches commented Mar 18, 2015

Comment #5 originally posted by connor.tumbleson on 2013-12-17T22:50:06.000Z:

Using aapt d strings apk.apk

I found tons of strings in the form of

as by aapt
{id:65538,v:2,tid:20003,mst:1,x:-1,y:-1,a:6000,b:3000,lm:{chp:1000,rep:0,bt:0,mp:[[101,0,1,0],[101,0,1,1],[101,0,1,2],[101,0,1,3],[101,0,1,4],[101,0,1,5],[100,0,0,0],[100,0,0,1],[100,0,0,2],[100,0,0,3],[100,0,0,4],[100,0,0,5]]},rm:{chp:1000,rep:0,bt:0,mp:[[100,0,1,0],[100,0,1,1],[100,0,1,2],[100,0,1,3],[100,0,1,4],[100,0,1,5],[101,0,0,0],[101,0,0,1],[101,0,0,2],[101,0,0,3],[101,0,0,4],[101,0,0,5]]}}

post decode
{id:65538,v:2,tid:20003,mst:1,x:-1,y:-1,a:6000,b:3000,lm:{chp:1000,rep:0,bt:0,mp:[[101,0,1,0],[101,0,1,1],[101,0,1,2],[101,0,1,3],[101,0,1,4],[101,0,1,5],[100,0,0,0],[100,0,0,1],[100,0,0,2],[100,0,0,3],[100,0,0,4],[100,0,0,5]]},rm:{chp:1000,rep:0,bt:0,mp:[[100,0,1,0],[100,0,1,1],[100,0,1,2],[100,0,1,3],[100,0,1,4],[100,0,1,5],[101,0,0,0],[101,0,0,1],[101,0,0,2],[101,0,0,3],[101,0,0,4],[101,0,0,5]]}}

The JSON input is valid according to RFC 4627 (JSON specfication). I ran it through and got it right back. This was pulled directly from that APK. You original report shows a JSON segment with double quotes. Double quotes and strings don't play nice with Android without escaping.

(Note: I haven't had time to do any real device testing yet)

@iBotPeaches

This comment has been minimized.

Show comment
Hide comment
@iBotPeaches

iBotPeaches Mar 18, 2015

Owner

Comment #6 originally posted by jtmuhone on 2013-12-18T12:00:32.000Z:

Hi,

I found the error.

It seems, that one should put only strings with maximum length of 32767 bytes to resource files, since the StringPool resource length overflows otherwise (https://github.com/android/platform_frameworks_base/blob/master/libs/androidfw/ResourceTypes.cpp, see decodeLength). However one can abuse that, and it works, since resource lengths are actually used only for checking if they are inside the byte array (same file, see ResStringPool::string8At). So the length overflows and return a wrong shorter value, but when android fetches the string, it actually get the original data.

I created a patch, it finds the original c string terminating null character instead of using the length and now it works! It also passes your tests.

I added the patch as an attachment.

Owner

iBotPeaches commented Mar 18, 2015

Comment #6 originally posted by jtmuhone on 2013-12-18T12:00:32.000Z:

Hi,

I found the error.

It seems, that one should put only strings with maximum length of 32767 bytes to resource files, since the StringPool resource length overflows otherwise (https://github.com/android/platform_frameworks_base/blob/master/libs/androidfw/ResourceTypes.cpp, see decodeLength). However one can abuse that, and it works, since resource lengths are actually used only for checking if they are inside the byte array (same file, see ResStringPool::string8At). So the length overflows and return a wrong shorter value, but when android fetches the string, it actually get the original data.

I created a patch, it finds the original c string terminating null character instead of using the length and now it works! It also passes your tests.

I added the patch as an attachment.

@iBotPeaches

This comment has been minimized.

Show comment
Hide comment
@iBotPeaches

iBotPeaches Mar 18, 2015

Owner

Comment #7 originally posted by connor.tumbleson on 2013-12-18T14:15:41.000Z:

Thanks for the patch!

I'll apply it and do some testing, and get this merged in.

Owner

iBotPeaches commented Mar 18, 2015

Comment #7 originally posted by connor.tumbleson on 2013-12-18T14:15:41.000Z:

Thanks for the patch!

I'll apply it and do some testing, and get this merged in.

@iBotPeaches

This comment has been minimized.

Show comment
Hide comment
@iBotPeaches

iBotPeaches Mar 18, 2015

Owner

Comment #8 originally posted by connor.tumbleson on 2013-12-18T14:43:26.000Z:

It has been merged!

1d745ac

Your insight has helped greatly. I found many bugs (ex # 238) that are similar to length being abused. I will investigate this greatly over the weekend and hopefully get some more fixes out.

Thanks again!

Owner

iBotPeaches commented Mar 18, 2015

Comment #8 originally posted by connor.tumbleson on 2013-12-18T14:43:26.000Z:

It has been merged!

1d745ac

Your insight has helped greatly. I found many bugs (ex # 238) that are similar to length being abused. I will investigate this greatly over the weekend and hopefully get some more fixes out.

Thanks again!

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