289823 - CERT_DecodeUserNotice fails

Status: NEW

(NSS :: Libraries, defect, P2)

Description

[Martin v. Löwis]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; de-DE; rv:1.7.6) Gecko/20050401 Firefox/1.0.2 (Debian package 1.0.2-2)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; de-DE; rv:1.7.6) Gecko/20050401 Firefox/1.0.2 (Debian package 1.0.2-2)

For the attached certificate, decoding the UserNotice of the third
PolicyInformation fails (this is the only UserNotice in the certificate);
CERT_DecodeUserNotice returns NULL.

This certificate is generated by OpenSSL, which can decode it fine, so I believe
the certificate is correctly encoded.

Reproducible: Always

Steps to Reproduce:
1.Apply patch #259031, to easily display the certificate
2.Load the certificate into the browser
3.set a gdb breakpoint on ProcessUserNotice
4.display the certificate, and step through the code.

Actual Results:  
CERT_DecodeUserNotice returns NULL

Expected Results:  
It should return a properly-decoded certificate.

Comment 1

[Martin v. Löwis]

Attachment: Demo certificate

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 180319 [details]
Demo certificate

Changing MIME type because it's all B64 encoded.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

BTW Martin, your test cert contains one of the most common errors made by
OpenSSL users when creating certs.  It is in the Authority Key Id extension.
That extension should contain EITHER the KeyID, OR the (Issuer and serial
number), but not both.  KeyID is preferred.  See RFC 3280.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Changed OS & HW since this concerns platform-independent code.

Comment 5

[Martin v. Löwis]

The certificate was primarily created to test bug #259031, trying to integrate
as much information as technically possible into a single certificate. So it is
no surprise that it contains inconsistent information.

However, out of curiosity: where precisely does RFC 3280 say that you should not
include both KeyID and (issuer,serialno)? The only slightly related remark I can
see is in 4.2.1.1, "The identification MAY be based on either the key identifier
(the subject key identifier in the issuer's certificate) or on the issuer name
and serial number". However, this is a requirement on the recipient of the
certificate, not on the issuer: the recipient has the choice of using either to
identify the key. (Actually, in the strict sense of RFC 2119, the "MAY" means
that the identification is truly optional, i.e. the recipient might identify the
key not at all, or through entirely different means. I doubt RFC 3280 is
intended in this way, though).

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Martin wrote:
> this is a requirement on the recipient of the certificate, not on the issuer: 
> the recipient has the choice of using either to identify the key.

Hmm, Hadn't thought of it that way.  I'll have to reread that section.
Thanks for challenging my understanding. :)

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

The problem is that CERT_NoticeReferenceTemplate in polcyxtn.c requires the
organization string to be an IA5String.  This template is used only in 
CERT_DecodeUserNotice.  

I am surprised that this code doesn't use the technique used elsewhere in 
NSS for handling strings of type DirectoryString and DisplayText.  That 
technique decodes the CHOICE using SEC_ASN1_ANY, (which actually just stores
the string in its encoded form, rather than actually decoding it), then calls
CERT_DecodeAVAValue to decode the string and convert it to UTF8.  

I think the fix for this is to change CERT_NoticeReferenceTemplate to decode
the organization as a SEC_ASN1_ANY, and then change CERT_DecodeUserNotice to
call CERT_DecodeAVAValue to decode/convert it, and finally to arrange for 
the final decoded/converted value to be found in 
usernotice->noticeReference.organization where the caller expects to find it.

Care must be taken to avoid leaking the decoded/converted data returned by
CERT_DecodeAVAValue.  Both the returned SECItem and the buffer to which 
they point have been malloced (not from an arenapool), but the contents need
to be copied into the arenapool found in usernotice->arena, and then freed. 

Perhaps we should create a new function like CERT_DecodeAVAValue, but which
takes an arena argument and doesn't call SECITEM_DupItem nor destroy the
arena, as CERT_DecodeAVAValue does, and call that function from
CERT_DecodeUserNotice.  That would save unnecessary allocation and copying.

Comment 8

[Christophe Ravel]

The target milestone is already released. Resetting target milestone.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, and this bug has priority 'P2'.
:beurdouche, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 10

[Marco Castelluccio [:marco]]

We have modified the bot to only consider P1 as high priority, so I'm cancelling the needinfo here.