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

what are the blockers for reassembling ZIPs like apksigner does? #35

Closed
eighthave opened this issue Apr 13, 2021 · 31 comments
Closed

what are the blockers for reassembling ZIPs like apksigner does? #35

eighthave opened this issue Apr 13, 2021 · 31 comments
Assignees
Labels
enhancement New feature or request

Comments

@eighthave
Copy link

eighthave commented Apr 13, 2021

apksigner assembles a whole new ZIP file when signing, including copying the contents, metadata, and even the odd padding that is added in one or more ways. Right now, apksigcopier can copy the contents, order, metadata, and extra field padding. What are the parts that apksigner can copy that apksigcopier cannot?

I guess we're seeing clearly why apksigner uses its own ZIP implementation, so we might not be able to do exactly what it does by using zipfile directly.

@obfusk obfusk self-assigned this Apr 13, 2021
@obfusk obfusk added the enhancement New feature or request label Apr 13, 2021
@obfusk
Copy link
Owner

obfusk commented Apr 13, 2021

AFAIK

  • Copying ZIP entries whose file size was not known at time of writing only works by accident (and can fail in rare cases). I'm working on that now.
  • I'm not trying to handle every possible ZIP file in existence, just ZIP files likely to be found in the wild as APKs. I don't know how much of the various extensions etc. apksigner handles. Or Python's ZipFile for that matter.

@obfusk
Copy link
Owner

obfusk commented Apr 13, 2021

FYI apksigner will refuse to sign APKs with duplicate entries:

$ apksigner sign ... urzip-twosig.apk
Exception in thread "main" com.android.apksig.apk.ApkFormatException: Multiple ZIP entries with the same name: META-INF/MANIFEST.MF

@obfusk
Copy link
Owner

obfusk commented Apr 13, 2021

I'm also not trying to handle "corrupt" zip files (with e.g. incorrect internal length fields).

Not sure how apksigner or Python handle those.

@obfusk
Copy link
Owner

obfusk commented Apr 13, 2021

The only thing I would currently consider "blocking" is the "file size was not known at time of writing" one.

@obfusk
Copy link
Owner

obfusk commented Apr 13, 2021

See also #19.

@eighthave
Copy link
Author

As far as I understand the apksigner code, it just copies as much as possible. That's different than ZipFile, which aims to parse and represent the data. So that means apksigner does not even have to be aware of ZIP extensions or fields since it is just copying bytes from valid places in the ZIP format.

@obfusk
Copy link
Owner

obfusk commented Apr 13, 2021

As far as I understand the apksigner code, it just copies as much as possible. That's different than ZipFile, which aims to parse and represent the data. So that means apksigner does not even have to be aware of ZIP extensions or fields since it is just copying bytes from valid places in the ZIP format.

It needs to do much less, this is true. But it still needs to parse the headers and various length fields etc. to do its job.

And I don't know enough about the specifics of ZIP extensions to know whether they could possibly change the interpretations of fields that apksigner/apksigcopier does need to use.

@obfusk
Copy link
Owner

obfusk commented Apr 13, 2021

apksigner still uses a ZIP file parser (com.android.apksig.internal.zip.*).

@obfusk
Copy link
Owner

obfusk commented Apr 13, 2021

Whether that parser and Python's ZipFile have any relevant differences, I don't know.

@obfusk
Copy link
Owner

obfusk commented Apr 13, 2021

The only thing I would currently consider "blocking" is the "file size was not known at time of writing" one.

This should be fixed. Testing it now on all the previously tested APKs to see if anything breaks.

@obfusk obfusk pinned this issue Apr 13, 2021
@eighthave
Copy link
Author

apksigner still uses a ZIP file parser (com.android.apksig.internal.zip.*).
Whether that parser and Python's ZipFile have any relevant differences, I don't know.

The apksigner and ZipFile parsers are quite different. For example, ZipFile lets you operate using date/time tuples which is a normal representation for dates in Python. apksigner just treats the date/time fields as bytes to copy. apksigner parses only enough of ZIP to navigate the bits it needs. ZipFile represents ZIP files in a way that is normal to Python code, so it is parsing the metadata and changing its representation.

@obfusk
Copy link
Owner

obfusk commented Apr 13, 2021

apksigner still parses the fields to some extent.

apksigcopier similarly relies on ZipInfo to provide the necessary metadata, and then intentionally copies the data from the file instead of re-encoding the Python representation.

@eighthave
Copy link
Author

eighthave commented Apr 13, 2021

It seems that jarsigner also rejects signing twosig.apk. Unfortunately, this does not trigger any more memories on the details of that test. It could be that it is more to maintain compatibility and ease of use rather than security. E.g. just try stripping all signature materials and copying the signature, and let the final verification be the test.

$ jarsigner -verbose -sigalg SHA1withRSA -digestalg SHA1 -keystore /home/hans/.android/release-keystore.jks ./.testfiles/test_verify_apksld4pqcx2/urzip-twosig.apk release -storepass lasdjfkhasdkjfhkjasfdh
 updating: META-INF/MANIFEST.MF
   adding: META-INF/RELEASE.SF
   adding: META-INF/RELEASE.RSA
   adding: META-INF/CERT.SF
   adding: META-INF/CERT.SF
jarsigner: unable to sign jar: java.util.zip.ZipException: duplicate entry: META-INF/CERT.SF

@obfusk
Copy link
Owner

obfusk commented Apr 13, 2021

Testing it now on all the previously tested APKs to see if anything breaks.

