970539 - NSS tool improvements/fixes: certutil/btoa/pp/httpserv

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Kai Engert [:KaiE:]]

Since Bob prefers to rather review large patches, instead of many individual patches,
and because I'm working on several small enhancements in parallel, I'm filing this bug to track that work in a single patch. See also the dependency list for related bugs.

The issues I'd like to fix here are:

lib: export CERT_AddExtensionByOID to allow encoding OIDs without registering and OID tag
alg1485: increase the limits for the common name, org name and org unit fields
pk11load: pass parameters to pkcs#11 module when falling back to single threaded

btoa: wrap output in BEGIN/END lines to make it easy to create PEM file format
certutil: support adding subject alt name extension
certutil: support adding generic extensions (by loading them from binary files)
certutil: support extracting (dumping) a certificate extension
certutil: support additional usages when verifying a certificate
httpserv: close the input file after loading a CRL
pp: add an option to print non-ascii characters as UTF8 (instead of the default '.')
pp: allow shortcut values for specifying the format used to interpret the input

Comment 1

[Kai Engert [:KaiE:]]

Attachment: patch v8

Comment 2

[Kai Engert [:KaiE:]]

Portions of the attached patch (the subject alt name extension code) have been originally created by Bob in
  https://bugzilla.redhat.com/show_bug.cgi?id=674684

Attached is a merged version that also includes bugfixes.

Comment 3

[Kai Engert [:KaiE:]]

Attachment: p9-misc-util.patch

merged to trunk, after a few context changes.

Comment 4

[Robert Relyea]

Comment on attachment 8394996 [details] [diff] [review]
p9-misc-util.patch

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

r-, A fiew small issues in certext.c (including a bug or two).  additional comments are in pp.c, certutil.c and pk11load.c The rest of the code is fine.

Comments prefixed with 'Nice to Fix:' are things I'd like you to look at, but not necessarily need to fix.
Comments prefixed with 'Just not the way I would do it:' are more "Bob" style preferences. They are not NSS style requirements If you ignore them no harm will come to you;).

::: cmd/certutil/certext.c
@@ +665,5 @@
>               isCriticalExt));
>  
>  }
>  
> +static SECStatus

Nice to fix:
    getOidFromString() and getTypeFromString() look like candidates for generic exported functions from NSS or NSSUtil. The generic exported version of getOidFromString should probably return an OidTag rather than an oid in the generic version (your local version can still return the full oidData. If you don't fix it in this patch, please write a bug to have it fixed.

@@ +751,5 @@
>       * walk down the comma separated list of names. NOTE: there is
>       * no sanity checks to see if the email address look like
>       * email addresses.
>       */
> +    for (cp=names; cp; cp = next) {

Just not the way I would do it:

certext.c loop at line 775. I would have created an iterator function in which has basically the body of 766 to 777 and turn length as well as cp. This is really a Bob style suggestion, and a quick examination may indicate that it's more trouble than it's worth. No penalty for ignoring this.

However, It would be good to initialize next to NULL unconditionally at the very top of the loop. There is not bug in the current code, but it does help verify code correctness (since we have next set to some rational value unconditionally rather then verify that each side of the if properly sets it.

@@ +775,4 @@
>          if (len <= 0) {
>              continue;
>          }
> +        if (readTypeFromName) {

The type parsing code assumes good data. At line 778, if no : is found, the variable 'type' is just left hanging. Possible options include:
     1) fail if we don't get a type value.
     2) add a comment to the code that if no type is supplied, use the previous type AND
	2a) either initialize type at the beginning to a rational value, or
        2b) add comment at line 801 that if type is still '0' we'll fail in the default: switch statement.
     Adding comments tells me and future hackers of the code that the uninitializedness of type is intentional.

NOTE: in case 1 strings line dns:bugzilla.mozilla.org,bugzilla.redhat.com would not be valid, but would mean dns:bugzilla.mozilla.org,dns:bugzilla.redhat.com in case 2a and 2b.

Case 2a would allow myname,email:[e-mail] where myname would default to something like other:myname. In 2b this would not be valid.

