Ensure that exporting an unsorted attributes signed payload doesn't sort them.

Author: bartonjs

src/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/Asn1/SignerInfoAsn.cs

@@ -35,9 +35,9 @@ internal struct SignerInfoAsn
         public AlgorithmIdentifierAsn DigestAlgorithm;
 
         [ExpectedTag(0)]
-        [SetOf]
         [OptionalValue]
-        public AttributeAsn[] SignedAttributes;
+        [AnyValue]
+        public ReadOnlyMemory<byte>? SignedAttributes;
 
         public AlgorithmIdentifierAsn SignatureAlgorithm;
 
@@ -49,4 +49,16 @@ internal struct SignerInfoAsn
         [OptionalValue]
         public AttributeAsn[] UnsignedAttributes;
     }
+
+    // This type is not properly from the ASN module, but it exists to allow for
+    // deserialization on demand of the signed attributes, so the deserialization
+    // and reserialization process does not modify the contents of the signed
+    // attributes.
+    [Choice]
+    internal struct SignedAttributesSet
+    {
+        [ExpectedTag(0)]
+        [SetOf]
+        public AttributeAsn[] SignedAttributes;
+    }
 }

src/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/CmsSigner.cs

@@ -151,14 +151,23 @@ internal SignerInfoAsn Sign(
                 }
 
                 // Use the serializer/deserializer to DER-normalize the attribute order.
-                newSignerInfo.SignedAttributes = Helpers.NormalizeSet(
+                SignedAttributesSet signedAttrsSet = new SignedAttributesSet();
+                signedAttrsSet.SignedAttributes = Helpers.NormalizeSet(
                     signedAttrs.ToArray(),
                     normalized =>
                     {
                         AsnReader reader = new AsnReader(normalized, AsnEncodingRules.DER);
                         hasher.AppendData(reader.PeekContentBytes().Span);
                     });
 
+                // Since this contains user data in a context where BER is permitted, use BER.
+                // There shouldn't be any observable difference here between BER and DER, though,
+                // since the top level fields were written by NormalizeSet.
+                using (AsnWriter attrsWriter = AsnSerializer.Serialize(signedAttrsSet, AsnEncodingRules.BER))
+                {
+                    newSignerInfo.SignedAttributes = attrsWriter.Encode();
+                }
+
                 dataHash = hasher.GetHashAndReset();
             }
 

src/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/SignerInfo.cs

@@ -21,6 +21,7 @@ public sealed class SignerInfo
 
         private readonly Oid _digestAlgorithm;
         private readonly AttributeAsn[] _signedAttributes;
+        private readonly ReadOnlyMemory<byte>? _signedAttributesMemory;
         private readonly Oid _signatureAlgorithm;
         private readonly ReadOnlyMemory<byte>? _signatureAlgorithmParameters;
         private readonly ReadOnlyMemory<byte> _signature;
@@ -37,12 +38,22 @@ internal SignerInfo(ref SignerInfoAsn parsedData, SignedCms ownerDocument)
             Version = parsedData.Version;
             SignerIdentifier = new SubjectIdentifier(parsedData.Sid);
             _digestAlgorithm = parsedData.DigestAlgorithm.Algorithm;
-            _signedAttributes = parsedData.SignedAttributes;
+            _signedAttributesMemory = parsedData.SignedAttributes;
             _signatureAlgorithm = parsedData.SignatureAlgorithm.Algorithm;
             _signatureAlgorithmParameters = parsedData.SignatureAlgorithm.Parameters;
             _signature = parsedData.SignatureValue;
             _unsignedAttributes = parsedData.UnsignedAttributes;
 
+            if (_signedAttributesMemory.HasValue)
+            {
+                SignedAttributesSet signedSet = AsnSerializer.Deserialize<SignedAttributesSet>(
+                    _signedAttributesMemory.Value,
+                    AsnEncodingRules.BER);
+
+                _signedAttributes = signedSet.SignedAttributes;
+                Debug.Assert(_signedAttributes != null);
+            }
+
             _document = ownerDocument;
         }
 

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

@@ -1010,6 +1010,56 @@ public static void VerifyUnsortedAttributeSignature()
             cms.CheckSignature(true);
         }
 
+        [Fact]
+        public static void VerifyUnsortedAttributeSignature_ImportExportImport()
+        {
+            SignedCms cms = new SignedCms();
+            cms.Decode(SignedDocuments.DigiCertTimeStampToken);
+
+            // Assert.NoThrows
+            cms.CheckSignature(true);
+
+            byte[] exported = cms.Encode();
+            cms = new SignedCms();
+            cms.Decode(exported);
+
+            // Assert.NoThrows
+            cms.CheckSignature(true);
+        }
+
+        [Fact]
+        public static void AddSignerToUnsortedAttributeSignature()
+        {
+            SignedCms cms = new SignedCms();
+            cms.Decode(SignedDocuments.DigiCertTimeStampToken);
+
+            // Assert.NoThrows
+            cms.CheckSignature(true);
+
+            using (X509Certificate2 cert = Certificates.RSAKeyTransferCapi1.TryGetCertificateWithPrivateKey())
+            {
+                cms.ComputeSignature(
+                    new CmsSigner(
+                        SubjectIdentifierType.IssuerAndSerialNumber,
+                        cert));
+
+                cms.ComputeSignature(
+                    new CmsSigner(
+                        SubjectIdentifierType.SubjectKeyIdentifier,
+                        cert));
+            }
+
+            // Assert.NoThrows
+            cms.CheckSignature(true);
+
+            byte[] exported = cms.Encode();
+            cms = new SignedCms();
+            cms.Decode(exported);
+
+            // Assert.NoThrows
+            cms.CheckSignature(true);
+        }
+
         [Theory]
         [InlineData(null, "0102", Oids.Pkcs7Data)]
         [InlineData(null, "010100", Oids.Pkcs7Data)]

src/System.Security.Cryptography.Pkcs/tests/SignedCms/SignerInfoTests.cs

@@ -591,6 +591,44 @@ public static void AddCounterSigner_RSA(SubjectIdentifierType identifierType)
             cms.CheckSignature(true);
         }
 
+        [Fact]
+        [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Not supported by crypt32")]
+        public static void AddCounterSignerToUnsortedAttributeSignature()
+        {
+            SignedCms cms = new SignedCms();
+            cms.Decode(SignedDocuments.DigiCertTimeStampToken);
+
+            // Assert.NoThrows
+            cms.CheckSignature(true);
+
+            SignerInfoCollection signers = cms.SignerInfos;
+            Assert.Equal(1, signers.Count);
+            SignerInfo signerInfo = signers[0];
+
+            using (X509Certificate2 cert = Certificates.RSAKeyTransferCapi1.TryGetCertificateWithPrivateKey())
+            {
+                signerInfo.ComputeCounterSignature(
+                    new CmsSigner(
+                        SubjectIdentifierType.IssuerAndSerialNumber,
+                        cert));
+
+                signerInfo.ComputeCounterSignature(
+                    new CmsSigner(
+                        SubjectIdentifierType.SubjectKeyIdentifier,
+                        cert));
+            }
+
+            // Assert.NoThrows
+            cms.CheckSignature(true);
+
+            byte[] exported = cms.Encode();
+            cms = new SignedCms();
+            cms.Decode(exported);
+
+            // Assert.NoThrows
+            cms.CheckSignature(true);
+        }
+
         [Fact]
         public static void AddCounterSigner_DSA()
         {