`ZipArchive::new(f)` returns `InvalidArchive("Extra field header truncated")` for a valid ZIP archive

[Mrakorez]

Describe the bug

ZipArchive::new(f) returns InvalidArchive("Extra field header truncated") for a ZIP archive that unzip and Android runtime handle successfully.

To Reproduce

1. Download an APK from APKMirror

2. Rename it for convenience:

   mv 'bin.mt.plus_2.26.2-26020493_minAPI21(arm64-v8a,armeabi-v7a,x86,x86_64)(nodpi)_apkmirror.com.apk' bin.mt.plus_2.26.2.apk

3. Verify it is a valid ZIP:

   unzip -l bin.mt.plus_2.26.2.apk | tail -5

4. Run the following code:

   let f = File::open("bin.mt.plus_2.26.2.apk").unwrap();
   let archive = ZipArchive::new(f);
   dbg!(&archive);

5. See error

Expected behavior

ZipArchive::new(f) returns Ok(ZipArchive<File>).

Desktop (please complete the following information):

• OS: Linux
• zip version: 8.1.0

[Its-Just-Nans]

Hello

Thanks for the report

It seems that the zip file is setting an extra field with an unknown extra field.

Right now we treat that as error since it's looks like it's not in the specs.

Here is the first file that is failing

0238CA3 LOCAL HEADER #18      04034B50
0238CA7 Extract Zip Spec      0A '1.0'
0238CA8 Extract OS            00 'MS-DOS'
0238CA9 General Purpose Flag  0800
        [Bit 11]              1 'Language Encoding'