You have implemented 2b, so if that's the intent, just comment the code as such.

@@ +776,5 @@
>              continue;
>          }
> +        if (readTypeFromName) {
> +            char *save=cp;
> +            cp = PORT_Strchr(cp, ':');

When doing the PORT_Strchr(cp,':'), you aren't checking that you've blown by a ',', so in all cases dns:bugzilla.mozilla.org,bugzilla.redhat.com,email:[e-mail] will fail because we will try to lok up 'bugzilla.redhat,com,email' as a type.

@@ +810,5 @@
> +        /* unformated data types */
> +        case certX400Address:
> +        case certEDIPartyName:
> +            /* turn a string into a data and len */
> +            rv = SECFailure; /* punt on these for now */

Missing break will cause you to fall through to certDirectory name. We should also fail noisily here (fprintf(stderr,"EDI Party Name and X.400 Address not supported\n");)

@@ +1943,5 @@
> +    const char *iter = nextExtension;
> +    
> +    if (!iter || !*iter)
> +        return SECFailure;
> +

suffers from the same problem as AddSubjectAltNames above, if the format has a colon after the comma, then we won't detect it (we'll probably fail looking up the oid, possibly to the confusion of the user). You can fix this pretty easily by looking for the ',' first and failing your ':' checks if the result is ever longer then where the ',' is.

@@ +2161,5 @@
> +            break;
> +        }
> +	    oid_item.data = NULL;
> +	    oid_item.len = 0;
> +	    rv = SEC_StringToOID(NULL, &oid_item, oid, oidLen);

Just a question:
Is there any reason we shouldn't use the new stringToOid function here and get named oids as well?

::: cmd/certutil/certutil.c
@@ +419,5 @@
>      return SECSuccess;
>  }
>  
>  static SECStatus
> +outputCert(CERTCertificate *the_cert, PRBool raw, PRBool ascii,

Nice to fix:

 outputCert() should really be called outputCertOrExtension()

::: cmd/pp/pp.c
@@ +132,5 @@
>      data.len = der.len;
>  
>      SECU_EnableWrap(wrap);
>  
>      /* Pretty print it */

Just not the way I would do it:

I actually prefer PORT_Strcmp(xxxx) == 0. It remindes my that '0' is success. !PORT_Strcmp() is shorter, but I have to wrap my head around the fact that !PORT_Strcmp means that the strings actually matches (which is a bit counter intitive to the bool operator, where success = 1).

::: lib/pk11wrap/pk11load.c
@@ +212,5 @@
>      if (!mod || !alreadyLoaded) {
>          PORT_SetError(SEC_ERROR_INVALID_ARGS);
>          return SECFailure;
>      }
>  

Just not the way I would do it:

I'm ok with colapsing the == PR_FALSE (I think I did this because PRBool isn't actually an 'c' bool). But I do prefer the == NULL. It reminds the review that the object is a pointer. Same with line 265.

Comment 5

[Kai Engert [:KaiE:]]

