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

[BUG] mResTable poisoned if framework is sparse #3298

Closed
oSumAtrIX opened this issue Aug 27, 2023 · 12 comments · Fixed by #3299
Closed

[BUG] mResTable poisoned if framework is sparse #3298

oSumAtrIX opened this issue Aug 27, 2023 · 12 comments · Fixed by #3299

Comments

@oSumAtrIX
Copy link

oSumAtrIX commented Aug 27, 2023

Information

  1. Apktool Version (apktool -version) - 2.8.1 (The issue is still present after 5483650)
  2. APK From? (Playstore, ROM, Other) - Playstore/ ApkMirror
  3. Java Version (java --version) - OpenJDK 17

Stacktrace/Logcat

As #3199

Issue

evidence.mp4

APK

https://www.apkmirror.com/apk/google-inc/youtube/youtube-18-32-39-release/

Steps to Reproduce

Decode & build

  1. apktool d youtube-18.32.39.apk -s (Notice how sparse resources are true)
  2. apktool b youtube-18.32.39
  3. Sign/ Align & install to test (This will not work under normal circumstances with YouTube specifically)

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? Yes
  3. Are you using the latest apktool version? Yes
@iBotPeaches
Copy link
Owner

Odd.

I'm fairly accurate that how it works now is correct and it was not in the past. I'm going exactly by the spec when it comes to determining whether it was a sparsely packed or not.

 * If the flag FLAG_SPARSE is set in `flags`, then this struct is followed
 * by an array of ResTable_sparseTypeEntry defining only the entries that
 * have values for this type. Each entry is sorted by their entry ID such
 * that a binary search can be performed over the entries. The ID and offset
 * are encoded in a uint32_t. See ResTabe_sparseTypeEntry.

https://github.com/aosp-mirror/platform_frameworks_base/blob/master/libs/androidfw/include/androidfw/ResourceTypes.h#L1397-L1421

So honestly not sure at the moment.

@oSumAtrIX
Copy link
Author

oSumAtrIX commented Aug 27, 2023

I can replicate the same issue as described in #3199 on v2.8.1-54-06c5f462-SNAPSHOT (beyond revision 5483650).

  1. Decode
  2. Set sparseResources manually to false
  3. Build
  4. Decode again
  5. Observe "Sparsely packed resources detected."

@iBotPeaches
Copy link
Owner

Odd again.

Taking a look.

@oSumAtrIX
Copy link
Author

oSumAtrIX commented Aug 27, 2023

One assertion that can be made is that AAPT2 does correctly not decode resources sparely, if not instructed to do that, because the app does not produce the stack dump in that case.

@iBotPeaches
Copy link
Owner

I did update the aapt2/aapt binaries as part of 2.8.0. Its very very rare that any change to upstream AOSP build tools causes a regression though - 8634050

@oSumAtrIX
Copy link
Author

oSumAtrIX commented Aug 27, 2023

Yes, AAPT2 is working fine is what I mean as we can see that not using the option to encode resources sparsely fixes the stack dump. What is odd, though, is that sparsely encoded resources are detected on an APK whose resources are not encoded sparsely by AAPT2 as seen in #3199 and now.

@iBotPeaches
Copy link
Owner

@oSumAtrIX - Are you positive you are running a bleeding edge build and/or grabbing upstream patches correctly?

I decoded the application you gave with the official 2.7.0 and the top of master.

➜  3298 grep -r -i 'sparseResources:' *
270/apktool.yml:sparseResources: false
282/apktool.yml:sparseResources: false
➜  3298 

I placed breakpoints, etc. Never replicated what you are saying. Both are indeed as I expect with sparseResources set to false.

@oSumAtrIX
Copy link
Author

oSumAtrIX commented Aug 27, 2023

2.7.0 is a fairly old build. I am running APKTool off commit 06c5f46. This is currently versioned as v2.8.1-54-06c5f462-SNAPSHOT which is later than 2.7.0. This commit comes after 5483650, which is supposedly fixing #3199, but I still can reproduce #3199 on 06c5f46. Replication steps are as follows: #3298 (comment).

I will follow up with a complete recording of the issue for reference in a minute.

@oSumAtrIX
Copy link
Author

oSumAtrIX commented Aug 27, 2023

Here is a followup recording (trimmed when nothing happens):

evidence.mp4

The recording shows, that APKTool detects resources of an APK as sparsely encoded that has been compiled by AAPT2 without sparsely encoding enabled.

@iBotPeaches
Copy link
Owner

Okay thanks. To be honest, I'm unaware why I cannot replicate still. So my guess if this is occurring then I bet I have a bug where we aren't isolating the framework decode from the application decode.

So a sparse framework may be leaking into main.

@iBotPeaches
Copy link
Owner

Yeah that was it. I have a patch pending, but I gotta take it a bit slow and figure out a more proper solution to prevent this cross contamination/poisoning.

#3299

@oSumAtrIX oSumAtrIX changed the title [BUG] Sparsely encoded resources presumably detected incorrectly [BUG] mResTable poisoned if framework is sparse Aug 27, 2023
@iBotPeaches
Copy link
Owner

Interesting that it took years to find this. It was never noticeable previously because we accidentally blew out the config for each package. Frameworks come first, so this was always last with the intended application which meant it seemed it worked. However, it was always additionally wrong since it was assuming sparse resources whether the spec order was different than intended. This is a good indication of a sparsely package table, but the real indicator is just whether the sparse flag is true or not.

The refactors to optimize since 2.7.0 to now adapted that into a more proper sparse check described above, at the cost of not realizing that we were storing all packages parsed into one single info class.

This is intended though, because you need to know frameworks, etc from the main app. So you do want one central storage mechanism. So tldr - I need to do some mini refactors and I'll get that PR cleaned up.

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 a pull request may close this issue.

2 participants