CI & tests pass :)

@obfusk
Copy link
Owner

obfusk commented Apr 13, 2021

So is the twosig test really worth the significant extra effort required to make it pass? Because I'm really not seeing how that provides any real world value. At least not until testing reveals otherwise.

@obfusk
Copy link
Owner

obfusk commented Apr 13, 2021

Note that according to http://www.saurik.com/id/17 Android will also reject APKs with multiple signatures IIUC.

@obfusk
Copy link
Owner

obfusk commented Apr 13, 2021

At this point I think the question becomes whether handling every possible ZIP file under the sun exactly the same way apksigner does is worth it, especially if those that are handled differently would be rejected by Android anyway.

@obfusk
Copy link
Owner

obfusk commented Apr 13, 2021

At this point I think apksigcopier handles any reasonable input AFAIK.

@obfusk
Copy link
Owner

obfusk commented Apr 13, 2021

Based on the current understanding of the usefulness, I'm not planning to support APKs with duplicate entries in apksigcopier.

You are of course free to remove them before calling apksigcopier from fdroidserver. But since it's non-trivial to implement I'd appreciate it if someone else does that if you really want that functionality.

@obfusk
Copy link
Owner

obfusk commented Apr 13, 2021

it's non-trivial to implement

You can't just call apk_strip_v1_signatures() since that would modify the unsigned APK, so you need to make (and shuffle around) a copy.

@obfusk
Copy link
Owner

obfusk commented Apr 13, 2021

I don't think there are currently any relevant differences between apksigner and apksigcopier for any reasonable input.

And I don't think there any issues with unreasonable input. There might be differences, but I don't think apksigner handles all possible input "nicely" (I've seen several NullPointerExceptions). And any unreasonable input should subsequently fail to validate.

@eighthave
Copy link
Author

If apksigner can make a perfect copy of the ZIP, I don't see why the Python code can't use the same algorithm to strip existing signatures while maintaining the rest. Do you have specific APKs where this fails? I only did a couple tests, but it worked there. What am I missing here?

@obfusk
Copy link
Owner

obfusk commented Apr 13, 2021

Do you have specific APKs where this fails?

No. That was my point. I can't guarantee the implementations are equal, but they seem to behave identically for any APK I've seen.

@obfusk
Copy link
Owner

obfusk commented Apr 13, 2021

FYI the reason the Python code failed on the duplicate entries in the twosig is the way it is generated. The actual ZIP file metadata is wrong:

$ zipinfo -v urzip-twosig.apk | grep -A2 META 
  META-INF/MANIFEST.MF

  offset of local header from start of archive:   8325
--
  META-INF/MANIFEST.MF

  offset of local header from start of archive:   8325
--
  META-INF/CERT.SF

  offset of local header from start of archive:   9009
--
  META-INF/CERT.SF

  offset of local header from start of archive:   9009
--
  META-INF/CERT.RSA

  offset of local header from start of archive:   10020
--
  META-INF/CERT.RSA

  offset of local header from start of archive:   10020

@obfusk
Copy link
Owner

obfusk commented Apr 13, 2021

@eighthave do we agree that there are no known blockers now?

@eighthave
Copy link
Author

Sounds like it then. The open question is how to integrate it into the existing fdroidserver code and workflow.

The tricky part is that ZIP tools can work with that file:

$ unzip -l ./.testfiles/test_verify_apkstua6tgfp/urzip-twosig.apk
Archive:  ./.testfiles/test_verify_apkstua6tgfp/urzip-twosig.apk
  Length      Date    Time    Name
---------  ---------- -----   ----
     1413  2013-10-15 18:21   res/drawable/ic_launcher.png
      588  2013-10-15 18:21   res/layout/activity_main.xml
     4636  2013-10-15 18:21   AndroidManifest.xml
     1172  2013-10-15 18:21   resources.arsc
     7336  2013-10-15 18:21   classes.dex
      417  2013-10-15 18:21   META-INF/MANIFEST.MF
      417  2013-10-15 18:21   META-INF/MANIFEST.MF
      470  2013-10-15 18:21   META-INF/CERT.SF
      470  2013-10-15 18:21   META-INF/CERT.SF
     2176  2013-10-15 18:21   META-INF/CERT.RSA
     2176  2013-10-15 18:21   META-INF/CERT.RSA
---------                     -------
    21271                     11 files

@obfusk
Copy link
Owner

obfusk commented Apr 13, 2021

The open question is how to integrate it into the existing fdroidserver code and workflow.

I fixed the code that generates the twosig APK, allowing apksigcopier to handle it. The original tests now pass. See the fdroidserver MR.

@obfusk
Copy link
Owner

obfusk commented Apr 13, 2021

The tricky part is that ZIP tools can work with that file

Listing is easy. I'm not sure apksigner would be able to work with it (if it didn't refuse to b/c of the dups).

@eighthave
Copy link
Author

I was just going to say, the core idea here is "be conservative in what you send, be liberal in what you accept" https://en.wikipedia.org/wiki/Robustness_principle where "accepting" is receiving APKs, and "sending" is running apksigner verify on them.

@obfusk
Copy link
Owner

obfusk commented Apr 13, 2021

Can we close this now?

@obfusk obfusk unpinned this issue Apr 13, 2021
@obfusk
Copy link
Owner

obfusk commented Apr 14, 2021

Can we close this now?

I'm assuming "yes". Let me know if I missed something.

@obfusk obfusk closed this as completed Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants