[BUG] resources.arsc parsing XML_TYPE_OVERLAY are not correctly read

[jpstotz]

Information

1. **Apktool Version: latest from GitHub 7a15c7b
2. **APK From? ** APK is linked here: [core]APP parsing error, and the overlay cannot parse skylot/jadx#1748 (comment)

The parsing method readOverlaySpec does not correctly work.

For example in the linked APK there is a XML_TYPE_OVERLAY chunk starting at offset 0x280000 with headerSize=1032 bytes and chunkSize=1056 bytes.

The current implementation only reads two fixed length UTF-16 strings of length 128 which consumes 128*2*2=512 bytes.

Afterwards it tries to read new chunks but the position is still in the middle of XML_TYPE_OVERLAY chunk which ends up in reading zero bytes and stopping reading the resource table.

But affectively after the first XML_TYPE_OVERLAY chunk (with name=AssistedDrivingBrandingResources) there is a second XML_TYPE_OVERLAY chunk (with name= car-ui-lib).
That second chunk (and other possible chunks afterwards) are never read by apktool a sit had already stopped parsing the resource table.

In my understanding readOverlaySpec() should be changed to skip unread header bytes before trying to read the next chunk:

    private void readOverlaySpec() throws AndrolibException, IOException {
        checkChunkType(Header.XML_TYPE_OVERLAY);
        String name = mIn.readNullEndedString(128, true);
        String actor = mIn.readNullEndedString(128, true);
        LOGGER.fine(String.format("Overlay name: \"%s\", actor: \"%s\")", name, actor));

        // check current position and jump to the end of the chunk
        int skip = mHeader.chunkSize - 8 - 256 - 256; // header size is 8 bytes and two UTF-16 strings of length 128 = 256 bytes each
        mIn.skipBytes(skip);

        while(nextChunk().type == Header.XML_TYPE_OVERLAY_POLICY) {
            readOverlayPolicySpec();
        }
    }

[iBotPeaches]

Thanks for the tip @jpstotz.

I see the mistake - these are not null terminated strings, but fixed length as you mentioned. So I don't think we need to jump/skip and just adjust the string parser for fixed reading.

Further more -- I think assuming overlay policy chunk will always follow an overlay spec chunk is probably not right as I'm looking at this code again.

[jpstotz]

@iBotPeaches The string reading is already correct they are read as fixed size strings. I think the name is just a bit misleading (at a first glance) for fixed size strings.

But even with the two fixed size strings we only cover 512 bytes of the chunk. What about the other more than 500 bytes in the linked example APK? They are all zero in the example APK so based on the data it is not possible to guess what data should be in it.

[iBotPeaches]

@jpstotz - I don't really see anything else in that spec except the header/string/string - https://github.com/aosp-mirror/platform_frameworks_base/blob/master/libs/androidfw/include/androidfw/ResourceTypes.h#L1669

So if you are saying we have 500~ more bytes that are not explained by the two fixed length strings, then I have no idea what they could be. We could read the size (via the header) and skip, but doesn't seem like the safest thing.

[jpstotz]

I am not an C expert but the definition uint16_t name[256]; does in my interpretation defines an array of 256 elements, each of type uint16_t (one UTF-16 character). So each array has a size of 512 bytes.

At the moment apktool code reads two strings of length 128: mIn.readNullEndedString(128, true);.
This means each string has a size of 128*2 = 256 bytes.

So if I am not mistaken based on this definition it should be mIn.readNullEndedString(256, true);.

Unfortunately this still doesn't explain the chunk size, but the chunk header size would match:

chunk header size: 1032 bytes = 8 + 512 + 512

8 bytes chunk header + two strings of length 512.

Assuming the ResTable_overlayable_policy_header would follow, not outside of the chunk but inside.

This structured comprises of

struct ResTable_overlayable_policy_header
{
  struct ResChunk_header header; // 8 bytes 
  PolicyFlags policy_flags; // = uint32_t = 4 bytes
  uint32_t entry_count; // = 4 bytes
};

entry_count defines how many ResTable_ref (uint32_t) follow.

In the linked example APK this gives me the values for the ResTable_overlayable_policy_header:
headerSize=16 chunkSize=24 entryCount=2

A headerSize of 16 bytes exactly matches sizeof(ResTable_overlayable_policy_header).

An entryCount of 2 means additional 8 bytes of data + 16 bytes header size = 24 bytes - again a match.

back to the XML_TYPE_OVERLAY chunk:

chunk size 1056 bytes = 1032 bytes XML_TYPE_OVERLAY header + 24 byte XML_TYPE_OVERLAY_POLICY chunk

Again a match. This seems to be the correct way to decode the XML_TYPE_OVERLAY_POLICY chunk.

So in the end you were correct that after parsing the two string an XML_TYPE_OVERLAY_POLICY chunk has to be parsed. Only the string length is wrong. In both cases it needs to be corrected to mIn.readNullEndedString(256, true);

Edit: There is also another small mistake: Parsing the XML_TYPE_OVERLAY_POLICY structure should not be done in a loop, just once. I have combined the changes in this branch: https://github.com/jpstotz/Apktool/tree/readOverlaySpec If you want I can open a PR.

An of course there is the fact that we have nested chunks which is not reflected by apktool. In my opinion the current way of reading chunks, ignoring the chunk size is dangerous. If there is a mistake in parsing a chunk, reading all chunks afterwards will fail.

[iBotPeaches]

Thanks for the details @jpstotz. So I believe in recap:

1. We should be reading 256, not 128 - as that was the proper size. It is a fixed read via the 2nd boolean, which I forgot.

2. Yes, ignoring chunk size is dangerous, but we only have a few places in codebase that keep track of how many bytes were read in order to properly determine if space is remaining in comparison to reported chunk size to skip.

3. I think while vs if for XML_TYPE_OVERLAY_POLICY may be beneficial to move back to the generic chunk parser, so we just read each chunk in a loop nonstop till end. I see a flaw that we always assume an overlay policy would be after an overlay chunk - which may not be true in sparse apks.

[jpstotz]

1. We should be reading 256, not 128 - as that was the proper size. It is a fixed read via the 2nd boolean, which I forgot.

Yes two strings of fixed-length 256 UTF-16 characters.

3. I think while vs if for XML_TYPE_OVERLAY_POLICY may be beneficial to move back to the generic chunk parser, so we just read each chunk in a loop nonstop till end. I see a flaw that we always assume an overlay policy would be after an overlay chunk - which may not be true in sparse apks.

I have not seen such samples but using the XML_TYPE_OVERLAY chunk size we should be able to determine if there is and XML_TYPE_OVERLAY_POLICY or not:

If there is no XML_TYPE_OVERLAY_POLICY chunk included in the XML_TYPE_OVERLAY, then it's size should be just the two strings and the chunk header -> 1024 + 8 = 1032 bytes.

If there is a XML_TYPE_OVERLAY_POLICY chunk included in the XML_TYPE_OVERLAY the chunk size has to be at least 1024 + 8 + 16 = 1048 bytes.

So I see only the two valid cases:

1. XML_TYPE_OVERLAY chunk size == 1032 --> no XML_TYPE_OVERLAY_POLICY chunk
2. XML_TYPE_OVERLAY chunk size >= 1048 --> XML_TYPE_OVERLAY_POLICY chunk included

Everything else should be invalid.

[iBotPeaches]

I've got a PR up that I believe resolves this all now. Confirmed working with the application you linked.

➜  3030 apktool d com.google.android.apps.maps.apk 
Picked up _JAVA_OPTIONS: -Dawt.useSystemAAFontSettings=on -Dswing.aatext=true
I: Using Apktool 2.7.1-603e52-SNAPSHOT on com.google.android.apps.maps.apk
I: Loading resource table...
I: Decoding AndroidManifest.xml with resources...
I: Loading resource table from file: /home/ibotpeaches/.local/share/apktool/framework/1.apk
I: Regular manifest package...
I: Decoding file-resources...
I: Decoding values */* XMLs...
I: Baksmaling classes.dex...
I: Baksmaling classes2.dex...
I: Baksmaling classes3.dex...
I: Baksmaling classes4.dex...
I: Baksmaling classes5.dex...
I: Baksmaling classes6.dex...
I: Copying assets and libs...
I: Copying unknown files...
I: Copying original files...
I: Copying META-INF/services directory
➜  3030