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

Upgrade pyzipper to use cryptography. #31

Closed
wants to merge 2 commits into from
Closed

Upgrade pyzipper to use cryptography. #31

wants to merge 2 commits into from

Conversation

Natim
Copy link

@Natim Natim commented Apr 14, 2023

The goal of this PR is to provide a pyzipper implementation based on Cryptography rather than pycryptodome.

Refs #30

pyzipper/zipfile_aes.py Outdated Show resolved Hide resolved
@BjarniRunar
Copy link

Hello, I have attempted this same thing, but came to the conclusion that it was impossible to get this to work in a way that is compatible with WinZip, 7z, and others without sacrificing performance and resorting to horrible hacks.

The reason is as follows:

  1. cryptography uses openssl under the hood
  2. openssl does not expose a mechanism for implementing a custom counter for AES CTR mode.
  3. The openssl CTR mode counter is hard-coded to use a big-endian counter
  4. WinZip compatible AES encryption CTR mode uses little-endian counter

Although it is possible to implement workarounds, do so requires initializing the entire AES state from scratch for every block, which is probably absolutely awful for performance.

Further discussion (and a demo of the workaround), see this Stackoverflow discussion: https://stackoverflow.com/questions/69144760/aes-ctr-decryption-cryptography-and-cryptodome-give-different-results/69145945#69145945

So based on my research and a quick scan of the code, this pull request will break compatibility with WinZip.

@Natim
Copy link
Author

Natim commented Apr 24, 2023

Thank you for your answer.

@Natim Natim closed this Apr 24, 2023
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