Allow SignedCms to verify documents whose signer didn't sort attributes.

The CMS RFC says that when computing the digest value for a SignerInfo
which contains attributes that the digest should be over the DER encoded
SET OF attributes.  DER rules for SET OF include applying a data sort.

Signers exist in the wild which do not appropriately sort their attributes before
computing the signature, and Windows has a compatibility fallback to allow
these signers' signatures to be considered valid.  This change adds a similar
fallback to the managed implementation of SignedCms so that documents
do not go from valid to invalid when transitioning from .NET Framework to
.NET Core.

Commit migrated from dotnet/corefx@dea4266

src/libraries/System.Security.Cryptography.Pkcs/src/Internal/Cryptography/Helpers.cs

@@ -432,15 +432,6 @@ public static byte[] GetSubjectKeyIdentifier(this X509Certificate2 certificate)
             }
         }
 
-        internal static void DigestWriter(IncrementalHash hasher, AsnWriter writer)
-        {
-#if netcoreapp
-            hasher.AppendData(writer.EncodeAsSpan());
-#else
-            hasher.AppendData(writer.Encode());
-#endif
-        }
-
         internal static byte[] OneShot(this ICryptoTransform transform, byte[] data)
         {
             return OneShot(transform, data, 0, data.Length);

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

@@ -3,6 +3,7 @@
 // See the LICENSE file in the project root for more information.
 
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Linq;
 using System.Security.Cryptography.Asn1;
 using System.Security.Cryptography.Pkcs.Asn1;
@@ -471,15 +472,27 @@ public void CheckSignature(X509Certificate2Collection extraStore, bool verifySig
 
         public void CheckHash()
         {
-            IncrementalHash hasher = PrepareDigest();
-            byte[] expectedSignature = hasher.GetHashAndReset();
-
-            if (!_signature.Span.SequenceEqual(expectedSignature))
+            if (!CheckHash(compatMode: false) && !CheckHash(compatMode: true))
             {
                 throw new CryptographicException(SR.Cryptography_BadSignature);
             }
         }
 
+        private bool CheckHash(bool compatMode)
+        {
+            using (IncrementalHash hasher = PrepareDigest(compatMode))
+            {
+                if (hasher == null)
+                {
+                    Debug.Assert(compatMode, $"{nameof(PrepareDigest)} returned null for the primary check");
+                    return false;
+                }
+
+                byte[] expectedSignature = hasher.GetHashAndReset();
+                return _signature.Span.SequenceEqual(expectedSignature);
+            }
+        }
+
         private X509Certificate2 FindSignerCertificate()
         {
             return FindSignerCertificate(SignerIdentifier, _document.Certificates);
@@ -542,7 +555,7 @@ private static X509Certificate2 FindSignerCertificate(
             return match;
         }
 
-        private IncrementalHash PrepareDigest()
+        private IncrementalHash PrepareDigest(bool compatMode)
         {
             HashAlgorithmName hashAlgorithmName = GetDigestAlgorithm();
 
@@ -586,7 +599,19 @@ private IncrementalHash PrepareDigest()
 
                 using (AsnWriter writer = new AsnWriter(AsnEncodingRules.DER))
                 {
-                    writer.PushSetOf();
+                    // Some CMS implementations exist which do not sort the attributes prior to
+                    // generating the signature.  While they are not, technically, validly signed,
+                    // Windows and OpenSSL both support trying in the document order rather than
+                    // a sorted order.  To accomplish this we will build as a SEQUENCE OF, but feed
+                    // the SET OF into the hasher.
+                    if (compatMode)
+                    {
+                        writer.PushSequence();
+                    }
+                    else
+                    {
+                        writer.PushSetOf();
+                    }
 
                     foreach (AttributeAsn attr in _signedAttributes)
                     {
@@ -615,10 +640,39 @@ private IncrementalHash PrepareDigest()
                         }
                     }
 
-                    writer.PopSetOf();
-                    Helpers.DigestWriter(hasher, writer);
+                    if (compatMode)
+                    {
+                        writer.PopSequence();
+
+#if netcoreapp
+                        Span<byte> setOfTag = stackalloc byte[1];
+                        setOfTag[0] = 0x31;
+
+                        hasher.AppendData(setOfTag);
+                        hasher.AppendData(writer.EncodeAsSpan().Slice(1));
+#else
+                        byte[] encoded = writer.Encode();
+                        encoded[0] = 0x31;
+                        hasher.AppendData(encoded);
+#endif
+                    }
+                    else
+                    {
+                        writer.PopSetOf();
+
+#if netcoreapp
+                        hasher.AppendData(writer.EncodeAsSpan());
+#else
+                        hasher.AppendData(writer.Encode());
+#endif
+                    }
                 }
             }
+            else if (compatMode)
+            {
+                // If there were no signed attributes there's nothing to be compatible about.
+                return null;
+            }
 
             if (invalid)
             {
@@ -633,40 +687,16 @@ private void Verify(
             X509Certificate2 certificate,
             bool verifySignatureOnly)
         {
-            IncrementalHash hasher = PrepareDigest();
             CmsSignature signatureProcessor = CmsSignature.ResolveAndVerifyKeyType(SignatureAlgorithm.Value, key: null);
 
             if (signatureProcessor == null)
             {
                 throw new CryptographicException(SR.Cryptography_Cms_UnknownAlgorithm, SignatureAlgorithm.Value);
             }
 
-#if netcoreapp
-            // SHA-2-512 is the biggest digest type we know about.
-            Span<byte> digestValue = stackalloc byte[512 / 8];
-            ReadOnlySpan<byte> digest = digestValue;
-            ReadOnlyMemory<byte> signature = _signature;
-
-            if (hasher.TryGetHashAndReset(digestValue, out int bytesWritten))
-            {
-                digest = digestValue.Slice(0, bytesWritten);
-            }
-            else
-            {
-                digest = hasher.GetHashAndReset();
-            }
-#else
-            byte[] digest = hasher.GetHashAndReset();
-            byte[] signature = _signature.ToArray();
-#endif
-
-            bool signatureValid = signatureProcessor.VerifySignature(
-                digest,
-                signature,
-                DigestAlgorithm.Value,
-                hasher.AlgorithmName,
-                _signatureAlgorithmParameters,
-                certificate);
+            bool signatureValid =
+                VerifySignature(signatureProcessor, certificate, compatMode: false) ||
+                VerifySignature(signatureProcessor, certificate, compatMode: true);
 
             if (!signatureValid)
             {
@@ -710,6 +740,48 @@ private void Verify(
             }
         }
 
+        private bool VerifySignature(
+            CmsSignature signatureProcessor,
+            X509Certificate2 certificate,
+            bool compatMode)
+        {
+            using (IncrementalHash hasher = PrepareDigest(compatMode))
+            {
+                if (hasher == null)
+                {
+                    Debug.Assert(compatMode, $"{nameof(PrepareDigest)} returned null for the primary check");
+                    return false;
+                }
+
+#if netcoreapp
+                // SHA-2-512 is the biggest digest type we know about.
+                Span<byte> digestValue = stackalloc byte[512 / 8];
+                ReadOnlySpan<byte> digest = digestValue;
+                ReadOnlyMemory<byte> signature = _signature;
+
+                if (hasher.TryGetHashAndReset(digestValue, out int bytesWritten))
+                {
+                    digest = digestValue.Slice(0, bytesWritten);
+                }
+                else
+                {
+                    digest = hasher.GetHashAndReset();
+                }
+#else
+                byte[] digest = hasher.GetHashAndReset();
+                byte[] signature = _signature.ToArray();
+#endif
+
+                return signatureProcessor.VerifySignature(
+                    digest,
+                    signature,
+                    DigestAlgorithm.Value,
+                    hasher.AlgorithmName,
+                    _signatureAlgorithmParameters,
+                    certificate);
+            }
+        }
+
         private HashAlgorithmName GetDigestAlgorithm()
         {
             return Helpers.GetDigestAlgorithm(DigestAlgorithm.Value);

src/libraries/System.Security.Cryptography.Pkcs/tests/Rfc3161/TimestampTokenInfoTests.cs

@@ -15,6 +15,7 @@ public static class TimestampTokenInfoTests
         [Theory]
         [InlineData(nameof(TimestampTokenTestData.FreeTsaDotOrg1))]
         [InlineData(nameof(TimestampTokenTestData.Symantec1))]
+        [InlineData(nameof(TimestampTokenTestData.DigiCert1))]
         public static void CreateFromParameters(string testDataName)
         {
             TimestampTokenTestData testData = TimestampTokenTestData.GetTestData(testDataName);
@@ -124,6 +125,8 @@ public static void CreateFromParameters(string testDataName)
         [InlineData(nameof(TimestampTokenTestData.FreeTsaDotOrg1), true)]
         [InlineData(nameof(TimestampTokenTestData.Symantec1), false)]
         [InlineData(nameof(TimestampTokenTestData.Symantec1), true)]
+        [InlineData(nameof(TimestampTokenTestData.DigiCert1), false)]
+        [InlineData(nameof(TimestampTokenTestData.DigiCert1), true)]
         public static void CreateFromValue(string testDataName, bool viaTry)
         {
             TimestampTokenTestData testData = TimestampTokenTestData.GetTestData(testDataName);