[release/6.0] Ignore non-X509 certificates in SignedCms (#67462)

bartonjs · web-flow · commit 7e2e3c9df103 · 2022-04-13T08:23:24.000-07:00
* Ignore non-X509 certificates in SignedCms

This allows for AttributeCertificateV1/AttributeCertificateV2/OtherCertificate
entries in the SignedCms without causing a decode error.  That data is not presented
to callers via the SignedCms object, but the behavior is consistent with SignedCms on
.NET Framework and its underlying WinCryptMsg counterpart.

* Add packaging changes for servicing
diff --git a/src/libraries/System.Security.Cryptography.Pkcs/src/System.Security.Cryptography.Pkcs.csproj b/src/libraries/System.Security.Cryptography.Pkcs/src/System.Security.Cryptography.Pkcs.csproj
@@ -6,6 +6,8 @@
     <Nullable>enable</Nullable>
     <TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent);netcoreapp3.1-windows;netcoreapp3.1;netstandard2.1-windows;netstandard2.1;netstandard2.0-windows;netstandard2.0;net461-windows</TargetFrameworks>
     <IsPackable>true</IsPackable>
+    <GeneratePackageOnBuild>true</GeneratePackageOnBuild>
+    <ServicingVersion>1</ServicingVersion>
     <PackageDescription>Provides support for PKCS and CMS algorithms.
 
 Commonly Used Types:
@@ -490,6 +492,10 @@ System.Security.Cryptography.Pkcs.EnvelopedCms</PackageDescription>
     <Compile Include="System\Security\Cryptography\Pkcs\Asn1\MessageImprint.xml.cs">
       <DependentUpon>System\Security\Cryptography\Pkcs\Asn1\MessageImprint.xml</DependentUpon>
     </Compile>
+    <AsnXml Include="System\Security\Cryptography\Pkcs\Asn1\OtherCertificateFormat.xml" />
+    <Compile Include="System\Security\Cryptography\Pkcs\Asn1\OtherCertificateFormat.xml.cs">
+      <DependentUpon>System\Security\Cryptography\Pkcs\Asn1\OtherCertificateFormat.xml</DependentUpon>
+    </Compile>
     <Compile Include="System\Security\Cryptography\Pkcs\Asn1\PkiFailureInfo.cs" />
     <Compile Include="System\Security\Cryptography\Pkcs\Asn1\PkiStatus.cs" />
     <AsnXml Include="System\Security\Cryptography\Pkcs\Asn1\PkiStatusInfo.xml" />
diff --git a/src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/Asn1/CertificateChoiceAsn.xml b/src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/Asn1/CertificateChoiceAsn.xml
@@ -15,12 +15,10 @@
         other[3] IMPLICIT OtherCertificateFormat
     }
 
-    OtherCertificateFormat ::= SEQUENCE {
-        otherCertFormat OBJECT IDENTIFIER,
-        otherCert ANY DEFINED BY otherCertFormat
-    }
-
-    Except we only support public key certificates, so just trim the choice here.
   -->
   <asn:AnyValue name="Certificate" universalTagNumber="16" />
+  <asn:AnyValue name="ExtendedCertificate" implicitTag="0" />
+  <asn:AnyValue name="AttributeCertificateV1" implicitTag="1" />
+  <asn:AnyValue name="AttributeCertificateV2" implicitTag="2" />
+  <asn:AsnType name="OtherCertificateFormat" typeName="System.Security.Cryptography.Pkcs.Asn1.OtherCertificateFormat" implicitTag="3" />
 </asn:Choice>
diff --git a/src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/Asn1/CertificateChoiceAsn.xml.cs b/src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/Asn1/CertificateChoiceAsn.xml.cs
@@ -12,6 +12,10 @@ namespace System.Security.Cryptography.Pkcs.Asn1
     internal partial struct CertificateChoiceAsn
     {
         internal ReadOnlyMemory<byte>? Certificate;
+        internal ReadOnlyMemory<byte>? ExtendedCertificate;
+        internal ReadOnlyMemory<byte>? AttributeCertificateV1;
+        internal ReadOnlyMemory<byte>? AttributeCertificateV2;
+        internal System.Security.Cryptography.Pkcs.Asn1.OtherCertificateFormat? OtherCertificateFormat;
 
 #if DEBUG
         static CertificateChoiceAsn()
@@ -28,6 +32,10 @@ static CertificateChoiceAsn()
             };
 
             ensureUniqueTag(new Asn1Tag((UniversalTagNumber)16), "Certificate");
