Add ZipCrypto reading support #115

BenjaminRi · 2019-10-19T23:22:14Z

Added two new functions to the API ( resolves #64 ):

ZipArchive::by_name_decrypt
ZipArchive::by_index_decrypt

These functions behave identically to their already existing counterparts without _decrypt, but take a password in the form of &[u8]. The password is not a &str because the ZipCrypto standard does not define which encoding is used, so UTF-8 cannot represent all possible passwords (for details, please read the comments in zipcrypto.rs).

The functions integrate seamlessly with the existing API. Substantial restructuring was necessary to insert the CryptoReader between the other readers. The code has been modified to facilitate the addition of further crypto or decompression routines.

Note that the API for read_zipfile_from_stream does not yet support ZipCrypto reading. However, this is not hard to add. Also, ZipCrypto writing support has not been added at this point in time. I want to make sure that my work is in line with what the authors and the users expect before I continue working on this.

In addition to that, a ZipCrypto writer will need cryptographically secure randomness (even though the crypto is weak). Where this notoriously hard to get randomness comes from is up for discussion.

BenjaminRi · 2019-10-27T22:49:54Z

@mvdnes Any thoughts on this PR? I'm willing to invest time and fix issues if needed.

BenjaminRi · 2020-05-21T22:18:42Z

Merged improvements from master into pkzip-cipher feature branch. Added unit test to verify correct ZipCrypto reader implementation. I have been using the feature branch regularly for half a year, and no problems were found so far. If there is interest to have ZipCrypto on master, I'm still open for feedback and willing to invest time.

Plecra · 2020-06-16T16:19:32Z

This is brilliant! I hope we can get this merged soon. Would it be possible to support Central Directory Encryption with this implementation?

@rylev and I have recently been given permission to accept PRs, and we should be able to give this a proper review soon.

BenjaminRi · 2020-06-16T18:48:12Z

Would it be possible to support Central Directory Encryption with this implementation?

Technically, yes. The ZipCrypto algorithm stands for itself and can be used to decrypt any binary stream, operating on the std::io::Read trait* (I also considered creating a separate crate just for ZipCrypto, but that's probably overkill). However, I am not sure if there is any ZIP software in existence that uses the weak ZipCrypto cipher to encrypt the Central Directory (relevant: zlib-ng/minizip-ng#141 ).

A higher value task would be to support ZIP files with strong crypto like AES encryption which is believed to provide real security. It seems like Central Directory Encryption is often (always?) done with Strong Encryption. The encryption method of choice is almost always AES because all the alternatives in the specification are deprecated & insecure by now (RC2, RC4, DES, 3DES). AES encrypted ZIP files are somewhat common.

However, I think Strong Encryption should be treated in a separate issue/PR because it's a separate subject that requires its own considerations. The restructuring of the reader that I've done in this PR can be used to hook any other crypto algorithm into the reader pipeline, so it's a good preparation for further work.

*Also, encryption is almost finished, it just needs to get cryptographically secure randomness from somewhere.

Plecra

LGTM! Could you run a cargo fmt on the changes?

BenjaminRi · 2020-06-21T13:13:02Z

LGTM! Could you run a cargo fmt on the changes?

Thanks! Do you mean just running cargo fmt on the changes or also additionally merging my branch such that it is up to date with master (26 commits happened in the meantime)?

BenjaminRi · 2020-06-21T15:55:07Z

@Plecra Merged all the improvements from master. Applied cargo fmt.

Plecra

Ah, sorry to be a pain, but I don't want to erase the commit history - I'd rather rebase the changes. I can handle that tomorrow if you'd like 😃

BenjaminRi · 2020-06-21T22:40:22Z

Ah, sorry to be a pain, but I don't want to erase the commit history - I'd rather rebase the changes. I can handle that tomorrow if you'd like 😃

How does merging erase the commit history? The merge from master pulls in the entire commit history as well.

I used to rebase my changes, but for read.rs the process was so grueling that after a few failed attempts during a difficult rebase operation, I started to do merges only (maybe that is also an indication that this file needs to be split up). The merge in read.rs is not trivial. I'd appreciate it a lot if you could handle it if you think rebasing is the way to go. You should arrive at the same resulting source files as I did after the merge.

rylev

Some small things. We can merge this, but if you're up for it, it would be great to fix the nitpicks before that.

rylev · 2020-06-23T14:14:31Z

src/read.rs

        if file_number >= self.files.len() {
            return Err(ZipError::FileNotFound);
        }
-        let data = &mut self.files[file_number];
+        let ref mut data = self.files[file_number];


I think we usually use the previous syntax. Is there a reason this was changed?

No, that was an oversight during the merge. Fixed.

rylev · 2020-06-23T14:14:52Z

src/read.rs

+        self.by_name_internal(name, None)
+    }
+
+    fn by_name_internal<'a>(


I'd rather call this by_name_with_optional_password

Okay. It's a bit long, but the name is clearer. Changed it to by_name_with_optional_password and by_index_with_optional_password.

rylev · 2020-06-23T14:17:39Z

src/read.rs

-        if data.encrypted {
-            return unsupported_zip_error("Encrypted files are not supported");
+        if password == None {
+            if data.encrypted {


I think this would be easier to follow like this:

match (password, data.encrypted) { (None, true) => return Err(..), (Some(_), false) => password = None, _ => {} }

Back when I wrote this code, I didn't even know this was possible with matcher clauses (I only learned that a week ago). Looks elegant, I applied that change.

rylev · 2020-06-23T14:18:22Z

src/zipcrypto.rs

+}
+
+impl ZipCryptoKeys {
+    //Used this paper to implement ZipCrypto algo


Can this be part of the module documentation?

Improved & fixed docstrings throughout the entire zipcrypto.rs document. Added module level documentation.

rylev · 2020-06-23T14:19:02Z

src/zipcrypto.rs

+
+impl<R: std::io::Read> ZipCryptoReader<R> {
+    pub fn new(file: R, password: &[u8]) -> ZipCryptoReader<R> {
+        //Note: The password is &[u8] and not &str because the documentation


nit: comments should have a space between the // and the first character

rylev · 2020-06-23T14:21:14Z

tests/zip_crypto.rs

+        // No password
+        let file = archive.by_index(0);
+        assert!(file.is_err());
+        if let Err(error) = file {


You can match inside of the Err as well so there's no need for the match expression below

Turned if expressions into cleaner matcher clause.

rylev · 2020-06-23T14:21:31Z

tests/zip_crypto.rs

+        if let Err(error) = file {
+            match error {
+                zip::result::ZipError::PasswordRequired => (),
+                _ => panic!(),


Can we add some descriptive text to these panics?

Added some meaningful panic messages.

rylev · 2020-06-23T14:22:19Z

tests/zip_crypto.rs

+
+    {
+        // Wrong password
+        let file = archive.by_index_decrypt(0, "wrong password".as_bytes());


nit: you can use b"wrong password" instead of as_bytes

…_index_internal` to `by_index_with_optional_password`

BenjaminRi · 2020-06-23T20:23:42Z

@rylev Thanks for the detailed review. I applied all your suggestions.

Plecra · 2020-06-23T20:50:09Z

Thanks for following up on that! I've created #157 to discuss more encryption features if you'd like to share your thoughts 😉

BenjaminRi · 2020-06-23T21:11:02Z

Fantastic! I'm very happy to see this PR accepted and merged 🎉

Benjamin Richner added 4 commits October 19, 2019 18:59

Added ZipCrypto support for stored and deflated files

f9f5029

Enable decryption with file reader by_name()

1f87098

Remove useless return in ZipCryptoReaderValid::read()

2a51123

Ignore password if file not encrypted. Clean up code structure.

744bb41

BenjaminRi changed the title ~~Added ZipCrypto support ( resolves #64 )~~ Add ZipCrypto reading support Oct 27, 2019

BenjaminRi force-pushed the pkzip-cipher branch from 848325d to 744bb41 Compare April 14, 2020 23:59

Benjamin Richner added 3 commits May 21, 2020 21:58

merge edition 2018 and other improvements from master

cf7ff17

Clean up whitespaces

f9116fc

Add ZipCrypto unit test

402ecb1

Plecra requested changes Jun 21, 2020

View reviewed changes

Benjamin Richner added 2 commits June 21, 2020 17:43

Merge improvements from master

bd3ed22

run cargo fmt

493afdb

BenjaminRi requested a review from Plecra June 21, 2020 15:52

Plecra approved these changes Jun 21, 2020

View reviewed changes

rylev approved these changes Jun 23, 2020

View reviewed changes

Benjamin Richner added 8 commits June 23, 2020 20:44

Merge improvements from master

dbae003

run cargo fmt

d50dcac

Use let .. = &mut instead of let ref mut .. =

07caeca

Rename by_name_internal to by_name_with_optional_password and `by…

fb4f3c5

…_index_internal` to `by_index_with_optional_password`

Use matcher clause instead of clumsy if expressions

4f36a27

Add space between comment and //

7677735

run cargo fmt

d410019

Use b prefix instead of as_bytes()

4737f14

Benjamin Richner added 3 commits June 23, 2020 21:35

Create meaningful panic messages in ZipCrypto test

9e2b14f

Turned clumsy if statements into matcher clause

2730ca7

Fixed and improved docstrings, added module level documentation

0b077c4

Plecra merged commit f99cdd0 into zip-rs:master Jun 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ZipCrypto reading support #115

Add ZipCrypto reading support #115

BenjaminRi commented Oct 19, 2019 •

edited

BenjaminRi commented Oct 27, 2019

BenjaminRi commented May 21, 2020

Plecra commented Jun 16, 2020 •

edited

BenjaminRi commented Jun 16, 2020

Plecra left a comment

BenjaminRi commented Jun 21, 2020

BenjaminRi commented Jun 21, 2020

Plecra left a comment

BenjaminRi commented Jun 21, 2020

rylev left a comment

rylev Jun 23, 2020

BenjaminRi Jun 23, 2020

rylev Jun 23, 2020

BenjaminRi Jun 23, 2020

rylev Jun 23, 2020

BenjaminRi Jun 23, 2020

rylev Jun 23, 2020

BenjaminRi Jun 23, 2020

rylev Jun 23, 2020

BenjaminRi Jun 23, 2020

rylev Jun 23, 2020

BenjaminRi Jun 23, 2020

rylev Jun 23, 2020

BenjaminRi Jun 23, 2020

rylev Jun 23, 2020

BenjaminRi Jun 23, 2020

BenjaminRi commented Jun 23, 2020

Plecra commented Jun 23, 2020

BenjaminRi commented Jun 23, 2020

Add ZipCrypto reading support #115

Add ZipCrypto reading support #115

Conversation

BenjaminRi commented Oct 19, 2019 • edited

BenjaminRi commented Oct 27, 2019

BenjaminRi commented May 21, 2020

Plecra commented Jun 16, 2020 • edited

BenjaminRi commented Jun 16, 2020

Plecra left a comment

Choose a reason for hiding this comment

BenjaminRi commented Jun 21, 2020

BenjaminRi commented Jun 21, 2020

Plecra left a comment

Choose a reason for hiding this comment

BenjaminRi commented Jun 21, 2020

rylev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenjaminRi commented Jun 23, 2020

Plecra commented Jun 23, 2020

BenjaminRi commented Jun 23, 2020

BenjaminRi commented Oct 19, 2019 •

edited

Plecra commented Jun 16, 2020 •

edited