incorrect smime_encryptionkeypref_template leads to QuickDER decoding failure

VERIFIED FIXED in 3.11.2

Status

NSS
Libraries
P1
normal
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: kaie, Assigned: Julien Pierre)

Tracking

({fixed1.8.0.5, fixed1.8.1})

3.11
3.11.2
fixed1.8.0.5, fixed1.8.1
Dependency tree / graph
Bug Flags:
blocking1.8.1 +
blocking1.8.0.5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

120 bytes, application/octet-stream
Details
1.76 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Robert Relyea
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

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

Comment 1

12 years ago
Created attachment 219381 [details] [diff] [review]
Hotfix v1
Attachment #219381 - Flags: superreview?(nelson)
Attachment #219381 - Flags: review?(rrelyea)
(Reporter)

Updated

12 years ago
Blocks: 333455
(Reporter)

Updated

12 years ago
Blocks: 332571
(Assignee)

Comment 2

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

12 years ago
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.
Attachment #219381 - Flags: review?(rrelyea) → review+
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.
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.  
(Reporter)

Comment 6

12 years ago
Created attachment 219420 [details]
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

12 years ago
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
}
(Reporter)

Comment 8

12 years ago
(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, }
};
(Reporter)

Comment 9

12 years ago
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.
Adding QuickDer author to CC list.
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.
Attachment #219381 - Flags: superreview?(nelson) → superreview+
(Reporter)

Comment 12

12 years ago
(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
(Reporter)

Comment 13

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

Updated

12 years ago
Blocks: 39468

Comment 14

12 years ago
I agree that the hot fix should be checked in on the trunk
to eliminate the risk of a regression in NSS 3.12.
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)
(Assignee)

Comment 16

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

Comment 17

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

Comment 18

12 years ago
Created attachment 220058 [details] [diff] [review]
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
Attachment #220058 - Flags: superreview?(rrelyea)
Attachment #220058 - Flags: review?(kengert)

Comment 19

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

12 years ago
Comment on attachment 220058 [details] [diff] [review]
fix template

r+ for NSS trunk only.
Well look at the damage there first;).
Attachment #220058 - Flags: superreview?(rrelyea) → superreview+
(Assignee)

Comment 21

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

Updated

11 years ago
Blocks: 332867
Priority: -- → P1
Target Milestone: --- → 3.11.2
(Assignee)

Comment 22

11 years ago
Has anyone verified this fix yet ?
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.  
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.
(Assignee)

Comment 25

11 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Summary: SEC_QuickDERDecodeItem unable to decode smime_encryptionkeypref_template → incorrect smime_encryptionkeypref_template leads to QuickDER decoding failure
Attachment #220058 - Flags: review?(kengert) → review+
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".
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Updated

11 years ago
Attachment #219381 - Flags: approval1.8.0.5?
(Assignee)

Comment 27

11 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
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!)
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.
(Assignee)

Comment 30

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

Comment 31

11 years ago
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.
Attachment #219381 - Attachment is obsolete: true
Attachment #219381 - Flags: approval1.8.0.5?
(Reporter)

Updated

11 years ago
Blocks: 340724
(Reporter)

Comment 32

11 years ago
I verified Julien's new patch fixes the bug.
(with the hot fix backed out)
Thanks!
Status: RESOLVED → VERIFIED
Flags: blocking1.8.1+
Flags: blocking1.8.0.5+
(Reporter)

Updated

11 years ago
Attachment #220058 - Flags: approval1.8.0.5?
(Reporter)

Updated

11 years ago
Keywords: fixed1.8.1
Comment on attachment 220058 [details] [diff] [review]
fix template

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #220058 - Flags: approval1.8.0.5? → approval1.8.0.5+
(Reporter)

Comment 34

11 years ago
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.
Keywords: fixed1.8.0.5
You need to log in before you can comment on or make changes to this bug.