Closed Bug 676100 Opened 13 years ago Closed 4 months ago

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

Categories

(NSS :: Libraries, defect, P4)

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dcooper16, Assigned: dcooper16)

References

Details

(Whiteboard: [SMIME-ECC])

Attachments

(4 files, 5 obsolete files)

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.)
Assignee: nobody → dcooper16
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
Attachment #550216 - Flags: review?(kaie)
Attachment #550216 - Flags: review?(kaie) → review?(wtc)
Hi Nalin, Bob Relyea has proposed, this patch should be tested thoroughly (once reviewed). CC'ing you FYI.
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.
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.
(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.
(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.)
(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.
Whiteboard: [SMIME-ECC]
(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.
Attachment #550216 - Attachment is obsolete: true
Attachment #550216 - Flags: review?(wtc)
Attachment #552646 - Flags: review?(wtc)
In reply to comment 10, 
Thanks, David, for doing all this work on NSS.
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.
Attachment #552646 - Attachment is obsolete: true
Attachment #552646 - Flags: review?(wtc)
Attachment #604172 - Flags: review?(wtc)
Blocks: 676118
Product: Core → MailNews Core
Version: Trunk → unspecified
Component: Security: S/MIME → Libraries
Product: MailNews Core → NSS

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.

Attachment #604172 - Attachment is obsolete: true
Attachment #604172 - Flags: review?(wtc)
Attachment #9090787 - Flags: review?(wkocher)
Attachment #9090787 - Flags: review?(kaie)
Attachment #9090787 - Flags: review?(jjones)
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.
Attachment #9090787 - Flags: review?(wkocher)

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/

QA Contact: jjones
Attached patch ECDH S/MIME tests (obsolete) — Splinter Review

(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.)

Attachment #9091128 - Flags: review?(kaie)
Attachment #9091128 - Flags: review?(jjones)
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.
Attachment #9091128 - Flags: review?(kaie)
Attachment #9091128 - Flags: review?(jjones)

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.

(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.

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

Assignee: dcooper16 → nobody
Status: ASSIGNED → NEW
Severity: normal → S3
Severity: S3 → S4
Status: NEW → UNCONFIRMED
Ever confirmed: false
Priority: -- → P4

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

Attachment #9090787 - Attachment is obsolete: true
Attachment #9090787 - Flags: review?(kaie)
Attachment #9090787 - Flags: review?(jc)

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.

Attachment #9091128 - Attachment is obsolete: true

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

Status: UNCONFIRMED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Assignee: nobody → dcooper16
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: