Validate the DER components of user-provided data

The parameters value for the PublicKey and the generator's SignatureAlgorithm
both are supposed to be DER-encoded; so validate that claim.

For the most part this isn't necessary, since rehydrating the signed cert is going
to fail to parse the certificate anyways; but being defensive is good.

5 files changed, 222 insertions, 24 deletions

diff --git a/src/Common/src/System/Security/Cryptography/DerSequenceReader.cs b/src/Common/src/System/Security/Cryptography/DerSequenceReader.cs
index 741e6085d5..2a963a0b7f 100644
--- a/src/Common/src/System/Security/Cryptography/DerSequenceReader.cs
+++ b/src/Common/src/System/Security/Cryptography/DerSequenceReader.cs
@@ -24,6 +24,8 @@ namespace System.Security.Cryptography
         internal const byte ContextSpecificConstructedTag3 = ContextSpecificConstructedTag0 | 3;
         internal const byte ConstructedSequence = ConstructedFlag | (byte)DerTag.Sequence;
 
+        // 0b1100_0000
+        internal const byte TagClassMask = 0xC0; 
         internal const byte TagNumberMask = 0x1F;
 
         internal static DateTimeFormatInfo s_validityDateTimeFormatInfo;
@@ -34,16 +36,17 @@ namespace System.Security.Cryptography
 
         internal int ContentLength { get; private set; }
 
-        private DerSequenceReader(bool startAtPayload, byte[] data)
+        private DerSequenceReader(bool startAtPayload, byte[] data, int offset, int length)
         {
             Debug.Assert(startAtPayload, "This overload is only for bypassing the sequence tag");
             Debug.Assert(data != null, "Data is null");
+            Debug.Assert(offset >= 0, "Offset is negative");
 
             _data = data;
-            _position = 0;
-            _end = data.Length;
+            _position = offset;
+            _end = offset + length;
 
-            ContentLength = data.Length;
+            ContentLength = length;
         }
 
         internal DerSequenceReader(byte[] data)
@@ -74,7 +77,7 @@ namespace System.Security.Cryptography
 
         internal static DerSequenceReader CreateForPayload(byte[] payload)
         {
-            return new DerSequenceReader(true, payload);
+            return new DerSequenceReader(true, payload, 0, payload.Length);
         }
 
         internal bool HasData
@@ -107,6 +110,61 @@ namespace System.Security.Cryptography
             _position += contentLength;
         }
 
+        internal void ValidateAndSkipDerValue()
+        {
+            byte tag = PeekTag();
+
+            // If the tag is in the UNIVERSAL class
+            if ((tag & TagClassMask) == 0)
+            {
+                // Tag 0 is special ("reserved for use by the encoding rules"), but mainly is used
+                // as the End-of-Contents marker for the indefinite length encodings, which DER prohibits.
+                //
+                // Tag 15 is reserved.
+                //
+                // So either of these are invalid.
+
+                if (tag == 0 || tag == 15)
+                    throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding);
+
+                // DER limits the constructed encoding to SEQUENCE and SET, as well as anything which gets
+                // a defined encoding as being an IMPLICIT SEQUENCE.
+
+                bool expectConstructed = false;
+
+                switch (tag & TagNumberMask)
+                {
+                    case 0x08: // External or Instance-Of
+                    case 0x0B: // EmbeddedPDV
+                    case (byte)DerTag.Sequence:
+                    case (byte)DerTag.Set:
+                    case 0x1D: // Unrestricted Character String
+                        expectConstructed = true;
+                        break;
+                }
+
+                bool isConstructed = (tag & ConstructedFlag) == ConstructedFlag;
+
+                if (expectConstructed != isConstructed)
+                    throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding);
+            }
+
+            EatTag((DerTag)tag);
+            int contentLength = EatLength();
+
+            if (contentLength > 0 && (tag & ConstructedFlag) == ConstructedFlag)
+            {
+                var childReader = new DerSequenceReader(true, _data, _position, _end - _position);
+
+                while (childReader.HasData)
+                {
+                    childReader.ValidateAndSkipDerValue();
+                }
+            }
+
+            _position += contentLength;
+        }
+
         /// <summary>
         /// Returns the next value encoded (this includes tag and length)
         /// </summary>
@@ -161,6 +219,15 @@ namespace System.Security.Cryptography
             EatTag(DerTag.BitString);
 
             int contentLength = EatLength();
+
+            if (contentLength < 1)
+                throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding);
+
+            byte unusedBits = _data[_position];
+
+            if (unusedBits > 7)
+                throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding);
+
             // skip the "unused bits" byte
             contentLength--;
             _position++;
@@ -184,6 +251,9 @@ namespace System.Security.Cryptography
             EatTag(DerTag.ObjectIdentifier);
             int contentLength = EatLength();
 
+            if (contentLength < 1)
+                throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding);
+
             // Each byte could cause 3 decimal characters to be written, plus a period. Over-allocate
             // and avoid re-alloc.
             StringBuilder builder = new StringBuilder(contentLength * 4);
@@ -457,6 +527,10 @@ namespace System.Security.Cryptography
             int answer = ScanContentLength(_data, _position, out bytesConsumed);
 
             _position += bytesConsumed;
+
+            if (answer > _end - _position)
+                throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding);
+
             return answer;
         }
 
diff --git a/src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/EncodingHelpers.cs b/src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/EncodingHelpers.cs
index b3b02e2186..066ed52def 100644
--- a/src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/EncodingHelpers.cs
+++ b/src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/EncodingHelpers.cs
@@ -4,6 +4,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Security.Cryptography;
 using System.Security.Cryptography.X509Certificates;
 
@@ -18,6 +19,23 @@ namespace Internal.Cryptography
             return new[] { Array.Empty<byte>(), Array.Empty<byte>(), derData };
         }
 
+        internal static void ValidateSignatureAlgorithm(byte[] signatureAlgorithm)
+        {
+            Debug.Assert(signatureAlgorithm != null);
+
+            // AlgorithmIdentifier ::= SEQUENCE { OBJECT IDENTIFIER, ANY }
+            DerSequenceReader validator = new DerSequenceReader(signatureAlgorithm);
+            validator.ReadOidAsString();
+
+            if (validator.HasData)
+            {
+                validator.ValidateAndSkipDerValue();
+            }
+
+            if (validator.HasData)
+                throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding);
+        }
+
         internal static byte[][] SegmentedEncodeSubjectPublicKeyInfo(this PublicKey publicKey)
         {
             if (publicKey == null)
@@ -49,6 +67,14 @@ namespace Internal.Cryptography
             }
             else
             {
+                DerSequenceReader validator =
+                    DerSequenceReader.CreateForPayload(publicKey.EncodedParameters.RawData);
+
+                validator.ValidateAndSkipDerValue();
+
+                if (validator.HasData)
+                    throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding);
+
                 algorithmIdentifier = DerEncoder.ConstructSegmentedSequence(
                     DerEncoder.SegmentedEncodeOid(publicKey.Oid),
                     publicKey.EncodedParameters.RawData.WrapAsSegmentedForSequence());
diff --git a/src/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/Pkcs10CertificationRequestInfo.cs b/src/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/Pkcs10CertificationRequestInfo.cs
index 09542a760d..6b2b54eee4 100644
--- a/src/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/Pkcs10CertificationRequestInfo.cs
+++ b/src/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/Pkcs10CertificationRequestInfo.cs
@@ -74,10 +74,13 @@ namespace System.Security.Cryptography.X509Certificates
 
             byte[] encoded = Encode();
             byte[] signature = signatureGenerator.SignData(encoded, hashAlgorithm);
