Skip to content

Improve Winzip AES Encryption HMAC verification - #135935

Closed
alinpahontu2912 wants to merge 4 commits into
dotnet:mainfrom
alinpahontu2912:winzip_hmac_double_pass
Closed

alinpahontu2912 wants to merge 4 commits into
dotnet:mainfrom
alinpahontu2912:winzip_hmac_double_pass

Conversation

@alinpahontu2912

Copy link
Copy Markdown
Member

Perform a double pass of the encrypted content of a zip entry created with WinZip AES Encryption. The first pass should only calculate the HMAC and validate it to ensure the content hasn't been tampered with while the second pass should perform the decryption and decompression of the content.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 6237b182-4cb5-44bd-8dcf-e460100ae342
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @karelz, @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Ciphertext modified between verification and decryption can produce altered plaintext without an authentication failure.

1 open finding
What changed in this PR

Moves WinZip AES authentication ahead of decryption and decompression in ZIP entry reads.

Changes:

  • Adds an upfront HMAC verification pass followed by a rewind.
  • Defers update-mode state changes until content loads successfully.
  • Adds authentication-failure, truncation, and rewind tests.
File Description
src/​libraries/​System.IO.Compression/​tests/​WinZipAesStreamConformanceTests.cs Tests verification, rewind, and truncated input.
src/​libraries/​System.IO.Compression/​src/​System/​IO/​Compression/​ZipCryptoStream.cs Uses the shared truncated-header resource.
src/​libraries/​System.IO.Compression/​src/​System/​IO/​Compression/​ZipArchiveEntry.cs Defers update-mode state and encryption setup.
src/​libraries/​System.IO.Compression/​src/​System/​IO/​Compression/​ZipArchiveEntry.Async.cs Adjusts asynchronous loading and key assignment.
src/​libraries/​System.IO.Compression/​src/​System/​IO/​Compression/​WinZipAesStream.cs Authenticates ciphertext before returning a decryption stream.
src/​libraries/​System.IO.Compression/​src/​Resources/​Strings.resx Generalizes the truncated-header resource name.
src/​libraries/​System.IO.Compression.ZipFile/​tests/​ZipFile.Encryption.cs Tests authentication failures when opening entries.

🧠 Review effort: Balanced

Comment on lines +304 to +306
_baseStream.Seek(encryptedDataStart, SeekOrigin.Begin);
_hmac.Dispose();
_hmac = null;
@alinpahontu2912
alinpahontu2912 requested a review from rzikm October 16, 2026 09:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Failed write-only opens can discard existing encrypted content, and the empty-payload test does not exercise zero ciphertext.

2 open findings
Previously missed (1)

In code that hasn't changed since last review

Medium severity Empty-entry fixture writes a UTF-8 BOM and misses zero-ciphertext validation

src/​libraries/​System.IO.Compression.ZipFile/​tests/​ZipFile.Encryption.cs:382

This fixture is not empty: CreateArchiveWithEntries uses StreamWriter with Encoding.UTF8, which writes a three-byte byte-order mark even for an empty string. The test therefore misses authentication failure with zero ciphertext, including the remaining == 0 verification path. Create and close the entry stream without a StreamWriter so that path is covered in both synchronous and asynchronous cases.

🧠 Review effort: Balanced

Comment on lines +1292 to +1294
if (IsEncrypted)
{
SetupEncryptionKeyMaterial(password);

@GrabYourPitchforks GrabYourPitchforks left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Placing hold pending offline discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants