Add Zip Archives password support#122093
Open
alinpahontu2912 wants to merge 94 commits into
Open
Conversation
…assword and unprotected
… with cryptography lib
This was referenced Dec 2, 2025
Open
Member
Author
|
Hey @bartonjs can you take a look again ? |
bartonjs
requested changes
May 8, 2026
| } | ||
| else | ||
| { | ||
| // For decryption: HMAC first (on ciphertext), then XOR |
Member
There was a problem hiding this comment.
Since this data protocol correctly uses Encrypt-then-MAC (EtM), best practice (and internal security requirements) dictates that you should verify the MAC entirely before doing any decryption. As far as I can tell, this implementation allows decrypted data to be processed by a caller before the MAC is verified.
While that is the more performant approach, it will require a security process exception (ideally before the code is committed). Alternatively, change to verifying the MAC beforehand.
Maybe that discussion was already and and I'm not privy to it, but it's a flaw in implementation that I see here. cc: @blowdart @GrabYourPitchforks
Comment on lines
+17
to
+18
| private const string TestPassword = "test-password"; | ||
| private const ushort PasswordVerifier = 0x1234; |
Comment on lines
+19
to
+20
| private const string TestPassword = "test-password"; | ||
| private const ushort PasswordVerifier = 0x1234; |
Comment on lines
+37
to
+38
| private const string TestPassword = "test-password"; | ||
|
|
Comment on lines
+972
to
+975
| public async Task DecryptEntries_SamePassword_7Zip(bool async) | ||
| { | ||
| string password = "S3cur3P@ssw0rd"; | ||
| using Stream archiveStream = await StreamHelpers.CreateTempCopyStream(passwordProtected("PasswordProtected_7ZIP_SamePassword.zip")); |
Comment on lines
+178
to
+180
| public System.Threading.Tasks.Task<System.IO.Stream> OpenAsync(System.IO.FileAccess access, System.ReadOnlySpan<char> password, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } | ||
| public System.Threading.Tasks.Task<System.IO.Stream> OpenAsync(System.IO.FileAccess access, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } | ||
| public System.Threading.Tasks.Task<System.IO.Stream> OpenAsync(System.ReadOnlySpan<char> password, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; } |
Comment on lines
+778
to
+786
| var crcStream = GetDataCompressor(encryptionStream, leaveBackingStreamOpen: true, onClose: null, streamForPosition: _archive.ArchiveStream); | ||
| await using (crcStream.ConfigureAwait(false)) | ||
| { | ||
| _storedUncompressedData.Seek(0, SeekOrigin.Begin); | ||
| await _storedUncompressedData.CopyToAsync(crcStream, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| // Restore CompressionMethod - AesCompressionMethodValue is used directly when writing headers | ||
| CompressionMethod = savedMethod; |
Comment on lines
+834
to
+845
| BitFlagValues savedFlags = _generalPurposeBitFlag; | ||
| ZipEncryptionMethod savedEncryption = Encryption; | ||
| ZipCompressionMethod savedCompressionMethod = CompressionMethod; | ||
|
|
||
| // For AES entries: set CompressionMethod to Aes so header writes method 99, | ||
| // but clear _encryptionMethod so WriteLocalFileHeaderAsync doesn't create a new | ||
| // AES extra field (the original one in _lhUnknownExtraFields will be used). | ||
| if (savedEncryption is ZipEncryptionMethod.Aes128 or ZipEncryptionMethod.Aes192 or ZipEncryptionMethod.Aes256) | ||
| { | ||
| CompressionMethod = (ZipCompressionMethod)WinZipAesMethod; | ||
| Encryption = ZipEncryptionMethod.None; | ||
| } |
Comment on lines
+131
to
+135
| if (overwrite && File.Exists(destinationFileName)) | ||
| { | ||
| tempPath = Path.GetTempFileName(); | ||
| extractPath = tempPath; | ||
| } |
Comment on lines
+137
to
+146
| // When overwriting, extract to a temporary file first to avoid corrupting the destination file | ||
| // if an exception occurs during extraction (e.g., password-protected archive, corrupted data). | ||
| string extractPath = destinationFileName; | ||
| string? tempPath = null; | ||
|
|
||
| if (overwrite && File.Exists(destinationFileName)) | ||
| { | ||
| tempPath = Path.GetTempFileName(); | ||
| extractPath = tempPath; | ||
| } |
| ThrowIfInvalidArchive(); | ||
| ValidateAccessForMode(access); | ||
|
|
||
| if (IsEncrypted && password.IsEmpty) |
Comment on lines
+242
to
+247
|
|
||
| ZipArchiveEntry lastEntry = _entries[_entries.Count - 1]; | ||
| if (lastEntry.IsEncrypted) | ||
| { | ||
| await lastEntry.ReadEncryptionSaltIfNeededAsync(cancellationToken).ConfigureAwait(false); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1545
Big Milestones and status: