certutil fails to create crl distribution point extension

NEW
Assigned to

Status

4 years ago
3 years ago

People

(Reporter: kaie, Assigned: kaie)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
Although certutil contains parameters and code to add a CRL distribution point extension, I've failed to use it.

The code incorrectly checks the result of PORT_GetError() and aborts wrongfully.
(Assignee)

Comment 1

4 years ago
Created attachment 8485771 [details] [diff] [review]
patch v1
Assignee: nobody → kaie
Attachment #8485771 - Flags: review?(rrelyea)
(Assignee)

Comment 2

4 years ago
Using the attached patch, I was able to add an extension with a single general name (URI), and selecting "finish" in both additional questions that asked for "reason flag" and "crl issuer name".
(Assignee)

Comment 3

4 years ago
Bob, do you have time to review this?
Flags: needinfo?(rrelyea)

Comment 4

4 years ago
Comment on attachment 8485771 [details] [diff] [review]
patch v1

Review of attachment 8485771 [details] [diff] [review]:
-----------------------------------------------------------------

r-. but the hard part and base idea of the patch is correct.

::: cmd/certutil/certext.c
@@ +1169,5 @@
>              current->distPointType = intValue;
>              current->distPoint.fullName = CreateGeneralName (arena);
> +            if (!current->distPoint.fullName) {
> +                rv = PORT_GetError();
> +            }

rv = SECFailure;

not rv = PORT_GetError();
This is a bug in the original code.

rv is a SECStatus (SECSuccess, SECFailure, SECWouldBlock).
PORT_GetError() returns a SECError (lots of values, none of which are SECFailure).

@@ +1230,3 @@
>          current->crlIssuer = CreateGeneralName (arena);
> +        if (current->crlIssuer == NULL && PORT_GetError() != 0) {
> +            rv = SECFailure;

Good, you fixed the same issue here. 

I actually don't think we want to do the PORT_GetError() here unless we need this code to execute and not fail if user does not select a valid general name type on the first call. I think we want to fail in that case as well, but I'll leave it to you. Did you have a case where you got '0' from PORT_GetError() but current->crlIssue was NULL?
Attachment #8485771 - Flags: review?(rrelyea) → review-

Comment 5

3 years ago
clearing need info, I effective answered kaie's question with the review in comment 4
Flags: needinfo?(rrelyea)
You need to log in before you can comment on or make changes to this bug.