Correctly implement fuzzers for new encryption streams - #131306
Correctly implement fuzzers for new encryption streams#131306alinpahontu2912 wants to merge 6 commits into
Conversation
|
Tagging subscribers to this area: @dotnet/area-meta |
There was a problem hiding this comment.
Pull request overview
This PR updates the fuzzers for the new ZIP encryption streams to exercise the actual ZipArchive encryption/decryption paths (ZipCrypto and WinZip AES) rather than constructing the internal crypto streams via reflection.
Changes:
- Replace reflection-based stream construction with
ZipArchive.CreateEntry(..., password, ZipEncryptionMethod)+Open/OpenAsync(password)round-trip workflows. - Add validation that the entry is encrypted and the expected encryption method is recorded, and verify plaintext round-trips correctly (sync + async).
- Add a “wrong password” scenario intended to ensure failures are handled as
InvalidDataException(but it currently doesn’t assert failure if no exception is thrown).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/libraries/Fuzzing/DotnetFuzzing/Fuzzers/ZipCryptoStreamFuzzer.cs | Moves from reflected ZipCryptoStream.Create to ZipArchive-based encrypt/decrypt round-trip for ZipCrypto (sync + async). |
| src/libraries/Fuzzing/DotnetFuzzing/Fuzzers/WinZipAesStreamFuzzer.cs | Moves from reflected WinZipAesStream.Create to ZipArchive-based encrypt/decrypt round-trip for AES128/192/256 (sync + async). |
Comments suppressed due to low confidence (2)
src/libraries/Fuzzing/DotnetFuzzing/Fuzzers/ZipCryptoStreamFuzzer.cs:101
- The “wrong password must fail” check currently passes even if decryption succeeds without throwing. Use Assert.Throws so the fuzzer reliably flags regressions where a wrong password is accepted.
// Decrypting with a wrong password must fail cleanly with InvalidDataException, never crash.
try
{
using Stream stream = readEntry.Open("wrong-password".AsSpan());
stream.CopyTo(Stream.Null);
}
catch (InvalidDataException)
{
// Expected: the header password verifier rejects the wrong key.
}
src/libraries/Fuzzing/DotnetFuzzing/Fuzzers/WinZipAesStreamFuzzer.cs:113
- The “wrong password must fail” check currently passes even if decryption succeeds without throwing. Use Assert.Throws so the fuzzer reliably flags regressions where a wrong password is accepted.
// Decrypting with a wrong password must fail cleanly with InvalidDataException, never crash.
try
{
using Stream stream = readEntry.Open("wrong-password".AsSpan());
stream.CopyTo(Stream.Null);
}
catch (InvalidDataException)
{
// Expected: the AES password verifier / HMAC rejects the wrong key.
}
|
@MihuBot fuzz WinZipAesStreamFuzzer |
|
Ran the fuzzer(s) successfully. Code coverage reports: |
|
@MihuBot fuzz ZipCryptoStreamFuzzer |
|
Ran the fuzzer(s) successfully. Code coverage reports: |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@MihuBot fuzz ZipCryptoStreamFuzzer |
|
@MihuBot fuzz WinZipAesStreamFuzzer |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@MihuBot fuzz ZipCryptoStreamFuzzer |
|
Ran the fuzzer(s) successfully. Code coverage reports: |
MihaZupan
left a comment
There was a problem hiding this comment.
Thanks, this looks a lot better!
|
@MihuBot fuzz ZipEncryptionStreamFuzzer |
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System.Buffers; |
|
Ran the fuzzer(s) successfully. Code coverage reports: |
| Stream wrongStream = async | ||
| ? await readEntry.OpenAsync(wrongPassword.AsSpan()) | ||
| : readEntry.Open(wrongPassword.AsSpan()); | ||
| try | ||
| { | ||
| if (async) | ||
| { | ||
| await wrongStream.CopyToAsync(Stream.Null); | ||
| } | ||
| else | ||
| { | ||
| wrongStream.CopyTo(Stream.Null); | ||
| } | ||
| } | ||
| finally | ||
| { | ||
| if (async) | ||
| { | ||
| await wrongStream.DisposeAsync(); | ||
| } | ||
| else | ||
| { | ||
| wrongStream.Dispose(); | ||
| } | ||
| } |
There was a problem hiding this comment.
| Stream wrongStream = async | |
| ? await readEntry.OpenAsync(wrongPassword.AsSpan()) | |
| : readEntry.Open(wrongPassword.AsSpan()); | |
| try | |
| { | |
| if (async) | |
| { | |
| await wrongStream.CopyToAsync(Stream.Null); | |
| } | |
| else | |
| { | |
| wrongStream.CopyTo(Stream.Null); | |
| } | |
| } | |
| finally | |
| { | |
| if (async) | |
| { | |
| await wrongStream.DisposeAsync(); | |
| } | |
| else | |
| { | |
| wrongStream.Dispose(); | |
| } | |
| } | |
| if (async) | |
| { | |
| await using Stream wrongStream = await readEntry.OpenAsync(wrongPassword.AsSpan()); | |
| await wrongStream.CopyToAsync(Stream.Null); | |
| } | |
| else | |
| { | |
| using Stream wrongStream = readEntry.Open(wrongPassword.AsSpan()); | |
| wrongStream.CopyTo(Stream.Null); | |
| } |
| // AES is authenticated: the password verifier and HMAC make accepting a wrong key | ||
| // cryptographically infeasible, so a wrong password must fail with InvalidDataException. |
There was a problem hiding this comment.
| // AES is authenticated: the password verifier and HMAC make accepting a wrong key | |
| // cryptographically infeasible, so a wrong password must fail with InvalidDataException. | |
| // The AES variant of ZIP encryption uses a form of HMAC that makes accidentally accepting the wrong key | |
| // statistically unlikely, so a wrong password must fail with InvalidDataException. |
"AES is authenticated" and "cryptographically infeasible" are too strong statements here :)
| RoundTrip(buffer.Memory, password, method, async: true).GetAwaiter().GetResult(); | ||
| } | ||
|
|
||
| private static async Task RoundTrip(ReadOnlyMemory<byte> content, string password, ZipEncryptionMethod method, bool async) |
There was a problem hiding this comment.
Just making the note that you may see less edge-case coverage because your decryption logic is only exercising well-formed inputs produced by our own library.
| if (bytes.IsEmpty) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // Use the first byte to select the encryption method so all variants get exercised. | ||
| ZipEncryptionMethod method = s_methods[bytes[0] % s_methods.Length]; | ||
| ReadOnlySpan<byte> payload = bytes.Slice(1); |
There was a problem hiding this comment.
| if (bytes.IsEmpty) | |
| { | |
| return; | |
| } | |
| // Use the first byte to select the encryption method so all variants get exercised. | |
| ZipEncryptionMethod method = s_methods[bytes[0] % s_methods.Length]; | |
| ReadOnlySpan<byte> payload = bytes.Slice(1); | |
| if (bytes.Length < 2) | |
| { | |
| return; | |
| } | |
| // Use the first byte to select the encryption method so all variants get exercised. | |
| ZipEncryptionMethod method = s_methods[bytes[0] % s_methods.Length]; | |
| ReadOnlySpan<byte> payload = bytes.Slice(2); // Slice 2 to keep MemoryMarshal.Cast<byte, char> below aligned |
Correctly implement fuzzers fro the new encryption streams: zipcryptostream and winzipaesstream