Support AES decryption #203
Conversation
|
@mbr can you please fix the conflicts? :) Then, I hope, such useful feature will be merged. |
|
All conflicts have been resolved. The MSRV is now |
|
The branch is up-to-date with master and could be merged. I added some simple tests which test the different AES encryption schemes (128/192/256) and whether they work with compressed and uncompressed files. Benchmarks would be quite interesting but would require some archive to be decrypted. That would mean either implementing encryption, but I don't know enough about cryptography to give any guarantees about security, or including a benchmark file.
Edit: Updating all |
Cargo.toml
Outdated
| @@ -11,11 +11,16 @@ Library to support the reading and writing of zip files. | |||
| edition = "2018" | |||
|
|
|||
| [dependencies] | |||
| flate2 = { version = "1.0.0", default-features = false, optional = true } | |||
| time = { version = "0.3", features = ["formatting", "macros" ], optional = true } | |||
| aes = { version = "0.6.0", optional = true } | |||
There was a problem hiding this comment.
What about using the latest aes version 0.7.5?
There seems to be breaking changes in the API.
Thank you for the hard work!
There was a problem hiding this comment.
I tried updating aes, pbkdf2, hmac and sha-1 (now sha1) together since they all use the same dependencies. Since this required some small changes to the decryption code, I also put in some more (possibly meaningless) checks.
This increased the performance of AES-128 from 42 MB/s to around 160 - 180 MB/s.
|
Just approved for running the CI |
|
The new commit changes the name of |
|
@Lireer please ping me or @Plecra when you will finish the work. By the way, maybe a more convenient way for discussing the PR will be our new Discord server - https://discord.gg/rQ7H9cSsF4 ? :) |
|
@zamazan4ik I consider this PR finished in the sense that it does what it's supposed to do, add aes decryption support. Things like benchmarks, aes encryption, and possible performance improvements can be added in other PRs. |
|
@Plecra if you have no additional comments (and I am really not sure regarding my comments - maybe it's intended design to not move these values under the feature), I want to merge this PR. |
|
LGTM |
src/aes.rs
Outdated
| &vec![0; <Hmac<Sha1> as KeySizeUser>::KeySize::to_usize()], | ||
| )), | ||
| ); | ||
| let computed_auth_code = &hmac.finalize().into_bytes()[0..AUTH_CODE_LENGTH]; |
There was a problem hiding this comment.
We can use finalize_reset here to make this simpler to read
There was a problem hiding this comment.
Before I realized that the Reset trait is now feature-gated, this was the only way to make it work. After I realized that finalize_reset can still be used I left it like this, because I thought the Hmac containing the password would be dropped earlier like this. But I guess it probably doesn't improve security at all, so making it more readable is more important.
src/aes_ctr.rs
Outdated
| /// XORs a slice in place with another slice. | ||
| #[inline] | ||
| fn xor(dest: &mut [u8], src: &[u8]) { | ||
| debug_assert_eq!(dest.len(), src.len()); |
There was a problem hiding this comment.
Worth exploring how the compiler optimizes this if we make it an assert_eq
There was a problem hiding this comment.
Doesn't seem to affect performance all that much. I will try with criterion if that makes it more noticable. Bencher runs report 2130 - 2160 MB/s with debug_assert_eq and 2120 - 2130 MB/s with assert_eq. Note the over 2000 MB/s, which is substantially more than the previously reported 170 MB/s. Differences to previous benchmarks are: Windows instead of NixOs and a newer CPU (probably the reason). The other benchmark runs about 50% faster too.
There was a problem hiding this comment.
Seems like badly written benchmark code was the real reason for the performance improvement. But the criterion benchmarks with high sample sizes don't show any significant changes when decrypting a ZipFile.
src/aes_ctr.rs
Outdated
| // `7z a -phelloworld -mem=AES256 -mx=0 aes256_40byte.zip 40byte_data.txt` | ||
| #[test] | ||
| fn crypt_aes_256_0_byte() { | ||
| let ciphertext = []; |
There was a problem hiding this comment.
The common part of these tests should be in its own function. Probably roundtrips(&[], b"", [0x0b, ...])
|
@zamazan4ik @Plecra thanks for the reviews, if you don't have any other comments the PR should be ready to be merged. |
|
ack! my bad for not being clearer, but we do need to remove |
This PR adds support for decrypting AES encrypted zip archives (compression method 99). It implements the special non-NIST variant of the CTR mode used by WinZip, but otherwise uses off-the-shelf crypto crates.
The feature itself is hidden behind an
aesfeature flag. We will leave it up to upstream to enable or disable it and/or mention it in the docs.Encryption can be added if desired, as the decryption methods can be used to encryption as well.