335021 - incorrect smime_encryptionkeypref_template leads to QuickDER decoding failure

Status: VERIFIED FIXED

(NSS :: Libraries, defect, P1)

Description

[Kai Engert (:KaiE:)]

Julien, Bob, Nelson,

in bug 178897 you changed NSS_SMIMEUtil_GetCertFromEncryptionKeyPreference to no longer use SEC_ASN1DecodeItem, but use SEC_QuickDERDecodeItem.

I found that SEC_QuickDERDecodeItem is unable to decode the DER that we produce in NSS_SMIMEUtil_CreateSMIMEEncKeyPrefs using smime_encryptionkeypref_template.

We currently have a regression in Mozilla Mail, S/Mime.
Although receipient encryption certs are available in the local cert database, we don't find them.

This regression is caused by this decoding failure.

In my tests I changed NSS_SMIMEUtil_GetCertFromEncryptionKeyPreference to use SEC_ASN1DecodeItem, and things work again.
To immediately fix the regression, I would like to propose that we make that change in NSS trunk and 3.11 branch to fix the regression.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: Hotfix v1

Comment 2

[Julien Pierre]

Interesting how the variable is named DERekp . Are we actually encoding these encryption key preferences as BER ? Or is there another reason why the quick decoder fails ?

Comment 3

[Robert Relyea]

Comment on attachment 219381 [details] [diff] [review]
Hotfix v1

r+ for a hot fix.

Kaie, could you post a copy of a sample ekp that failes to decode. That will tell us if the problem is BER coding or not.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

The old decoder decodes both BER and DER.  
The new "quick DER" decoder decodes only DER. 
There are two questions to be asked here:
a) does the spec for the relevant item being decoded here require it to be DER
   encoded? or is BER permitted?
   If BER is permitted, then reverting to the dual BER/DER decoder is the 
   right fix.  If DER is required, then I think it's very likely that the 
   problem is that the object has been BER encoded.
b) Is the item in question actually BER encoded?

We need an attachment here showing the encoded data that the decoder doesn't 
like.  Kai, can you produce that?

If our encoder is encoding the wrong thing, then I'd oppose reintroducing 
the wrong decoder as a mere workaround.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Here's something to check.  When we're encoding the SMIME encryption prefs,
do we actually re-encode the certificate from its components?  
If so, that's wrong, and that's probably the problem.  
The CRMF/CMMF library has that same problem.  

If you see the smime prefs template pointing to the certificate template,
that's the problem.  When encoding/decoding certs inside of a large template
the cert should always be encoded/decoded as a SEC_ASN1_ANY type.  When 
encoding, the already encoded cert should be provided.  Similarly when 
decoding the encoded cert should be kept intact, and then, if necessary
be separately decoded.  

Comment 6

[Kai Engert (:KaiE:)]

Attachment: smimeenckeyprefs.secitem

00000000  a0 76 30 62 31 0b 30 09  06 03 55 04 06 13 02 5a  |.v0b1.0...U....Z|
00000010  41 31 25 30 23 06 03 55  04 0a 13 1c 54 68 61 77  |A1%0#..U....Thaw|
00000020  74 65 20 43 6f 6e 73 75  6c 74 69 6e 67 20 28 50  |te Consulting (P|
00000030  74 79 29 20 4c 74 64 2e  31 2c 30 2a 06 03 55 04  |ty) Ltd.1,0*..U.|
00000040  03 13 23 54 68 61 77 74  65 20 50 65 72 73 6f 6e  |..#Thawte Person|
00000050  61 6c 20 46 72 65 65 6d  61 69 6c 20 49 73 73 75  |al Freemail Issu|
00000060  69 6e 67 20 43 41 02 10  17 73 38 d4 e5 4b 09 e4  |ing CA...s8..K..|
00000070  3a ec f6 a2 f3 51 6b 57                           |:....QkW|
00000078

Comment 7

[Robert Relyea]

OK, I looks like the problem isn't the data, but maybe the template?
The data is pretty simpe DER, but the template has a choice with a selector.
I think that's why the Quick decoder failed.

data decoded with pp:

