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

fix: decoding APK with many compact entries and unknown uses-sdk attrs #3705

Merged
merged 3 commits into from
Oct 4, 2024
Merged

fix: decoding APK with many compact entries and unknown uses-sdk attrs #3705

merged 3 commits into from
Oct 4, 2024

Conversation

IgorEisberg
Copy link
Contributor

@IgorEisberg IgorEisberg commented Oct 3, 2024

This fixes 2 new issues with a stock APK sourced from an Android 15 ROM.

https://drive.google.com/file/d/1x9udLN4W5I7chyGp1ZY8Cyfhu1vXezU9/view
(classes files removed to reduce size, they are irrelevant)

  1. mIn.readShort() for size in readEntryData is incorrect and the size < 0 check is not possible.
    Entry size is stored by AAPT2 as an unsigned short and thus will never be negative.
    Reading it as a signed short will cause negative entry sizes in compactly packed entries in
    very large string pools and will result in a lot of "APKTOOL_DUMMYVAL_" values.

  2. sdkInfo isn't stored properly for APKs with unexpected properties in uses-sdk tag.
    As far as I can tell, these attributes serve no purpose and can be ignored.
    In the given APK, additional "android:versionCode" and "android:versionName" attributes appear
    in the uses-sdk tag, purpose unknown and they don't represent the actual version of the app.

   E: uses-sdk (line=26)
     A: http://schemas.android.com/apk/res/android:minSdkVersion(0x0101020c)=35
     A: http://schemas.android.com/apk/res/android:versionCode(0x0101021b)=31
     A: http://schemas.android.com/apk/res/android:versionName(0x0101021c)="3.1"
     A: http://schemas.android.com/apk/res/android:targetSdkVersion(0x01010270)=35

This fixes 2 new issues with a stock APK sourced from an Android 15 ROM.

https://drive.google.com/file/d/1x9udLN4W5I7chyGp1ZY8Cyfhu1vXezU9/view

1) mIn.readShort() for size in readEntryData is incorrect and the size < 0 check is not possible.
   Entry size is stored by AAPT2 as an unsigned short and thus will never be negative.
   Reading it as a signed short will cause negative entry sizes in compactly packed entries in
   very large string pools and will result in a lot of "APKTOOL_DUMMYVAL_" values.

2) sdkInfo isn't stored properly for APKs with unexpected properties in uses-sdk tag.
   As far as I can tell, these attributes serve no purpose and can be ignored.
   In the given APK, additional "android:versionCode" and "android:versionName" attributes appear
   in the uses-sdk tag, purpose unknown and they don't represent the actual version of the app.

   E: uses-sdk (line=26)
     A: http://schemas.android.com/apk/res/android:minSdkVersion(0x0101020c)=35
     A: http://schemas.android.com/apk/res/android:versionCode(0x0101021b)=31
     A: http://schemas.android.com/apk/res/android:versionName(0x0101021c)="3.1"
     A: http://schemas.android.com/apk/res/android:targetSdkVersion(0x01010270)=35
@iBotPeaches
Copy link
Owner

So we could build a sample of compactly packed resources that exceeds a signed short max (~32k) thus to trigger the flaw of reading a signed short vs unsigned

@IgorEisberg
Copy link
Contributor Author

IgorEisberg commented Oct 3, 2024

Wrote the sample app. Could you write the test? Not really great at unit testing.
https://drive.google.com/file/d/1WF979N5qTWzGhQNtTMS8BET1dvPwecQ0/view

Before patch:

    ...
    <string name="dummy_32763">@null</string>
    <string name="dummy_32764">@null</string>
    <string name="dummy_32765">@null</string>
    <string name="dummy_32766">@null</string>
    <string name="dummy_32767">@null</string>
    <string name="APKTOOL_DUMMYVAL_0x7f018000">@null</string>
    <string name="APKTOOL_DUMMYVAL_0x7f018001">@null</string>
    <string name="APKTOOL_DUMMYVAL_0x7f018002">@null</string>
    <string name="APKTOOL_DUMMYVAL_0x7f018003">@null</string>
    <string name="APKTOOL_DUMMYVAL_0x7f018004">@null</string>
</resources>

After patch:

    ...
    <string name="dummy_32763">@null</string>
    <string name="dummy_32764">@null</string>
    <string name="dummy_32765">@null</string>
    <string name="dummy_32766">@null</string>
    <string name="dummy_32767">@null</string>
    <string name="dummy_32768">@null</string>
    <string name="dummy_32769">@null</string>
    <string name="dummy_32770">@null</string>
    <string name="dummy_32771">@null</string>
    <string name="dummy_32772">@null</string>
</resources>

@iBotPeaches
Copy link
Owner

Easy source for sample? I like to keep sources for samples https://github.com/iBotPeaches/TestApks

@IgorEisberg
Copy link
Contributor Author

Easy source for sample? I like to keep sources for samples https://github.com/iBotPeaches/TestApks

Well... I didn't make a whole project just to compile a simple resources dir with the --enable-compact-entries flag passed to AAPT2. ^_^''

@IgorEisberg IgorEisberg closed this Oct 4, 2024
@IgorEisberg IgorEisberg deleted the sdk35fix1 branch October 4, 2024 08:13
@IgorEisberg IgorEisberg restored the sdk35fix1 branch October 4, 2024 08:59
@IgorEisberg IgorEisberg reopened this Oct 4, 2024
@iBotPeaches
Copy link
Owner

Well... I didn't make a whole project just to compile a simple resources dir with the --enable-compact-entries flag passed to AAPT2. ^_^''

Fair, I always appreciate the reminder when it comes months later how I generated x. Sometimes I use a simple bash script to perform the actions if Gradle doesn't support it yet - https://github.com/iBotPeaches/TestApks/blob/master/ARSCTest/build.sh

@iBotPeaches
Copy link
Owner

Okay added a test. If it passes, I'll merge this one.

@iBotPeaches iBotPeaches merged commit 24541c3 into iBotPeaches:master Oct 4, 2024
25 checks passed
iBotPeaches added a commit that referenced this pull request Oct 4, 2024
@IgorEisberg IgorEisberg deleted the sdk35fix1 branch October 4, 2024 13:14
@iBotPeaches iBotPeaches added this to the v2.11.0 milestone Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants