From 12d8b284fcb0980a386bacca3f9eb5611ea28a45 Mon Sep 17 00:00:00 2001 From: Jeremy Barton Date: Wed, 6 Nov 2024 10:27:27 -0800 Subject: [PATCH 1/2] 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 00b7aac675333b..028051f664028d 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 00000000000000..82f8afc382771e --- /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 c1f5e620625efb..058b01216299eb 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 f5b38dc0e16b62..90058b9f487a4f 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 c4bfbf0a5ebc01..1a36e7e4c2cdbe 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 @@ -6,6 +6,7 @@ + From 62d3edfa3a9a35575ac7d5bae32ebd34cd8010a7 Mon Sep 17 00:00:00 2001 From: Jeremy Barton Date: Thu, 7 Nov 2024 18:10:38 +0000 Subject: [PATCH 2/2] Add packaging info --- .../System.Formats.Asn1/src/System.Formats.Asn1.csproj | 4 ++-- 1 file changed, 2 insertions(+), 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 b0ac5cf5bce58b..0dd7177ebd84c5 100644 --- a/src/libraries/System.Formats.Asn1/src/System.Formats.Asn1.csproj +++ b/src/libraries/System.Formats.Asn1/src/System.Formats.Asn1.csproj @@ -4,8 +4,8 @@ enable $(NetCoreAppCurrent);netstandard2.0;net461 true - false - 1 + true + 2 Provides classes that can read and write the ASN.1 BER, CER, and DER data formats. Commonly Used Types: