263779 - certutil doesn't place or handle extensions in cert requests

Status: RESOLVED FIXED

(NSS :: Tools, enhancement, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

certutil has command line options that specify information to be added to 
a cert in the form of cert extensions.  The -7 and -8 options, for example,
specify email and DNS names to be included in the subjectAltName extension.
However, certutil's -R command, which generates a cert request, does not 
add the requested extensions into the generated PKCS 10 cert request.  

PKCS 10 certificate requests (ASN.1 CertificationRequestInfo ) contain 
a set of attributes, one of which may be an "extensionRequest" attribute,
as defined in PKCS 9.  This attribute is used to convey a set of extensions
that the requestor wishes to have the CA place into the cert.  

certutil -R should place any requested cert extensions into the PKCS 10
cert request that it generates.  

Likewise, certutil -C, which reads in the request and issues the cert,
should process the attributes in the cert request, and should include
any requested extensions in the issued cert.  certutil -C should probably 
also allow additional extensions to be requested from the command line,
so that the issued extensions are the union of those requested from the
request file and those specified on the command line.  

This is an enhancement request and a bug.  IMO it is a bug that this wasn't
implemented when support for extensions (such as -7 and -8) was added to 
certutil.  So, I think it should get higher priority that the typical 
enhancement request.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

There exist server products that bundle certutil as the tool to 
generate Cert Signing Requests to be sent to real CAs.  The users 
of those products are unable to generate CSRs with the subjectAltNames
due to this bug.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Reassigning ot Neil, since bugzilla won't let him "take" it. :-(

Comment 3

[Wan-Teh Chang]

I just gave Neil all the abilities he needs
in Bugzilla.

Comment 4

[Neil Williams]

Attachment: added new PKCS9 OID preparatory to certutil patch

The fix for this bug involves additions to the OID tables. This change is being
added as a separate patch from the certutil changes which will follow.

Comment 5

[Neil Williams]

Attachment: Correction to OID patch

New OID used same index as a previous patch. CVS update didn't notice?

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 175990 [details] [diff] [review]
Correction to OID patch

This patch is fine.  

I'm surprised that cvs update didn't detect a merge conflict with the previous
patch.	Please mark the previous patch obsolete and remove the review request
from it.  Thanks.

Comment 7

[Neil Williams]

Checking in nss/lib/util/secoid.c;
/cvsroot/mozilla/security/nss/lib/util/secoid.c,v  <--  secoid.c
new revision: 1.30; previous revision: 1.29
done
Checking in nss/lib/util/secoidt.h;
/cvsroot/mozilla/security/nss/lib/util/secoidt.h,v  <--  secoidt.h
new revision: 1.18; previous revision: 1.17
done

Comment 8

[Wan-Teh Chang]

Comment on attachment 175990 [details] [diff] [review]
Correction to OID patch

Neil,

With your patch applied, these comments are
no longer accurate:

>Index: nss/lib/util/secoid.c
>     /* More PKIX OIDs */
>     OD( pkixCAIssuers, SEC_OID_PKIX_CA_ISSUERS,
>         "PKIX CA issuers access method", 
>         CKM_INVALID_MECHANISM, INVALID_CERT_EXTENSION ),
>+    OD( pkcs9ExtensionRequest, SEC_OID_PKCS9_EXTENSION_REQUEST,
>+    	"PKCS #9 Extension Request",
>+        CKM_INVALID_MECHANISM, INVALID_CERT_EXTENSION ),
> };

>Index: nss/lib/util/secoidt.h
>     /* More PKIX OIDs */
>     SEC_OID_PKIX_CA_ISSUERS          = 273,
>+    SEC_OID_PKCS9_EXTENSION_REQUEST  = 274,

Please remove "PKIX" from those two comments.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 175987 [details] [diff] [review]
added new PKCS9 OID preparatory to certutil patch

obsoleteing old patch that was replaced by a newer one.

Comment 10

[Neil Williams]

Attachment: fix processing of extensions for cert requests

This patch fixes the way attributes are processed during cert request creation.
It also addresses the way cert extensions are merged during cert creation.
During cert creation command line options override extension requests in a cert
request.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 176610 [details] [diff] [review]
fix processing of extensions for cert requests

I sent patch feedback to Neil.	r-

Comment 12

[Neil Williams]

Attachment: Incorporates review suggestions.

Comment 13

[Neil Williams]

Attachment: This fixes a review comment I missed in the previous patch.

Please disregard the previous patch.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 176806 [details] [diff] [review]
Incorporates review suggestions.

cancelling review request on this obsolete patch.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 176818 [details] [diff] [review]
This fixes a review comment I missed in the previous patch.

r-

>+SECStatus
>+CERT_FinishCertificateRequestAttributes(CERTCertificateRequest *req)
>+{   SECItem *extlist;
>+    SECOidData *oidrec;
>+    CERTAttribute *attribute;
>+   
>+    if (req->attributes == NULL)
>+        return SECSuccess;
>+    if (req->arena) {

I'm pretty sure that should be "if (!req->arena) {", right?

>+	PORT_SetError(SEC_ERROR_INVALID_ARGS);
>+        return SECFailure;
>+    }
>+
>+    extlist = SEC_ASN1EncodeItem(req->arena, NULL, &req->attributes,
>+                            SEC_ASN1_GET(CERT_SequenceOfCertExtensionTemplate));

I sent more review comments in email.

Comment 16

[Neil Williams]

Attachment: incorpoates latest review comments

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 176932 [details] [diff] [review]
incorpoates latest review comments

r=nelson

Comment 18

[Neil Williams]

Checking in nss/cmd/certutil/certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.89; previous revision: 1.88
done
Checking in nss/lib/certdb/cert.h;
/cvsroot/mozilla/security/nss/lib/certdb/cert.h,v  <--  cert.h
new revision: 1.53; previous revision: 1.52
done
Checking in nss/lib/certdb/certxutl.c;
/cvsroot/mozilla/security/nss/lib/certdb/certxutl.c,v  <--  certxutl.c
new revision: 1.4; previous revision: 1.3
done
Checking in nss/lib/certhigh/certreq.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certreq.c,v  <--  certreq.c
new revision: 1.4; previous revision: 1.3
done
Checking in nss/lib/nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.145; previous revision: 1.144
done

Comment 19

[Robert Relyea]

Comment on attachment 176932 [details] [diff] [review]
incorpoates latest review comments

Question ---
1) These signatures don't seem to match...

AddExtensions(extHandle, emailAddrs, PR_FALSE, PR_FALSE, PR_FALSE,	       
       PR_FALSE, PR_FALSE, PR_FALSE, PR_FALSE)

and

AddExtensions(void *, const char *, const char *, PRBool, PRBool, PRBool,
PRBool, PRBool, PRBool);

2) shouldn't the previous line contain the new bools passed into CertReq?

bob

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Reopening.  
I agree that the code should not be passing PR_FALSE for a const char *.  
Weird that the compilers didn't complain about that.
Bob did some compiler warn you about that?  If so, which compiler/platform?

I'll let Neil reply to your question in comment 19.

Comment 21

[Neil Williams]

(In reply to comment #19)
> (From update of attachment 176932 [details] [diff] [review] [edit])
> Question ---
> 1) These signatures don't seem to match...
> 
> AddExtensions(extHandle, emailAddrs, PR_FALSE, PR_FALSE, PR_FALSE,	       
>        PR_FALSE, PR_FALSE, PR_FALSE, PR_FALSE)
> 
> and
> 
> AddExtensions(void *, const char *, const char *, PRBool, PRBool, PRBool,
> PRBool, PRBool, PRBool);
> 
> 2) shouldn't the previous line contain the new bools passed into CertReq?
> 
> bob
> 

You're right. This was fixed by attachment #177202 [details] [diff] [review] to bug 285208. Looks like I
neglected to add a note to this bug.

Comment 22

[Wan-Teh Chang]

There are compiler warnings at lines 187 and 191
of certreq.c, rev. 1.4.  We are using a CERTAttribute
pointer to point at a SECItem.

The pointer in question is a member of the 'attributes'
array in the CERTCertificateRequestStr structure:

struct CERTCertificateRequestStr {
    PRArenaPool *arena;
    SECItem version;
    CERTName subject;
    CERTSubjectPublicKeyInfo subjectPublicKeyInfo;
    CERTAttribute **attributes;
};

The 'attributes' array used to contain SECItem pointers.
In certt.h rev. 1.29 it was changed to contain CERTAttribute
pointers (bug 284200).

In certreq.c at lines 187 and 191, those pointers still point
to SECItem's.

Neil, Nelson, please look into this.

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

This is a case where the fix for one old bug unmasked another old bug.
The fix for *this* bug (bug 263779) did not introduce any new error.
Rather, it corrected an error, an incorrect type in the declaration 
of a structure member, making that structure agree with the ASN.1 
definition of a cert request, and with the template for that request.

That correction led to new warnings which reveal the fact that function 
CERT_CreateCertificateRequest does some VERY WRONG things when it is 
passed a non-NULL value for its last argument, the "attributes" argument. 
It constructs an array of structures of the wrong type.  If passed to 
the encoder, the encode would crash.  It has done those wrong things for 
years.  That code is no better and no worse after the fix to 263779 than
before, except that now its error is revealed in these warnings.

So, I will reclose this bug, and I will open a new bug for the errors
in CERT_CreateCertificateRequest.

Comment 24

[timeless]

nelson: where's the new bug?

Comment 25

[Wan-Teh Chang]

The new bug is bug 290263.