From ba0e960e6b6d26b452e695c8a5e6f296ff40f3c6 Mon Sep 17 00:00:00 2001 From: Jeremy Barton Date: Wed, 6 Nov 2024 10:27:27 -0800 Subject: [PATCH 1/3] Fix erroneous success in AsnDecoder.ReadEncodedValue When AsnDecoder.ReadEncodedValue is used on a payload where the encoded length plus the number of bytes representing the encoded length plus the number of bytes representing the tag exceeds int.MaxValue it erroneously reports success. Every known caller of this method immediately follows it with a call to a Span or Memory .Slice, which results in an ArgumentException that the requested length exceeds the number of bytes available in the buffer. Because AsnDecoder was extracted out of AsnReader, most AsnDecoder tests are done indirectly as AsnReader tests, or are done in the same test class that tests the related functionality on AsnReader. Since there's not a clear analogue for ReadEncodedValue (that does not immediately Slice), this change introduces a new subtree for AsnDecoder tests, only populating it with (Try)ReadEncodedValue tests. It also adds "large length" tests for ReadSetOf and ReadSequence, for good measure. --- .../Formats/Asn1/AsnDecoder.Sequence.cs | 2 +- .../tests/Decoder/ReadEncodedValueTests.cs | 237 ++++++++++++++++++ .../tests/Reader/ReadSequence.cs | 28 +++ .../tests/Reader/ReadSetOf.cs | 28 +++ .../tests/System.Formats.Asn1.Tests.csproj | 1 + 5 files changed, 295 insertions(+), 1 deletion(-) create mode 100644 src/libraries/System.Formats.Asn1/tests/Decoder/ReadEncodedValueTests.cs diff --git a/src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnDecoder.Sequence.cs b/src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnDecoder.Sequence.cs index f66ffcb85ad1a..7b26e725de3e8 100644 --- a/src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnDecoder.Sequence.cs +++ b/src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnDecoder.Sequence.cs @@ -75,7 +75,7 @@ public static void ReadSequence( if (length.HasValue) { - if (length.Value + headerLength > source.Length) + if (length.Value > source.Length - headerLength) { throw GetValidityException(LengthValidity.LengthExceedsInput); } diff --git a/src/libraries/System.Formats.Asn1/tests/Decoder/ReadEncodedValueTests.cs b/src/libraries/System.Formats.Asn1/tests/Decoder/ReadEncodedValueTests.cs new file mode 100644 index 0000000000000..82f8afc382771 --- /dev/null +++ b/src/libraries/System.Formats.Asn1/tests/Decoder/ReadEncodedValueTests.cs @@ -0,0 +1,237 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Test.Cryptography; +using Xunit; + +namespace System.Formats.Asn1.Tests.Decoder +{ + public sealed class ReadEncodedValueTests + { + [Theory] + [InlineData(AsnEncodingRules.BER)] + [InlineData(AsnEncodingRules.CER)] + [InlineData(AsnEncodingRules.DER)] + public static void ReadEncodedValue_Primitive(AsnEncodingRules ruleSet) + { + // OCTET STRING (6 content bytes) + // NULL + ReadOnlySpan data = + [ + 0x04, 0x06, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, + 0x05, 0x00, + ]; + + ExpectSuccess(data, ruleSet, Asn1Tag.PrimitiveOctetString, 2, 6); + } + + [Theory] + [InlineData(AsnEncodingRules.BER)] + [InlineData(AsnEncodingRules.CER)] + [InlineData(AsnEncodingRules.DER)] + public static void ReadEncodedValue_Indefinite(AsnEncodingRules ruleSet) + { + // CONSTRUCTED OCTET STRING (indefinite) + // OCTET STRING (1 byte) + // OCTET STRING (5 bytes) + // END OF CONTENTS + // NULL + ReadOnlySpan data = + [ + 0x24, 0x80, + 0x04, 0x01, 0x01, + 0x04, 0x05, 0x02, 0x03, 0x04, 0x05, 0x06, + 0x00, 0x00, + 0x05, 0x00, + ]; + + // BER: Indefinite length encoding is OK, no requirements on the contents. + // CER: Indefinite length encoding is required for CONSTRUCTED, the contents are invalid for OCTET STRING, + // but (Try)ReadEncodedValue doesn't pay attention to that. + // DER: Indefinite length encoding is never permitted. + + if (ruleSet == AsnEncodingRules.DER) + { + ExpectFailure(data, ruleSet); + } + else + { + ExpectSuccess(data, ruleSet, Asn1Tag.ConstructedOctetString, 2, 10, indefiniteLength: true); + } + } + + [Theory] + [InlineData(AsnEncodingRules.BER)] + [InlineData(AsnEncodingRules.CER)] + [InlineData(AsnEncodingRules.DER)] + public static void ReadEncodedValue_DefiniteConstructed(AsnEncodingRules ruleSet) + { + // CONSTRUCTED OCTET STRING (11 bytes) + // OCTET STRING (1 byte) + // OCTET STRING (5 bytes) + // NULL + ReadOnlySpan data = + [ + 0x24, 0x0A, + 0x04, 0x01, 0x01, + 0x04, 0x05, 0x02, 0x03, 0x04, 0x05, 0x06, + 0x05, 0x00, + ]; + + // BER: Indefinite length encoding is OK, no requirements on the contents. + // CER: Indefinite length encoding is required for CONSTRUCTED, so fail. + // DER: CONSTRUCTED OCTET STRING is not permitted, but ReadEncodedValue doesn't check for that, + // since the length is in minimal representation, the read is successful + + if (ruleSet == AsnEncodingRules.CER) + { + ExpectFailure(data, ruleSet); + } + else + { + ExpectSuccess(data, ruleSet, Asn1Tag.ConstructedOctetString, 2, 10); + } + } + + [Theory] + [InlineData(AsnEncodingRules.BER)] + [InlineData(AsnEncodingRules.CER)] + [InlineData(AsnEncodingRules.DER)] + public static void ReadEncodedValue_OutOfBoundsLength(AsnEncodingRules ruleSet) + { + // SEQUENCE (3 bytes), but only one byte remains. + ReadOnlySpan data = [0x30, 0x03, 0x00]; + + ExpectFailure(data, ruleSet); + } + + [Theory] + [InlineData(AsnEncodingRules.BER)] + [InlineData(AsnEncodingRules.CER)] + [InlineData(AsnEncodingRules.DER)] + public static void ReadEncodedValue_LargeOutOfBoundsLength(AsnEncodingRules ruleSet) + { + // SEQUENCE (int.MaxValue bytes), but no bytes remain. + ReadOnlySpan data = [0x30, 0x84, 0x7F, 0xFF, 0xFF, 0xFF]; + + ExpectFailure(data, ruleSet); + } + + [Theory] + [InlineData(AsnEncodingRules.BER)] + [InlineData(AsnEncodingRules.CER)] + [InlineData(AsnEncodingRules.DER)] + public static void ReadEncodedValue_ExtremelyLargeLength(AsnEncodingRules ruleSet) + { + if (!Environment.Is64BitProcess) + { + return; + } + + // OCTET STRING ((int.MaxValue - 6) bytes), span will be inflated to make it look valid. + byte[] data = "04847FFFFFF9".HexToByteArray(); + + unsafe + { + fixed (byte* ptr = data) + { + // Verify that the length can be interpreted this large, but that it doesn't read that far. + ReadOnlySpan span = new ReadOnlySpan(ptr, int.MaxValue); + ExpectSuccess(span, ruleSet, Asn1Tag.PrimitiveOctetString, 6, int.MaxValue - 6); + } + } + } + + private static void ExpectSuccess( + ReadOnlySpan data, + AsnEncodingRules ruleSet, + Asn1Tag expectedTag, + int expectedContentOffset, + int expectedContentLength, + bool indefiniteLength = false) + { + Asn1Tag tag; + int contentOffset; + int contentLength; + int bytesConsumed; + + bool read = AsnDecoder.TryReadEncodedValue( + data, + ruleSet, + out tag, + out contentOffset, + out contentLength, + out bytesConsumed); + + Assert.True(read, "AsnDecoder.TryReadEncodedValue unexpectedly returned false"); + Assert.Equal(expectedTag, tag); + Assert.Equal(expectedContentOffset, contentOffset); + Assert.Equal(expectedContentLength, contentLength); + + int expectedBytesConsumed = expectedContentOffset + expectedContentLength + (indefiniteLength ? 2 : 0); + Assert.Equal(expectedBytesConsumed, bytesConsumed); + + contentOffset = contentLength = bytesConsumed = default; + + tag = AsnDecoder.ReadEncodedValue( + data, + ruleSet, + out contentOffset, + out contentLength, + out bytesConsumed); + + Assert.Equal(expectedTag, tag); + Assert.Equal(expectedContentOffset, contentOffset); + Assert.Equal(expectedContentLength, contentLength); + Assert.Equal(expectedBytesConsumed, bytesConsumed); + } + + private static void ExpectFailure(ReadOnlySpan data, AsnEncodingRules ruleSet) + { + Asn1Tag tag; + int contentOffset; + int contentLength; + int bytesConsumed; + + bool read = AsnDecoder.TryReadEncodedValue( + data, + ruleSet, + out tag, + out contentOffset, + out contentLength, + out bytesConsumed); + + Assert.False(read, "AsnDecoder.TryReadEncodedValue unexpectedly returned true"); + Assert.Equal(default, tag); + Assert.Equal(default, contentOffset); + Assert.Equal(default, contentLength); + Assert.Equal(default, bytesConsumed); + + int seed = Environment.CurrentManagedThreadId; + Asn1Tag seedTag = new Asn1Tag(TagClass.Private, seed, (seed & 1) == 0); + tag = seedTag; + contentOffset = contentLength = bytesConsumed = seed; + + try + { + tag = AsnDecoder.ReadEncodedValue( + data, + ruleSet, + out contentOffset, + out contentLength, + out bytesConsumed); + + Assert.Fail("ReadEncodedValue should have thrown AsnContentException"); + } + catch (AsnContentException e) + { + Assert.IsType(e); + } + + Assert.Equal(seedTag, tag); + Assert.Equal(seed, contentOffset); + Assert.Equal(seed, contentLength); + Assert.Equal(seed, bytesConsumed); + } + } +} diff --git a/src/libraries/System.Formats.Asn1/tests/Reader/ReadSequence.cs b/src/libraries/System.Formats.Asn1/tests/Reader/ReadSequence.cs index c1f5e620625ef..058b01216299e 100644 --- a/src/libraries/System.Formats.Asn1/tests/Reader/ReadSequence.cs +++ b/src/libraries/System.Formats.Asn1/tests/Reader/ReadSequence.cs @@ -405,5 +405,33 @@ public static void ReadSequenceOf_PreservesOptions(AsnEncodingRules ruleSet) outer.ThrowIfNotEmpty(); initial.ThrowIfNotEmpty(); } + + [Theory] + [InlineData(AsnEncodingRules.BER)] + [InlineData(AsnEncodingRules.CER)] + [InlineData(AsnEncodingRules.DER)] + public static void ExtremelyLargeContentLength(AsnEncodingRules ruleSet) + { + int start = Environment.CurrentManagedThreadId; + int contentOffset = start; + int contentLength = start; + int bytesConsumed = start; + + ReadOnlySpan input = [0x30, 0x84, 0x7F, 0xFF, 0xFF, 0xFF, 0x00, 0x00]; + + try + { + AsnDecoder.ReadSequence(input, ruleSet, out contentOffset, out contentLength, out bytesConsumed); + Assert.Fail("ReadSequence should have thrown AsnContentException"); + } + catch (AsnContentException e) + { + Assert.IsType(e); + } + + Assert.Equal(start, contentOffset); + Assert.Equal(start, contentLength); + Assert.Equal(start, bytesConsumed); + } } } diff --git a/src/libraries/System.Formats.Asn1/tests/Reader/ReadSetOf.cs b/src/libraries/System.Formats.Asn1/tests/Reader/ReadSetOf.cs index f5b38dc0e16b6..90058b9f487a4 100644 --- a/src/libraries/System.Formats.Asn1/tests/Reader/ReadSetOf.cs +++ b/src/libraries/System.Formats.Asn1/tests/Reader/ReadSetOf.cs @@ -390,5 +390,33 @@ public static void ReadSetOf_PreservesOptions(AsnEncodingRules ruleSet) outer.ThrowIfNotEmpty(); initial.ThrowIfNotEmpty(); } + + [Theory] + [InlineData(AsnEncodingRules.BER)] + [InlineData(AsnEncodingRules.CER)] + [InlineData(AsnEncodingRules.DER)] + public static void ExtremelyLargeContentLength(AsnEncodingRules ruleSet) + { + int start = Environment.CurrentManagedThreadId; + int contentOffset = start; + int contentLength = start; + int bytesConsumed = start; + + ReadOnlySpan input = [0x31, 0x84, 0x7F, 0xFF, 0xFF, 0xFF, 0x00, 0x00]; + + try + { + AsnDecoder.ReadSetOf(input, ruleSet, out contentOffset, out contentLength, out bytesConsumed); + Assert.Fail("ReadSetOf should have thrown AsnContentException"); + } + catch (AsnContentException e) + { + Assert.IsType(e); + } + + Assert.Equal(start, contentOffset); + Assert.Equal(start, contentLength); + Assert.Equal(start, bytesConsumed); + } } } diff --git a/src/libraries/System.Formats.Asn1/tests/System.Formats.Asn1.Tests.csproj b/src/libraries/System.Formats.Asn1/tests/System.Formats.Asn1.Tests.csproj index 68bdc72edf895..3626af0c77d1a 100644 --- a/src/libraries/System.Formats.Asn1/tests/System.Formats.Asn1.Tests.csproj +++ b/src/libraries/System.Formats.Asn1/tests/System.Formats.Asn1.Tests.csproj @@ -5,6 +5,7 @@ + From d30b24396434e48f001f300dab3618474afb3e68 Mon Sep 17 00:00:00 2001 From: Jeremy Barton Date: Thu, 7 Nov 2024 18:09:38 +0000 Subject: [PATCH 2/3] Add packaging info --- .../System.Formats.Asn1/src/System.Formats.Asn1.csproj | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libraries/System.Formats.Asn1/src/System.Formats.Asn1.csproj b/src/libraries/System.Formats.Asn1/src/System.Formats.Asn1.csproj index 445fa72447844..d356e31964195 100644 --- a/src/libraries/System.Formats.Asn1/src/System.Formats.Asn1.csproj +++ b/src/libraries/System.Formats.Asn1/src/System.Formats.Asn1.csproj @@ -5,6 +5,8 @@ true $(DefineConstants);CP_NO_ZEROMEMORY true + true + 1 Provides classes that can read and write the ASN.1 BER, CER, and DER data formats. From d2e3336810cf4336bcc953fab38049a671159f75 Mon Sep 17 00:00:00 2001 From: Jeremy Barton Date: Thu, 7 Nov 2024 18:39:48 +0000 Subject: [PATCH 3/3] Apparently packaging info is no longer needed. --- .../System.Formats.Asn1/src/System.Formats.Asn1.csproj | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/libraries/System.Formats.Asn1/src/System.Formats.Asn1.csproj b/src/libraries/System.Formats.Asn1/src/System.Formats.Asn1.csproj index d356e31964195..445fa72447844 100644 --- a/src/libraries/System.Formats.Asn1/src/System.Formats.Asn1.csproj +++ b/src/libraries/System.Formats.Asn1/src/System.Formats.Asn1.csproj @@ -5,8 +5,6 @@ true $(DefineConstants);CP_NO_ZEROMEMORY true - true - 1 Provides classes that can read and write the ASN.1 BER, CER, and DER data formats.