jordan.sfbay.redhat.com(76) pp -t none -i /tmp/x
pp: don't know how to print out 'none' files
File contains: [0]: {
    Sequence {
        Set {
            Sequence {
                X520 Country Name
                "ZA"
            }
        }
        Set {
            Sequence {
                X520 Organization Name
                "Thawte Consulting (Pty) Ltd."
            }
        }
        Set {
            Sequence {
                X520 Common Name
                "Thawte Personal Freemail Issuing CA"
            }
        }
    }
    17:73:38:d4:e5:4b:09:e4:3a:ec:f6:a2:f3:51:6b:57
}

Comment 8

[Kai Engert (:KaiE:)]

(In reply to comment #5)
> When we're encoding the SMIME encryption prefs,
> do we actually re-encode the certificate from its components?  
> ...
> If you see the smime prefs template pointing to the certificate template,
> that's the problem.

If I understand the template correclty, I don't see a cert template nested.

It seems we do not nest the cert, but only include issuer, serial number, recipient key id, subject key id.

static const SEC_ASN1Template smime_encryptionkeypref_template[] = {
    { SEC_ASN1_CHOICE,
	  offsetof(NSSSMIMEEncryptionKeyPreference,selector), NULL,
	  sizeof(NSSSMIMEEncryptionKeyPreference) },
    { SEC_ASN1_POINTER | SEC_ASN1_CONTEXT_SPECIFIC | SEC_ASN1_XTRN | 0,
	  offsetof(NSSSMIMEEncryptionKeyPreference,id.issuerAndSN),
	  SEC_ASN1_SUB(CERT_IssuerAndSNTemplate),
	  NSSSMIMEEncryptionKeyPref_IssuerSN },
    { SEC_ASN1_POINTER | SEC_ASN1_CONTEXT_SPECIFIC | 1,
	  offsetof(NSSSMIMEEncryptionKeyPreference,id.recipientKeyID),
	  NSSCMSRecipientKeyIdentifierTemplate,
	  NSSSMIMEEncryptionKeyPref_IssuerSN },
    { SEC_ASN1_POINTER | SEC_ASN1_CONTEXT_SPECIFIC | SEC_ASN1_XTRN | 2,
	  offsetof(NSSSMIMEEncryptionKeyPreference,id.subjectKeyID),
	  SEC_ASN1_SUB(SEC_OctetStringTemplate),
	  NSSSMIMEEncryptionKeyPref_SubjectKeyID },
    { 0, }
};

Comment 9

[Kai Engert (:KaiE:)]

Nelson, you said, the quick decoder is supposed to decode DER.
We now know the data that it fails to decode is DER.
I conclude there is a bug in the quick DER decoder?

Are you ok with taking the hotfix on the NSS 3.11 branch? If yes, could you please give me a second review?

It would be good to have it on the client snapshot we're going to produce shortly.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Adding QuickDer author to CC list.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 219381 [details] [diff] [review]
Hotfix v1

How about if we do this "hot fix" on the 3.11 branch only, and not the trunk?
I'm OK with that.  r=nelson for the branch.

Comment 12

[Kai Engert (:KaiE:)]

(In reply to comment #11)
> (From update of attachment 219381 [details] [diff] [review] [edit])
> How about if we do this "hot fix" on the 3.11 branch only, and not the trunk?
> I'm OK with that.  r=nelson for the branch.

Sounds good to me, thanks!
Hotfix checked in to NSS_3_11_BRANCH.

Checking in smimeutil.c;
/cvsroot/mozilla/security/nss/lib/smime/smimeutil.c,v  <--  smimeutil.c
new revision: 1.16.28.1; previous revision: 1.16
done

Comment 13

[Kai Engert (:KaiE:)]

Nelson, you probably want this on the branch only, because a real fix should be produced for the trunk of NSS.

This makes sense, if the real fix can be produced short term.

However, we should make sure the real fix should be produced before a 3.12 release, to ensure we won't regress again.

Landing the hotfix on the trunk (as a temporary fix) would ensure we can't regression again accidentially.

The Mozilla release management defines blocker bugs, that block a release.

If NSS has such a mechanism, and you choose to not checkin the hotfix to the trunk, this bug should be made a blocker bug for the 3.12 release.

Comment 14

[Wan-Teh Chang]

I agree that the hot fix should be checked in on the trunk
to eliminate the risk of a regression in NSS 3.12.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

I want to give Julien an easy test bed for debugging.
Also, to keep this bug in view.  (e.g. out of sight, out of mind)

Comment 16

[Julien Pierre]

Here are some tips to debug the issue using QuickDER :

1) build NSS in debug mode. QuickDER has some additional checks on templates that are #ifdef DEBUG

2) Try adding the SEC_ASN1_DEBUG_BREAK flag to entries in the templates you are suspecting. QuickDER will assert right at the point where it reaches this component. Start putting this flag at the bottom of the templates, and them move it back up until there is an actual assert hit. Then it should be simple to narrow which component quickder failed to decode. Certainly easier than to step through the decoder.

