Make Add/Remove UnsignedAttribute work with counter signers
authorKrzysztof Wicher <mordotymoja@gmail.com>
Sat, 21 Sep 2019 16:39:04 +0000 (09:39 -0700)
committerJeremy Barton <jbarton@microsoft.com>
Sat, 21 Sep 2019 16:39:04 +0000 (09:39 -0700)
Commit migrated from https://github.com/dotnet/corefx/commit/4c074ae47660448eae66a362076124177709640a

src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/SignerInfo.cs
src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/SignerInfoCollection.cs
src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/SubjectIdentifier.cs
src/libraries/System.Security.Cryptography.Pkcs/tests/SignedCms/SignerInfoTests.netcoreapp.cs

index b03ed5e..b964275 100644 (file)
@@ -120,18 +120,94 @@ namespace System.Security.Cryptography.Pkcs
 
         public Oid SignatureAlgorithm => new Oid(_signatureAlgorithm);
 
-        public void AddUnsignedAttribute(AsnEncodedData unsignedAttribute)
+        private delegate void WithSelfInfoDelegate(ref SignerInfoAsn mySigned);
+
+        private void WithSelfInfo(WithSelfInfoDelegate action)
         {
-            int myIdx = _document.SignerInfos.FindIndexForSigner(this);
+            if (_parentSignerInfo == null)
+            {
+                int myIdx = _document.SignerInfos.FindIndexForSigner(this);
 
-            if (myIdx < 0)
+                if (myIdx < 0)
+                {
+                    throw new CryptographicException(SR.Cryptography_Cms_SignerNotFound);
+                }
+
+                ref SignedDataAsn signedData = ref _document.GetRawData();
+                ref SignerInfoAsn mySigner = ref signedData.SignerInfos[myIdx];
+
+                action(ref mySigner);
+
+                // Re-normalize the document
+                _document.Reencode();
+            }
+            else
             {
-                throw new CryptographicException(SR.Cryptography_Cms_SignerNotFound);
+                // we are one level deep, we need to update signer and counter signer attributes
+                int parentIdx = _document.SignerInfos.FindIndexForSigner(_parentSignerInfo);
+
+                if (parentIdx == -1)
+                {
+                    throw new CryptographicException(SR.Cryptography_Cms_NoSignerAtIndex);
+                }
+
+                ref SignedDataAsn documentData = ref _document.GetRawData();
+                ref SignerInfoAsn parentData = ref documentData.SignerInfos[parentIdx];
+
+                if (parentData.UnsignedAttributes == null)
+                {
+                    throw new CryptographicException(SR.Cryptography_Cms_NoSignerAtIndex);
+                }
+
+                ref AttributeAsn[] unsignedAttrs = ref parentData.UnsignedAttributes;
+
+                for (int i = 0; i < unsignedAttrs.Length; i++)
+                {
+                    ref AttributeAsn attributeAsn = ref unsignedAttrs[i];
+
+                    if (attributeAsn.AttrType.Value == Oids.CounterSigner)
+                    {
+                        for (int j = 0; j < attributeAsn.AttrValues.Length; j++)
+                        {
+                            ref ReadOnlyMemory<byte> counterSignerBytes = ref attributeAsn.AttrValues[j];
+                            SignerInfoAsn counterSigner = SignerInfoAsn.Decode(counterSignerBytes, AsnEncodingRules.BER);
+
+                            var counterSignerId = new SubjectIdentifier(counterSigner.Sid);
+
+                            if (SignerIdentifier.IsEquivalentTo(counterSignerId))
+                            {
+                                // counterSigner represent the current state of `this`
+                                action(ref counterSigner);
+
+                                using (AsnWriter writer = new AsnWriter(AsnEncodingRules.DER))
+                                {
+                                    counterSigner.Encode(writer);
+                                    counterSignerBytes = writer.Encode();
+                                }
+
+                                // Re-normalize the document
+                                _document.Reencode();
+
+                                return;
+                            }
+                        }
+                    }
+                }
+
+                throw new CryptographicException(SR.Cryptography_Cms_NoSignerAtIndex);
             }
+        }
 
