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

[WIP] Add AES encryption support to ZipFile #443

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Numpsy
Copy link
Contributor

@Numpsy Numpsy commented Apr 11, 2020

A work-in-progress attempt to add AES encryption support to ZipFile.

Notes:

There is currently no way to tell ZipFile what encryption algorithm to use when adding entries, so the only way to add an AES entry is with the variant of Add() that takes a ZipEntry (and set the AESKeySize property on it). Perhaps needs to be considered together with #380.

I copied the AddExtraDataAES function from the zip stream code, it would be better being shared somewhere. (an extension method on ZipExtraData maybe?)

It tweaks the logic in GetOutputStream() to try to ensure that the crypto streams returned from it are disposed - perhaps could be a separate PR as it might be a sort of existing issue with it not disposing streams when it should?

The Write() overload in ZipAESEncryptionStream takes the same approach as ZipOutputStream and calls the crypto transform TransformBlock() on each write. I'm not sure if that is technically correct, or if it should store the data and transform it in complete blocks? (or perhaps it doesn't matter with the way the transform is implemented)

Doesn't yet add any new unit tests (would likely end up conflicting with other PRs in the same area, could maybe do with a pass over later to see what state the various separate encryption tests are in)

I certify that I own, and have sufficient rights to contribute, all source code and related material intended to be compiled or integrated with the source code for the SharpZipLib open source product (the "Contribution"). My Contribution is licensed under the MIT License.

@Numpsy
Copy link
Contributor Author

Numpsy commented Apr 19, 2020

With regards to the AddExtraDataAES bit - is there any value in adapting that into the TaggedData machinery (ExtendedUnixData/NTTaggedData etc) rather than having the structure inline at the points where it's read/written?

@Numpsy
Copy link
Contributor Author

Numpsy commented Aug 3, 2020

Some things to be decided:

  1. The API to use for enabling AES encryption.
    I was working Support AES encryption in FastZip.CreateZip #380 towards adding a common enum for both apis, so possibly a property of type ZipEncryptionMethod would be sufficient?

  2. The logix for when to 'apply' encryption to the entries (does it set the AESKeySize properties on new entries in the Add functions, as soon as the entries are added, or when the updates are commited, when it knows both the current setting and current password)

It also possibly needs checking for how it handles (or not) directory entries that have AESKeySize set - it doesn't do the encryption machinery for folders like ZipOutputStream does, so might need to review things like EncryptionMethodForHeader being used for directories.

@Numpsy Numpsy added encryption zip Related to ZIP file format labels Aug 3, 2020
@piksel
Copy link
Member

piksel commented Aug 4, 2020

  1. Yeah, I think that's ideal

  2. I would presume that setting it when committing the updates is less error-prone. Do you see any down sides to it?

@Numpsy
Copy link
Contributor Author

Numpsy commented Aug 4, 2020

I can't think of any, just need to document that entries are encrypted based on the value of the setting at that point (in case anyone gets the idea that you can add multiple entries with different and/or some/no encryption in one go).

The ZipCrypto case already decides what to do based on if the password is set when the updates are run, so it's consistent with that as well.

@codecov
Copy link

codecov bot commented May 2, 2021

Codecov Report

Merging #443 (164c452) into master (5eea92a) will decrease coverage by 0.25%.
The diff coverage is 26.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #443      +/-   ##
==========================================
- Coverage   73.24%   72.98%   -0.26%     
==========================================
  Files          68       69       +1     
  Lines        8712     8758      +46     
==========================================
+ Hits         6381     6392      +11     
- Misses       2331     2366      +35     
Impacted Files Coverage Δ
...e.SharpZipLib/Encryption/ZipAESEncryptionStream.cs 0.00% <0.00%> (ø)
src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs 77.70% <77.77%> (-0.11%) ⬇️
...ICSharpCode.SharpZipLib/BZip2/BZip2OutputStream.cs 79.19% <0.00%> (+0.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5eea92a...164c452. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
encryption zip Related to ZIP file format
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants