290263 - CERT_CreateCertificateRequest creates an invalid array of attributes

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

An ASN.1 encoded CERTRequest contains an array of SEQUENCES (structures)
known as attributes.  Each Attribute contains an OID and a SET OF (array of)
other ASN.1 encoded values.  

NSS's ASN.1 encoder and decoder have "templates" that describe a certificate
request, and there is a corresponding structure that contains the unencoded
form.  The encoder encodes the data in the structure using the template, 
and the decode decodes the data into the structure.  

The template used for encoding and decoding Cert request attributes requires
the data for each attribute to be contained in a struct named CERTAttribute.
That struct contains a SECItem for the attribute's OID, and a pointer to 
an array of pointers to secitems that contain the other parts of the attribute.

The encoder expects to receive an array of pointers to CERTAttribute structs.
The decoder produces as output an array of pointers to CertAttribute structs
that it has allocated.  
If the encoder is given an array of pointers to some other type, it will not
properly encode the values, and may crash.  After the decoder has produced
an array of pointers to CERTAttribute structs as output, any code that
interprets that array as pointers to some other type will get incorrect 
results and may crash.  

A SECItem cannot contain all the info needed to encode an attribute.
It lacks the pointer to the array of other secitems needed by the encoder.
SO, it has NEVER been correct to give the encoder an array of pointers to 
SECItems, and expect it to correctly encode them as attributes.  

But that is exactly what function CERT_CreateCertificateRequest does when
it is called with a non-NULL value for the last argument.  It has always
done this.  It was always wrong.  

This bug went unnoticed for YEARS.  There are two reasons for that.

a) no NSS code ever tried to pass a non-NULL value for that last argument 
to CERT_CreateCertificate request prior to NSS 3.10.  Even now 3.10 does not.
and

b) the struct CERTCertificateRequest contained an erroneous declaration 
of the attributes member.  It was erroneously declared as a SECItem **,
when it always should have been a CERTAttribute **.  

If it had been properly declared from the start, attempts to pass arrays
of pointers to the wrong types woudl have generated warnings.  But since
it was declared as SECItem **, code was written that built and assigned
arrays of the wrong type to the attribute member of the CERTCertificateRequest,
and the compiler did not warn about these errors, making the erroneous code
appear to be correct.

In NSS 3.10, ther type of the attributes member of struct CERTCertificateRequest
was finally corrected to be CERTAttribute **.  Erroneous code that actually 
used SECItem ** started to cause new warnings.  That led to the discovery
that function CERT_CreateCertificateRequest generates arrays of the wrong 
type.  

CERT_CreateCertificateRequest has been wrong for YEARS and no-one has noticed
because all users of that function pass NULL for the last argument.  On that
basis, I believe fixing this bug is probably P3, and certainly no more than P2.

Rather than trying to perfect it now on the eve of the 3.10 release, I 
propose to simply have it return an error if it is passed a non-NULL last
argument.  This will avoid calling the ASN.1 encoder with an invalid list
of pointers, and thus will avoid any potential crash.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch to simply eliminate erroneous code

This patch will do as a short term solution.  It simply sets an error code and
returns NULL if the last argument is non-NULL.	Wan-Teh, if you believe this
bug is worthy of P1 for 3.10, please review this patch.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: larger patch, redefines behavior with non-null attrbutes

This patch reimplements CERT_CreateCertificateRequest and redefines the
content of the SECItems that may be passed to it.  Instead of being
individual attributes already pre-encoded, it expects them now to be
cert attributeValues pre-encoded.  It combines them as the values of one
attribute, which is a cert extensions request attribute.  

Other possible alternative redefinitions of the SECItems could be 
defined.  This is one idea.  I do NOT propose to check this into 3.10.
We need more time to think about this before makign such a change, IMO.

Comment 3

[Wan-Teh Chang]

Comment on attachment 180671 [details] [diff] [review]
patch to simply eliminate erroneous code

>+    if (!certreq) {
>+	PORT_FreeArena(arena, PR_FALSE);
>+	return NULL;
>+    }
>+    /* below here it is safe to goto loser */
> 
>-    if (certreq != NULL) {
>+    {
...
>-    } else {
>-	PORT_FreeArena(arena, PR_FALSE);
>     }
> 
>     return certreq;

This change doesn't change the logic at all.  It
makes the code easier to read, but leaves behind
an unnessary pair of curly braces.

If you really want an early return on certreq
allocation failure, please remove the pair of
curly braces and move all the code in between
to the left by one indent level.

We need a comment to explain why we fail if
attributes is not NULL.

I don't understand the code well enough to know if
we need to fix this in 3.10.  I trust your decision,
Nelson.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Reassigning to Neil for NSS 3.11.

Comment 5

[Neil Williams]

Attachment: Nelson's patch with some comment changes

Patch 180675 is a reasonable solution to this problem. Since no one ever passed
and attribute list to CERT_CreateCertificateRequest before (because it didn't
work) this is more of an academic excercise in correctness. Certutil builds
CSRs with attributes but it calls CERT_StartCertificateRequestAttributes to do
the job. Note that this fix will only build CSRs of type
SEC_OID_PKCS9_EXTENSION_REQUEST.

Comment 6

[Neil Williams]

Nelson, I tested your patch and it works fine. The only change I made was to
finish the comment at the head of CERT_CreateCertificateRequest.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 198743 [details] [diff] [review]
Nelson's patch with some comment changes

Since I wrote a large part of this patch, I don't think I should
review it.  I'm asking Bob to review it.  However,
There is one typo in the new comment that should be fixed.

>+ * single attribute of the cert request. In this implentation there is at most

s/implenta/implementa/

Comment 8

[Neil Williams]

Attachment: spelling correction in the commnet

Bob, this is the same patch that Nelson asked you for a review of. I just fixed
the mis-spelling.

Comment 9

[Robert Relyea]

Comment on attachment 198743 [details] [diff] [review]
Nelson's patch with some comment changes

remove review request on obsolete patch

Comment 10

[Robert Relyea]

Comment on attachment 199114 [details] [diff] [review]
spelling correction in the commnet

r+ because this patch does do what it is claims to do. I do have a problem with
the overall plan.

I would like to see us 'replace' this interface with one that really takes
attributes. I doubt that PKCS9_EXTENTION will be the last attribute type we
need to support.

What I would like to see is the following:

1) a new function with a signature (CERTNAME*, CERTSubjectPublickeyInfo *,
CERTAttributes **).
2) CERT_CreateCertificateRequest() is implemented by creating a
SEC_OID_PKCS9_EXTENSION_REQUEST and calling the new function.

bob

Comment 11

[Neil Williams]

/cvsroot/mozilla/security/nss/lib/certhigh/certreq.c,v  <--  certreq.c
new revision: 1.6; previous revision: 1.5

Thanks Bob. Do you want to open a new bug addressing your recommendation? I'd
like to resolve this one.

Comment 12

[Neil Williams]

Changing resolution because original problem has been addressed. Bob's
suggestion in his review comment can be the subject of a new bug (enhancement?).