+            ensureUniqueTag(new Asn1Tag(TagClass.ContextSpecific, 0), "ExtendedCertificate");
+            ensureUniqueTag(new Asn1Tag(TagClass.ContextSpecific, 1), "AttributeCertificateV1");
+            ensureUniqueTag(new Asn1Tag(TagClass.ContextSpecific, 2), "AttributeCertificateV2");
+            ensureUniqueTag(new Asn1Tag(TagClass.ContextSpecific, 3), "OtherCertificateFormat");
         }
 #endif
 
@@ -60,6 +68,90 @@ internal void Encode(AsnWriter writer)
                 wroteValue = true;
             }
 
+            if (ExtendedCertificate.HasValue)
+            {
+                if (wroteValue)
+                    throw new CryptographicException();
+
+                // Validator for tag constraint for ExtendedCertificate
+                {
+                    if (!Asn1Tag.TryDecode(ExtendedCertificate.Value.Span, out Asn1Tag validateTag, out _) ||
+                        !validateTag.HasSameClassAndValue(new Asn1Tag(TagClass.ContextSpecific, 0)))
+                    {
+                        throw new CryptographicException();
+                    }
+                }
+
+                try
+                {
+                    writer.WriteEncodedValue(ExtendedCertificate.Value.Span);
+                }
+                catch (ArgumentException e)
+                {
+                    throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding, e);
+                }
+                wroteValue = true;
+            }
+
+            if (AttributeCertificateV1.HasValue)
+            {
+                if (wroteValue)
+                    throw new CryptographicException();
+
+                // Validator for tag constraint for AttributeCertificateV1
+                {
+                    if (!Asn1Tag.TryDecode(AttributeCertificateV1.Value.Span, out Asn1Tag validateTag, out _) ||
+                        !validateTag.HasSameClassAndValue(new Asn1Tag(TagClass.ContextSpecific, 1)))
+                    {
+                        throw new CryptographicException();
+                    }
+                }
+
+                try
+                {
+                    writer.WriteEncodedValue(AttributeCertificateV1.Value.Span);
+                }
+                catch (ArgumentException e)
+                {
+                    throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding, e);
+                }
+                wroteValue = true;
+            }
+
+            if (AttributeCertificateV2.HasValue)
+            {
+                if (wroteValue)
+                    throw new CryptographicException();
+
+                // Validator for tag constraint for AttributeCertificateV2
+                {
+                    if (!Asn1Tag.TryDecode(AttributeCertificateV2.Value.Span, out Asn1Tag validateTag, out _) ||
+                        !validateTag.HasSameClassAndValue(new Asn1Tag(TagClass.ContextSpecific, 2)))
+                    {
+                        throw new CryptographicException();
+                    }
+                }
+
+                try
+                {
+                    writer.WriteEncodedValue(AttributeCertificateV2.Value.Span);
+                }
+                catch (ArgumentException e)
+                {
+                    throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding, e);
+                }
+                wroteValue = true;
+            }
+
+            if (OtherCertificateFormat.HasValue)
+            {
+                if (wroteValue)
+                    throw new CryptographicException();
+
+                OtherCertificateFormat.Value.Encode(writer, new Asn1Tag(TagClass.ContextSpecific, 3));
+                wroteValue = true;
+            }
+
             if (!wroteValue)
             {
                 throw new CryptographicException();
@@ -107,6 +199,28 @@ private static void DecodeCore(ref AsnValueReader reader, ReadOnlyMemory<byte> r
                 tmpSpan = reader.ReadEncodedValue();
                 decoded.Certificate = rebindSpan.Overlaps(tmpSpan, out offset) ? rebind.Slice(offset, tmpSpan.Length) : tmpSpan.ToArray();
             }
+            else if (tag.HasSameClassAndValue(new Asn1Tag(TagClass.ContextSpecific, 0)))
+            {
+                tmpSpan = reader.ReadEncodedValue();
+                decoded.ExtendedCertificate = rebindSpan.Overlaps(tmpSpan, out offset) ? rebind.Slice(offset, tmpSpan.Length) : tmpSpan.ToArray();
+            }
+            else if (tag.HasSameClassAndValue(new Asn1Tag(TagClass.ContextSpecific, 1)))
+            {
+                tmpSpan = reader.ReadEncodedValue();
+                decoded.AttributeCertificateV1 = rebindSpan.Overlaps(tmpSpan, out offset) ? rebind.Slice(offset, tmpSpan.Length) : tmpSpan.ToArray();
+            }
+            else if (tag.HasSameClassAndValue(new Asn1Tag(TagClass.ContextSpecific, 2)))
+            {
+                tmpSpan = reader.ReadEncodedValue();
+                decoded.AttributeCertificateV2 = rebindSpan.Overlaps(tmpSpan, out offset) ? rebind.Slice(offset, tmpSpan.Length) : tmpSpan.ToArray();
+            }
+            else if (tag.HasSameClassAndValue(new Asn1Tag(TagClass.ContextSpecific, 3)))
+            {
+                System.Security.Cryptography.Pkcs.Asn1.OtherCertificateFormat tmpOtherCertificateFormat;
+                System.Security.Cryptography.Pkcs.Asn1.OtherCertificateFormat.Decode(ref reader, new Asn1Tag(TagClass.ContextSpecific, 3), rebind, out tmpOtherCertificateFormat);
+                decoded.OtherCertificateFormat = tmpOtherCertificateFormat;
+
+            }
             else
             {
                 throw new CryptographicException();
diff --git a/src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/Asn1/OtherCertificateFormat.xml b/src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/Asn1/OtherCertificateFormat.xml
@@ -0,0 +1,17 @@
+<?xml version="1.0" encoding="utf-8" ?>
+<asn:Sequence
+  xmlns:asn="http://schemas.dot.net/asnxml/201808/"
+  name="OtherCertificateFormat"
+  namespace="System.Security.Cryptography.Pkcs.Asn1">
+
+  <!--
+    https://tools.ietf.org/html/rfc5652#section-10.2.2
+
+    OtherCertificateFormat ::= SEQUENCE {
+        otherCertFormat OBJECT IDENTIFIER,
+        otherCert ANY DEFINED BY otherCertFormat
+    }
+  -->
+  <asn:ObjectIdentifier name="OtherCertFormat" backingType="string" />
+  <asn:AnyValue name="OtherCert" />
+</asn:Sequence>
diff --git a/src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/Asn1/OtherCertificateFormat.xml.cs b/src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/Asn1/OtherCertificateFormat.xml.cs
@@ -0,0 +1,98 @@
+﻿// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+#pragma warning disable SA1028 // ignore whitespace warnings for generated code
+using System;
+using System.Formats.Asn1;
+using System.Runtime.InteropServices;
+
+namespace System.Security.Cryptography.Pkcs.Asn1
+{
+    [StructLayout(LayoutKind.Sequential)]
+    internal partial struct OtherCertificateFormat
+    {
+        internal string OtherCertFormat;
+        internal ReadOnlyMemory<byte> OtherCert;
+
+        internal void Encode(AsnWriter writer)
+        {
+            Encode(writer, Asn1Tag.Sequence);
+        }
+
+        internal void Encode(AsnWriter writer, Asn1Tag tag)
+        {
+            writer.PushSequence(tag);
+
+            try
+            {
+                writer.WriteObjectIdentifier(OtherCertFormat);
+            }
+            catch (ArgumentException e)
+            {
+                throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding, e);
+            }
+            try
+            {
+                writer.WriteEncodedValue(OtherCert.Span);
+            }
+            catch (ArgumentException e)
+            {
+                throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding, e);
+            }
+            writer.PopSequence(tag);
+        }
+
+        internal static OtherCertificateFormat Decode(ReadOnlyMemory<byte> encoded, AsnEncodingRules ruleSet)
+        {
+            return Decode(Asn1Tag.Sequence, encoded, ruleSet);
+        }
+
+        internal static OtherCertificateFormat Decode(Asn1Tag expectedTag, ReadOnlyMemory<byte> encoded, AsnEncodingRules ruleSet)
+        {
+            try
+            {
+                AsnValueReader reader = new AsnValueReader(encoded.Span, ruleSet);
+
+                DecodeCore(ref reader, expectedTag, encoded, out OtherCertificateFormat decoded);
+                reader.ThrowIfNotEmpty();
+                return decoded;
+            }
+            catch (AsnContentException e)
+            {
+                throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding, e);
+            }
+        }
+
+        internal static void Decode(ref AsnValueReader reader, ReadOnlyMemory<byte> rebind, out OtherCertificateFormat decoded)
+        {
+            Decode(ref reader, Asn1Tag.Sequence, rebind, out decoded);
+        }
+
+        internal static void Decode(ref AsnValueReader reader, Asn1Tag expectedTag, ReadOnlyMemory<byte> rebind, out OtherCertificateFormat decoded)
+        {
+            try
+            {
+                DecodeCore(ref reader, expectedTag, rebind, out decoded);
+            }
+            catch (AsnContentException e)
+            {
+                throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding, e);
+            }
+        }
+
+        private static void DecodeCore(ref AsnValueReader reader, Asn1Tag expectedTag, ReadOnlyMemory<byte> rebind, out OtherCertificateFormat decoded)
+        {
+            decoded = default;
+            AsnValueReader sequenceReader = reader.ReadSequence(expectedTag);
+            ReadOnlySpan<byte> rebindSpan = rebind.Span;
+            int offset;
+            ReadOnlySpan<byte> tmpSpan;
+
+            decoded.OtherCertFormat = sequenceReader.ReadObjectIdentifier();
+            tmpSpan = sequenceReader.ReadEncodedValue();
+            decoded.OtherCert = rebindSpan.Overlaps(tmpSpan, out offset) ? rebind.Slice(offset, tmpSpan.Length) : tmpSpan.ToArray();
+
+            sequenceReader.ThrowIfNotEmpty();
+        }
+    }
+}
diff --git a/src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/SignedCms.cs b/src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/SignedCms.cs
@@ -89,8 +89,16 @@ public X509Certificate2Collection Certificates
 
                 foreach (CertificateChoiceAsn choice in certChoices)
                 {
-                    Debug.Assert(choice.Certificate.HasValue);
-                    coll.Add(new X509Certificate2(choice.Certificate.Value.ToArray()));
+                    if (choice.Certificate.HasValue)
+                    {
+                        coll.Add(new X509Certificate2(choice.Certificate.Value
+#if NET5_0_OR_GREATER
+                            .Span
+#else
+                            .ToArray()
+#endif
+                            ));
+                    }
                 }
 
                 return coll;
diff --git a/src/libraries/System.Security.Cryptography.Pkcs/tests/SignedCms/SignedCmsTests.cs b/src/libraries/System.Security.Cryptography.Pkcs/tests/SignedCms/SignedCmsTests.cs
@@ -1602,6 +1602,15 @@ public static void CheckNoSignature_FromCoreFx_TamperIssuerName()
             signers[0].CheckHash();
         }
 
+        [Fact]
+        public static void Decode_CanDecodeWithAttributeCertificate()
+        {
+            SignedCms cms = new SignedCms();
+            cms.Decode(SignedDocuments.TstWithAttributeCertificate);
+            Assert.Equal(2, cms.Certificates.Count);
+            cms.CheckSignature(verifySignatureOnly: true);
+        }
+
         private static void CheckNoSignature(byte[] encoded, bool badOid=false)
         {
             SignedCms cms = new SignedCms();
diff --git a/src/libraries/System.Security.Cryptography.Pkcs/tests/SignedCms/SignedDocuments.cs b/src/libraries/System.Security.Cryptography.Pkcs/tests/SignedCms/SignedDocuments.cs
