2014073 - Incorrect/obsolete CMS structure definition for S/MIME

Status: ASSIGNED

(NSS :: Libraries, defect, P3)

Description

[Taavi]

Steps to reproduce:

1. Generate an S/MIME email that follows RFC
2. Add CRL to the CMS structure based on RFC 3852 or RFC 5652

Looking at NSS code (https://github.com/nss-dev/nss/blob/master/lib/smime/cmsasn1.c#L90) the definition seems to be based on some ancient CMS version.

const SEC_ASN1Template NSSCMSSignedDataTemplate[] = {
/***/
          SEC_ASN1_XTRN | 1,
      offsetof(NSSCMSSignedData, crls),
      SEC_ASN1_SUB(CERT_SetOfSignedCrlTemplate) },

RFC 5652 (from 2009) and RFC 3852 (from 2004) define SignedData as follows:

      SignedData ::= SEQUENCE {
        version CMSVersion,
        digestAlgorithms DigestAlgorithmIdentifiers,
        encapContentInfo EncapsulatedContentInfo,
        certificates [0] IMPLICIT CertificateSet OPTIONAL,
        crls [1] IMPLICIT RevocationInfoChoices OPTIONAL,
        signerInfos SignerInfos }

Unfortunately NSS does not seem to check the version field appropriately and does not read crls as RevocationInfoChoices.

Actual results:

Thunderbird refuses to verify the CMS structure when there's more than one CRL (RevocationInfoChoice) in crls.

Expected results:

Even if Thunderbird/NSS does not make use of the crls* in SignedData, it should at least follow S/MIME 3.1 and have up-to-date CMS parsing data structures available to tolerate this field being used. (This also probably avoids unnecessary incompatibility with CAdES-L.)

• • In this case crls can be one or more CertificateList or OtherRevocationInfoFormat, which can contain OCSPResponse (RFC 5940).

Comment 1

[Taavi]

Vaguely related is https://bugzilla.mozilla.org/show_bug.cgi?id=36454, but that's a feature request that asks for the revocation data to be used. However, it existing not being tolerated is a bug.

Comment 2

[Taavi]

Re-reading it I might've worded it a bit odd. Just to clarify, NSS seems to tolerate CertificateList in crls but it definitely does not tolerate RevocationInfoChoice (->OtherRevocationInfoFormat) in crls. Which it should.

Structure for reference:

      RevocationInfoChoices ::= SET OF RevocationInfoChoice

      RevocationInfoChoice ::= CHOICE {
        crl CertificateList,
        other [1] IMPLICIT OtherRevocationInfoFormat }

      OtherRevocationInfoFormat ::= SEQUENCE {
        otherRevInfoFormat OBJECT IDENTIFIER,
        otherRevInfo ANY DEFINED BY otherRevInfoFormat }

Comment 3

[Taavi]

Attachment: galileo-modified.eml

CMS with a CRL and an OCSP response

Comment 4

[Taavi]

I've added an example S/MIME email that contains an RevocationInfoChoice that contains an OCSP response. It is correctly handled by OpenSSL, but not by Thunderbird.

Comment 5

[Taavi]

Attachment: Bug 2014073 - Use SEC_SetOfAnyTemplate for CMS SignedData crls field. r?kaie

The CMS SignedData crls field is defined as RevocationInfoChoices per
RFC 5652, which is a SET OF RevocationInfoChoice (a CHOICE between a
CRL and other revocation info formats like OCSP responses). The previous
ASN.1 template used CERT_SetOfSignedCrlTemplate, which required every
entry to be a valid CertificateList, causing parse failures when
OtherRevocationInfoFormat entries were present.

Change the template to SEC_SetOfAnyTemplate (matching the rawCerts
pattern) and rename the field to rawCrls. The CRL data was never
processed (marked with XXX TODO), so this change only prevents
rejection of valid messages.

Comment 6

[Taavi]

Attachment: Bug 2014073 - Improve S/MIME error reporting for opaque-signed and unsupported CMS types. r?kaie

When an opaque-signed message (smime-type=signed-data) fails CMS
decoding, report a signature error instead of "cannot decrypt".

When an unsupported CMS content type (e.g. authEnveloped-data) is
encountered, show the decryption error page instead of a blank message.

Comment 7

[Taavi]

Attachment: Bug 2014073 - Implement CMS AuthEnvelopedData decryption (RFC 5083/5084). r?kaie

Add support for decoding and decrypting AuthEnvelopedData CMS content
type with AES-GCM (AES-128/192/256-GCM). This enables Thunderbird to
read S/MIME messages encrypted with authenticated encryption.

AEAD ciphers do not support streaming, so AuthEnvelopedData bypasses
the CMS streaming filter. The ASN.1 decoder accumulates ciphertext in
rawContent, and one-shot PK11_Decrypt runs after the full structure
(including the mac field) is parsed.

Also adds GCMParameters decoding to PK11_ParamFromAlgid (RFC 5084).

Comment 8

[Phabricator Automation]

Comment on attachment 9554370 [details]
Bug 2014073 - Implement CMS AuthEnvelopedData decryption (RFC 5083/5084). r?kaie

Revision D288495 was moved to bug 1835697. Setting attachment 9554370 [details] to obsolete.

Comment 9

[Phabricator Automation]

Comment on attachment 9554242 [details]
Bug 2014073 - Improve S/MIME error reporting for opaque-signed and unsupported CMS types. r?kaie

Revision D288451 was moved to bug 594565. Setting attachment 9554242 [details] to obsolete.

Comment 10

[Phabricator Automation]

Comment on attachment 9554370 [details]
Bug 2014073 - Implement CMS AuthEnvelopedData decryption (RFC 5083/5084). r?kaie

Revision D288495 was moved to bug 1835697. Setting attachment 9554370 [details] to obsolete.

Comment 11

[Taavi]

Attachment: Bug 2014073 - Parse CMS SignedData crls field as RFC 5652 RevocationInfoChoices. r?kaie

The CMS SignedData crls field is defined as RevocationInfoChoices per
RFC 5652, which is a SET OF RevocationInfoChoice (a CHOICE between a
CRL and other revocation info formats like OCSP responses). The previous
ASN.1 template used CERT_SetOfSignedCrlTemplate, which required every
entry to be a valid CertificateList, causing parse failures when
OtherRevocationInfoFormat entries were present.

Change the template to SEC_SetOfAnyTemplate (matching the rawCerts
pattern) and rename the field to rawRevInfos, reflecting that the
contents are RevocationInfoChoices rather than CRLs specifically. The
CRL data was never processed (marked with XXX TODO), so this change
only prevents rejection of valid messages.

Adds a gtest with an RFC 5940 example (from pyasn1-modules) whose crls
field carries both a CRL and an OCSP response; it fails to parse without
the template change.

Comment 12

[Kai Engert [:KaiE:]]

gtests/smime_gtest/testvectors/signeddata-crl-ocsp.der is already expired. That's good. Because your test code works, it confirms our low level code accepts that it has a signature. I'm mentioning that, because for S/MIME emails, we treat email signatures from an expired certificate as invalid, but it seems that doesn't affect for your low level test.

You're changing the template to be able to parse the data that you've provided. Can we be certain that users don't have stored S/MIME messages in their archive that they must be able to access, but which only matched the old template, and could no longer be opened after this change?

Comment 13

[Taavi]

(In reply to Kai Engert [:KaiE:] from comment #12)

You're changing the template to be able to parse the data that you've provided. Can we be certain that users don't have stored S/MIME messages in their archive that they must be able to access, but which only matched the old template, and could no longer be opened after this change?

Yes, because the template now accepts both structures (CRL and/or OCSP) instead of just the (old) one (CRL).

Comment 14

[Kai Engert [:KaiE:]]

Your initial patch changes the nested attributes of the NSSCMSMessage data that is used in existing public APIs. That is forbidden because of the ABI compatibility promises that NSS gives.

NSS supports using an application that was built against an older version of the NSS headers, in combination with binary libraries that were built from a newer version of the NSS code.

An application might call NSS_CMSMessage_CreateFromDER(), receive an NSSCMSMessage object, and access the NSSCMSMessage.contentInfo.content.signedData->crls attribute (or the NSSCMSMessage.contentInfo.content.NSSCMSEnvelopedData.originatorInfo->crls attribute).

If it did, the compiled code expects those variables to be of type CERTSignedCrl**.
If the application attempts to access that, but the variable is actually of type SECItem** instead, this will fail or crash.

This scenario might happen on a long-term-support version of an operating system that offers NSS using a system-wide package, that upgrades the system to a newer version of NSS. An organization might be deploying an individual application that uses that API. Because the API exposes that attribute, the app might use that field.

I see two possible ways to fix that.

(1)
Keep the existing output type, unmodified, for using them with the API.
Continue to use the existing template, unmodified, when encoding the types.

Add a separate, new template definition, with the structure you have suggested, only to be used internally by the APIs.
Define a new structure type, strictly internally, that uses the modified variable.

When parsing from DER, parse using the new internal structure and type.

Don't return that data to the caller. Rather, transform it to the type that was originally expected.

The transformation would iterate the new attributes that are found in rawRevInfos and use them to populate the original crls variable, and ignoring all the other parsed attributes.

The intention is that old applications that receive old messages, that used a format that worked, must continue to work with the new NSS version, and must still be able to obtain all the attributes that worked on the past.

(2)
Introduce a new generation of the API, replacements for the CMS decoder functions.

When doing so, we should use a different approach for the output values from the API. I think we should move away from the old approach where a complex nested C structure is directly returned.

The reason is, our current implementation might differ from the spec in many additional aspects, and those might require additional changes to the data that we return from decoding etc.

I would like to avoid that we'd introduce generation 2 of the API, and notice soon after that the new API is still wrong. We don't want to go to a version 3 API soon afterwards.

So, instead of returning the data directly from the API, I suggest to return an opaque pointer, and have the consumers use accessor functions to obtain the attributes.

That's the idea, but it might not be trivial and more work.

Comment 15

[Kai Engert [:KaiE:]]

Attachment: explore-cms-crl-abi-incompat-options.patch

This patch contains untested exploration of option (1)

Comment 16

[Kai Engert [:KaiE:]]

Attachment: 2014073-abi-diff.txt (for abandoned revision)

Comment 17

[Taavi]

I will apply the patch to avoid changing the ABI.

However, having access to the field properly would be necessary to utilize the "stapled" OCSP response in Thunderbird later on.

Comment 18

[Kai Engert [:KaiE:]]

(In reply to Taavi from comment #17)

However, having access to the field properly would be necessary to utilize the "stapled" OCSP response in Thunderbird later on.

That will require a new API.

Comment 19

[Taavi]

I really couldn't find a better way to implement this than something inspired by your patch. I updated the patchset.

Is it possible that at some point the current APIs could be switched to the new structures as an intentional ABI break? Or is it perpetual? Requires a parallel API?

Comment 20

[Kai Engert [:KaiE:]]

(In reply to Taavi from comment #19)

Is it possible that at some point the current APIs could be switched to the new structures as an intentional ABI break?

No. The only way is to add another API.

Comment 21

[BugBot [:suhaib / :marco]]

The severity field is not set for this bug.
:jschanck, could you have a look please?

For more information, please visit BugBot documentation.