Fix SignedCms certificate collection modification with attribute certificates

When adding or removing certificates from the certificateSet collection, we assumed that the collection would
only contain X.509 certificates. This changes the implementation so that when looking for certificates, we skip
over choices that are not an X.509 certificate.

Author: vcsjones

src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/SignedCms.cs

@@ -686,7 +686,7 @@ public void AddCertificate(X509Certificate2 certificate)
             {
                 foreach (CertificateChoiceAsn cert in _signedData.CertificateSet!)
                 {
-                    if (cert.Certificate!.Value.Span.SequenceEqual(rawData))
+                    if (cert.Certificate is not null && cert.Certificate.Value.Span.SequenceEqual(rawData))
                     {
                         throw new CryptographicException(SR.Cryptography_Cms_CertificateAlreadyInCollection);
                     }
@@ -721,7 +721,7 @@ public void RemoveCertificate(X509Certificate2 certificate)
 
                 foreach (CertificateChoiceAsn cert in _signedData.CertificateSet!)
                 {
-                    if (cert.Certificate!.Value.Span.SequenceEqual(rawData))
+                    if (cert.Certificate is not null && cert.Certificate.Value.Span.SequenceEqual(rawData))
                     {
                         PkcsHelpers.RemoveAt(ref _signedData.CertificateSet, idx);
                         Reencode();

src/libraries/System.Security.Cryptography.Pkcs/tests/SignedCms/SignedCmsTests.netcoreapp.cs

@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.Collections.Generic;
+using System.Formats.Asn1;
 using System.Linq;
 using System.Security.Cryptography.X509Certificates;
 using Test.Cryptography;
@@ -604,6 +605,55 @@ public static void CreateSignature_Ecdsa_ThrowsWithRsaSignaturePadding()
             }
         }
 
+        [Fact]
+        public static void AddCertificate_CollectionContainsAttributeCertificate()
+        {
+            SignedCms signedCms = new SignedCms();
+            signedCms.Decode(SignedDocuments.TstWithAttributeCertificate);
+            signedCms.CheckSignature(true);
+
+            int countBefore = CountCertificateChoices(SignedDocuments.TstWithAttributeCertificate);
+
+            using (X509Certificate2 cert = Certificates.RSA2048SignatureOnly.GetCertificate())
+            {
+                signedCms.AddCertificate(cert);
+                byte[] reEncoded = signedCms.Encode();
+                int countAfter = CountCertificateChoices(reEncoded);
+                Assert.Equal(countBefore + 1, countAfter);
+
+                signedCms = new SignedCms();
+                signedCms.Decode(reEncoded);
+                signedCms.CheckSignature(true);
+            }
+        }
+
+        [Fact]
+        public static void RemoveCertificate_Existing_CollectionContainsAttributeCertificate()
+        {
+            SignedCms signedCms = new SignedCms();
+            signedCms.Decode(SignedDocuments.TstWithAttributeCertificate);
+            int countBefore = CountCertificateChoices(SignedDocuments.TstWithAttributeCertificate);
+
+            signedCms.RemoveCertificate(signedCms.Certificates[0]);
+            byte[] reEncoded = signedCms.Encode();
+            int countAfter = CountCertificateChoices(reEncoded);
+            Assert.Equal(countBefore - 1, countAfter);
+        }
+
+        [Fact]
+        public static void RemoveCertificate_NonExisting_CollectionContainsAttributeCertificate()
+        {
+            SignedCms signedCms = new SignedCms();
+            signedCms.Decode(SignedDocuments.TstWithAttributeCertificate);
+
+            using (X509Certificate2 cert = Certificates.RSA2048SignatureOnly.GetCertificate())
+            {
+                // Remove a non-existing certificate so that we are forced to enumerate the entire 'certificates[0]'
+                // collection (including attribute certificates) looking for it.
+                Assert.Throws<CryptographicException>(() => signedCms.RemoveCertificate(cert));
+            }
+        }
+
         private static void VerifyWithExplicitPrivateKey(X509Certificate2 cert, AsymmetricAlgorithm key)
         {
             using (var pubCert = new X509Certificate2(cert.RawData))
@@ -664,5 +714,36 @@ private static void VerifyCounterSignatureWithExplicitPrivateKey(X509Certificate
                 Assert.Equal(counterSignerPubCert, cms.SignerInfos[0].CounterSignerInfos[0].Certificate);
             }
         }
+
+        private static int CountCertificateChoices(byte[] encoded)
+        {
+            AsnReader reader = new AsnReader(encoded, AsnEncodingRules.BER);
+            reader = reader.ReadSequence();
+            reader.ReadObjectIdentifier();
+            reader = reader.ReadSequence(new Asn1Tag(TagClass.ContextSpecific, 0));
+            reader = reader.ReadSequence();
+
+            reader.ReadInteger(); // version
+            reader.ReadSetOf(); // digestAlgorithms
+            reader.ReadSequence(); // encapsulatedContentInfo
+
+            Asn1Tag expectedTag = new Asn1Tag(TagClass.ContextSpecific, 0, true); // certificates[0]
+
+            if (reader.PeekTag() == expectedTag)
+            {
+                AsnReader certs = reader.ReadSetOf(expectedTag);
+                int count = 0;
+
+                while (certs.HasData)
+                {
+                    certs.ReadEncodedValue();
+                    count++;
+                }
+
+                return count;
+            }
+
+            return 0;
+        }
     }
 }