Add Zip Archives password support by alinpahontu2912 · Pull Request #122093 · dotnet/runtime

alinpahontu2912 · 2025-12-02T14:58:58Z

Big Milestones and status:

Read Encrypted Entries (ZipCrypto, WinZip AES)
Create Encrypted Entries (ZipCrypto, Winzip AES)
Update Mode
Add runtime assets for checking compat with WinRar/7zip/WinZip/etc
Security check for Cryptography methods
Final API design

…assword and unprotected

… with cryptography lib

rzikm

The implementation has some duplication that I would like to see removed and streamlined, but otherwise looks reasonable.

Copilot

Pull request overview

Copilot reviewed 41 out of 41 changed files in this pull request and generated 2 comments.

alinpahontu2912 · 2026-04-30T10:19:00Z

Hey @bartonjs can you take a look again ?

Copilot

Pull request overview

Copilot reviewed 41 out of 41 changed files in this pull request and generated 2 comments.

Copilot

Pull request overview

Copilot reviewed 41 out of 41 changed files in this pull request and generated 4 comments.

+            // Verify the salt matches — use constant-time comparison because the salt is
+            // derived from secret key material and a timing side-channel could leak information.


@@ -1,4 +1,4 @@
-<Project Sdk="Microsoft.NET.Sdk">
+<Project Sdk="Microsoft.NET.Sdk">


@@ -1,4 +1,4 @@
-<Project Sdk="Microsoft.NET.Sdk">
+<Project Sdk="Microsoft.NET.Sdk">


@@ -1,4 +1,4 @@
-<Project Sdk="Microsoft.NET.Sdk">
+<Project Sdk="Microsoft.NET.Sdk">


Copilot

Pull request overview

Copilot reviewed 42 out of 42 changed files in this pull request and generated 7 comments.

@@ -1,4 +1,4 @@
-<Project Sdk="Microsoft.NET.Sdk">
+<Project Sdk="Microsoft.NET.Sdk">


@@ -1,4 +1,4 @@
-<Project Sdk="Microsoft.NET.Sdk">
+<Project Sdk="Microsoft.NET.Sdk">


@@ -1,4 +1,4 @@
-<Project Sdk="Microsoft.NET.Sdk">
+<Project Sdk="Microsoft.NET.Sdk">


