Avoid rooting cryptography through ZipArchive on browser - #130688
Avoid rooting cryptography through ZipArchive on browser#130688alinpahontu2912 with Copilot wants to merge 19 commits into
Conversation
Co-authored-by: alinpahontu2912 <56953855+alinpahontu2912@users.noreply.github.com>
Co-authored-by: alinpahontu2912 <56953855+alinpahontu2912@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
Co-authored-by: alinpahontu2912 <56953855+alinpahontu2912@users.noreply.github.com>
Co-authored-by: alinpahontu2912 <56953855+alinpahontu2912@users.noreply.github.com>
Co-authored-by: alinpahontu2912 <56953855+alinpahontu2912@users.noreply.github.com>
…aphicException on failure Match Unix behavior - map BCrypt non-success/non-OOM failures to CryptographicException instead of delegating to GetRandomBytes (which throws InvalidOperationException). Co-authored-by: alinpahontu2912 <56953855+alinpahontu2912@users.noreply.github.com>
Co-authored-by: alinpahontu2912 <56953855+alinpahontu2912@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/libraries/System.IO.Compression/tests/ZipArchive/zip_ReadTests.cs:1072
- This browser-only test validates ZipCrypto decryption, but this PR also changes browser behavior by swapping in WinZip AES stubs. Consider also asserting that opening the AES-encrypted entry in the same archive throws PlatformNotSupportedException, so the new stub path is exercised on Browser and regressions are caught.
ZipArchiveEntry entry = archive.GetEntry("hello.txt");
Assert.NotNull(entry);
Assert.Equal(ZipEncryptionMethod.ZipCrypto, entry.EncryptionMethod);
using Stream entryStream = await OpenEntryStream(async, entry, Password);
using StreamReader reader = new(entryStream);
src/libraries/Common/src/Interop/Windows/BCrypt/Interop.BCryptGenRandom.GetRandomBytes.cs:30
- GetCryptographicallySecureRandomBytes currently forwards to GetRandomBytes, which throws InvalidOperationException on most failures. Since the method name (and the Unix implementation) implies cryptographic RNG semantics, it should throw CryptographicException for non-OOM failures to better match RandomNumberGenerator behavior.
// BCryptGenRandom with BCRYPT_USE_SYSTEM_PREFERRED_RNG is always cryptographically secure.
internal static unsafe void GetCryptographicallySecureRandomBytes(byte* buffer, int length) =>
GetRandomBytes(buffer, length);
- Rename the sole BCrypt-backed method in Interop.BCryptGenRandom.GetRandomBytes.cs to GetCryptographicallySecureRandomBytes directly, removing the pass-through alias wrapper (file name kept unchanged). - Add a short comment at the ZipCryptoStream.Random.cs call site noting the call is cryptographically secure on all platforms. - Extend DecryptZipCryptoEntry_Browser to also assert that creating a WinZip AES-encrypted entry on browser throws PlatformNotSupportedException. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 52519a8a-ff2e-46fc-97c8-77e133223f9c
Keep the original GetRandomBytes name in the BCrypt interop file and restore GetCryptographicallySecureRandomBytes as a thin wrapper, with a comment explaining why the wrapper exists: it gives the Windows method the same name as the Unix crypto-secure API, so shared cross-platform callers (ZipCryptoStream) reliably get a cryptographically secure implementation on every platform. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 52519a8a-ff2e-46fc-97c8-77e133223f9c
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.Async.cs:785
keySizeBitsis computed but never used in this AES update-mode path. This is dead code and can produce warnings / confusion; it should be removed (the key size was already baked into_derivedAesKeyMaterial).
// Determine the actual compression method to use
// The AES extra field stores the real compression method
bool useDeflate = _compressionLevel != CompressionLevel.NoCompression;
Stream encryptionStream = WinZipAesStream.Create(
…rator elsewhere Co-authored-by: alinpahontu2912 <56953855+alinpahontu2912@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
src/libraries/System.IO.Compression/src/System/IO/Compression/WinZipAes.PlatformNotSupported.cs:9
- The WinZipAesKeyMaterial stub is declared as
internal readonly struct WinZipAesKeyMaterial;which is not valid C# syntax (types can’t be forward-declared with a trailing semicolon). This will fail to compile on browser builds.
internal readonly struct WinZipAesKeyMaterial;
src/libraries/System.IO.Compression/src/System.IO.Compression.csproj:128
System.Security.Cryptographydoesn’t target$(NetCoreAppCurrent)-wasi(its csproj only includes windows/unix/android/apple/browser/$(NetCoreAppCurrent)), so keeping thisProjectReferencefor wasi will break the wasi build. The condition should exclude wasi as well as browser.
<ProjectReference Include="$(LibrariesProjectRoot)System.Security.Cryptography\src\System.Security.Cryptography.csproj" Condition="'$(TargetPlatformIdentifier)' != 'browser'" />
| <Compile Include="System\IO\Compression\WinZipAesStream.cs" Condition="'$(TargetPlatformIdentifier)' != 'browser'" /> | ||
| <Compile Include="System\IO\Compression\WinZipAesKeyMaterial.cs" Condition="'$(TargetPlatformIdentifier)' != 'browser'" /> | ||
| <Compile Include="System\IO\Compression\WinZipAes.PlatformNotSupported.cs" Condition="'$(TargetPlatformIdentifier)' == 'browser'" /> |
Co-authored-by: alinpahontu2912 <56953855+alinpahontu2912@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/libraries/System.IO.Compression/src/System/IO/Compression/WinZipAes.PlatformNotSupported.cs:10
WinZipAesKeyMaterialis declared with a trailing semicolon, which isn’t valid C# (C# doesn’t support forward-declaring structs). Browser builds will fail to compile this file.
{
internal readonly struct WinZipAesKeyMaterial;
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.Async.cs:786
- In this AES update-mode branch,
int keySizeBits = GetAesKeySizeBits(Encryption);(a few lines above) is assigned but never used. If warnings are treated as errors, this will break the build; if the call is only for validation, discard the value explicitly.
// The AES extra field stores the real compression method
bool useDeflate = _compressionLevel != CompressionLevel.NoCompression;
Stream encryptionStream = WinZipAesStream.Create(
baseStream: _archive.ArchiveStream,
src/libraries/System.IO.Compression/tests/ZipArchive/zip_ReadTests.cs:1078
- This browser-only test validates ZipCrypto decryption, but it doesn’t cover creating a ZipCrypto entry on browser. Since this PR changes the ZipCrypto encryption path (header salt generation + password buffer clearing), adding a round-trip create/read assertion here would help prevent regressions.
using MemoryStream createStream = new();
ZipArchive createArchive = await CreateZipArchive(async, createStream, ZipArchiveMode.Create, leaveOpen: true);
Assert.Throws<PlatformNotSupportedException>(() => createArchive.CreateEntry("aes.txt", Password, ZipEncryptionMethod.Aes256));
await DisposeZipArchive(async, createArchive);
}
Fixes #130650
which is regression from #122093
Using
ZipArchiveon browser-wasm unnecessarily retainedSystem.Security.Cryptography, increasing trimmed application size even though WinZip AES is unsupported there.Changes
PlatformNotSupportedException.Guid.NewGuid().Array.Clear.System.Security.Cryptography.