0238CAB Compression Method    0000 'Stored'
0238CAD Last Mod Time         42D4A6A9 'Thu Jun 20 14:53:18 2013'
0238CB1 CRC                   A8DE3EC7
0238CB5 Compressed Length     00000074
0238CB9 Uncompressed Length   00000074
0238CBD Filename Length       0015
0238CBF Extra Length          0002                                           <---- here
0238CC1 Filename              'assets/h5_qr_back.png'
0238CD6 Null Padding in Extra                                                <---- here
0238CD8 PAYLOAD               .PNG........IHDR.......*......9.....;IDA
                              Tx.c ..`Y.r...,@&IZ...5.Z....Q-
                              .ZF.`j.G...6.mT..6k.......|...l......IEN
                              D.B`.

We can see that the Extra Length is set to 2 Which is technically not possible - it should have a value of 4 at minimum

Specs

  4.3.7  Local file header:

      local file header signature     4 bytes  (0x04034b50)
      version needed to extract       2 bytes
      general purpose bit flag        2 bytes
      compression method              2 bytes
      last mod file time              2 bytes
      last mod file date              2 bytes
      crc-32                          4 bytes
      compressed size                 4 bytes
      uncompressed size               4 bytes
      file name length                2 bytes
      extra field length              2 bytes

      file name (variable size)
      extra field (variable size)

4.5.1 In order to allow different programs and different types
   of information to be stored in the 'extra' field in .ZIP
   files, the following structure MUST be used for all
   programs storing data in this field:

       header1+data1 + header2+data2 . . .

   Each header MUST consist of:

       Header ID - 2 bytes
       Data Size - 2 bytes

   Note: all fields stored in Intel low-byte/high-byte order.

   The Header ID field indicates the type of data that is in
   the following data block.

   Header IDs of 0 thru 31 are reserved for use by PKWARE.
   The remaining IDs can be used by third party vendors for
   proprietary usage.

@Mrakorez Do you have any idea by which tool this file was generated ?

[chenxiaolong]

Android APKs are normally processed through zipalign. It uses all 0's in the extra field for padding the data until the specified alignment (usually 4 bytes): https://android.googlesource.com/platform/build.git/+/refs/tags/android-16.0.0_r4/tools/zipalign/ZipEntry.cpp#222

[Its-Just-Nans]

Android APKs are normally processed through zipalign. It uses all 0's in the extra field for padding the data until the specified alignment (usually 4 bytes): https://android.googlesource.com/platform/build.git/+/refs/tags/android-16.0.0_r4/tools/zipalign/ZipEntry.cpp#222

Thanks I thought that the padding was only in the apk padding block. It seems not https://android.googlesource.com/platform/build.git/+/refs/tags/android-16.0.0_r4/tools/zipalign/ZipFile.cpp#501

So I think my pull request is making sense

Btw the comment on the function is quite funny This will potentially confuse something that put "extra" data in here earlier, but I can't find an actual problem.

Seems like they are writing the extra field even if it's less than 4: https://android.googlesource.com/platform/build.git/+/refs/tags/android-16.0.0_r4/tools/zipalign/ZipEntry.cpp#492

[Mrakorez]

Thanks for the responses.

As chenxiaolong mentioned, this file was processed by zipalign. However, this is standard procedure for every APK - and I encountered the problematic file only after successfully opening and reading 100+ others.

If the issue were simply about extra field length, I wouldn't have been able to process a single APK, right?

The last version that handles the problematic file correctly is 6.0.0.

See the example below using Magisk v30.6: it has 131 entries with Extra Length of 1–3 and is read successfully, while MT Manager (682 such entries) fails.

Magisk Example

~/dev/sandbox/rust
❯ zipdetails com.topjohnwu.magisk.apk
| lines
| where $it =~ `Extra Length\s*000[1-3]`
| tee { print ($in | first 5) }
| length
| print $"Extra Length 1-3: ($in)"
╭───┬───────────────────────────────────────╮
│ 0 │ 074918 Extra Length          0003 (3) │
│ 1 │ 9F6DA8 Extra Length          0002 (2) │
│ 2 │ 9F6EDF Extra Length          0003 (3) │
│ 3 │ 9F6FDD Extra Length          0001 (1) │
│ 4 │ 9F7909 Extra Length          0001 (1) │
╰───┴───────────────────────────────────────╯
Extra Length 1-3: 131

~/dev/sandbox/rust
❯ open src/main.rs
use std::{env::args, fs::File, process::exit};
use zip::ZipArchive;

fn main() {
    let arg = match args().nth(1) {
        Some(arg) => arg,
        None => exit(1),
    };

    let f = File::open(&arg).unwrap();

    match ZipArchive::new(f) {
        Ok(archive) => println!("{}: {}", arg, archive.len()),
        Err(e) => eprintln!("{e:?}"),
    };
}

~/dev/sandbox/rust
❯ ./ziplength com.topjohnwu.magisk.apk
com.topjohnwu.magisk.apk: 1063

~/dev/sandbox/rust
❯ zipdetails bin.mt.plus_2.26.2.apk
| lines
| where $it =~ `Extra Length\s*000[1-3]`
| tee { print ($in | first 5) }
| length
| print $"Extra Length 1-3: ($in)"
╭───┬────────────────────────────────────────╮
│ 0 │ 004638D Extra Length          0001 (1) │
│ 1 │ 0238CBF Extra Length          0002 (2) │
│ 2 │ 0238D68 Extra Length          0002 (2) │
│ 3 │ 0301B23 Extra Length          0001 (1) │
│ 4 │ 179C389 Extra Length          0003 (3) │
╰───┴────────────────────────────────────────╯
Extra Length 1-3: 682

~/dev/sandbox/rust
❯ ./ziplength bin.mt.plus_2.26.2.apk
InvalidArchive("Extra field header truncated")

[Its-Just-Nans]

Great, thanks for the information about 6.0.0

Looking at it with git bisect the problematic commit was e86d376

I think the problem is that previously it was

// parsing field and failing here
let len = reader.read_u16_le()?; // return a io::error

// but skipping io::error
match parse_extra_field(&mut result) {
        Ok(()) => {}
        Err(ZipError::Io(..)) => {}                                          // skipping io::error
        Err(e) => return Err(e),
    }

After the commit

// catching the io::error to return an Error
let len = match reader.read_u16_le() {
    Ok(len) => len,
    Err(e) if e.kind() == io::ErrorKind::UnexpectedEof => {
        return Err(invalid!("Extra field header truncated"))                    // catching the io::error to return an Error
    }
    Err(e) => return Err(e.into()),
};

// and handle all error as error
parse_extra_field(&mut result)?;

I will add test(s) to my PR

@Pr0methean for info

[Its-Just-Nans]

Also thanks for the example on the both com.topjohnwu.magisk.apk and bin.mt.plus_2.26.2.apk

The differences are

• bin.mt.plus_2.26.2.apk contains padding in both local and central headers
• com.topjohnwu.magisk.apk contains padding only in local headers

The bug is only when parsing the central headers.

Note:

• we also need to check the parsing for local headers