iBotPeaches / Apktool

A tool for reverse engineering Android apk files
https://apktool.org/
Apache License 2.0
20.35k stars 3.6k forks source link

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

Closed IgorEisberg closed 1 month ago

IgorEisberg commented 1 month ago

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 "APKTOOLDUMMYVAL" 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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

IgorEisberg commented 1 month ago

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. ^_^''

iBotPeaches commented 1 month ago

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 commented 1 month ago

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