+        public override int Read(Span<byte> destination)
+        {
+            if (_encrypting)
+            {
+                throw new NotSupportedException(SR.ReadingNotSupported);
+            }
+            int n = _base.Read(destination);
+            for (int i = 0; i < n; i++)
+                destination[i] = DecryptAndUpdateKeys(destination[i]);
+            return n;


+            foreach (ZipGenericExtraField field in extraFields)
+            {
+                if (field.Tag == HeaderId && field.Size >= DataSize)
+                {
+                    byte[] data = field.Data;
+                    aesExtraField = new WinZipAesExtraField(
+                        vendorVersion: BinaryPrimitives.ReadUInt16LittleEndian(data.AsSpan(0, 2)),
+                        aesStrength: data[4],
+                        compressionMethod: BinaryPrimitives.ReadUInt16LittleEndian(data.AsSpan(5, 2))
+                    );


+                ushort headerId = BinaryPrimitives.ReadUInt16LittleEndian(extraFieldData.Slice(offset, 2));
+                ushort fieldSize = BinaryPrimitives.ReadUInt16LittleEndian(extraFieldData.Slice(offset + 2, 2));
+
+                if (offset + 4 + fieldSize > extraFieldData.Length)
+                    break; // Not enough data for this field
+
+                if (headerId == HeaderId && fieldSize >= DataSize)
+                {
+                    ReadOnlySpan<byte> data = extraFieldData.Slice(offset + 4, fieldSize);
+                    aesExtraField = new WinZipAesExtraField(
+                        vendorVersion: BinaryPrimitives.ReadUInt16LittleEndian(data.Slice(0, 2)),
+                        aesStrength: data[4],
+                        compressionMethod: BinaryPrimitives.ReadUInt16LittleEndian(data.Slice(5, 2))
+                    );
+                    return true;


+        if (overwrite && File.Exists(destinationFileName))
+        {
+            tempPath = Path.GetTempFileName();
+            extractPath = tempPath;
+        }
+
+        try
+        {
+            FileStream fs = new FileStream(extractPath, fileStreamOptions);
+            await using (fs.ConfigureAwait(false))
+            {
+                Stream es = await source.OpenAsync(password.Span, cancellationToken: cancellationToken).ConfigureAwait(false);
+                await using (es.ConfigureAwait(false))
+                {
+                    await es.CopyToAsync(fs, cancellationToken).ConfigureAwait(false);
+                }
+            }
+
+            // Move the temporary file to the destination only after successful extraction
+            if (tempPath is not null)
+            {
+                File.Move(tempPath, destinationFileName, overwrite: true);
+            }


bartonjs · 2026-05-08T22:33:19Z

+            {
+                // WriteAsync requires Memory<byte>, so we must copy to a heap buffer for the async path
+                byte[] authCodeArray = authCode.Slice(0, 10).ToArray();
+                await _baseStream.WriteAsync(authCodeArray.AsMemory(0, 10), cancellationToken).ConfigureAwait(false);


It seems weird that you're doing bounded AsMemory after doing a bounded Slice.ToArray

bartonjs · 2026-05-08T22:41:14Z

+                }
+                else
+                {
+                    // For decryption: HMAC first (on ciphertext), then XOR


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

bartonjs · 2026-05-08T23:04:18Z

+        }
+        private async Task FinalizeEncryptionAsync(bool isAsync, CancellationToken cancellationToken)


Suggested change

}

private async Task FinalizeEncryptionAsync(bool isAsync, CancellationToken cancellationToken)

}

private async Task FinalizeEncryptionAsync(bool isAsync, CancellationToken cancellationToken)

bartonjs · 2026-05-08T23:06:43Z

+                        // Write Auth Code
+                        WriteAuthCodeCoreAsync(false, CancellationToken.None).GetAwaiter().GetResult();
+
+                        if (_baseStream.CanWrite)


In what circumstance would CanWrite be false while encrypting and WriteAuthCodeCoreAsync didn't throw?

bartonjs · 2026-05-08T23:09:27Z

+                    // Ensure header is written even for empty files
+                    if (!_headerWritten)
+                    {
+                        await WriteHeaderAsync(CancellationToken.None).ConfigureAwait(false);
+                    }
+
+                    // Encrypt remaining partial data
+                    await FinalizeEncryptionAsync(true, CancellationToken.None).ConfigureAwait(false);
+
+                    // Write Auth Code
+                    await WriteAuthCodeCoreAsync(true, CancellationToken.None).ConfigureAwait(false);
+
+                    if (_baseStream.CanWrite)
+                    {
+                        await _baseStream.FlushAsync().ConfigureAwait(false);
+                    }


I recommend factoring out the block here and in synchronous dispose to a FinishEncrypting(bool isAsync). 4 steps is on the fence between "we'll keep them in sync" and "we'll fail to". But the fact they both need the comment warning about empty files makes me think it's too complicated to be duplicated.

bartonjs · 2026-05-08T23:10:14Z

+        }
+        public override bool CanRead => !_encrypting && !_disposed;


Suggested change

}

public override bool CanRead => !_encrypting && !_disposed;

}

public override bool CanRead => !_encrypting && !_disposed;

bartonjs · 2026-05-08T23:10:27Z

+        public override long Length => throw new NotSupportedException();
+        public override long Position


Suggested change

public override long Length => throw new NotSupportedException();

public override long Position

public override long Length => throw new NotSupportedException();

public override long Position

bartonjs · 2026-05-08T23:11:15Z

@@ -1,4 +1,4 @@
-<Project Sdk="Microsoft.NET.Sdk">
+<Project Sdk="Microsoft.NET.Sdk">


As Copilot has said twice: Remove the BOM.

alinpahontu2912 added 22 commits October 20, 2025 15:38

working zipcrypto, hardcoded password

527c698

add tests with multiple entries with same password and entries with p…

255642b

…assword and unprotected

reading/extracting experimentation and tests

968bb60

add enum for encryption and write methods for zipcrypto stream

19cbc8e

allow encryption of entries

0dec737

add more read/write tests for update mode and small refactors

f30fbfb

add default password and encryption method to ziparchive

aac140b

avoid ambigous calls

eb1a460

avoid ambigous calls and fi some nitpicks

0a7c885

recognize aes encrypted files + build fais due to circular dependency…

817165c

… with cryptography lib

fixed some comments

797921d

remove storing password fields

623f238

initial reading of winzip encrypted files

b61b37c

address zipcrypto comments

122f85a

initial writing test works

55c0613

add more tests for winzipaesstream and work on async methods

cc6d30d

Merge remote-tracking branch 'upstream/main' into zip_password

b056780

async methods done, fix ae-1 validation and add more tests

6732696

Solve some comments

8efa772

refactor tests and ci disable local ones

d5bb294

improve winzipaesstream writing by generating larger keystream

0c659fc

update tests, fixed async opening and updated stream positioning

e2b21ff

github-actions Bot added the area-System.IO.Compression label Dec 2, 2025

dotnet-policy-service Bot assigned alinpahontu2912 Dec 2, 2025

Merge branch 'main' into zip_password

dbcee70

rzikm self-requested a review December 11, 2025 12:22

Merge branch 'main' into zip_password

ff9160b

build-analysis Bot mentioned this pull request Apr 22, 2026

Many WasmTestOnChrome-MONO-ST tests failing unable to load dotnet.*.js #127257

Open

alinpahontu2912 marked this pull request as ready for review April 23, 2026 11:06

alinpahontu2912 changed the title ~~[DRAFT][WIP]Add Zip Archives password support~~ Add Zip Archives password support Apr 23, 2026

rzikm reviewed Apr 23, 2026

View reviewed changes

alinpahontu2912 added 2 commits April 24, 2026 14:18

address comments and reduce duplication

8be1b7b

remove local tests and fix fuzzing bug

b09f09c

Copilot AI review requested due to automatic review settings April 24, 2026 13:24

Copilot started reviewing on behalf of alinpahontu2912 April 24, 2026 13:25 View session

Copilot AI reviewed Apr 24, 2026

View reviewed changes

Comment thread src/libraries/System.IO.Compression/src/System/IO/Compression/WinZipAesStream.cs

Comment thread src/libraries/System.IO.Compression/src/System/IO/Compression/WinZipAesStream.cs Outdated

iremyux reviewed Apr 27, 2026

View reviewed changes

Comment thread src/libraries/System.IO.Compression/src/System/IO/Compression/WinZipAesStream.cs

Comment thread src/libraries/System.IO.Compression/src/System/IO/Compression/WinZipAesStream.cs Outdated

remove unreachable loop and move dispose

7de87d0

alinpahontu2912 requested a review from bartonjs April 30, 2026 10:19

Merge branch 'main' into zip_password

d759bc3

Copilot AI review requested due to automatic review settings April 30, 2026 10:39

Copilot started reviewing on behalf of alinpahontu2912 April 30, 2026 10:39 View session

Copilot AI reviewed Apr 30, 2026

View reviewed changes

Comment thread src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.Async.cs

Comment thread src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.Async.cs Outdated

fix copilot comments

53aba0b

alinpahontu2912 requested a review from rzikm April 30, 2026 12:21

build-analysis Bot mentioned this pull request Apr 30, 2026

IL3050/IL2026 - StringTests, TimeZoneInfoTests fail AOT/trimmer analysis in System.Runtime.IOS.Tests #127445

Open

fix trimming warnings

22a5c72

Copilot AI review requested due to automatic review settings May 7, 2026 10:04

Copilot started reviewing on behalf of alinpahontu2912 May 7, 2026 10:04 View session

Copilot AI reviewed May 7, 2026

View reviewed changes

alinpahontu2912 and others added 2 commits May 7, 2026 15:23

add tests based on runtime assets

8177578

Merge branch 'dotnet:main' into zip_password

8f8eae7

Copilot AI review requested due to automatic review settings May 7, 2026 13:27

Copilot started reviewing on behalf of alinpahontu2912 May 7, 2026 13:27 View session

Copilot AI reviewed May 7, 2026

View reviewed changes

bartonjs requested changes May 8, 2026

View reviewed changes

		// Verify the salt matches — use constant-time comparison because the salt is
		// derived from secret key material and a timing side-channel could leak information.

		@@ -1,4 +1,4 @@
		<Project Sdk="Microsoft.NET.Sdk">
		<Project Sdk="Microsoft.NET.Sdk">

		}
		private async Task FinalizeEncryptionAsync(bool isAsync, CancellationToken cancellationToken)

		}
		public override bool CanRead => !_encrypting && !_disposed;

		public override long Length => throw new NotSupportedException();
		public override long Position

Conversation

alinpahontu2912 commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rzikm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alinpahontu2912 commented Apr 30, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

alinpahontu2912 commented Dec 2, 2025 •

edited

Loading