All users were logged out of Bugzilla on October 13th, 2018

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

ASSIGNED
Assigned to

Status

ASSIGNED
7 years ago
6 years ago

People

(Reporter: dcooper16, Assigned: dcooper16)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [SMIME-ECC])

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 550216 [details] [diff] [review]
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.)
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)

Updated

7 years ago
Attachment #550216 - Flags: review?(kaie) → review?(wtc)

Comment 2

7 years ago
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.
(Assignee)

Comment 4

7 years ago
Created attachment 551096 [details]
Microsoft Outlook 2007 email encrypted using both RSA and ECDH.  Recipients identified via IssuerAndSerialNumber.
(Assignee)

Comment 5

7 years ago
Created attachment 551098 [details]
Microsoft Outlook 2010 email encrypted using both RSA and ECDH.  Recipients identified via SubjectKeyIdentifier.
(Assignee)

Comment 6

7 years ago
Created attachment 551104 [details]
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.
(Assignee)

Comment 7

7 years ago
(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

7 years ago
(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

7 years ago
(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]
(Assignee)

Comment 10

7 years ago
Created attachment 552646 [details] [diff] [review]
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.
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.
(Assignee)

Comment 12

7 years ago
Created attachment 604172 [details] [diff] [review]
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.
Attachment #552646 - Attachment is obsolete: true
Attachment #552646 - Flags: review?(wtc)
Attachment #604172 - Flags: review?(wtc)
(Assignee)

Updated

7 years ago
Blocks: 676118
Component: Security: S/MIME → Security: S/MIME
Product: Core → MailNews Core
Version: Trunk → unspecified
Component: Security: S/MIME → Libraries
Product: MailNews Core → NSS
You need to log in before you can comment on or make changes to this bug.