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] apktool b generates bad hiddenapi_class_data_item #2918

Closed
RenateUSB opened this issue Nov 2, 2022 · 9 comments · Fixed by #2941
Closed

[BUG] apktool b generates bad hiddenapi_class_data_item #2918

RenateUSB opened this issue Nov 2, 2022 · 9 comments · Fixed by #2941

Comments

@RenateUSB
Copy link

RenateUSB commented Nov 2, 2022

Information

  1. Apktool Version 2.6.1
  2. Windows 10
  3. Onyx Poke3 framework.jar, but narrowed down some

The original problem was that apktool d -api 29, apktool b -api 29 succeeded but the resulting jar did not work.
The deeper dive of apktool d -api 29, apktool b -api 29, apktool d -api 29 gave the below error.
I then did a fresh apktool d -api 29 and tested it with apktool b -api 29, apktool d -api 29.
I deleted classes as much as I could until the apktool b -api 29, apktool d -api 29 returned no error, then backed up.
At this point it consisted of only two smali classes that still gave the same error.
Deleting various virtual methods from ProtoOutputStream might cause the error to go away, or not.
I also cleaned all greylist-max-? to simple greylist
Various versions caused the OOB to be 7 instead of 6.

  1. baksmali is using an array of length 6 to be indexed by 3 bits (masked by 0x7), a recipe for disaster.
  2. apktool is still using baksmali that does not support greylist-max-r. (This example never used greylist-max-r.)
  3. Even the hidden API flags that it did decode are all wrong, many greylist were changed to whitelist. writeRepeatedFixed32Impl(II)V even changed from greylist to blacklist.

Stacktrace/Logcat

Error occurred while disassembling class Landroid.util.proto.ProtoOutputStream; - skipping class
java.lang.ArrayIndexOutOfBoundsException: 6
        at org.jf.dexlib2.HiddenApiRestriction.getAllFlags(HiddenApiRestriction.java:108)
        at org.jf.dexlib2.dexbacked.DexBackedMethod.getHiddenApiRestrictions(DexBackedMethod.java:204)

Steps to Reproduce

  1. unzip fw.zip
  2. apktool b FW0 -o fw1.jar -api 29
  3. apktool d fw1.jar -o FW1 -api 29

fw.zip

@RenateUSB
Copy link
Author

RenateUSB commented Nov 2, 2022

Ok, here is a further case, similar to the preceeding one.
It does not generate the error of the preceeding one but it does screw up the hidden API markers.
This example still has the problematic ProtoStream class, ProtoOutputStream was removed.
It has a demo class of HapiObject (super java.lang.object) and HapiProto (super android.util.proto.ProtoStream).
HapiObject is happy with the hidden API markers propagated.
HapiProto has all the markers changed to greylist
ProtoStream, as before, has changes to the markers.
This is (part of) the original smali code of both Hapi files.

.field public whitelist HAPI0_whitelist:I

.field public greylist HAPI1_greylist:I

.field public blacklist HAPI2_blacklist:I

.field public greylist-max-o HAPI3_greylist_max_o:I

.field public greylist-max-p HAPI4_greylist_max_p:I

.field public greylist-max-q HAPI5_greylist_max_q:I

Steps to Reproduce

  1. unzip hapi.zip
  2. apktool b FW0 -o fw1.jar -api 29
  3. apktool d fw1.jar -o FW1 -api 29
  4. Examine FW1\smali\android\util\HapiProto.smali

hapi.zip

@RenateUSB
Copy link
Author

RenateUSB commented Nov 2, 2022

Ok, we're narrowing it down!
The problem is if a class has a super with a greylist constructor.
We're even back to getting that array OOB error.
ProtoStream has been pruned down to nothing.

.class public abstract Landroid/util/proto/ProtoStream;
.super Ljava/lang/Object;
.source "ProtoStream.java"


# direct methods

.method public constructor greylist <init>()V
    .locals 0

    .prologue
    invoke-direct {p0}, Ljava/lang/Object;-><init>()V

    return-void
.end method

Steps to Reproduce

  1. unzip min.zip
  2. apktool b FW0 -o fw1.jar -api 29
  3. apktool d fw1.jar -o FW1 -api 29
  4. Generates array OOB and hidden API markers in HapiProto are messed up. Compare to HapiObject.

min.zip

Edit: Changing the greylist to whitelist in ProtoStream still breaks things, but changes the fields that it can decode in HapiProto to whitelist.
Getting rid of the marker in ProtoStream removes all markers from HapiProto and produces no errors.

@RenateUSB
Copy link
Author

RenateUSB commented Nov 3, 2022

Apparently a fix was merged??? JesusFreke/smali#816
But the commit is not part of the git??? JesusFreke/smali@3436508
See: #2662

@iBotPeaches
Copy link
Owner

@RenateUSB - Just looks like it was rebased/squashed - JesusFreke/smali@81bd303

@JesusFreke - You have a release planned soon for some of these merged, but unreleased fixes?

@RenateUSB
Copy link
Author

RenateUSB commented Nov 5, 2022

Just to beat a dead horse...
In a dex file the classes must always have any super before a subclass.
So in that "min.zip" example above, the class order was:

HapiObject
ProtoStream
HapiProto

In the hiddenapi_class_data_item the hidden API flags are concatenated in order of class,
Unfortunately the current (incorrect) order that is being used is alphabetic.
In decoding the dex it's trying to decode the third set of flags (since HapiProto is last) and grabbing the flags for ProtoStream.
Moreover, since ProtoStream has less flags it's actually reading past the end of the valid flags.
It's grabbing a random 0x0e which when masked with 0x07 gives you the spurious 6 that is reported as error.
The fix?

HapiObject
ProtoStream
ZapiProto

This works fine. Now alphabetical order and class order match.
Ok, renaming classes might break something later...

This is quite a serious bug. It means rebuilding any api>=29 Android core file has about zero chance of being correct even if no error is generated.

@iBotPeaches
Copy link
Owner

I believe fixed with: #2941

@r6680jc
Copy link

r6680jc commented Nov 24, 2022

@RenateUSB

Have you tested apktool 2.7.0 to decompile and recompile framework.jar?

In my case, this issue still exists (same as self build smali/baksmali).

@RenateUSB
Copy link
Author

RenateUSB commented Nov 24, 2022

@r6680jc No, not yet, but I'll check it out.
I have grown fond of directly patching 4 bytes in 30 M though.
#2947

Edit: I can confirm that the hidden API data appears to be correct in 15,000 classes.
Unfortunately, a round-trip of framework.jar causes my Android to be a train wreck.
A round-trip changes the order of classes so it's not easy to compare.
Still investigating...

Trivia: What class has the most fields & methods.
android.util.StatsLogInternal has 4500? WTH?

@r6680jc
Copy link

r6680jc commented Nov 25, 2022

@RenateUSB

It's just the resulting framework.jar with any classes.dex rebuilt without modification makes the device bootloop.

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

Successfully merging a pull request may close this issue.

3 participants