390710 - CERTNameConstraintsTemplate is incorrect

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Alexei Volkov]

Attachment: cert to reproduce the bug

SEC_QuickDERDecoder fails to decode attached cert(in particular it's name constraints extension), but SEC_ASN1Decoder successfully completes the operation.

This problem responsible for 10% libpkix test failures.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

I'm kind of amazed by this.  How do these certs pass in mozilla?
NSS uses QuickDer whenever it can (when the thing being decoded MUST be 
DER and cannot be BER).  I'm pretty sure it decodes all certs with QuickDer.
So, how does this succeed in Mozilla?

What error code does QuickDer return for this cert?

Comment 2

[Julien Pierre]

Nelson, I don't know if these certs pass in Mozilla.
The extensions get decoded separately from the cert as you may know. It's the individual extension decode that fails, not the whole cert. I'm not sure why it would yet. I know we used to have everything from NIST PKITS and PDVAL pass on the NSS_LIBPKIX_BRANCH . It may be that some of the changes that were made to QuickDER after 3.10 broke those certs, or changes to one of the templates. I'm looking the issue now.

Comment 3

[Julien Pierre]

From certutil -L, tip :

            Name: Certificate Name Constraints
            Critical: True
            Error: Parsing extension: security library: improperly formatted DER-encoded message.
            Raw: Sequence {
                [0]: {
                    Sequence {
                        [4]: {
                            Sequence {
                                Set {
                                    Sequence {
                                        X520 Country Name
                                        "US"
                                    }
                                }
                                Set {
                                    Sequence {
                                        X520 Organization Name
                                        "Test Certificates"
                                    }
                                }
                            }
                        }
                    }
                }
            }

NSS_LIBPKIX_BRANCH :

            Name: Certificate Name Constraints
            Critical: True
            Permitted Subtree:
                Directory Name: "O=Test Certificates,C=US"

Comment 4

[Julien Pierre]

Good news, it's also all good in NSS_3_11_BRANCH, so it's broken on the trunk only. So Mozilla shouldn't be affected, except the firefox alpha which is the only one with a 3.12 snapshot at this point. Still digging.

Comment 5

[Julien Pierre]

The QuickDER code hasn't changed between 3.11 branch and the trunk, so it is either something in the templates, or the calling code passing a different block to decode.

Comment 6

[Julien Pierre]

What happened is that 3.12 now uses the SEC_QuickDERDecodeItem decoder for this extension, whereas 3.11 and earlier were using SEC_ASN1DecodeItem . Neither the templates nor the QuickDER code changed. The QuickDER decoder is stricter when it comes to ASN.1 syntax than the older decoder. In this case, CERTNameConstraintsTemplate has two optional subcomponents that are defined in the templates as SEC_ASN1_OPTIONAL | SEC_ASN1_CONTEXT_SPECIFIC | 0 and SEC_ASN1_OPTIONAL | SEC_ASN1_CONTEXT_SPECIFIC | 1 . That means the correct tag values are 0x80 and 0x81 . The extension from the cert has a tag of 0xa0, which doesn't match . The solution is to fix the template and add SEC_ASN1_CONSTRUCTED to the subcomponents. Patch forthcoming.

Comment 7

[Julien Pierre]

Attachment: Fix template and delete dead ones

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 275049 [details] [diff] [review]
Fix template and delete dead ones

r=nelson for trunk
This bug shows that our cert testing in all.sh is inadequate.  
I'm a little surprised that our PKITS testing didn't catch this. 
Or have we not run PKITS testing on the trunk in ages?

Comment 9

[Julien Pierre]

Nelson,

I'm pretty sure it's the later. Thanks for the review.

cvs commit: Examining .
Checking in genname.c;
/cvsroot/mozilla/security/nss/lib/certdb/genname.c,v  <--  genname.c
new revision: 1.33; previous revision: 1.32
done

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Well, then we need to run the PKITS test again, ASAP, (after reviewing my
patches, of course :)