233586 - EDIPartyName should be "CONSTRUCTED" (probably X400AddressName also)

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[yassir.elley]

User-Agent:       
Build Identifier: Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.2.1) Gecko/20030711

CERT_DecodeGeneralName fails if generalName is EDIPartyName.
Specifically, the first call to sec_asn1d_confirm_identifier() fails
because the template has set expect_tag_modifiers to 0x80, while the
found_tag_modifiers are 0xa0.

Within the context of GeneralName, EDIPartyName is an implicitly tagged
SEQUENCE. ASN.1 dictates that implicitly tagged types are primitive or
constructed, depending on the underlying type. Since the underlying type
is SEQUENCE in this case, the implictly tagged type should be constructed.

I think CERT_EDIPartyNameTemplate needs to have SEC_ASN1_CONSTRUCTED included
in its "kind", so that it expects 0xa0 and not 0x80 for tag_modifiers.
[and it continues to expect 0x05 as the tag_number, of course]

I have not tried the X400Address, but I suspect it has exactly the same
issue, since it is also an implicitly tagged SEQUENCE and its template
does not have SEC_ASN1_CONSTRUCTED as part of its "kind".

I took a look at:
http://www.opensource.apple.com/darwinsource/10.3/SecurityNssAsn1-11/nssDER/Source/nameTemplates.cpp
and it has both EDIPartyName and X400Address marked as "CONSTRUCTED".

I also looked at the test certs that can be gotten to from
http://csrc.nist.gov/pki/testing/x509paths.html
and I couldn't see any obvious test cases which used EDIPartyName
or X400Address.




Reproducible: Always
Steps to Reproduce:
1.Create a certificate with a subjectAlternativeName of type "ediPartyName"
2.Read in the cert file and decode the DER bytes into a CERTCertificate
3.Call CERT_GetCertificateNames on the CERTCertificate

Actual Results:  
CERT_GetCertificateNames returns NULL.

Expected Results:  
CERT_GetCertificateNames should have returned a CERTGeneralName with
type = ediPartyName, and with the name.other field filled
with a SECItem representing the EDIPartyName.

Comment 1

[yassir.elley]

Attachment: test certificate used to reproduce bug

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

I'm taking this bug, but leaving it unconfirmed for now.

I need to know EXACTLY what version or versions of NSS you used to test.
And if you've gotten different answers from different versions of NSS,
please advise which versions gave which results.

NSS has two decoders, the general ASN.1 decoder, that can decode both DER
and BER, and the "QuickDERDecoder" which can decode only DER.   
The quick DER decoder was added after NSS 3.4, and slowly many calls to 
the general ASN1 decoder have been switched over to the QuickDER decoder.

There is a subtle difference in the meaning of SEC_ASN1_CONSTRUCTED in 
templates between the two decoders, IIRC.  This is because in BER, any of 
the simple types may be constructed or not, while DER does not allow them 
to be constructed.  So, to make a single template that works with both 
decoders, there is (or is intended to be) some variation in interpretation 
of that named bit when it appears in the template.   

IIRC, the BER/DER decoder should ALLOW the decoding of contrusted tags, 
even when the template doesn't include this bit.  That is, IIRC the BER/DER
decoder treats a non-zero value of this bit in the template as REQUIRING that
bit to be non-zero in the tag, but a zero value does not require a zero.

BTW, thanks for the test cert.  I've not seen ANY cert with either an X.400
or EDI general name in it before.  When I am SURE that this test cert is
correctly encoded I'll confirm this bug.  But I won't get to that right away.

Comment 3

[yassir.elley]

>(In reply to comment #2)
> I'm taking this bug, but leaving it unconfirmed for now.

Thanks for looking into this. There is no particular urgency.

> 
> I need to know EXACTLY what version or versions of NSS you used to test.
> And if you've gotten different answers from different versions of NSS,
> please advise which versions gave which results.

I used NSS3.9, NSPR4.4.1, DBM1.6 when I tested this. I have not used
any other versions of NSS.

> 
> NSS has two decoders, the general ASN.1 decoder, that can decode both DER
> and BER, and the "QuickDERDecoder" which can decode only DER.   
> The quick DER decoder was added after NSS 3.4, and slowly many calls to 
> the general ASN1 decoder have been switched over to the QuickDER decoder.
> 

My test was using the general ASN.1 decoder. i.e. CERT_DecodeGeneralName uses
SEC_ASN1DecodeItem (not SEC_QuickDERDecodeItem).

> There is a subtle difference in the meaning of SEC_ASN1_CONSTRUCTED in 
> templates between the two decoders, IIRC.  This is because in BER, any of 
> the simple types may be constructed or not, while DER does not allow them 
> to be constructed.  So, to make a single template that works with both 
> decoders, there is (or is intended to be) some variation in interpretation 
> of that named bit when it appears in the template.

I agree that, in BER, any of the simple string types may be constructed or not.
Of course, simple non-string types such as INTEGER or OBJECT IDENTIFIER must be
primitive. I believe this is irrelevant for this bug because we are dealing
with an implicitly tagged SEQUENCE of EDIPartyName (which MUST be constructed
under both BER and DER)


A.2 Implicitly Tagged Module, 1988 Syntax

GeneralName ::= CHOICE {
     otherName                       [0]     AnotherName,
     rfc822Name                      [1]     IA5String,
     dNSName                         [2]     IA5String,
     x400Address                     [3]     ORAddress,
     directoryName                   [4]     Name,
     ediPartyName                    [5]     EDIPartyName,
     uniformResourceIdentifier       [6]     IA5String,
     iPAddress                       [7]     OCTET STRING,
     registeredID                    [8]     OBJECT IDENTIFIER }

EDIPartyName ::= SEQUENCE {
     nameAssigner            [0]     DirectoryString OPTIONAL,
     partyName               [1]     DirectoryString }

> 
> IIRC, the BER/DER decoder should ALLOW the decoding of contrusted tags, 
> even when the template doesn't include this bit.  That is, IIRC the BER/DER
> decoder treats a non-zero value of this bit in the template as REQUIRING that
> bit to be non-zero in the tag, but a zero value does not require a zero.

I am not seeing these semantics in my test. CERT_EDIPartyNameTemplate has
a zero value for the constructed bit, and is requiring a zero value in the
DER encoding.

> 
> BTW, thanks for the test cert.  I've not seen ANY cert with either an X.400
> or EDI general name in it before.  When I am SURE that this test cert is
> correctly encoded I'll confirm this bug.  But I won't get to that right away.

Take your time.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Thanks for the added info.  

Lowering severity from blocker to normal.
This bug doesn't meet the criteria for blocker, AFAIK, since it doesn't 
block continued development or building or testing of NSS or mozilla.  
I will look at this bug for NSS 3.10

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Confirming bug, and taking it.  Patch forthcoming.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Accepting bug.  

Question for the submittor and other CC recipients of this bug:
I interpret X.690, section 8.14.2 as saying that explicitly tagged 
identifier octets must always be encoded as constructed.  Do you concur?

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Attachment: proposed patch v1

This patch sets the constructed flag for ediPartyName, x400Address, and 
OtherName choices.  According to
http://www.itu.int/ITU-T/asn1/database/itu-t/x/x411/1999/MTSAbstractService.html#MTSAbstractService.ORAddress

ORAddress is a sequence.  

Julien, please review.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

I'm nominating this bug for NSS 3.9.1.  

Comment 9

[yassir.elley]

I agree with your interpretation of X.690, section 8.14.2.
Thanks for looking into this!

Comment 10

[Wan-Teh Chang]

Comment on attachment 144641 [details] [diff] [review]
proposed patch v1

Running the NISCC tests inside Purify takes
a long time, so we want to start that after
all the NSS 3.9.1 fixes have been checked in.

Julien, please review this patch and check
with Yassir as to whether Sun needs this fix
in NSS 3.9.1.

Comment 11

[Julien Pierre]

Comment on attachment 144641 [details] [diff] [review]
proposed patch v1

Patch looks good and works with the attached cert.
We should add at least one cert with this GeneralName EDIParty encoding to our
test suite to exercise this.

Comment 12

[yassir.elley]

Attachment: test certificate with an X400Address GeneralName

in case it's useful, here's a cert with an X400Address GeneralName

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 144641 [details] [diff] [review]
proposed patch v1

Thanks for the review Julien.  I will checkin on trunk.  
Wan-teh, please review for branch.

Comment 14

[Robert Relyea]

Comment on attachment 144641 [details] [diff] [review]
proposed patch v1

r=relyea

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

patch checked in on trunk and 3.9 branch.

Comment 16

[Wan-Teh Chang]

Comment on attachment 144641 [details] [diff] [review]
proposed patch v1

Requesting drivers' approval for Mozilla 1.7 final.

Mozilla doesn't need this fix.	However, we are
certifying the NSS 3.9.1 release, and it is our
desire that Mozilla 1.7 final uses a certified
NSS release.

This fix has a very low risk.

Comment 17

[chris hofmann]

Comment on attachment 144641 [details] [diff] [review]
proposed patch v1

a=chofmann for 1.7

Comment 18

[Wan-Teh Chang]

Comment on attachment 144641 [details] [diff] [review]
proposed patch v1

Patch checked into NSS_CLIENT_TAG (Mozilla 1.7 final).