[release/9.0-staging] Fix erroneous success in AsnDecoder.ReadSequence

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.

Co-authored-by: Jeremy Barton <[email protected]>

Author: github-actions[bot]

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);
                 }

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<byte> 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<byte> 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<byte> 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<byte> 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<byte> 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<byte> span = new ReadOnlySpan<byte>(ptr, int.MaxValue);
+                    ExpectSuccess(span, ruleSet, Asn1Tag.PrimitiveOctetString, 6, int.MaxValue - 6);
+                }
+            }
+        }
+
+        private static void ExpectSuccess(
+            ReadOnlySpan<byte> 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<byte> 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<AsnContentException>(e);
+            }
+
+            Assert.Equal(seedTag, tag);
+            Assert.Equal(seed, contentOffset);
+            Assert.Equal(seed, contentLength);
+            Assert.Equal(seed, bytesConsumed);
+        }
+    }
+}

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<byte> 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<AsnContentException>(e);
+            }
+
+            Assert.Equal(start, contentOffset);
+            Assert.Equal(start, contentLength);
+            Assert.Equal(start, bytesConsumed);
+        }
     }
 }

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<byte> 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<AsnContentException>(e);
+            }
+
+            Assert.Equal(start, contentOffset);
+            Assert.Equal(start, contentLength);
+            Assert.Equal(start, bytesConsumed);
+        }
     }
 }

src/libraries/System.Formats.Asn1/tests/System.Formats.Asn1.Tests.csproj

@@ -5,6 +5,7 @@
   </PropertyGroup>
   <ItemGroup>
     <Compile Include="Asn1TagTests.cs" />
+    <Compile Include="Decoder\ReadEncodedValueTests.cs" />
     <Compile Include="Reader\ComprehensiveReadTests.cs" />
     <Compile Include="Reader\OverlappedReads.cs" />
     <Compile Include="Reader\ParseTag.cs" />