diff --git a/src/libraries/System.IO.Compression/ref/System.IO.Compression.cs b/src/libraries/System.IO.Compression/ref/System.IO.Compression.cs index 40175713e69b3f..9cd601b6c2a63c 100644 --- a/src/libraries/System.IO.Compression/ref/System.IO.Compression.cs +++ b/src/libraries/System.IO.Compression/ref/System.IO.Compression.cs @@ -58,6 +58,7 @@ public GZipStream(System.IO.Stream stream, System.IO.Compression.CompressionLeve public GZipStream(System.IO.Stream stream, System.IO.Compression.CompressionLevel compressionLevel, bool leaveOpen) { } public GZipStream(System.IO.Stream stream, System.IO.Compression.CompressionMode mode) { } public GZipStream(System.IO.Stream stream, System.IO.Compression.CompressionMode mode, bool leaveOpen) { } + public GZipStream(System.IO.Stream stream, System.IO.Compression.CompressionMode mode, bool leaveOpen, bool strictValidation) { } public System.IO.Stream BaseStream { get { throw null; } } public override bool CanRead { get { throw null; } } public override bool CanSeek { get { throw null; } } diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/DeflateStream.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/DeflateStream.cs index c8cd4071f5bc00..63beab71b5ec62 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/DeflateStream.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/DeflateStream.cs @@ -17,6 +17,7 @@ public partial class DeflateStream : Stream private Stream _stream; private CompressionMode _mode; private bool _leaveOpen; + private bool _strictValidation; private Inflater? _inflater; private Deflater? _deflater; private byte[]? _buffer; @@ -49,7 +50,7 @@ public DeflateStream(Stream stream, CompressionLevel compressionLevel, bool leav /// Internal constructor to check stream validity and call the correct initialization function depending on /// the value of the CompressionMode given. /// - internal DeflateStream(Stream stream, CompressionMode mode, bool leaveOpen, int windowBits, long uncompressedSize = -1) + internal DeflateStream(Stream stream, CompressionMode mode, bool leaveOpen, int windowBits, long uncompressedSize = -1, bool strictValidation = false) { if (stream == null) throw new ArgumentNullException(nameof(stream)); @@ -60,10 +61,11 @@ internal DeflateStream(Stream stream, CompressionMode mode, bool leaveOpen, int if (!stream.CanRead) throw new ArgumentException(SR.NotSupported_UnreadableStream, nameof(stream)); - _inflater = new Inflater(windowBits, uncompressedSize); + _inflater = new Inflater(windowBits, uncompressedSize, strictValidation); _stream = stream; _mode = CompressionMode.Decompress; _leaveOpen = leaveOpen; + _strictValidation = strictValidation; break; case CompressionMode.Compress: @@ -266,6 +268,10 @@ internal int ReadCore(Span buffer) while (true) { int bytesRead = _inflater.Inflate(buffer.Slice(totalRead)); + if (bytesRead == -1 && _strictValidation) + { + throw new InvalidDataException(SR.GenericInvalidData); + } totalRead += bytesRead; if (totalRead == buffer.Length) { @@ -858,11 +864,15 @@ public async Task CopyFromSourceToDestinationAsync() { await _destination.WriteAsync(new ReadOnlyMemory(_arrayPoolBuffer, 0, bytesRead), _cancellationToken).ConfigureAwait(false); } - else if (_deflateStream._inflater.NeedsInput()) + else if (_deflateStream._inflater.NeedsInput() && bytesRead == 0) { // only break if we read 0 and ran out of input, if input is still available it may be another GZip payload break; } + else if (bytesRead == -1 && _deflateStream._strictValidation) + { + throw new InvalidDataException(SR.GenericInvalidData); + } } // Now, use the source stream's CopyToAsync to push directly to our inflater via this helper stream @@ -890,11 +900,15 @@ public void CopyFromSourceToDestination() { _destination.Write(_arrayPoolBuffer, 0, bytesRead); } - else if (_deflateStream._inflater.NeedsInput()) + else if (_deflateStream._inflater.NeedsInput() && bytesRead == 0) { // only break if we read 0 and ran out of input, if input is still available it may be another GZip payload break; } + else if (bytesRead == -1 && _deflateStream._strictValidation) + { + throw new InvalidDataException(SR.GenericInvalidData); + } } // Now, use the source stream's CopyToAsync to push directly to our inflater via this helper stream @@ -935,11 +949,15 @@ public override async Task WriteAsync(byte[] buffer, int offset, int count, Canc { await _destination.WriteAsync(new ReadOnlyMemory(_arrayPoolBuffer, 0, bytesRead), cancellationToken).ConfigureAwait(false); } - else if (_deflateStream._inflater.NeedsInput()) + else if (_deflateStream._inflater.NeedsInput() && bytesRead == 0) { // only break if we read 0 and ran out of input, if input is still available it may be another GZip payload break; } + else if (bytesRead == -1 && _deflateStream._strictValidation) + { + throw new InvalidDataException(SR.GenericInvalidData); + } } } @@ -972,11 +990,15 @@ public override void Write(byte[] buffer, int offset, int count) { _destination.Write(_arrayPoolBuffer, 0, bytesRead); } - else if (_deflateStream._inflater.NeedsInput()) + else if (_deflateStream._inflater.NeedsInput() && bytesRead == 0) { // only break if we read 0 and ran out of input, if input is still available it may be another GZip payload break; } + else if (bytesRead == -1 && _deflateStream._strictValidation) + { + throw new InvalidDataException(SR.GenericInvalidData); + } } } diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Inflater.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Inflater.cs index f6d17eb58559d2..7901f91b21a420 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Inflater.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Inflater.cs @@ -22,6 +22,7 @@ internal sealed class Inflater : IDisposable private ZLibNative.ZLibStreamHandle _zlibStream; // The handle to the primary underlying zlib stream private GCHandle _inputBufferHandle; // The handle to the buffer that provides input to _zlibStream private readonly long _uncompressedSize; + private readonly bool _strictValidation; private long _currentInflatedCount; private object SyncLock => this; // Used to make writing to unmanaged structures atomic @@ -29,7 +30,7 @@ internal sealed class Inflater : IDisposable /// /// Initialized the Inflater with the given windowBits size /// - internal Inflater(int windowBits, long uncompressedSize = -1) + internal Inflater(int windowBits, long uncompressedSize = -1, bool strictValidation = false) { Debug.Assert(windowBits >= MinWindowBits && windowBits <= MaxWindowBits); _finished = false; @@ -37,6 +38,7 @@ internal Inflater(int windowBits, long uncompressedSize = -1) _windowBits = windowBits; InflateInit(windowBits); _uncompressedSize = uncompressedSize; + _strictValidation = strictValidation; } public int AvailableOutput => (int)_zlibStream.AvailOut; @@ -119,7 +121,8 @@ public unsafe int InflateVerified(byte* bufPtr, int length) private unsafe void ReadOutput(byte* bufPtr, int length, out int bytesRead) { - if (ReadInflateOutput(bufPtr, length, ZLibNative.FlushCode.NoFlush, out bytesRead) == ZLibNative.ErrorCode.StreamEnd) + var result = ReadInflateOutput(bufPtr, length, ZLibNative.FlushCode.NoFlush, out bytesRead); + if (result == ZLibNative.ErrorCode.StreamEnd) { if (!NeedsInput() && IsGzipStream() && _inputBufferHandle.IsAllocated) { @@ -130,6 +133,10 @@ private unsafe void ReadOutput(byte* bufPtr, int length, out int bytesRead) _finished = true; } } + else if (result == ZLibNative.ErrorCode.BufError && _strictValidation) + { + bytesRead = -1; + } } /// diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/GZipStream.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/GZipStream.cs index 8e674d13b97a82..eb96265da16882 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/GZipStream.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/GZipStream.cs @@ -20,6 +20,11 @@ public GZipStream(Stream stream, CompressionMode mode, bool leaveOpen) _deflateStream = new DeflateStream(stream, mode, leaveOpen, ZLibNative.GZip_DefaultWindowBits); } + public GZipStream(Stream stream, CompressionMode mode, bool leaveOpen, bool strictValidation) + { + _deflateStream = new DeflateStream(stream, mode, leaveOpen, ZLibNative.GZip_DefaultWindowBits, strictValidation: strictValidation); + } + // Implies mode = Compress public GZipStream(Stream stream, CompressionLevel compressionLevel) : this(stream, compressionLevel, leaveOpen: false) { diff --git a/src/libraries/System.IO.Compression/tests/CompressionStreamUnitTests.Gzip.cs b/src/libraries/System.IO.Compression/tests/CompressionStreamUnitTests.Gzip.cs index 2f7195e7aaf8b8..8e8e505337291d 100644 --- a/src/libraries/System.IO.Compression/tests/CompressionStreamUnitTests.Gzip.cs +++ b/src/libraries/System.IO.Compression/tests/CompressionStreamUnitTests.Gzip.cs @@ -2,6 +2,10 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Buffers; +using System.Collections.Generic; +using System.Diagnostics; +using System.Linq; +using System.Text; using System.Threading; using System.Threading.Tasks; using Xunit; @@ -64,7 +68,7 @@ public async Task ConcatenatedEmptyGzipStreams() ArrayPool.Shared.Return(rentedBuffer); // use 3 buffers-full so that we can prime the stream with the first buffer-full, - // test that CopyTo successfully flushes this at the beginning of the operation, + // test that CopyTo successfully flushes this at the beginning of the operation, // then populates the second buffer-full and reads its entirety despite every // payload being 0 length before it reads the final buffer-full. int minCompressedSize = 3 * actualBufferSize; @@ -167,9 +171,9 @@ private async Task TestConcatenatedGzipStreams(int streamCount, TestScenario sce bool isCopy = scenario == TestScenario.Copy || scenario == TestScenario.CopyAsync; using (MemoryStream correctDecompressedOutput = new MemoryStream()) - // For copy scenarios use a derived MemoryStream to avoid MemoryStream's Copy optimization + // For copy scenarios use a derived MemoryStream to avoid MemoryStream's Copy optimization // that turns the Copy into a single Write passing the backing buffer - using (MemoryStream compressedStream = isCopy ? new DerivedMemoryStream() : new MemoryStream()) + using (MemoryStream compressedStream = isCopy ? new DerivedMemoryStream() : new MemoryStream()) using (MemoryStream decompressorOutput = new MemoryStream()) { for (int i = 0; i < streamCount; i++) @@ -281,6 +285,181 @@ public void DerivedStream_ReadWriteSpan_UsesReadWriteArray() } } + // https://stackoverflow.com/questions/9456563/gzipstream-doesnt-detect-corrupt-data-even-crc32-passes + [Fact] + public void StrictValidation() + { + var data = Enumerable.Range(0, 32).Select(i => (byte)i).ToArray(); + byte[] cmpData; + + // Compress + using (var cmpStream = new MemoryStream()) + { + using (var hgs = new GZipStream(cmpStream, CompressionMode.Compress)) + { + hgs.Write(data, 0, data.Length); + } + cmpData = cmpStream.ToArray(); + } + + int corruptBytesNotDetected = 0; + int corruptBytesDetected = 0; + + // corrupt data byte by byte + for (var byteToCorrupt = 0; byteToCorrupt < cmpData.Length; byteToCorrupt++) + { + // corrupt the data + cmpData[byteToCorrupt]++; + + using (var decomStream = new MemoryStream(cmpData)) + { + using (var hgs = new GZipStream(decomStream, CompressionMode.Decompress, false, strictValidation: true)) + { + using (var reader = new StreamReader(hgs)) + { + try + { + string sampleOut = reader.ReadToEnd(); + + // if we get here, the corrupt data was not detected by GZipStream + // ... okay so long as the correct data is extracted + corruptBytesNotDetected++; + } + catch (InvalidDataException) + { + corruptBytesDetected++; + } + } + } + } + + // restore the data + cmpData[byteToCorrupt]--; + } + + Assert.Equal(0, corruptBytesNotDetected); + Assert.Equal(52, corruptBytesDetected); + } + + [Fact] + public void StrictValidation2() + { + var source = Enumerable.Range(0, 32).Select(i => (byte)i).ToArray(); + var codec = new (Func compress, Func decompress)[] + { + ( + s => new System.IO.Compression.GZipStream(s, System.IO.Compression.CompressionLevel.Fastest), + s => new System.IO.Compression.GZipStream(s, CompressionMode.Decompress, false, strictValidation: true) + ) + }; + string r = null; + + try + { + r = RoundTrip(source, codec[0], Truncate); + } + catch(Exception ex) + { + Debug.WriteLine(ex.Message); + } + + Assert.Equal("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", r); + } + + private static string RoundTrip( + ReadOnlySpan source, + (Func compress, Func decompress) codec, + Func, IEnumerable<(Stream input, bool? shouldBeOkay)>> peturbations) + { + byte[] data; + using (var compressed = new MemoryStream()) + using (var gzip = codec.compress(compressed)) + { + foreach (var b in source) + { + gzip.WriteByte(b); + } + gzip.Dispose(); + data = compressed.ToArray(); + } + + var actual = new StringBuilder(); + foreach (var entry in peturbations(data)) + { + try + { + using (var input = entry.input) + using (var gzip = codec.decompress(input)) + using (var roundtrip = new MemoryStream()) + { + gzip.CopyTo(roundtrip); + var result = roundtrip.ToArray(); + } + actual.Append("/"); + } + catch (Exception) + { + actual.Append("x"); + } + } + + return actual.ToString(); + } + + private static IEnumerable<(Stream, bool? shouldBeLegal)> Truncate(ReadOnlyMemory source) + { + var data = source.ToArray(); + for (int i = 0; i < data.Length; i++) + { + yield return ( + new MemoryStream(data, 0, i, writable: false), + CanByteBeManipulatedSafely(data.Length, i, truncated: true)); + } + } + + private static bool? CanByteBeManipulatedSafely(int length, int index, bool truncated) + { + // https://tools.ietf.org/html/rfc1952.html#section-2.2 + switch (index) + { + case 0: // ID1 + case 1: // ID2 + case 2: // CM (always gzip) + return false; + //FLG some flags values may be illegal but ignoring for now + case 3: + return truncated ? false : (bool?)null; + // MTIME + case 4: + case 5: + case 6: + case 7: + return truncated ? false : true; + //XFL some flags values may be illegal but ignoring for now + case 8: + return truncated ? false : (bool?)null; + // OS + case 9: + return truncated ? false : (bool?)null; + default: + // footer + if (length > 18 && index > length - 4) + { + // isize - should be validated + return false; + } + else if (length > 18 && index > length - 8) + { + // CRC32 - should be validated - technically certain manipulations could be undetected + return false; + } + // depending on flags/xflags there may be header entries in here that are mutable + // however if FLG.FHCRC was set thyen they aren't, so just assum none can be changed + return false; + } + } + + private sealed class DerivedGZipStream : GZipStream { public bool ReadArrayInvoked = false, WriteArrayInvoked = false;