certutil doesn't place or handle extensions in cert requests

RESOLVED FIXED in 3.10

Status

NSS
Tools
P2
enhancement
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Neil Williams)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

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.
(Reporter)

Comment 1

13 years ago
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.
Priority: -- → P2
Summary: certutil doesn't handle extensions in cert requests → certutil doesn't place or handle extensions in cert requests
Target Milestone: --- → 3.10
(Reporter)

Comment 2

13 years ago
Reassigning ot Neil, since bugzilla won't let him "take" it. :-(
Assignee: wtchang → neil.williams

Comment 3

13 years ago
I just gave Neil all the abilities he needs
in Bugzilla.
(Assignee)

Comment 4

13 years ago
Created attachment 175987 [details] [diff] [review]
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.
(Assignee)

Updated

13 years ago
Attachment #175987 - Flags: review?(nelson)
(Assignee)

Comment 5

13 years ago
Created attachment 175990 [details] [diff] [review]
Correction to OID patch

New OID used same index as a previous patch. CVS update didn't notice?
Attachment #175990 - Flags: review?(nelson)
(Reporter)

Comment 6

13 years ago
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.
Attachment #175990 - Flags: review?(nelson) → review+
(Assignee)

Comment 7

13 years ago
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

13 years ago
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.
(Reporter)

Comment 9

13 years ago
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.
Attachment #175987 - Attachment is obsolete: true
Attachment #175987 - Flags: review?(nelson) → review-
(Assignee)

Comment 10

13 years ago
Created attachment 176610 [details] [diff] [review]
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.
Attachment #176610 - Flags: review?(nelson)
(Reporter)

Comment 11

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

I sent patch feedback to Neil.	r-
Attachment #176610 - Flags: review?(nelson) → review-
(Assignee)

Comment 12

13 years ago
Created attachment 176806 [details] [diff] [review]
Incorporates review suggestions.
Attachment #176806 - Flags: review?(nelson)
(Assignee)

Comment 13

13 years ago
Created attachment 176818 [details] [diff] [review]
This fixes a review comment I missed in the previous patch.

Please disregard the previous patch.
Attachment #176818 - Flags: review?(nelson)
(Assignee)

Updated

13 years ago
Attachment #176806 - Attachment is obsolete: true
(Reporter)

Comment 14

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

cancelling review request on this obsolete patch.
Attachment #176806 - Flags: review?(nelson)
(Reporter)

Comment 15

13 years ago
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.
Attachment #176818 - Flags: review?(nelson) → review-
(Assignee)

Comment 16

13 years ago
Created attachment 176932 [details] [diff] [review]
incorpoates latest review comments
Attachment #176818 - Attachment is obsolete: true
Attachment #176932 - Flags: review?(nelson)
(Reporter)

Comment 17

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

r=nelson
Attachment #176932 - Flags: review?(nelson) → review+
(Assignee)

Comment 18

13 years ago
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
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Comment 19

13 years ago
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
(Reporter)

Comment 20

13 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 21

13 years ago
(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.
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED

Comment 22

13 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

13 years ago
Priority: P2 → P1
(Reporter)

Comment 23

13 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago13 years ago
Priority: P1 → P2
Resolution: --- → FIXED

Comment 24

13 years ago
nelson: where's the new bug?

Comment 25

13 years ago
The new bug is bug 290263.
You need to log in before you can comment on or make changes to this bug.