Comment 17

[Julien Pierre]

I created a test program and checked it against the data Kai attached.
I don't see a bug in the QuickDER decoder. The DER data simply doesn't match any of the choices in the template.

The first tag in the data is 0xa0.
The choices defined in the template are 
SEC_ASN1_CONTEXT_SPECIFIC | 0
SEC_ASN1_CONTEXT_SPECIFIC | 1
SEC_ASN1_CONTEXT_SPECIFIC | 2
which translate to 0x80, 0x81, and 0x82 .

SEC_ASN1_POINTER and SEC_ASN1_XTRN are modifiers that have no effect on the expected tag. POINTER is only about memory-management, and XTRN is for invoking a function to retrieve the sub-template from different shared library.

To make the template match the data, one would have to add SEC_ASN1_CONSTRUCTED . I'm wondering why SEC_ASN1DecodeItem allows this to decode without it; and why SEC_ASN1EncodeItem encoded the data as constructed as well.

Comment 18

[Julien Pierre]

Attachment: fix template

This template works with both decoders using the data provided by Kai. But I didn't test the encoding step. Kai, please make sure this change doesn't break encoding of the EKP also in the client

Comment 19

[Robert Relyea]

Hmmm... The secasn1d.c and derdec.c functions explicitly ignore the SEC_ASN1_CONSTRUCTED tag. I wonder if we should do the same in QuickDERDecode. We have a lot of templates that have CONTEXT_SPECIFIC without the explicit SEC_ASN1_CONSTRUCTED tag.

bob

Comment 20

[Robert Relyea]

Comment on attachment 220058 [details] [diff] [review]
fix template

r+ for NSS trunk only.
Well look at the damage there first;).

Comment 21

[Julien Pierre]

Thanks for the review, Bob. I checked this in to the tip :

Checking in smimeutil.c;
/cvsroot/mozilla/security/nss/lib/smime/smimeutil.c,v  <--  smimeutil.c
new revision: 1.17; previous revision: 1.16
done

Re: comment 19,

I think it is a valid case to implicitly tag with SEC_ASN1_CONTEXT_SPECIFIC but without the constructed bit, for example if the subtemplate is for a primitive type rather than a sequence.

Perhaps the legacy decoder figures out the expected value of SEC_ASN1_CONSTRCUTED by looking at the subtemplate ? That would seem odd to me. IMO, the tag value info should be completely in the parent for this retagging case.

So, I wouldn't be eager to change the behavior of the QuickDER decoder until we can understand why the legacy decoder assumes constructed for this case.

Comment 22

[Julien Pierre]

Has anyone verified this fix yet ?

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

For context specific tags, whether the tag should be constructed or not
should be determined by the subtemplate, not by the template of the 
context-specific tag itself, IINM.  Otherwise, how do you implement a 
context specific ANY ?  

So, I don't think adding the SEC_ASN1_CONSTRUCTED flag to context specific 
tags is the right answer.  

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Please ignore my comment 23.  I needed more coffee when I wrote that.  :-/
I actually DO think that template change is the right one.  

