379625 - Accept SMIME preferences even when they contain NULL parameters

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

Attachment: signed KMail test email #1

This can be reproduced with the mail/news client in Thunderbird and in
Seamonkey, trunk and recent releases.

I received a signed email from a KMail user.  His message had an attribute
of type "PKCS #9 S/MIME Capabilities", from which we create an S/MIME profile.
It said that his client prefers 3DES and AES.  RC2-40 was not even listed
among the ciphers his client supports.

Then I sent him an encrypted reply.  My reply was encrypted with RC2-40.
That's wrong.

Something failed, but it's difficult to say exactly what.
Did we fail to generate an "S/MIME profile" from the received signed message?
Did we not find the S/MIME profile when we went to encrypt the reply?
Was the failure in PSM? or in NSS? or ?  

I will attach two such signed emails I received, with which the problem
can be reproduced. 

The signed message contained these signed attributes:

Using pp, I looked at the signature.  Here's part of what I found:

             Authenticated Attributes:
                    Attribute (1):
                        Type: PKCS #9 Content Type
                        Value (1): PKCS #7 Data
                    Attribute (2):
                        Type: PKCS #9 Signing Time
                        Value (1): Mon Apr 30 16:39:49 2007
                    Attribute (3):
                        Type: PKCS #9 Message Digest
                        Value (1):
                            64:23:d9:3e:71:7b:5b:11:66:6b:4c:0d:bf:da:a9:03:
                            16:8d:58:9c
                    Attribute (4):
                        Type: PKCS #9 S/MIME Capabilities
                        Value (1) (encoded): Sequence {
                            Sequence {
                                AES-128-CBC
                                NULL
                            }
                            Sequence {
                                DES-EDE3-CBC
                                NULL
                            }
                        }

