284200 - No public way to parse Sequence of Extension on Windows

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

Some ASN.1 documents have "Extensions" (that is, Sequence of Extension) 
in them.  NSS has two differently-named but otherwise identical templates 
for parsing these:

CERT_SequenceOfCertExtensionTemplate    (in certdb.c)
SEC_CERTExtensionsTemplate              (in crl.c)

But neither of these seem to be exported (in nss.def).  
Also, I have not found a public function that decodes a caller-supplied 
DER buffer using either of those templates.  

We have functions that decode those things inside of certs and CRLs, 
but they appear in lots of other places, too.

So, I propose to export one of those templates and/or functions to 
encode and decode with them.  I propose to do this for NSS 3.10.

If you object to that plan, please speak up right away.
If you know of a function that I've missed that decodes with either of
those templates, please let me know.  No point in duplicating it.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

This bug blocks bug 284191, which is wanted for NSS 3.10.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1 - export CERT_SequenceOfCertExtensionTemplate

This small patch exports CERT_SequenceOfCertExtensionTemplate and the 
function that returns the address of that template (for windows).

Reviewers: please scrutinize the addition to nss.def to see if it looks
acceptable.  It's not strict alphabetical order.  It is patterned after
the file section for NSS 3.2, but if you prefer it could be patterned after
the section for NSS 3.9.

Comment 3

[Wan-Teh Chang]

Comment on attachment 175997 [details] [diff] [review]
patch v1 - export CERT_SequenceOfCertExtensionTemplate

>Index: lib/certdb/certt.h
...
> struct CERTCertificateRequestStr {
>     PRArenaPool *arena;
>     SECItem version;
>     CERTName subject;
>     CERTSubjectPublicKeyInfo subjectPublicKeyInfo;
>-    SECItem **attributes;
>+    CERTAttribute **attributes;
> };

What is this change?  It's not clear how this change
is related to the export of CERT_SequenceOfCertExtensionTemplate.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

That change really is part of the fix for bug 284191 and bug 263779.  
It should have been part of the patch for bug 284191, rather than this patch.
I hope to check in the patches for both bugs (this and 284191) together as
a single checkin when both are approved.

As noted in bug 284191, struct CERTCertificateRequestStr is supposed to 
correspond to CERT_CertificateRequestTemplate.  But the struct is wrong,
because it has the wrong type for "attribute".  The template correctly 
decodes (or encodes) the attribute as a CERT_SetOfAttributeTemplate,
that is, as a SET OF Attribute.  But the struct erroneously corresponds
to a SET OF ANY.  

It is clearly the case that no NSS-based code has ever properly constructed
a cert request with an attribute, nor properly used a parse cert request
attribute.  If it had, it would have failed, since a SECItem is quite 
different from a CERTAttribute.

This fix to the struct is binary compatible.  It does not alter the size
of the struct, and makes the struct properly reflect the data structure 
that is actually put into memory by the decoder (or used by the encoder).
It is not API compatible, in the sense that any code that actually expects 
an attribute to be represented by a SECItem (e.g. code such as
x->attribute[0]->data ) will no longer compile.  However, such code was 
always in error, and never could have worked properly.

Comment 5

[Neil Williams]

Comment on attachment 175997 [details] [diff] [review]
patch v1 - export CERT_SequenceOfCertExtensionTemplate

Looks good to me. Had a question about
NSS_Get_CERT_SequenceOfCertExtensionTemplate
but it was answered to my satifaction.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

/cvsroot/mozilla/security/nss/lib/certdb/certt.h,v  <--  certt.h
new revision: 1.29; previous revision: 1.28

/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.72; previous revision: 1.71

/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.143; previous revision: 1.142

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Many thanks for the review and superreviews.