SignedCms: Improve NetFx compat for SignedCms wrapping EnvelopedCms#30405
SignedCms: Improve NetFx compat for SignedCms wrapping EnvelopedCms#30405bartonjs merged 3 commits intodotnet:masterfrom
Conversation
In NetFX if a SignedCms is created using only CmsSigners with IssuerAndSerial as the signer identifier type, the document gets encoded using the older PKCS7 structural definition instead of the newer CMS one. RFC 5652 has a long section (5.2.1) on how to read these documents compatibly. Since the defaults in SignedCms / CmsSigner are the PKCS7 behavior, not reading it means that Signed(Enveloped) documents from NetFX cannot be read.
bartonjs
force-pushed
the
signedcms_over_envelopedcms
branch
from
2e9c258 to
ab14272
Compare
|
|
||
| rented = ArrayPool<byte>.Shared.Rent(wrappedContent.Length); | ||
|
|
||
| if (!reader.TryCopyOctetStringBytes(rented, out bytesWritten)) |
There was a problem hiding this comment.
Out of curiosity: When is TryGetPrimitiveOctetStringBytes going to fail and TryCopyOctetStringBytes will succeed?
There was a problem hiding this comment.
Is it the primitive vs non-primitive?
There was a problem hiding this comment.
Yep. TryGetPrimitive returns false if the tag is correct, but the Primitive vs Constructed is not. TryCopy can handle both.
|
|
||
| return rented.AsSpan(0, bytesWritten).ToArray(); | ||
| } | ||
| catch (Exception) when (contentType != Oids.Pkcs7Data) |
There was a problem hiding this comment.
Should this be CryptographicException?
There was a problem hiding this comment.
There shouldn't be any other errors there, but the guidance from the RFC is
... If the
implementation is unable to ASN.1 decode the SignedData type using
the CMS SignedData encapContentInfo eContent OCTET STRING syntax,
then the implementation MAY attempt to decode the SignedData type
using the PKCS #7 SignedData contentInfo content ANY syntax and
compute the message digest accordingly.
So I went with "if anything goes wrong, fall back".
| if (!Detached) | ||
| { | ||
| _signedData.EncapContentInfo.Content = content; | ||
| using (AsnWriter writer = new AsnWriter(AsnEncodingRules.DER)) |
There was a problem hiding this comment.
Is this always writing empty octet string? (_hasData == false) if so we could cache this value
There was a problem hiding this comment.
!_hasData here means "we're the first signer" and thus "we have to build the 'document data' in addition to the 'signer data'". It doesn't mean that content is empty.
There was a problem hiding this comment.
I presume double wrapping (contentType != Pks7Data) is intended here?
There was a problem hiding this comment.
Well, it's single wrapping. This is putting back the OCTET STRING that got lost by changing from [OctetString] to [AnyValue] in the serializer type. The current CMS RFCs say to always write the OCTET STRING.
Rather than match what NetFx writes I just went with "be capable of reading what NetFx writes", but since the CMS update was from 1999, it seems that the "new" format should be pretty well supported by readers.
| [Fact] | ||
| public static void CheckSignedEncrypted_IssuerSerial_FromNetFx() | ||
| { | ||
| CheckSignedEncrypted( |
There was a problem hiding this comment.
Those can be collapsed into a single theory with MemberData - I'm fine either way though
There was a problem hiding this comment.
Yep, but when passing in byte[] that produces test names that are hard to understand in failure messages and result logs.
| // Due to the way the underlying Windows CMS API behaves a copy of the content | ||
| // bytes will be held separate once the content is "bound" (first signature or decode) | ||
| private ReadOnlyMemory<byte>? _heldContent; | ||
| // During decode, if the PKCS#7 fallback for a missing OCTET STRING is present, this |
There was a problem hiding this comment.
Nit: blank line above this?
4 participants
The SignedCms implementation on .NET Framework writes using the older PKCS7 encoding when writing documents only signed with IssuerAndSerialNumber as the signer identifier type.
As far as SignedData is concerned, the only difference between PKCS7 and CMS is that when the content type is not the OID for "data" (plain bytes) CMS wraps the bytes in an OCTET STRING, PKCS7 does not. Unfortunately, this means that .NET Core cannot read those messages from .NET Framework.
This change adds 4 more static documents to the test suite, each of NetFx and CoreFx signing an EnvelopedCms with each of IssuerAndSerialNumber and SubjectKeyIdentifier. Before the product code changes the only test that failed was CoreFx reading the NetFx-prepared document. Now CoreFx is capable of reading older PKCS7 messages.
Additionally, this change fixes a CMS conformance issue, where the content-type attribute should be added to all signers when the document content type is not "data".