676100 - Some ASN.1 Templates for key agreement in NSS are incorrect

Status: REOPENED

(NSS :: Libraries, defect, P4)

Description

[David Cooper]

Attachment: Patch to correct Templates for key agreement in smime/cmsasn1.c

The templates in smime/cmsasn1.c related to key agreement contain a number of errors.  In some places, the templates do not match the data structures to which they refer and in other cases they do not match the ASN.1 as specified in RFC 5652.  This patch attempts to address the problems, which are as follows:

1) In NSSCMSOriginatorIdentifierOrKeyTemplate, subjectKeyIdentifier and originatorKey are incorrectly marked as having EXPLICIT tagging and the tags are incorrect.

2) NSSCMSRecipientKeyIdentifierTemplate did not match the NSSCMSRecipientKeyIdentifier as each item in NSSCMSRecipientKeyIdentifier is of type "SECItem *", but the template did not treat them as pointers.

3) NSSCMSRecipientEncryptedKeyTemplate incorrectly treated encryptedKey as a BIT STRING rather than an OCTET STRING.

4) NSSCMSRecipientInfoTemplate incorrectly marked kari and kekri as having EXPLICIT tagging.

5) ukm in NSSCMSKeyAgreeRecipientInfo is of type "SECItem *", but it is not treated as a pointer in NSSCMSKeyAgreeRecipientInfoTemplate.  In this case I corrected the problem by changing the type of ukm to "SECItem" in NSSCMSKeyAgreeRecipientInfo.  I did this since the sample (commented out) code in NSS_CMSUtil_EncryptSymKey_ESDH also treats ukm as if it were of type "SECItem" rather than "SECItem *".  (NOTE:  When I made this change to smime/cmst.h I found that  I couldn't simply recompile the code as not all files that needed to be recompiled got recompiled, so I had to run make clean and then recompile from scratch to get the code to compile correctly.)

I have tested most of these changes by doing interoperability testing with Microsoft Outlook 2010 to verify that these changes lead to correct encoding and parsing of ECDH encrypted CMS messages. (I don't have any examples of messages encrypted using static-static ECDH, so I have no examples of messages in which the subjectKeyIdentifier choice of OriginatorIdentifierOrKey is used.)

Comment 1

[Ludovic Hirlimann [:Usul]]

Comment on attachment 550216 [details] [diff] [review]
Patch to correct Templates for key agreement in smime/cmsasn1.c

David , you need to set review flags if you want your patches to get into the tree.

Comment 2

[Kai Engert [:KaiE:]]

Hi Nalin, Bob Relyea has proposed, this patch should be tested thoroughly (once reviewed). CC'ing you FYI.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

NSS has one encoder and two decoders for ASN.1 BERT/DER.
The combination of OPTIONAL and INLINE in the same template is supported only
by one decoder, not the encoder, and then only for string-ish primitive types,
not constructed types.  The usual and preferred solution to this is to make optional items NOT be inline, but rather be POINTER.  That works with all 
our encoders and decoders.  Please don't create templates that cannot be 
encoded by our encoder.

There are several different situations in ASN.1 where an item is encoded with 
EXPLICIT encoding, even though the ASN.1 for it says it is implicit.  For all
such situations, our encoders and deciders require that the template say 
EXPLICIT, regardless of the ASN.1 abstract notation says.  In the past, there
have been many errors (now corrected) where the template did not say EXPLICIT
in those cases.  All such cases known have been fixed.  

This patch takes several templates that are presently EXPLICIT, and removes
the EXPLICIT from them.  Without more study than I have time for now, I am 
not sure if any of these are cases where EXPLICIT encoding is required 
despite the ASN.1 saying it is implicit, but that possibility concerns me.
It would be a real shame if any of those fixes got undone.

Comment 4

[David Cooper]

Attachment: Microsoft Outlook 2007 email encrypted using both RSA and ECDH. Recipients identified via IssuerAndSerialNumber.

Comment 5

[David Cooper]

Attachment: Microsoft Outlook 2010 email encrypted using both RSA and ECDH. Recipients identified via SubjectKeyIdentifier.

Comment 6

[David Cooper]

Attachment: Thunderbird email encrypted using ECDH. User keying material present.

This message was created using Thunderbird with the patch from this bug along with the patches from bug #591640, bug #676108, and bug #676118 applied.  The encoding in this message is similar to that created by Outlook 2007 (attachment 551096 [details]), except that the optional UserKeyingMaterial field is included.

In case it helps in the review of this patch, I have attached three email messages.  All three of the emails use the originatorKey choice of OriginatorIdentifierOrKey.  I do not know of any examples of the other two choices of OriginatorIdentifierOrKey being used (the other two options would only be used in static-static DH, static-static ECDH, or MQV).  Of course, all three messages also include a recipient encrypted key.

For the NSSCMSRecipientInfo, the messages include examples of both key transport (for RSA) and key agreement (for ECDH), but not KEKRecipientInfo.

The message that I created using a patched version of Thunderbird includes UserKeyingMaterial, whereas the messages created using Outlook do not.  I have verified that Outlook can decrypt the messages created using Thunderbird with the ukm field and that Thunderbird can decrypt the messages created using Outlook without the ukm field.  (These templates also work if I modify NSS to not include a ukm in the messages that it creates.)

The message created by Outlook 2010 uses the NSSCMSRecipientKeyIdentifierTemplate, but only the subjectKeyIdentifier field is present.  Also, I have only decoded messages that identify recipients via subjectKeyIdentifier.  All of the messages that I have encoded have identified recipients via IssuerAndSerialNumber.

Comment 7

[David Cooper]

(In reply to Nelson Bolyard (seldom reads bugmail) from comment #3)
> NSS has one encoder and two decoders for ASN.1 BERT/DER.
> The combination of OPTIONAL and INLINE in the same template is supported only
> by one decoder, not the encoder, and then only for string-ish primitive
> types,
> not constructed types.  The usual and preferred solution to this is to make
> optional items NOT be inline, but rather be POINTER.  That works with all 
> our encoders and decoders.  Please don't create templates that cannot be 
> encoded by our encoder.

Okay.  I'll admit that I don't fully understand all of the options in these templates.  In this case (NSSCMSRecipientKeyIdentifierTemplate) I encountered a problem trying to decrypt a message that was encrypted using Outlook 2010 (such as in attachment 551098 [details]) in which the recipient was identified by subjectKeyIdentifier.  I noticed that the corresponding template was incorrect and made a change that allowed me to correctly parse the message.  (As noted, when creating encrypted messages NSS always identifies recipients using IssuerAndSerialNumber so I only testing the decoding side of this template.) So, what should the template for NSSCMSRecipientKeyIdentifierTemplate be?

> There are several different situations in ASN.1 where an item is encoded
> with 
> EXPLICIT encoding, even though the ASN.1 for it says it is implicit.  For all
> such situations, our encoders and deciders require that the template say 
> EXPLICIT, regardless of the ASN.1 abstract notation says.  In the past, there
> have been many errors (now corrected) where the template did not say EXPLICIT
> in those cases.  All such cases known have been fixed.

I did find a couple of instances in which the SEC_ASN1_EXPLICIT tag was included even though doing so wouldn't seem to align with the ASN.1.  However, the only instances that I saw seemed different than in these cases.  For example, they both used SEC_ASN1_SUB(SEC_AnyTemplate) even though the underlying ASN.1 was not ANY.  My guess is that the templates that I made changes to in this patch had simply never been used before now and so errors in them went undetected.

> This patch takes several templates that are presently EXPLICIT, and removes
> the EXPLICIT from them.  Without more study than I have time for now, I am 
> not sure if any of these are cases where EXPLICIT encoding is required 
> despite the ASN.1 saying it is implicit, but that possibility concerns me.
> It would be a real shame if any of those fixes got undone.

My patch removes SEC_ASN1_EXPLICIT from four places in two templates.  As noted in comment 6, I have created and parsed messages that include the originatorKey choice of OriginatorIdentifierOrKey and the kari choice of RecipientInfo, and so I know that encoding/decoding did not work correctly for these items until I changed the templates.  The two untested cases seem sufficiently similar that I would be surprised if SEC_ASN1_EXPLICIT was appropriate for them.

Comment 8

[Kaspar]

(In reply to David Cooper from comment #7)
> In this case (NSSCMSRecipientKeyIdentifierTemplate) I
> encountered a problem trying to decrypt a message that was encrypted using
> Outlook 2010 (such as in attachment 551098 [details]) in which the recipient
> was identified by subjectKeyIdentifier.  I noticed that the corresponding
> template was incorrect and made a change that allowed me to correctly parse
> the message.

I can't really comment on the changes proposed by your patch, but from what I remember when working on bug 559243, I don't think that the NSSCMSRecipientKeyIdentifierTemplate is relevant for decrypting this sort of Outlook 2010 messages - only the NSSCMSRecipientIdentifierTemplate had to be fixed at the time.

(Note well: I'm not saying that the changes you're suggesting for NSSCMSRecipientKeyIdentifierTemplate are unnecessary or even incorrect, I'm simply pointing out that they are not needed for the typical "decrypt an Outlook-2010-generated message" case.)

Comment 9

[Kaspar]

(In reply to Kaspar Brand from comment #8)
> I can't really comment on the changes proposed by your patch, but from what
> I remember when working on bug 559243, I don't think that the
> NSSCMSRecipientKeyIdentifierTemplate is relevant for decrypting this sort of
> Outlook 2010 messages - only the NSSCMSRecipientIdentifierTemplate had to be
> fixed at the time.

Ok, meanwhile I realized that the message in attachment 551098 [details] has a KeyAgreeRecipientInfo (kari) part, and this one uses the RecipientKeyIdentifier type to identify a recipient's encrypted key, so it's very well possible that this template needs to be adjusted. Please ignore my earlier statement.

Comment 10

[David Cooper]

Attachment: Patch to correct Templates for key agreement in smime/cmsasn1.c

(In reply to Nelson Bolyard (seldom reads bugmail) from comment #3)
> NSS has one encoder and two decoders for ASN.1 BERT/DER.
> The combination of OPTIONAL and INLINE in the same template is supported only
> by one decoder, not the encoder, and then only for string-ish primitive
> types,
> not constructed types.  The usual and preferred solution to this is to make
> optional items NOT be inline, but rather be POINTER.  That works with all 
> our encoders and decoders.  Please don't create templates that cannot be 
> encoded by our encoder.

Okay, I modified NSSCMSRecipientKeyIdentifierTemplate so that it doensn't use SEC_ASN1_INLINE.  I also verified that I could encode and decode messages using all elements of the template.  In the process of making these changes, I looked more closely at the ASN.1 for RecipientKeyIdentifier and noticed that there were more problems with the template.  The current template treats RecipientKeyIdentifier as if it were a SEQUENCE of three OCTET STRINGs.  However, the actual ASN.1 is

   RecipientKeyIdentifier ::= SEQUENCE {
     subjectKeyIdentifier SubjectKeyIdentifier,
     date GeneralizedTime OPTIONAL,
     other OtherKeyAttribute OPTIONAL }

   OtherKeyAttribute ::= SEQUENCE {
     keyAttrId OBJECT IDENTIFIER,
     keyAttr ANY DEFINED BY keyAttrId OPTIONAL }

So, I modified the template elements for date and other so that they match the ASN.1.  I also noticed that OtherKeyAttribute is also used by KEKIdentifier, which is defined as:

   KEKIdentifier ::= SEQUENCE {
     keyIdentifier OCTET STRING,
     date GeneralizedTime OPTIONAL,
     other OtherKeyAttribute OPTIONAL }

The template for KEKIdentifier had similar problems and so I fixed that one as well.  Again, I also verified that I could both encode and decode messages using all of the elements of these templates that I changed.

> There are several different situations in ASN.1 where an item is encoded
> with 
> EXPLICIT encoding, even though the ASN.1 for it says it is implicit.  For all
> such situations, our encoders and deciders require that the template say 
> EXPLICIT, regardless of the ASN.1 abstract notation says.  In the past, there
> have been many errors (now corrected) where the template did not say EXPLICIT
> in those cases.  All such cases known have been fixed.
>
> This patch takes several templates that are presently EXPLICIT, and removes
> the EXPLICIT from them.  Without more study than I have time for now, I am 
> not sure if any of these are cases where EXPLICIT encoding is required 
> despite the ASN.1 saying it is implicit, but that possibility concerns me.
> It would be a real shame if any of those fixes got undone.

I have now also encoded and decoded messages using the subjectKeyIdentifier choice of OriginatorIdentifierOrKey and the kekri choice of RecipientInfo and verified that encoding works correctly for these items with SEC_ASN1_EXPLICIT removed.  I did, however, discover one more encoding problem during these tests.  The subjectKeyIdentifier choice of OriginatorIdentifierOrKey was incorrectly marked as SEC_ASN1_CONSTRUCTED, which was resulting in the wrong tag being assign, so I removed that as well.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 10, 
Thanks, David, for doing all this work on NSS.

Comment 12

[David Cooper]

Attachment: Patch to correct Templates for key agreement in smime/cmsasn.c

I didn't make any changes to the patch.  I just updated it so that it works with the current (i.e., HEAD) code in CVS.

Comment 13

[David Cooper]

Attachment: Patch to correct Templates for key agreement in smime/cmsasn.c

This is a patch to correct some errors in the Templates for CMS -- particularly the parts used for key agreement. It was originally submitted several years ago, but review of it stopped for some reason. I have not made any changes to the patch, I just updated it so that it can be applied to the current NSS code.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 9090787 [details] [diff] [review]
Patch to correct Templates for key agreement in smime/cmsasn.c

I'm not a reviewer for this code.

Comment 15

[Kai Engert [:KaiE:]]

This isn't easy to review and be certain about the results. How could we be certain that it doesn't introduce any regressions? We have a set of CMS test messages and Thunderbird tests them. If the tests still pass, can we be certain it doesn't introduce regressions?

I think we need automated tests cases for the class of CMS messages that you are fixing. The tests should fail with the existing code, and work with your new code. How could we get those? Could you describe the messages we need to produce? Someone will have to extend the tests in nss/tests/smime/

Comment 16

[David Cooper]

Attachment: ECDH S/MIME tests

(In reply to Kai Engert (:kaie:) from comment #15)

This isn't easy to review and be certain about the results. How could we be certain that it doesn't introduce any regressions? We have a set of CMS test messages and Thunderbird tests them. If the tests still pass, can we be certain it doesn't introduce regressions?

Certainly it should be carefully reviewed before it is accepted. However, I do not believe it is an overly large concern for two reasons:

• The changes in the templates are only related to the parts of the ASN.1 used with key agreement, which isn't yet fully supported. So, my guess is that the parts of the templates the patch changes haven't been used.

• For most of the changes, it is very easy to see by looking at the ASN.1 in RFC 5652 that the current templates are wrong.

I think we need automated tests cases for the class of CMS messages that you are fixing. The tests should fail with the existing code, and work with your new code. How could we get those? Could you describe the messages we need to produce? Someone will have to extend the tests in nss/tests/smime/

This patch is a start. It adds one new set of tests in which a message is encrypted for a recipient with an elliptic curve key, the resulting file is decrypted, and then the result is compared against the original plaintext.

This patch also modifies the test involving sending an encrypted message to multiple recipients so that one of the recipients has an elliptic curve key.

I see that the nss/tests directory includes a few uses of OpenSSL. Perhaps some more tests could be added that involve (1) encrypting a message using OpenSSL and then decrypting the result using cmsutil and (2) encrypting a message using cmsutil and then decrypting the result using OpenSSL.

(Note that the ECDH tests in this patch will only work if the patch to fix bug #676118 is also applied.)

Comment 17

[David Cooper]

Comment on attachment 9091128 [details] [diff] [review]
ECDH S/MIME tests

I just submitted a patch to bug #676118 that obsoletes Attachment #9093918 [details] [diff]. The patch that I submitted to bug #676118 includes the contents of Attachment #9093918 [details] [diff], but also includes several tests involving using cmsutil to decrypt messages that were encrypted with ECDH using OpenSSL.

These tests can be helpful in testing out the patch for this bug. Using the current NSS with the OpenSSL-created test messages results in the following error message:
> cmsutil: failed to decode message.
> cmsutil: problem decoding: SEC_ERROR_BAD_DER: security library: improperly formatted DER-encoded message.

cmsutil fails before getting to the part addressed by bug #676118, since the parsing error occurs first.

Comment 18

[J.C. Jones [:jcj] (he/they)]

Just to set expectations here - I have basically no background in these ASN.1 updates, and thus these reviews are not straightforward to me. Friday I'm tagging NSS_3_47_BETA1 with a release the following week, so I expect to start looking at these in earnest for NSS 3.48.

Comment 19

[David Cooper]

(In reply to J.C. Jones [:jcj] (he/him) from comment #18)

Just to set expectations here - I have basically no background in these ASN.1 updates, and thus these reviews are not straightforward to me. Friday I'm tagging NSS_3_47_BETA1 with a release the following week, so I expect to start looking at these in earnest for NSS 3.48.

That's okay. I understand that everyone wants to be certain that this patch doesn't accidentally break any existing code in the process of fixing the templates for key agreement algorithms. From my point of view, I think it would be great if this bug and bug #676118 could be addressed in time for the fixes to appear in Thunderbird 76, which I am guessing will be released around August 2020. I don't know when the fixes would to need to make it into NSS for that to happen.

If there is anything I can do to help with the review of these patches, please let me know.

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 21

[Kai Engert [:KaiE:]]

Attachment: WIP: Bug 676100 - Correct Templates for key agreement in smime/cmsasn.c

Comment 22

[Kai Engert [:KaiE:]]

Comment on attachment 9090787 [details] [diff] [review]
Patch to correct Templates for key agreement in smime/cmsasn.c

Replaced by https://phabricator.services.mozilla.com/D204654

Comment 23

[Kai Engert [:KaiE:]]

Comment on attachment 9091128 [details] [diff] [review]
ECDH S/MIME tests

Merged into https://phabricator.services.mozilla.com/D164332
so we have only one patch for all tests.

Comment 24

[Kai Engert [:KaiE:]]

r=kaie on David's template patch plus code formatting fixes. I'll land it.

Comment 25

[Kai Engert [:KaiE:]]

https://hg.mozilla.org/projects/nss/rev/c4a0fa94c133aaa97f267e74c83f4d5b7298f4ee

milestone 3.100

Comment 26

[Robert Relyea]

    NSSCMSKeyAgreeRecipientInfo is a public structure included in the union of NSSCMSRecipientInfo. The change was to an unused structure, so in general, it was fine, but it was to the largest existing struct in the union, which changes the sized of NSSCMSRecipientInfo, and moves around the later points in a non-opaque public structure which is used in public APIs. This breaks NSS ABI.

The solution is to create a fixed structure that is identical to the old NSSCMSKeyAgreeRecipientInfo and include it as a dummy max value in the Union. Note the comment above NSSCMSRecipientInfo.

Comment 27

[Robert Relyea]

OK, that solution won't work because the new structure is bigger.

We need a solution soon. This is holding up us shipping NSS in RHEL (sigh). Kai is this referenced at all in Thunderbird?

Comment 28

[Robert Relyea]

OK, it turns out reverting the cmst.h change that caused the breakage is actually pretty straightforward. since the item is allocated out of the arena pool, we can just tell the Decoder that it refers to pointer. In the encrypt case, we change the allocate of data to allocating a full SECITEM.

Comment 29

[Robert Relyea]

since I needed a patch for RHEL anyway, I'll take this bug and attach the patch once it passes our tests.

Comment 30

[Kai Engert [:KaiE:]]

I'm investigating.
The change in size was reported in the NSS automatic checking of ABI changes, and was accepted in
https://hg.mozilla.org/projects/nss/rev/97e4f6f65af5f632f79ece17c32f3ee962191e85

The report had mentioned a change of size in the sub-type, but that was missed.

Yes Thunderbird uses this type for S/MIME, and the Thunderbird version 128 that uses the modified NSS type has already been publicly released.

This is unfortunate.

Can we investigate whether any known applications would indeed be affected by this change?

It would only affect applications that are using NSS libsmime, and which read or modify the respective members of the structure.

In Thunderbird, I can find only one place where type NSSCMSRecipientInfo is used.
A pointer to that type is obtained as the result of calling NSS_CMSRecipientInfo_Create,
and the result is not inspected, not modified,
but only passed on to NSS_CMSEnvelopedData_AddRecipient.

In other words, for Thunderbird, we'd be certain that only new code that knows about the new size and layout, is used to process the data.

I think this also means, if Red Hat requires that the change gets reverted, and a different implementation is used, then it wouldn't be a problem for Thunderbird.

Comment 31

[Kai Engert [:KaiE:]]

Likewise, the outer type NSSCMSEnvelopedData is also used in an opaque way, only.
An instance is obtained by calling NSS_CMSEnvelopedData_Create,
and the result pointer is passed on to calls to NSS_CMSContentInfo_SetContent_EnvelopedData, NSS_CMSEnvelopedData_GetContentInfo, NSS_CMSEnvelopedData_AddRecipient, but never inspected or modified by the application code.

Comment 32

[Kai Engert [:KaiE:]]

Fedora has already shipped the ABI change, because it's at version 3.103
Have you heard of anyone complaining about bugs related to the NSS S/MIME library during the past months, when NSS was upgraded to version 3.101 ?
If your suggested solution is to revert the ABI change and use a different implementation, this would also be done on Fedora, I assume.

Comment 33

[Kai Engert [:KaiE:]]

Bob, did you already decide what to do?

Comment 34

[Robert Relyea]

Yes, I think we should revert the ABI. We don't have ABI guarrentees in S/MIME, but we do in RHEL. I now have a patch that reverts the ABI change while preservering the core change from this bug. (basically a small change to the template a couple of changes to the callers that use the structure). I'll attach it here soon. We were in a middle of a RHEL release, so I needed to get that our and fixed first.

Comment 35

[Robert Relyea]

We currently have the structures in a public header, and it's returned by some public functions. I've verified that Thunderbird and Firefox don't reference the structures, but we should look a moving those private so we can be more free with changes to them. If we do that with ifdefs we can drop them from new RHEL and evenutally drop the public versions completely.

The main issue with the change was the structure increased in size, and the structure that was changed was the controlling size in a union to a structure that is returned by public functions. There were fields after the union, and the structure itself is often used in arrays, so the change had some potential issues if anyone actually used it.

Comment 36

[Robert Relyea]

Attachment: nss-3.101-fix-cms-abi-break.patch

Heres the patch for preview I'll create a phabricator version once I get some breathing room.