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

APK signature check v1/v2 using the apksig library from Google #431

Merged
merged 2 commits into from
Jan 18, 2019

Conversation

jpstotz
Copy link
Collaborator

@jpstotz jpstotz commented Jan 16, 2019

Perform APK signature verification vor v1 and v2 signatures and show the result in a new tree node.
Also the errors and warnings generated by apksig library are shown in a simple HTML based view.

@skylot
Copy link
Owner

skylot commented Jan 17, 2019

@jpstotz looks good.
List of questions/suggestions for future improvements:

  • can we delete Certificate node, because it duplicates functionality?
  • don't show Apk Signature for not apk files (dex, jar, etc)
  • use common font and color scheme from jadx settings for HtmlPanel
  • maybe improve strings localization (not all can be translated now)
  • maybe combine several warnings and errors by same type. For example many apps have a lot of such warnings with a tons of same strings:
META-INF/CHANGES not protected by signature. Unauthorized modifications to this JAR entry will not be detected. Delete or move the entry outside of META-INF/.
META-INF/README.md not protected by signature. Unauthorized modifications to this JAR entry will not be detected. Delete or move the entry outside of META-INF/.
... repeated for every files

better to restructure it to:

Entries not protected by signature. Unauthorized modifications to this JAR entry will not be detected. Delete or move the entry outside of META-INF/ :
 - META-INF/CHANGES
 - META-INF/README.md
 - ... list of other files

Copy link
Owner

@skylot skylot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some code improvements suggestions

@jpstotz
Copy link
Collaborator Author

jpstotz commented Jan 17, 2019

The signature page is now localized as long as the text is not coming from the apksig library.
Additionally HTML escaping was required. I found a nice HTML escaper in apache commons text that extends a String builder with escaping.

can we delete Certificate node, because it duplicates functionality?

For APK files yes, for plain JAR files it makes sense to keep the certificate nodes.

don't show Apk Signature for not apk files (dex, jar, etc)

Done.

use common font and color scheme from jadx settings for HtmlPanel

JEditorPane does not have a color scheme system. Applying a font to JEditorPane does not have any effect.

maybe improve strings localization (not all can be translated now)

Done

maybe combine several warnings and errors by same type. For example many apps have a lot of such warnings with a tons of same strings:

There is no general way to do so. I implemented it for the issue type JAR_SIG_UNPROTECTED_ZIP_ENTRY. The code that does so is not nice but it works.

@skylot skylot merged commit d1af751 into skylot:master Jan 18, 2019
@skylot
Copy link
Owner

skylot commented Jan 18, 2019

@jpstotz thank you, looks great!

@skylot
Copy link
Owner

skylot commented Jan 18, 2019

@jpstotz I found a strange thing in your commit. You add google maven repository but it doesn't contain the version of apksig you used (2.3.0), and this version fetched from mavenCentral. The latest stable version from google repository is 3.3.0 (see here) and this version also work without any changes in code.
So, this is a typo in version number or you did it intentionally and library can't be upgraded for some reason?

@jpstotz
Copy link
Collaborator Author

jpstotz commented Jan 18, 2019

@skylot That is typo. Originally I used a different library from the Google repo until I noticed that the signature verification functionality wasn't implemented in the library but in the apksig library which was a dependency of that library. When I changed to the apksig library I missed the fact that it is available in multiple repositories and took the wrong one.

@skylot
Copy link
Owner

skylot commented Feb 12, 2019

🎉 This PR is included in version 0.9.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

2 participants