Skip to content

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

Attention: Patch coverage is 26.41509% with 39 lines in your changes missing coverage. Please review.

Project coverage is 72.98%. Comparing base (5eea92a) to head (164c452).
Report is 78 commits behind head on master.

Files with missing lines Patch % Lines
...e.SharpZipLib/Encryption/ZipAESEncryptionStream.cs 0.00% 35 Missing ⚠️
src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs 77.77% 4 Missing ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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