Closed
Bug 263779
Opened 20 years ago
Closed 19 years ago
certutil doesn't place or handle extensions in cert requests
Categories
(NSS :: Tools, enhancement, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
3.10
People
(Reporter: nelson, Assigned: neil.williams)
Details
Attachments
(3 files, 3 obsolete files)
2.12 KB,
patch
|
nelson
:
review+
|
Details | Diff | Splinter Review |
18.79 KB,
patch
|
nelson
:
review-
|
Details | Diff | Splinter Review |
16.41 KB,
patch
|
nelson
:
review+
|
Details | Diff | Splinter Review |
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•20 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•20 years ago
|
||
Reassigning ot Neil, since bugzilla won't let him "take" it. :-(
Assignee: wtchang → neil.williams
Comment 3•20 years ago
|
||
I just gave Neil all the abilities he needs in Bugzilla.
Assignee | ||
Comment 4•19 years ago
|
||
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•19 years ago
|
Attachment #175987 -
Flags: review?(nelson)
Assignee | ||
Comment 5•19 years ago
|
||
New OID used same index as a previous patch. CVS update didn't notice?
Attachment #175990 -
Flags: review?(nelson)
Reporter | ||
Comment 6•19 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•19 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•19 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•19 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•19 years ago
|
||
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•19 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•19 years ago
|
||
Attachment #176806 -
Flags: review?(nelson)
Assignee | ||
Comment 13•19 years ago
|
||
Please disregard the previous patch.
Attachment #176818 -
Flags: review?(nelson)
Assignee | ||
Updated•19 years ago
|
Attachment #176806 -
Attachment is obsolete: true
Reporter | ||
Comment 14•19 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•19 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•19 years ago
|
||
Attachment #176818 -
Attachment is obsolete: true
Attachment #176932 -
Flags: review?(nelson)
Reporter | ||
Comment 17•19 years ago
|
||
Comment on attachment 176932 [details] [diff] [review] incorpoates latest review comments r=nelson
Attachment #176932 -
Flags: review?(nelson) → review+
Assignee | ||
Comment 18•19 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
Closed: 19 years ago
Resolution: --- → FIXED
Comment 19•19 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•19 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•19 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
Closed: 19 years ago → 19 years ago
Resolution: --- → FIXED
Comment 22•19 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•19 years ago
|
Priority: P2 → P1
Reporter | ||
Comment 23•19 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
Closed: 19 years ago → 19 years ago
Priority: P1 → P2
Resolution: --- → FIXED
Comment 24•19 years ago
|
||
nelson: where's the new bug?
Comment 25•19 years ago
|
||
The new bug is bug 290263.
You need to log in
before you can comment on or make changes to this bug.
Description
•