-            ref SignedDataAsn signedData = ref _document.GetRawData();
-            ref SignerInfoAsn mySigner = ref signedData.SignerInfos[myIdx];
+        public void AddUnsignedAttribute(AsnEncodedData unsignedAttribute)
+        {
+            WithSelfInfo((ref SignerInfoAsn mySigner) =>
+                {
+                    AddUnsignedAttribute(ref mySigner, unsignedAttribute);
+                });
+        }
 
+        private static void AddUnsignedAttribute(ref SignerInfoAsn mySigner, AsnEncodedData unsignedAttribute)
+        {
             int existingAttribute = mySigner.UnsignedAttributes == null ? -1 : FindAttributeIndexByOid(mySigner.UnsignedAttributes, unsignedAttribute.Oid);
 
             if (existingAttribute == -1)
@@ -161,23 +237,18 @@ namespace System.Security.Cryptography.Pkcs
                 Array.Resize(ref modifiedAttr.AttrValues, newIndex + 1);
                 modifiedAttr.AttrValues[newIndex] = unsignedAttribute.RawData;
             }
-
-            // Re-normalize the document
-            _document.Reencode();
         }
 
         public void RemoveUnsignedAttribute(AsnEncodedData unsignedAttribute)
         {
-            int myIdx = _document.SignerInfos.FindIndexForSigner(this);
-
-            if (myIdx < 0)
-            {
-                throw new CryptographicException(SR.Cryptography_Cms_SignerNotFound);
-            }
-
-            ref SignedDataAsn signedData = ref _document.GetRawData();
-            ref SignerInfoAsn mySigner = ref signedData.SignerInfos[myIdx];
+            WithSelfInfo((ref SignerInfoAsn mySigner) =>
+                {
+                    RemoveUnsignedAttribute(ref mySigner, unsignedAttribute);
+                });
+        }
 
+        private static void RemoveUnsignedAttribute(ref SignerInfoAsn mySigner, AsnEncodedData unsignedAttribute)
+        {
             (int outerIndex, int innerIndex) = FindAttributeLocation(mySigner.UnsignedAttributes, unsignedAttribute, out bool isOnlyValue);
 
             if (outerIndex == -1 || innerIndex == -1)
@@ -193,9 +264,6 @@ namespace System.Security.Cryptography.Pkcs
             {
                 PkcsHelpers.RemoveAt(ref mySigner.UnsignedAttributes[outerIndex].AttrValues, innerIndex);
             }
-
-            // Re-normalize the document
-            _document.Reencode();
         }
 
         private SignerInfoCollection GetCounterSigners(AttributeAsn[] unsignedAttrs)
@@ -767,11 +835,14 @@ namespace System.Security.Cryptography.Pkcs
 
         private static int FindAttributeIndexByOid(AttributeAsn[] attributes, Oid oid, int startIndex = 0)
         {
-            for (int i = startIndex; i < attributes.Length; i++)
+            if (attributes != null)
             {
-                if (attributes[i].AttrType.Value == oid.Value)
+                for (int i = startIndex; i < attributes.Length; i++)
                 {
-                    return i;
+                    if (attributes[i].AttrType.Value == oid.Value)
+                    {
+                        return i;
+                    }
                 }
             }
 
@@ -780,13 +851,16 @@ namespace System.Security.Cryptography.Pkcs
 
         private static int FindAttributeValueIndexByEncodedData(ReadOnlyMemory<byte>[] attributeValues, ReadOnlySpan<byte> asnEncodedData, out bool isOnlyValue)
         {
-            for (int i = 0; i < attributeValues.Length; i++)
+            if (attributeValues != null)
             {
-                ReadOnlySpan<byte> data = attributeValues[i].Span;
-                if (data.SequenceEqual(asnEncodedData))
+                for (int i = 0; i < attributeValues.Length; i++)
                 {
-                    isOnlyValue = attributeValues.Length == 1;
-                    return i;
+                    ReadOnlySpan<byte> data = attributeValues[i].Span;
+                    if (data.SequenceEqual(asnEncodedData))
+                    {
+                        isOnlyValue = attributeValues.Length == 1;
+                        return i;
+                    }
                 }
             }
 
@@ -796,19 +870,22 @@ namespace System.Security.Cryptography.Pkcs
 
         private static (int, int) FindAttributeLocation(AttributeAsn[] attributes, AsnEncodedData attribute, out bool isOnlyValue)
         {
-            for (int outerIndex = 0; ; outerIndex++)
+            if (attributes != null)
             {
-                outerIndex = FindAttributeIndexByOid(attributes, attribute.Oid, outerIndex);
-
-                if (outerIndex == -1)
+                for (int outerIndex = 0; ; outerIndex++)
                 {
-                    break;
-                }
+                    outerIndex = FindAttributeIndexByOid(attributes, attribute.Oid, outerIndex);
 
-                int innerIndex = FindAttributeValueIndexByEncodedData(attributes[outerIndex].AttrValues, attribute.RawData, out isOnlyValue);
-                if (innerIndex != -1)
-                {
-                    return (outerIndex, innerIndex);
+                    if (outerIndex == -1)
+                    {
+                        break;
+                    }
+
+                    int innerIndex = FindAttributeValueIndexByEncodedData(attributes[outerIndex].AttrValues, attribute.RawData, out isOnlyValue);
+                    if (innerIndex != -1)
+                    {
+                        return (outerIndex, innerIndex);
+                    }
                 }
             }
 
index 286a98c..1035ac6 100644 (file)
@@ -76,59 +76,17 @@ namespace System.Security.Cryptography.Pkcs
         public bool IsSynchronized => false;
         public object SyncRoot => this;
 
-        internal int FindIndexForSigner(SignerInfo signer)
+        private static int FindIndexForSigner(SignerInfo[] signerInfos, SignerInfo signer)
         {
             Debug.Assert(signer != null);
             SubjectIdentifier id = signer.SignerIdentifier;
-            X509IssuerSerial issuerSerial = default;
-
-            if (id.Type == SubjectIdentifierType.IssuerAndSerialNumber)
-            {
-                issuerSerial = (X509IssuerSerial)id.Value;
-            }
 
-            for (int i = 0; i < _signerInfos.Length; i++)
+            for (int i = 0; i < signerInfos.Length; i++)
             {
-                SignerInfo current = _signerInfos[i];
+                SignerInfo current = signerInfos[i];
                 SubjectIdentifier currentId = current.SignerIdentifier;
 
-                if (currentId.Type != id.Type)
-                {
-                    continue;
-                }
-
-                bool equal = false;
-
-                switch (id.Type)
-                {
-                    case SubjectIdentifierType.IssuerAndSerialNumber:
-                    {
-                        X509IssuerSerial currentIssuerSerial = (X509IssuerSerial)currentId.Value;
-
-                        if (currentIssuerSerial.IssuerName == issuerSerial.IssuerName &&
-                            currentIssuerSerial.SerialNumber == issuerSerial.SerialNumber)
-                        {
-                            equal = true;
-                        }
-
-                        break;
-                    }
-                    case SubjectIdentifierType.SubjectKeyIdentifier:
-                        if ((string)id.Value == (string)currentId.Value)
-                        {
-                            equal = true;
-                        }
-
-                        break;
-                    case SubjectIdentifierType.NoSignature:
-                        equal = true;
-                        break;
-                    default:
-                        Debug.Fail($"No match logic for SubjectIdentifierType {id.Type}");
-                        throw new CryptographicException();
-                }
-
-                if (equal)
+                if (id.IsEquivalentTo(currentId))
                 {
                     return i;
                 }
@@ -136,5 +94,10 @@ namespace System.Security.Cryptography.Pkcs
 
             return -1;
         }
+
+        internal int FindIndexForSigner(SignerInfo signer)
+        {
+            return FindIndexForSigner(_signerInfos, signer);
+        }
     }
 }
index ffbb110..9a9affc 100644 (file)
@@ -127,5 +127,40 @@ namespace System.Security.Cryptography.Pkcs
                     throw new CryptographicException();
             }
         }
