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

ZipAlign creates corrupted APK for already aligned APK #27

Closed
MuntashirAkon opened this issue Apr 13, 2023 · 27 comments
Closed

ZipAlign creates corrupted APK for already aligned APK #27

MuntashirAkon opened this issue Apr 13, 2023 · 27 comments

Comments

@MuntashirAkon
Copy link

Title says it all. In case you're wondering why it's necessary, we don't know if an APK is aligned or not in first place. So, we need to do that for both aligned and unaligned APKs.

@MuntashirAkon
Copy link
Author

I am also reiterating the need for #6. Adding tests would greatly reduce issues like this. I feel that most issues I opened here could've been resolved if there were adequate tests.

@Axelen123
Copy link
Contributor

Axelen123 commented Apr 13, 2023

I haven't encountered this myself, but my guess is that it occurs because this line assumes all compressed entries have a data descriptor, which may not always be the case.

@MuntashirAkon
Copy link
Author

Well, I need to look into this. But the zipalign library has an option to verify if an APK is zip aligned. Maybe that could be implemented apart from this.

Here's the source code: https://github.com/MuntashirAkon/zipalign-android/blob/master/zipalign/src/main/cpp/zipalign/ZipAlign.cpp#L124-L168

Seems quite easy to implement. I can implement that if you want.

@MuntashirAkon
Copy link
Author

I haven't encountered this myself, but my guess is that it occurs because this line assumes all compressed entries have a data descriptor, which may not always be the case.

Well, I've tried about a dozen APK files, each of them has this issue. So, I believe that most of them do not have a data descriptor. But is there a way to actually detect this using the ZipFile API? I don't see any option to do that. And how it is that it only affects signed APK files, and not unsigned ones?

@REAndroid
Copy link
Owner

ZipAlign is borrowed class and I am not full understood it. But my guess is that at line 149 byte[] extra = entry.getExtra(); is always null.

Now I am focusing on new archive management lib archive2 which also solves such issue, until then hopefully someone will find a solution.
@Axelen123
@Kirlif

@Axelen123
Copy link
Contributor

Well, I've tried about a dozen APK files, each of them has this issue. So, I believe that most of them do not have a data descriptor. But is there a way to actually detect this using the ZipFile API? I don't see any option to do that.

I dont think there is, since the Java Zip library does not expose flags.
REAndroids guess is also plausible.

Pull request #21 uses a different zip implementation, so its possible its not affected by the issue. I havent tested the static methods on ZipAlign much, but if you do test it only to find that its broken, please let me know

@REAndroid
Copy link
Owner

Oh @Axelen123 is right, the problem was "flag" is calculated based on assumption that all non-STORED entries have data descriptor and counts 16 bytes. But non-STORED entries will not always have data descriptor.

Until archive2 ready , here is my ugly commit

@MuntashirAkon
Copy link
Author

Until archive2 ready , here is my ugly commit

It didn't work. It still fails with the following error:

java.util.zip.DataFormatException: invalid stored block lengths
        at java.util.zip.Inflater.inflateBytes(Native Method)
        at java.util.zip.Inflater.inflate(Inflater.java:278)
        at java.util.zip.Inflater.inflate(Inflater.java:299)

@REAndroid
Copy link
Owner

  • Are you testing on android os? Maybe getFlag(ZipEntry) is returning null.

Anyways if you are interested, archive2 is ready now for early experiment

@MuntashirAkon
Copy link
Author

  • Are you testing on android os? Maybe getFlag(ZipEntry) is returning null.

No, hidden API enforcement policies are bypassed. It's something to do with calculating the offsets, I think.

@MuntashirAkon
Copy link
Author

MuntashirAkon commented Apr 24, 2023

if you are interested, archive2 is ready now for early experiment

Getting NPE for some APKs:

java.lang.NullPointerException: Attempt to invoke virtual method 'long com.reandroid.archive2.block.CentralEntryHeader.getCrc()' on a null object reference
at com.reandroid.archive2.block.LocalFileHeader.mergeZeroValues(LocalFileHeader.java:32)
at com.reandroid.archive2.model.LocalFileDirectory.visitLocalFile(LocalFileDirectory.java:63)
at com.reandroid.archive2.model.LocalFileDirectory.visitLocalFile(LocalFileDirectory.java:48)
at com.reandroid.archive2.model.LocalFileDirectory.visit(LocalFileDirectory.java:42)
at com.reandroid.archive2.Archive.(Archive.java:47)
at com.reandroid.archive2.Archive.(Archive.java:62)