In reply to comment 19, I finally remembered why secasn1d.c ignores the 
CONSTRUCTED bit.  It's a BER and DER decoder.  In BER, ANY of the byte 
oriented STRING types (e.g. OCTET_STRING, UTF8_STRING, PRINTABLE_STRING,
T61_STIRNG, IA5_STRING, etc., etc.) may be constructed or primative, 
anywhere they occur (except inside constructed components of the same type).
So, a BER decoder cannot insist on the presence or absence of the CONSTRUCTED
bit in any of those tag types.  Consequently, our BER/DER deocder doesn't 
try to enforce the correctness of the CONSTRUCTED bit until it knows that 
the tag type requires it to be of one fixed value, even for BER.

Comment 25

[Julien Pierre]

I checked the fix in to NSS_3_11_BRANCH also.

Checking in smimeutil.c;
/cvsroot/mozilla/security/nss/lib/smime/smimeutil.c,v  <--  smimeutil.c
new revision: 1.16.28.2; previous revision: 1.16.28.1
done

Kai, feel free to reopen if this doesn't solve your problem.

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

The NSS_3_11_BRANCH now contains BOTH fixes, the template fix, and the older
"hot fix" (which changed the code to use the older BER/DER decoder).

I think the results of the tests of the "fixed templates" are not valid
or conclusive unless and until we back out the "hot fix", and revert to 
using the QuickDER decoder again.

So, I'm reopening this bug until that "hot fix" is backed out and the 
"template fix" is verified without the "hot fix".

Comment 27

[Julien Pierre]

I backed out the hot fix on the NSS_3_11_BRANCH :

Checking in smimeutil.c;
/cvsroot/mozilla/security/nss/lib/smime/smimeutil.c,v  <--  smimeutil.c
new revision: 1.16.28.3; previous revision: 1.16.28.2
done

Comment 28

[Nelson Bolyard (seldom reads bugmail)]

Julien, did you test that the email clients will be able to send encrytped
email with that hot fix backed out?  (I suspect they won't!)

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

The bad non-DER template caused NSS to encode a great many SMIME encyrption
key prefs in a non-DER encoding.  Correcting the template will cause the 
encoder to start encoding them properly, but won't enable the QuickDER 
decoder to be able to decode the old ones that were improperly encoded.

So, I suspect we must leave this decoder using the older BER/DER decoder until
such time as there are no longer any improperly encoded prefs left to decode.

Comment 30

[Julien Pierre]

Nelson,

I believe SEC_ASN1EncodeItem always encodes in DER . You have to use streaming mode to get BER with the encoder, and that requires using different APIs. The question is whether the encoded structure contained the constructed bit or not, not whether it is BER or DER.

Kai ran into the problem where SEC_QuickDERDecodeItem saw a structure that had the constructed bit encoded in it, but was unable to decode it, because its interpretation of the template didn't match - it didn't expect the constructed bit in the encoded component. The template was fixed with the addition of SEC_ASN1_CONSTRUCTED, so SEC_QuickDERDecodeItem will now expect that bit and decode the structure properly.

I don't believe the addition of SEC_ASN1_CONSTRUCTED to the template will change anything to the encoding process - which was already adding the constructed bit. So, I think the code we have now is correct.

I would still like Kai to verify what the behavior is with now this fix in Mozilla mail, both with new profiles and with old ones, rather than speculate as to whether we need to change anything else at this point.

Comment 31

[Kai Engert (:KaiE:)]

Comment on attachment 219381 [details] [diff] [review]
Hotfix v1

I would have prefered to keep this hotfix on the branch. I didn't yet have the time to play with your other patch to verify it is correct.

Comment 32

[Kai Engert (:KaiE:)]

I verified Julien's new patch fixes the bug.
(with the hot fix backed out)
Thanks!

Comment 33

[Daniel Veditz [:dveditz]]

Comment on attachment 220058 [details] [diff] [review]
fix template

approved for 1.8.0 branch, a=dveditz for drivers

Comment 34

[Kai Engert (:KaiE:)]

checked in to mozilla 1.8.0 branch
Checking in lib/smime/smimeutil.c;
/cvsroot/mozilla/security/nss/lib/smime/smimeutil.c,v  <--  smimeutil.c
new revision: 1.16.30.1; previous revision: 1.16
done

I verified the fix works on that branch.