+
+        internal bool IsEquivalentTo(SubjectIdentifier other)
+        {
+            SubjectIdentifier currentId = other;
+
+            if (currentId.Type != Type)
+            {
+                return false;
+            }
+
+            X509IssuerSerial issuerSerial = default;
+
+            if (Type == SubjectIdentifierType.IssuerAndSerialNumber)
+            {
+                issuerSerial = (X509IssuerSerial)Value;
+            }
+
+            switch (Type)
+            {
+                case SubjectIdentifierType.IssuerAndSerialNumber:
+                    {
+                        X509IssuerSerial currentIssuerSerial = (X509IssuerSerial)currentId.Value;
+
+                        return currentIssuerSerial.IssuerName == issuerSerial.IssuerName &&
+                            currentIssuerSerial.SerialNumber == issuerSerial.SerialNumber;
+                    }
+                case SubjectIdentifierType.SubjectKeyIdentifier:
+                    return (string)Value == (string)currentId.Value;
+                case SubjectIdentifierType.NoSignature:
+                    return true;
+                default:
+                    Debug.Fail($"No match logic for SubjectIdentifierType {Type}");
+                    throw new CryptographicException();
+            }
+        }
     }
 }
index 7fe11be..2513b83 100644 (file)
@@ -77,6 +77,60 @@ namespace System.Security.Cryptography.Pkcs.Tests
             Assert.Equal(secondSignerCounterSignature, cms.SignerInfos[0].CounterSignerInfos[0].GetSignature());
         }
 
+        [Fact]
+        public static void SignerInfo_RemoveUnsignedAttributeFromCounterSigner_Removes()
+        {
+            SignedCms cms = new SignedCms();
+            cms.Decode(SignedDocuments.OneRsaSignerTwoRsaCounterSigners);
+
+            AsnEncodedData attribute1 = CreateTimestampToken(1);
+            AsnEncodedData attribute2 = CreateTimestampToken(2);
+
+            SignerInfo counterSigner = cms.SignerInfos[0].CounterSignerInfos[0];
+            counterSigner.AddUnsignedAttribute(attribute1);
+            counterSigner.AddUnsignedAttribute(attribute2);
+
+            ReReadSignedCms(ref cms);
+
+            SignerInfo signer = cms.SignerInfos[0];
+
+            // attributes got sorted, therefore number changed
+            Assert.Equal(2, signer.CounterSignerInfos.Count);
+            Assert.Equal(0, signer.CounterSignerInfos[0].UnsignedAttributes.Count);
+            Assert.Equal(1, signer.CounterSignerInfos[1].UnsignedAttributes.Count);
+            Assert.Equal(2, signer.CounterSignerInfos[1].UnsignedAttributes[0].Values.Count);
+
+            counterSigner = signer.CounterSignerInfos[1];
+            counterSigner.RemoveUnsignedAttribute(counterSigner.UnsignedAttributes[0].Values[0]);
+
+            // We didn't modify existing instances
+            Assert.Equal(2, signer.CounterSignerInfos.Count);
+            Assert.Equal(1, counterSigner.UnsignedAttributes.Count);
+            Assert.Equal(2, counterSigner.UnsignedAttributes[0].Values.Count);
+
+            // parent got updated
+            Assert.Equal(0, signer.CounterSignerInfos[0].UnsignedAttributes.Count);
+            Assert.Equal(1, signer.CounterSignerInfos[1].UnsignedAttributes.Count);
+
+            // We did modify the state
+            Assert.Equal(1, cms.SignerInfos[0].CounterSignerInfos[1].UnsignedAttributes.Count);
+            Assert.Equal(1, cms.SignerInfos[0].CounterSignerInfos[1].UnsignedAttributes[0].Values.Count);
+
+            ReReadSignedCms(ref cms);
+
+            counterSigner = cms.SignerInfos[0].CounterSignerInfos[1];
+            Assert.Equal(1, counterSigner.UnsignedAttributes.Count);
+            Assert.Equal(1, counterSigner.UnsignedAttributes[0].Values.Count);
+            counterSigner.RemoveUnsignedAttribute(counterSigner.UnsignedAttributes[0].Values[0]);
+
+            // We didn't modify current instance
+            Assert.Equal(1, counterSigner.UnsignedAttributes.Count);
+            Assert.Equal(1, counterSigner.UnsignedAttributes[0].Values.Count);
+
+            // We did modify the state
+            Assert.Equal(0, cms.SignerInfos[0].CounterSignerInfos[1].UnsignedAttributes.Count);
+        }
+
         [Theory]
         [MemberData(nameof(SignedDocumentsWithAttributesTestData))]
         public static void SignerInfo_RemoveUnsignedAttributes_RemoveAllAttributesFromBeginning(byte[] document)
@@ -221,6 +275,186 @@ namespace System.Security.Cryptography.Pkcs.Tests
             cms.CheckSignature(true);
         }
 
+        [Fact]
+        public static void SignerInfo_RemoveNonMatchingUnsignedAttributesFromCounterSigner_Throws()
+        {
+            SignedCms cms = new SignedCms();
+            cms.Decode(SignedDocuments.OneRsaSignerTwoRsaCounterSigners);
+
+            int numberOfAttributes = cms.SignerInfos[0].UnsignedAttributes.Count;
+            Assert.NotEqual(0, numberOfAttributes);
+
+            AsnEncodedData fakeAttribute = new AsnEncodedData(
+                cms.SignerInfos[0].UnsignedAttributes[0].Oid,
+                cms.SignerInfos[0].UnsignedAttributes[0].Values[0].RawData.Skip(1).ToArray());
+            Assert.Throws<CryptographicException>(() => cms.SignerInfos[0].CounterSignerInfos[0].RemoveUnsignedAttribute(fakeAttribute));
+
+            Assert.Equal(numberOfAttributes, cms.SignerInfos[0].UnsignedAttributes.Count);
+        }
+
+        [Fact]
+        public static void SignerInfo_AddOneUnsignedAttributeToCounterSigner_Adds()
+        {
+            SignedCms cms = new SignedCms();
+            cms.Decode(SignedDocuments.OneRsaSignerTwoRsaCounterSigners);
+
+            SignerInfo signer = cms.SignerInfos[0];
+            Assert.Equal(2, signer.UnsignedAttributes.Count);
+            Assert.Equal(2, signer.CounterSignerInfos.Count);
+
+            SignerInfo counterSigner0 = signer.CounterSignerInfos[0];
+            SignerInfo counterSigner1 = signer.CounterSignerInfos[1];
+
+            Assert.Equal(0, counterSigner0.UnsignedAttributes.Count);
+            Assert.Equal(0, counterSigner1.UnsignedAttributes.Count);
+
+            AsnEncodedData attribute = CreateTimestampToken(1);
+            counterSigner0.AddUnsignedAttribute(attribute);
+
+            // we didn't modify existing instances
+            Assert.Equal(2, signer.CounterSignerInfos.Count);
+            Assert.Equal(0, counterSigner0.UnsignedAttributes.Count);
+            Assert.Equal(0, counterSigner1.UnsignedAttributes.Count);
+
+            // inner counter signers did get updated
+            Assert.Equal(1, signer.CounterSignerInfos[0].UnsignedAttributes.Count);
+            Assert.Equal(1, signer.CounterSignerInfos[0].UnsignedAttributes[0].Values.Count);
+            Assert.Equal(0, signer.CounterSignerInfos[1].UnsignedAttributes.Count);
+
+            // verify we didn't modify anything we shouldn't
+            Assert.Equal(1, cms.SignerInfos.Count);
+            Assert.Equal(2, cms.SignerInfos[0].UnsignedAttributes.Count);
+            Assert.Equal(2, cms.SignerInfos[0].CounterSignerInfos.Count);
+
+            // note: counter signers got sorted when encoded
+            Assert.Equal(0, cms.SignerInfos[0].CounterSignerInfos[0].UnsignedAttributes.Count);
+            Assert.Equal(1, cms.SignerInfos[0].CounterSignerInfos[1].UnsignedAttributes.Count);
+            Assert.Equal(1, cms.SignerInfos[0].CounterSignerInfos[1].UnsignedAttributes[0].Values.Count);
+            VerifyAttributesAreEqual(cms.SignerInfos[0].CounterSignerInfos[1].UnsignedAttributes[0].Values[0], attribute);
+
+            ReReadSignedCms(ref cms);
+
+            Assert.Equal(2, cms.SignerInfos[0].UnsignedAttributes.Count);
+            Assert.Equal(2, cms.SignerInfos[0].CounterSignerInfos.Count);
+            Assert.Equal(0, cms.SignerInfos[0].CounterSignerInfos[0].UnsignedAttributes.Count);
+            Assert.Equal(1, cms.SignerInfos[0].CounterSignerInfos[1].UnsignedAttributes.Count);
+            Assert.Equal(1, cms.SignerInfos[0].CounterSignerInfos[1].UnsignedAttributes[0].Values.Count);
+            VerifyAttributesAreEqual(cms.SignerInfos[0].CounterSignerInfos[1].UnsignedAttributes[0].Values[0], attribute);
+        }
+
+        [Fact]
+        public static void SignerInfo_AddMultipleUnsignedAttributeToCounterSigner_Adds()
+        {
+            SignedCms cms = new SignedCms();
+            cms.Decode(SignedDocuments.OneRsaSignerTwoRsaCounterSigners);
+
+            SignerInfo signer = cms.SignerInfos[0];
+            Assert.Equal(2, signer.UnsignedAttributes.Count);
+            Assert.Equal(2, signer.CounterSignerInfos.Count);
+
+            SignerInfo counterSigner0 = signer.CounterSignerInfos[0];
+            SignerInfo counterSigner1 = signer.CounterSignerInfos[1];
+
+            Assert.Equal(0, counterSigner0.UnsignedAttributes.Count);
+            Assert.Equal(0, counterSigner1.UnsignedAttributes.Count);
+
+            AsnEncodedData attribute1 = CreateTimestampToken(1);
+            AsnEncodedData attribute2 = CreateTimestampToken(2);
+
+            // note: using same instance to add
+            counterSigner0.AddUnsignedAttribute(attribute1);
+            SignerInfo signerAfterFirstCall = cms.SignerInfos[0];
+            counterSigner0.AddUnsignedAttribute(attribute2);
+
+            List<AsnEncodedData> expectedAttributes = new List<AsnEncodedData>()
+            {
+                attribute1,
+                attribute2
+            };
+
+            // we didn't modify existing instances
+            Assert.Equal(2, signer.CounterSignerInfos.Count);
+            Assert.Equal(0, counterSigner0.UnsignedAttributes.Count);
+            Assert.Equal(0, counterSigner1.UnsignedAttributes.Count);
+
+            // inner counter signers did get updated
+            Assert.Equal(1, signer.CounterSignerInfos[0].UnsignedAttributes.Count);
+
+            // this is rather less obvious:
+            //   - on first Add call we update the parent so the `signer` gets updated
+            //   - on second Add call we also update the parent but `signer` is not actually the parent anymore
+            Assert.Equal(1, signer.CounterSignerInfos[0].UnsignedAttributes[0].Values.Count);
+            VerifyAttributesAreEqual(signer.CounterSignerInfos[0].UnsignedAttributes[0].Values[0], attribute1);
+            Assert.Equal(0, signer.CounterSignerInfos[1].UnsignedAttributes.Count);
+
+            Assert.Equal(0, signerAfterFirstCall.CounterSignerInfos[0].UnsignedAttributes.Count);
+            Assert.Equal(1, signerAfterFirstCall.CounterSignerInfos[1].UnsignedAttributes.Count);
+            Assert.Equal(2, signerAfterFirstCall.CounterSignerInfos[1].UnsignedAttributes[0].Values.Count);
+            VerifyAttributesContainsAll(signerAfterFirstCall.CounterSignerInfos[1].UnsignedAttributes, expectedAttributes);
+
+            // verify we didn't modify anything we shouldn't
+            Assert.Equal(1, cms.SignerInfos.Count);
+            Assert.Equal(2, cms.SignerInfos[0].UnsignedAttributes.Count);
+            Assert.Equal(2, cms.SignerInfos[0].CounterSignerInfos.Count);
+
+            // note: counter signers got sorted when encoded
+            counterSigner0 = cms.SignerInfos[0].CounterSignerInfos[0];
+            counterSigner1 = cms.SignerInfos[0].CounterSignerInfos[1];
+
+            Assert.Equal(0, counterSigner0.UnsignedAttributes.Count);
+
+            // attributes should be merged into a single one with two values
+            Assert.Equal(1, counterSigner1.UnsignedAttributes.Count);
+            Assert.Equal(2, counterSigner1.UnsignedAttributes[0].Values.Count);
+
+            VerifyAttributesContainsAll(counterSigner1.UnsignedAttributes, expectedAttributes);
+
+            ReReadSignedCms(ref cms);
+
+            Assert.Equal(2, cms.SignerInfos[0].UnsignedAttributes.Count);
+            Assert.Equal(2, cms.SignerInfos[0].CounterSignerInfos.Count);
+            Assert.Equal(0, cms.SignerInfos[0].CounterSignerInfos[0].UnsignedAttributes.Count);
+            Assert.Equal(1, cms.SignerInfos[0].CounterSignerInfos[1].UnsignedAttributes.Count);
+            Assert.Equal(2, cms.SignerInfos[0].CounterSignerInfos[1].UnsignedAttributes[0].Values.Count);
+            VerifyAttributesContainsAll(cms.SignerInfos[0].CounterSignerInfos[1].UnsignedAttributes, expectedAttributes);
+        }
+
+        [Fact]
+        public static void SignerInfo_AddRemoveUnsignedAttributeWithDetachtedCounterSigner_Throws()
+        {
+            SignedCms cms = new SignedCms();
+            cms.Decode(SignedDocuments.OneRsaSignerTwoRsaCounterSigners);
+
+            // Detatch one counter signer
+            SignerInfo counterSigner = cms.SignerInfos[0].CounterSignerInfos[0];
+            cms.SignerInfos[0].RemoveCounterSignature(0);
+
+            // we shouldn't throw
+            Assert.Equal(0, counterSigner.UnsignedAttributes.Count);
+
+            AsnEncodedData attribute = CreateTimestampToken(1);
+            Assert.Throws<CryptographicException>(() => counterSigner.AddUnsignedAttribute(attribute));
+            Assert.Throws<CryptographicException>(() => counterSigner.RemoveUnsignedAttribute(attribute));
+        }
+
+        [Fact]
+        public static void SignerInfo_AddRemoveUnsignedAttributeWithDetachtedSigner_Throws()
+        {
+            SignedCms cms = new SignedCms();
+            cms.Decode(SignedDocuments.OneRsaSignerTwoRsaCounterSigners);
+
+            // Detatch signer (and its counter signers)
+            SignerInfo counterSigner = cms.SignerInfos[0].CounterSignerInfos[0];
+            cms.RemoveSignature(0);
+
+            // we shouldn't throw
+            Assert.Equal(0, counterSigner.UnsignedAttributes.Count);
+
+            AsnEncodedData attribute = CreateTimestampToken(1);
+            Assert.Throws<CryptographicException>(() => counterSigner.AddUnsignedAttribute(attribute));
+            Assert.Throws<CryptographicException>(() => counterSigner.RemoveUnsignedAttribute(attribute));
+        }
+
         private static void VerifyAttributesContainsAll(CryptographicAttributeObjectCollection attributes, List<AsnEncodedData> expectedAttributes)
         {
             var indices = new HashSet<int>();