Let me know if you need a sample.

@MuntashirAkon
Copy link
Author

Another similar NPE:

java.lang.NullPointerException: Attempt to invoke virtual method 'com.reandroid.archive2.block.CommonHeader$GeneralPurposeFlag com.reandroid.archive2.block.CentralEntryHeader.getGeneralPurposeFlag()' on a null object reference
 at com.reandroid.archive2.block.LocalFileHeader.mergeZeroValues(LocalFileHeader.java:41)
 at com.reandroid.archive2.model.LocalFileDirectory.visitLocalFile(LocalFileDirectory.java:63)
 at com.reandroid.archive2.model.LocalFileDirectory.visitLocalFile(LocalFileDirectory.java:48)
 at com.reandroid.archive2.model.LocalFileDirectory.visit(LocalFileDirectory.java:42)
 at com.reandroid.archive2.Archive.<init>(Archive.java:47)
 at com.reandroid.archive2.Archive.<init>(Archive.java:62)

@MuntashirAkon
Copy link
Author

MuntashirAkon commented Apr 24, 2023

The new API works great except the aforementioned cases. BTW, here's the class I use for verification (it was converted from the ZipAlign library for Android):

ZipAlignVerifier.java
public class ZipAlignVerifier {
    public static final String TAG = ZipAlignVerifier.class.getSimpleName();

    public static final int kPageAlignment = 4096;

    public static boolean verify(File file, int alignment, boolean pageAlignSharedLibs) {
        Archive zipFile;
        boolean foundBad = false;
        Log.d(TAG, String.format(Locale.ROOT, "Verifying alignment of %s (%d)...\n", file, alignment));

        try {
            zipFile = new Archive(file);
        } catch (IOException e) {
            Log.e(TAG, String.format(Locale.ROOT, "Unable to open '%s' for verification\n", file), e);
            return false;
        }
        List<ArchiveEntry> entries = zipFile.getEntryList();
        long lastFileOffset;
        for (ArchiveEntry pEntry : entries) {
            String name = pEntry.getName();
            lastFileOffset = pEntry.getFileOffset();
            if (pEntry.getMethod() == ZipEntry.DEFLATED) {
                Log.d(TAG, String.format(Locale.ROOT, "%8d %s (OK - compressed)\n", lastFileOffset, name));
            } else if (pEntry.isDirectory()) {
                // Directory entries do not need to be aligned.
                Log.d(TAG, String.format(Locale.ROOT, "%8d %s (OK - directory)\n", lastFileOffset, name));
            } else {
                int alignTo = getAlignment(pageAlignSharedLibs, alignment, pEntry);
                if ((lastFileOffset % alignTo) != 0) {
                    Log.w(TAG, String.format(Locale.ROOT, "%8d %s (BAD - %d)\n", lastFileOffset, name, (lastFileOffset % alignTo)));
                    foundBad = true;
                    break;
                } else {
                    Log.d(TAG, String.format(Locale.ROOT, "%8d %s (OK)\n", lastFileOffset, name));
                }
            }
        }

        Log.d(TAG, String.format(Locale.ROOT, "Verification %s\n", foundBad ? "FAILED" : "successful"));

        return !foundBad;
    }

    private static int getAlignment(boolean pageAlignSharedLibs, int defaultAlignment, ZipEntry pEntry) {
        if (!pageAlignSharedLibs) {
            return defaultAlignment;
        }

        if (pEntry.getName().endsWith(".so")) {
            return kPageAlignment;
        }

        return defaultAlignment;
    }
}

@REAndroid
Copy link
Owner

Thanks I was hopping someone to test it on android os, how is the performance relative to old API ?

@REAndroid
Copy link
Owner

Let me know if you need a sample.

Yes please

@MuntashirAkon
Copy link
Author

MuntashirAkon commented Apr 24, 2023

Yes please

app-release.apk.zip

(Just remove the zip extension)

@REAndroid
Copy link
Owner

app-release.apk.zip