(In reply to Robert Relyea from comment #4)
> Nice to fix:
>     getOidFromString() and getTypeFromString() look like candidates for
> generic exported functions from NSS or NSSUtil. 

Ok. I've renamed them to SEC_NumberOrNameStringToOID and CERT_GetGeneralNameTypeFromString.

> The generic exported version
> of getOidFromString should probably return an OidTag rather than an oid in
> the generic version (your local version can still return the full oidData.

It's not clear to me why you dislike returning full oidData. The existing, exported function SEC_StringToOID (which is called as part of convenience function getOidFromString) doesn't return an OidTag either, but rather returns a full oidData, too.

If I understand correctly, we cannot return an OidTag if the input was a numeric, dotted OID, we would have to register that OID first in the dynamic OID table, but I'm not sure this makes sense, given we don't have a name that can be associated with it.

Comment 6

[Robert Relyea]

> It's not clear to me why you dislike returning full oidData. The existing, exported function 
> SEC_StringToOID (which is called as part of convenience function getOidFromString) doesn't 
> return an OidTag either, but rather returns a full oidData, too.

So it's not wrong to return the oidData, it's just inconvienient in most circumstances. Internally NSS uses the oidTag pretty much everywhere because it's a simple integer. It make comparisons easy because it's not a string compare, and it's handy because you can use it in switch statements throughout the code. You can always get an oidtag from oiddata and and oiddata from an oidtag, but the latter is more efficient (just a table lookup), where the former requires a hash table lookup.

Upshot is, if we make it external, the preference is to return the oidtag (particularly since we've already looked up the oid pointer), rather than the oidData. 

> If I understand correctly, we cannot return an OidTag if the input was a numeric, dotted OID, we 
> would have to register that OID first in the dynamic OID table, but I'm not sure this makes sense,
> given we don't have a name that can be associated with it.

Only if the lookup failed (it it's a dotted OID that doesn't match an existing oid). In that case, it probably make sense to add it to the dynamic table. Very little else in NSS is going to know what to do with an oid that doesn't have an entry in the oid table.

bob

Comment 7

[Kai Engert [:KaiE:]]

Bob, I'm afraid you're asking for too much.

The original getOidFromString() function was intended as a little convenience function, which is only being used in the utility code, and all it does is to look in two places, and return the binary OID, which is what the consumer requires.

Your request to fully generalize this utility function seems slightly over top to me, because nobody else currently seems to require that utility function. And it wouldn't solve the requirement we have for this new code, because, because we'd require yet another convenience function to avoid the code duplication in two places, wrapping the new generalized function you're asking for.

Comment 8

[Kai Engert [:KaiE:]]

The more I think about it, the less sense does a function SEC_NumberOrNameStringToOIDTag make to me.

In order to find an already registered OIDtag from a string presentation, we'd need to convert it to a binary OID first, then do a lookup of that blob in the table of registered OIDs. That means, even if we repeatedly need to convert the same string presentation, we're not creating efficient code, because we must always do the conversion and the lookup.

I'll revert this chance and remove function SEC_NumberOrNameStringToOID, and file the separate bug for further discussion, as you had originally offered in comment 4

Comment 9

[Kai Engert [:KaiE:]]

(In reply to Kai Engert (:kaie) from comment #8)
> I'll ...
> file the separate bug for further discussion, as you had originally offered
> in comment 4

bug 1003413

Comment 10

[Robert Relyea]

re: comment 9, that is sufficient. The function was a 'Nice to fix:'. You don't need it here to complete this patch.

Comment 11

[Kai Engert [:KaiE:]]

thanks

Comment 12

[Kai Engert [:KaiE:]]

Attachment: p12-970539.patch

I think I've addresses all requests, except that one that was moved to the separate bug.

I've also added tests for the new certutil features.

Comment 13

[Kai Engert [:KaiE:]]

And thanks a lot for noticing the issue with the parser code.

Comment 14

[Robert Relyea]

Comment on attachment 8422773 [details] [diff] [review]
p12-970539.patch

r+ rrelyea

Comment 15

[Kai Engert [:KaiE:]]

Thank you Bob!
https://hg.mozilla.org/projects/nss/rev/04c846cc7a85

Comment 16

[Kai Engert [:KaiE:]]

bustage fix:
https://hg.mozilla.org/projects/nss/rev/f78800443f33

Comment 17

[Robert Relyea]

> https://hg.mozilla.org/projects/nss/rev/f78800443f33
r+ C++ vs C issue;).

bob

Comment 18

[Chris Newman]

Actually that's a C99 vs. pre-C99 issue. I guess the project is still supporting compilers that are 15 years out of date on C standards?

Comment 19

[Wan-Teh Chang]

Attachment: Remove unused function SEC_NumberOrNameStringToOIDTag

The SEC_NumberOrNameStringToOIDTag function passes the second argument
of a wrong type to SEC_StringToOID. This generates a compiler warning.
Since the function is not used, I just deleted it.

Patch checked in: https://hg.mozilla.org/projects/nss/rev/dee23d59c940

Comment 20

[Kai Engert [:KaiE:]]

Comment on attachment 8434559 [details] [diff] [review]
Remove unused function SEC_NumberOrNameStringToOIDTag

r=kaie