And my reply looked like this:

      SEQUENCE {
        OBJECT IDENTIFIER data (1 2 840 113549 1 7 1) (PKCS #7)
        SEQUENCE {
          OBJECT IDENTIFIER rc2CBC (1 2 840 113549 3 2)
          SEQUENCE {
            INTEGER 160
            OCTET STRING 3B EE E9 74 FF FE 87 90
            }
          } ...

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: signed KMail test email #2

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Hmmm.  I was comparing the encoding of the SMIME preferences as done by
KMail and as done by NSS, and I found one difference that may be a clue.

When KMail encodes the preference for the cipher, if the cipher has a 
fixed key size, kmail encodes it with an explicit DER NULL parameter.
NSS encodes it with no parameter.  

Compare the decoded output of KMail's SMIME prefs (above in comment 0)
with the following decoded prefs from NSS.

                        Value (1) (encoded): Sequence {
                            Sequence {
                                DES-EDE3-CBC
                            }                  <- note: no NULL
                            Sequence {
                                RC2-CBC
                                128 (0x80)
                            }
                            Sequence {
                                RC2-CBC
                                64 (0x40)
                            }
                            Sequence {
                                DES-CBC
                            }                  <- note: no NULL
                            Sequence {
                                RC2-CBC
                                40 (0x28)
                            }
                        }

Here's how dumpasn1 shows them.  First, here is how NSS's SMIME prefs look
as decoded by dumpasn1.

       SEQUENCE {
         SEQUENCE {
           OBJECT IDENTIFIER des-EDE3-CBC (1 2 840 113549 3 7)
           }
         SEQUENCE {
           OBJECT IDENTIFIER rc2CBC (1 2 840 113549 3 2)
           INTEGER 128
           }
         SEQUENCE {
           OBJECT IDENTIFIER rc2CBC (1 2 840 113549 3 2)
           INTEGER 64
           }
         SEQUENCE {
           OBJECT IDENTIFIER desCBC (1 3 14 3 2 7)
           }
         SEQUENCE {
           OBJECT IDENTIFIER rc2CBC (1 2 840 113549 3 2)
           INTEGER 40
           }
         }

And here is the dumpasn1 version of the kmail prefs.

    SEQUENCE {
      SEQUENCE {
        OBJECT IDENTIFIER '2 16 840 1 101 3 4 1 2'
        NULL                                               <--  notice
        }
      SEQUENCE {
        OBJECT IDENTIFIER des-EDE3-CBC (1 2 840 113549 3 7)
        NULL                                               <--  notice
        }
      }

Perhaps NSS doesn't know what to do with those NULL parameters.
That's where I would look first.

Comment 3

[Nicholas Sushkin]

Thanks for filing the bug. Hopefully it will be resolved soon. I'll be happy to participate in testing. I'll also try to alert gnupg-users mailing list to see if anyone can help.

Comment 4

[Nicholas Sushkin]

(In reply to comment #2)
> When KMail encodes the preference for the cipher, if the cipher has a 
> fixed key size, kmail encodes it with an explicit DER NULL parameter.
> NSS encodes it with no parameter.  

According to S/MIME 3.1, it MUST NOT use NULLs. Quoting http://www.apps.ietf.org/rfc/rfc3851.html#sec-2.5.2 ,
"In the event that there are no differentiating 
parameters for a particular OID, the parameters MUST be omitted, and MUST NOT be encoded as NULL."

I posted on gnupg-devel to that effect.

> Perhaps NSS doesn't know what to do with those NULL parameters.
> That's where I would look first.

Incorrect encoding shouldn't cause Thunderbird to choose weak encryption. IMHO, if Thunderbird fails to parse SMimeCapabilities, it should use Rule 2 Unknown Capabilities and pick tripleDES. See http://www.apps.ietf.org/rfc/rfc3851.html#sec-2.7.1.2

Perhaps the problem is that the default encoding in smime-utils.c is RC2 instead of tripleDES.

Perhaps line
http://mxr.mozilla.org/security/source/security/nss/lib/smime/smimeutil.c#391
should be changed from

chosen_cipher = SMIME_RC2_CBC_40;           /* the default, LCD */

to

chosen_cipher = SMIME_DES_EDE3_168;         /* the default, LCD */

After all, Section 2.7 of S/MIME 3.1 RFC states that all implementations
MUST support tripleDES, but only SHOULD support RC2/40. Quoting http://www.apps.ietf.org/rfc/rfc3851.html#sec-2.7,

"Sending and receiving agents MUST support encryption and decryption with DES EDE3 CBC, hereinafter called "tripleDES" [CMSALG]. Receiving agents SHOULD support encryption and decryption using the RC2 [CMSALG] or a compatible algorithm at a key size of 40 bits, hereinafter called "RC2/40". Sending and receiving agents SHOULD support encryption and decryption with AES [CMSAES] at a key size of 128, 192, and 256 bits."

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Nicholas,

Thanks for all the citations.  Presently, AFAIK, Mozilla's mail clients 
only claim conformance with S/MIME 3.0 (RFC 2630, 2632, 2633), not 3.1. 
RFC 2633

RFC 2633 says the same thing about "the parameters MUST be omitted, and 
MUST NOT be encoded as NULL." as RFC 3851, and NSS is apparently choosing
to enforce this, though I see no particular reason why it should.

We can either choose to be strict, requiring other implementations to 
conform to the above paragraph (as NSS now does, apparently), or we can 
choose to follow the old Internet maxim "Be generous in what you accept,
and strict in what you send."  I would prefer that we do the latter. 

In the absence of a valid set of knownn capabilities for the recipient,
RFC 2633 offers two rules:

2.7.1.2 Rule 2: Unknown Capabilities, Known Use of Encryption
  which applies only when we have previously received an encrypted message
  from our intended recipient.  (doesn't apply in this KMail case)m, 

2.7.1.3 Rule 3: Unknown Capabilities, Unknown Version of S/MIME
  which applies only when we don't know what version of S/MIME the recipient
  supports.  

There is no rule in RFC 2633 (AFAICT) that covers the case of 
Unknown capabilities, unknown use of encryption, known version.
It appears to me that NSS is choosing to follow Rule 3's recommendation 
for that case.

In summary, there are two separate issues here:
a) being strict or lenient about capabilities with explicit NULL parameters,
and 
b) the choice of weak or strong crypto in the absence of known capabilities.

In 2001, when the current NSS libSMIME was written, the "lingua Franca" 
of S/MIME readers was RC2-40.  Now, 6 years later, I think it is time for
us to revisit that, and choose to use 3DES by default.  But that is the 
subject of another bug.  

I'm narrowing the scope of this bug to the first of those two issues:
being strict or lenient about capabilities with explicit NULL parameters,

I COULD mark this bug invalid, since KMail is not conforming to the RFC.
Instead, I will turn it into an enhancement request, asking that NSS be
lenient and accept explicit NULL parameters.  But I must tell you frankly
that this enhancement request will be low priority for the NSS team.
This would be a good opportunity for someone to submit a patch.

Someone still needs to determine if this is NSS or PSM that is being strict.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

This strictness is definitely in NSS, in function 
nss_SMIME_FindCipherForSMIMECap at 
http://mxr.mozilla.org/security/source/security/nss/lib/smime/smimeutil.c#336

I'm gonna write a patch.

Comment 7

[Nicholas Sushkin]

(In reply to comment #5)
> b) the choice of weak or strong crypto in the absence of known capabilities.
> 
> In 2001, when the current NSS libSMIME was written, the "lingua Franca" 
> of S/MIME readers was RC2-40.  Now, 6 years later, I think it is time for
> us to revisit that, and choose to use 3DES by default.  But that is the 
> subject of another bug.  

Is it Bug 84213 "S/MIME should not support weak crypto"? If not, would you submit a new bug or tell me which component it should be submitted against?

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Allow DER NULLs for ALGIDs that require no parameeter

Please read the previous comments for this bug to know what this patch
is trying to do, then review the patch.  Thanks.

I have tested this with the email that Nicholas Sushkin sent to me
using KMail, which had the erroneous NULLs in it.  I sent him an 
encrypted reply, and THIS time, the reply was 3DES.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Let's try to squeeze this into NSS 3.11.7

Comment 10

[Wan-Teh Chang]

Comment on attachment 263804 [details] [diff] [review]
Allow DER NULLs for ALGIDs that require no parameeter

r=wtc.

You should take the opportunity to update the XXX comment.  SECITEM_CompareItem
allows NULL pointers as arguments now.  It is still insufficient here though
because it doesn't consider DER NULL equal to empty.

>+		break;  /* DER NULL == NULL, bingo */

You may want to say "DER NULL == empty, bingo" instead.

Does the last comment lose its alignment with the comment before it?  It's
hard to tell.

Comment 11

[Wan-Teh Chang]

Why don't we need to handle the "NULL (empty) == DER NULL" case?

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh asked: 
> Why don't we need to handle the "NULL (empty) == DER NULL" case?

I'm not sure I understand the question.  I *think* you're observing the 
asymmetry of the new code, that it only allows DER NULLs in the SMIME
capabilities records to match NULL pointers in the table, and not vice versa.
Assuming that's the basis of the question, then the answer is that the table
doesn't contain any DER NULLs, and never will if it's properly constructed.

Another way to have implemented this fix would have been to have added new 
rows in the table, rows that contain DER NULLs.  But since the table is used
for both encoding and decoding, I didn't want to risk that we might start
sending out the DER NULLs, which are disallowed by the relevant RFCs cited
above.

Thanks for your review!

Comment 13

[Robert Relyea]

Comment on attachment 263804 [details] [diff] [review]
Allow DER NULLs for ALGIDs that require no parameeter

r+ This patch looks good to me!

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Bug 379625 – Accept SMIME preferences even when they contain NULL parameters.
r=rrelyea,wtc

security/nss/lib/smime/smimeutil.c; new: 1.19;      previous: 1.18
security/nss/lib/smime/smimeutil.c; new: 1.16.28.5; previous: 1.16.28.4

Comment 15

[Wan-Teh Chang]

Comment on attachment 263804 [details] [diff] [review]
Allow DER NULLs for ALGIDs that require no parameeter

My question in comment 11 meant:

Why don't we need to handle the following case?

    (!cap->parameters.data || !cap->parameters.len) &&
    (smime_cipher_map[i].parms->len     == 2  &&
     smime_cipher_map[i].parms->data[0] == SEC_ASN1_NULL &&
     smime_cipher_map[i].parms->data[1] == 0)

So I believe you correctly understood my question.

One more question: is the following possible?

    (smime_cipher_map[i].parms != NULL) &&
    (!smime_cipher_map[i].parms->data || !smime_cipher_map[i].parms->len)

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, I would consider either of the following conditions to be errors 
in the smime_cipher_map table.  Perhaps we should add assertions that they do 
not exist, since if they did, they would be "internal" errors in this code.

(smime_cipher_map[i].parms != NULL && 
 smime_cipher_map[i].parms->len     == 2  &&
 smime_cipher_map[i].parms->data[0] == SEC_ASN1_NULL &&
 smime_cipher_map[i].parms->data[1] == 0)   /* miscoded table */

(smime_cipher_map[i].parms != NULL &&
  (!smime_cipher_map[i].parms->data || !smime_cipher_map[i].parms->len)) 

Comment 17

[Wan-Teh Chang]

Thank you for the answer, Nelson.