+            byte[] signatureAlgorithm = signatureGenerator.GetSignatureAlgorithmIdentifier(hashAlgorithm);
+
+            EncodingHelpers.ValidateSignatureAlgorithm(signatureAlgorithm);
 
             return DerEncoder.ConstructSequence(
                 encoded.WrapAsSegmentedForSequence(),
-                signatureGenerator.GetSignatureAlgorithmIdentifier(hashAlgorithm).WrapAsSegmentedForSequence(),
+                signatureAlgorithm.WrapAsSegmentedForSequence(),
                 DerEncoder.SegmentedEncodeBitString(signature));
         }
     }
diff --git a/src/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/TbsCertificate.cs b/src/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/TbsCertificate.cs
index 7cc09ae0c2..75685417c6 100644
--- a/src/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/TbsCertificate.cs
+++ b/src/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/TbsCertificate.cs
@@ -51,11 +51,6 @@ namespace System.Security.Cryptography.X509Certificates
             Debug.Assert(SerialNumber != null);
             Debug.Assert(Issuer != null);
 
-            if (SignatureAlgorithm != null)
-            {
-                ValidateSignatureAlgorithm();
-            }
-
             List<byte[][]> encodedFields = new List<byte[][]>();
 
             byte version = Version;
@@ -74,6 +69,8 @@ namespace System.Security.Cryptography.X509Certificates
 
             // SignatureAlgorithm: Use the specified value, or ask the generator (without mutating the class)
             byte[] signatureAlgorithm = SignatureAlgorithm ?? signatureGenerator.GetSignatureAlgorithmIdentifier(hashAlgorithm);
+            EncodingHelpers.ValidateSignatureAlgorithm(signatureAlgorithm);
+
             encodedFields.Add(signatureAlgorithm.WrapAsSegmentedForSequence());
 
             // For public API allowing self-sign ease-of-use, this could be (Issuer ?? Subject).
@@ -184,23 +181,14 @@ namespace System.Security.Cryptography.X509Certificates
 
             byte[] encoded = Encode(signatureGenerator, hashAlgorithm);
             byte[] signature = signatureGenerator.SignData(encoded, hashAlgorithm);
+            byte[] signatureAlgorithm = signatureGenerator.GetSignatureAlgorithmIdentifier(hashAlgorithm);
+
+            EncodingHelpers.ValidateSignatureAlgorithm(signatureAlgorithm);
 
             return DerEncoder.ConstructSequence(
                 encoded.WrapAsSegmentedForSequence(),
-                signatureGenerator.GetSignatureAlgorithmIdentifier(hashAlgorithm).WrapAsSegmentedForSequence(),
+                signatureAlgorithm.WrapAsSegmentedForSequence(),
                 DerEncoder.SegmentedEncodeBitString(signature));
         }
-
-        private void ValidateSignatureAlgorithm()
-        {
-            DerSequenceReader algReader = new DerSequenceReader(SignatureAlgorithm);
-            algReader.ReadOid();
-
-            if (algReader.HasData)
-                algReader.SkipValue();
-
-            if (algReader.HasData)
-                throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding);
-        }
     }
 }
diff --git a/src/System.Security.Cryptography.X509Certificates/tests/CertificateCreation/CertificateRequestUsageTests.cs b/src/System.Security.Cryptography.X509Certificates/tests/CertificateCreation/CertificateRequestUsageTests.cs
index 3164f6a108..f9cfe0d5f1 100644
--- a/src/System.Security.Cryptography.X509Certificates/tests/CertificateCreation/CertificateRequestUsageTests.cs
+++ b/src/System.Security.Cryptography.X509Certificates/tests/CertificateCreation/CertificateRequestUsageTests.cs
@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
+using System.Globalization;
 using Test.Cryptography;
 using Xunit;
 
@@ -368,5 +369,111 @@ namespace System.Security.Cryptography.X509Certificates.Tests.CertificateCreatio
                 Assert.Throws<InvalidOperationException>(() => request.CreateSelfSigned(now, now.AddDays(1)));
             }
         }
+
+        [Theory]
+        [InlineData("Length Exceeds Payload", "0501")]
+        [InlineData("Leftover Data", "0101FF00")]
+        [InlineData("Constructed Null", "2500")]
+        [InlineData("Primitive Sequence", "1000")]
+        // SEQUENCE
+        // 30 10
+        //   SEQUENCE
+        //   30 02
+        //     OCTET STRING
+        //       04 01 -- Length exceeds data for inner SEQUENCE, but not the outer one.
+        //   OCTET STRING
+        //   04 04
+        //      00 00 00 00
+        [InlineData("Big Length, Nested", "3010" + "30020401" + "040400000000")]
+        [InlineData("Big Length, Nested - Context Specific", "A010" + "30020401" + "040400000000")]
+        [InlineData("Tag Only", "05")]
+        [InlineData("Empty", "")]
+        [InlineData("Reserved Tag", "0F00")]
+        [InlineData("Zero Tag", "0000")]
+        public static void InvalidPublicKeyEncoding(string caseName, string parametersHex)
+        {
+            var generator = new InvalidSignatureGenerator(
+                Array.Empty<byte>(),
+                parametersHex.HexToByteArray(),
+                new byte[] { 0x05, 0x00 });
+
+            CertificateRequest request = new CertificateRequest(
+                new X500DistinguishedName("CN=Test"),
+                generator.PublicKey,
+                HashAlgorithmName.SHA256);
+
+            DateTimeOffset now = DateTimeOffset.UtcNow;
+
+            Exception exception = Assert.Throws<CryptographicException>(
+                () => request.Create(request.SubjectName, generator, now, now.AddDays(1), new byte[1]));
+
+            if (CultureInfo.CurrentCulture.Name == "en-US")
+            {
+                Assert.Contains("ASN1", exception.Message);
+            }
+        }
+
+        [Theory]
+        [InlineData("Empty", "")]
+        [InlineData("Empty Sequence", "3000")]
+        [InlineData("Empty OID", "30020600")]
+        [InlineData("Non-Nested Data", "300206035102013001")]
+        [InlineData("Indefinite Encoding", "3002060351020130800000")]
+        [InlineData("Dangling LengthLength", "300206035102013081")]
+        public static void InvalidSignatureAlgorithmEncoding(string caseName, string sigAlgHex)
+        {
+            var generator = new InvalidSignatureGenerator(
+                Array.Empty<byte>(),
+                new byte[] { 0x05, 0x00 },
+                sigAlgHex.HexToByteArray());
+
+            CertificateRequest request = new CertificateRequest(
+                new X500DistinguishedName("CN=Test"),
+                generator.PublicKey,
+                HashAlgorithmName.SHA256);
+
+            DateTimeOffset now = DateTimeOffset.UtcNow;
+
+            Exception exception = Assert.Throws<CryptographicException>(
+                () =>
+                request.Create(request.SubjectName, generator, now, now.AddDays(1), new byte[1]));
+
+            if (CultureInfo.CurrentCulture.Name == "en-US")
+            {
+                Assert.Contains("ASN1", exception.Message);
+            }
+        }
+
+        private class InvalidSignatureGenerator : X509SignatureGenerator
+        {
+            private readonly byte[] _signatureAlgBytes;
+            private readonly PublicKey _publicKey;
+
+            internal InvalidSignatureGenerator(byte[] keyBytes, byte[] parameterBytes, byte[] sigAlgBytes)
+            {
+                _signatureAlgBytes = sigAlgBytes;
+
+                Oid oid = new Oid("2.1.2.1", "DER");
+                _publicKey = new PublicKey(
+                    oid,
+                    parameterBytes == null ? null : new AsnEncodedData(oid, parameterBytes),
+                    new AsnEncodedData(oid, keyBytes));
+            }
+
+            protected override PublicKey BuildPublicKey()
+            {
+                return _publicKey;
+            }
+
+            public override byte[] GetSignatureAlgorithmIdentifier(HashAlgorithmName hashAlgorithm)
+            {
+                return _signatureAlgBytes;
+            }
+
+            public override byte[] SignData(byte[] data, HashAlgorithmName hashAlgorithm)
+            {
+                throw new InvalidOperationException("The test should not have made it this far");
+            }
+        }
     }
 }