Is this compressed by archive2 ?

@MuntashirAkon
Copy link
Author

Is this compressed by archive2 ?

No, this is just an example APK file which archive2 fails to open.

@REAndroid
Copy link
Owner

REAndroid commented Apr 24, 2023

Nice check this commit and this

@MuntashirAkon
Copy link
Author

All working fine with ZipAlignVerifier. So far, I've only tested reading the archive files. I don't know about writing though.

@MuntashirAkon
Copy link
Author

MuntashirAkon commented Apr 25, 2023

Here's the method I used for aligning an APK file:

    public static void align(File input, File output) throws IOException {
        File dir = output.getParentFile();
        if (dir != null && !dir.exists()) {
            dir.mkdirs();
        }
        Archive archive = new Archive(input);
        try (ApkWriter apkWriter = new ApkWriter(output, archive.mapEntrySource().values())) {
            apkWriter.write();
        }
        if (!ZipAlignVerifier.verify(output, 4, true)) {
            throw new IOException("Could not verify aligned APK file.");
        }
    }

Before aligning the file, the verifier outputs the following error:

      39 .DS_Store (OK - compressed)
     480 AndroidManifest.xml (OK - compressed)
    2976 classes.dex (OK - compressed)
 1485993 lib/.DS_Store (OK - compressed)
 1486449 lib/arm64-v8a/libcmph.so (BAD - 3697)
Verification FAILED

After aligning the file, I still get a failure:

      39 .DS_Store (OK - compressed)
     480 AndroidManifest.xml (OK - compressed)
    2976 classes.dex (OK - compressed)
 1485993 lib/.DS_Store (OK - compressed)
 1486902 lib/arm64-v8a/libcmph.so (BAD - 54)
Verification FAILED

Conclusion
Alignment in archive2 is not working.

@REAndroid
Copy link
Owner

REAndroid commented Apr 25, 2023

Thanks for your excellent testing report!
As on this commit @Axelen123 suggested that all native libraries must have alignment of 4096

You can control each file alignment as:

    public static void align(File input, File output) throws IOException {
        File dir = output.getParentFile();
        if (dir != null && !dir.exists()) {
            dir.mkdirs();
        }        
        Archive archive = new Archive(input);
        ApkWriter apkWriter = new ApkWriter(output, archive.mapEntrySource().values());
        
        ZipAligner zipAligner = new ZipAligner();
        zipAligner.setDefaultAlignment(4);
        zipAligner.clearFileAlignment();

        // see ZipAligner.apkAligner();
        
        apkWriter.write();
        if (!ZipAlignVerifier.verify(output, 4, true)) {
            throw new IOException("Could not verify aligned APK file.");
        }
    }

NB: Updated with this commit

@Axelen123
Copy link
Contributor

The aligner for archive2 should work properly in #31.

@MuntashirAkon
Copy link
Author

Getting this error:

java.lang.ArrayIndexOutOfBoundsException: src.length=102400 srcPos=0 dst.length=16 dstPos=0 length=-1
	at java.lang.System.arraycopy(System.java:524)
	at com.reandroid.archive2.io.FileChannelInputStream.readBuffer(FileChannelInputStream.java:84)
	at com.reandroid.archive2.io.FileChannelInputStream.read(FileChannelInputStream.java:62)
	at com.reandroid.archive2.block.ZipHeader.readBasic(ZipHeader.java:45)
	at com.reandroid.archive2.block.ZipHeader.readBytes(ZipHeader.java:33)
	at com.reandroid.archive2.model.LocalFileDirectory.visitLocalFile(LocalFileDirectory.java:68)
	at com.reandroid.archive2.model.LocalFileDirectory.visit(LocalFileDirectory.java:41)
	at com.reandroid.archive2.Archive.<init>(Archive.java:47)
	at com.reandroid.archive2.Archive.<init>(Archive.java:62)

@REAndroid
Copy link
Owner

Did you checked this commit

@MuntashirAkon
Copy link
Author

Did you checked this commit

Thanks. All known issues seem to have been fixed.

@MuntashirAkon
Copy link
Author

For future reference, the ZipAlign helper class I've used in my project is located here. The API is identical to the AOSP zipalign.

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

No branches or pull requests

3 participants