1117897 - Export SEC_CheckCrlTimes and SEC_GetCrlTimes (and remove unused CERT_FindCertURLExtension)

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Paul Wouters]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141201111517

Steps to reproduce:

libreswan uses a private NSS database for X.509 validation in IKE. To support OCSP and CRL fetching using NSS, we now have to copy the SEC_CheckCrlTimes() function because it is not exported.

Comment 1

[Kai Engert [:KaiE:]]

I've been told that there's another function that's required: CERT_FindCertURLExtension

Comment 2

[Kai Engert [:KaiE:]]

Attachment: Patch v1

Comment 3

[Wan-Teh Chang]

Comment on attachment 8546015 [details] [diff] [review]
Patch v1

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

r=wtc. Please note the things I requested before exporting these functions.

::: lib/nss/nss.def
@@ +1064,5 @@
>  ;+};
> +;+NSS_3.18 { 	# NSS 3.18 release
> +;+    global:
> +CERT_FindCertURLExtension;
> +SEC_CheckCrlTimes;

SEC_CheckCrlTimes: I think it is fine to export SEC_CheckCrlTimes because
the corresponding CERT_CheckCertValidTimes function for certificates is
already exported. We may want to also export SEC_GetCrlTimes because the
corresponding CERT_GetCertTimes is exported.

Things we should do first:
1. SEC_CheckCrlTimes isn't used by NSS itself:
   http://mxr.mozilla.org/nss/ident?i=SEC_CheckCrlTimes
   So we should review its code before exporting it.
2. SEC_CheckCrlTimes and CERT_CheckCertValidTimes are very similar. We
   may want to do what this comment in certdb.c suggests:
   http://mxr.mozilla.org/nss/source/lib/certdb/certdb.c#1046

   1046 /* These routines should probably be combined with the cert
   1047  * routines using an common extraction routine.
   1048  */
3. Optional: change the SEC_ prefix to CERT_ prefix.

CERT_FindCertURLExtension: it seems fine to export this function. We
should copy the comment to the .h file and document that the returned
char string needs to be freed with PORT_Free. We should change the
type of the |tag| and |catag| parameters to SECOidTag.

This is also a function that NSS itself doesn't use, so we should
review its code before exporting it. In particular, it uses the
SEC_OID_NS_CERT_EXT_BASE_URL extension. I am not sure if we should
support that.

Comment 4

[Kai Engert [:KaiE:]]

(In reply to Wan-Teh Chang from comment #3)
> We may want to also export SEC_GetCrlTimes because the
> corresponding CERT_GetCertTimes is exported.

ok


> Things we should do first:
> 1. SEC_CheckCrlTimes isn't used by NSS itself:
>    http://mxr.mozilla.org/nss/ident?i=SEC_CheckCrlTimes
>    So we should review its code before exporting it.

I reviewed it, by comparing it with the existing, similar, and actively used code from CERT_CheckCertValidTimes.

The calculations and the comparison statements are identical. If CERT_CheckCertValidTimes is correct, then those parts in SEC_CheckCrlTimes are correct, too, and it shouldn't be necessary to review those statements.

The "cert" function has a input parameter NULL check. I think the "crl" function should get it, too, I'll add it.

The "cert" function calls PORT_Error, I'll add equivalent calls to the "crl" function.


> 2. SEC_CheckCrlTimes and CERT_CheckCertValidTimes are very similar. We
>    may want to do what this comment in certdb.c suggests:
>    http://mxr.mozilla.org/nss/source/lib/certdb/certdb.c#1046
> 
>    1046 /* These routines should probably be combined with the cert
>    1047  * routines using an common extraction routine.
>    1048  */

A common helper function, called by both APIs, would have to be parameterized on error code values, and kind of check - because each function does a different additional check, each with an early function return.

We'd save the current duplication of the LL_I2L/LL_MUL/LL_SUB lines.

I believe the parameterized function would be more difficult to read, with its parameters, and the savings on the duplications is minimal, therefore I'm not convinced the common helper function is that useful. In my opinion, the functions are very small, and having them separate is more readable.

If you still think we should combine, let's file a separate bug that suggests the refactoring.


> CERT_FindCertURLExtension: it seems fine to export this function. We
> should copy the comment to the .h file and document that the returned
> char string needs to be freed with PORT_Free.

ok


> We should change the
> type of the |tag| and |catag| parameters to SECOidTag

ok


> This is also a function that NSS itself doesn't use, so we should
> review its code before exporting it. 

Based on your comments, it seems you have already reviewed parts of it :-)

I looked through the function, and it seems good to me. I've added a check for a NULL input parameter.


> In particular, it uses the
> SEC_OID_NS_CERT_EXT_BASE_URL extension. I am not sure if we should
> support that.

Given the code already implements it, it seems worthwile to keep it.
Resolving to the base is probably reasonable in many situations.

If you're worried that some callers might want to obtain the raw values, then I suggest we parameterize the function. I'll do a minimal invasive approach, to avoid re-indenting all of the function.

Comment 5

[Kai Engert [:KaiE:]]

Attachment: Alternative patch v2

Comment 6

[Kai Engert [:KaiE:]]

Nothing has happened within the past month, but we must go ahead with this work.

If I don't get any r+ on the alternative patch v2 in the next few days, I will mark v2 as obsolete and check in the simpler v1.

Comment 7

[Wan-Teh Chang]

Comment on attachment 8551459 [details] [diff] [review]
Alternative patch v2

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

::: lib/certdb/certdb.c
@@ +1067,5 @@
>      LL_I2L(tmp1, PR_USEC_PER_SEC);
>      LL_MUL(llPendingSlop, llPendingSlop, tmp1);
>      LL_SUB(notBefore, notBefore, llPendingSlop);
>      if ( LL_CMP( t, <, notBefore ) ) {
> +	PORT_SetError(SEC_ERROR_CRL_EXPIRED);

The SEC_CheckCrlTimes function seems to only rely on its return value and not the error code. So it may be unnecessary to set the error code in this function. If I asked you to do this in a previous code review, you may ignore that request. Sorry.

The reason is that the return values secCertTimeExpired vs. secCertTimeNotValidYet convey one more bit of information than the error code, which is SEC_ERROR_CRL_EXPIRED in both cases. And secCertTimeUndetermined is already documented to imply a null pointer argument.

But I won't object to setting error code if you think it is better.

::: lib/certdb/certv3.c
@@ +64,3 @@
>  char *
> +CERT_FindCertURLExtension(CERTCertificate *cert, SECOidTag tag, SECOidTag catag,
> +                          PRBool useBaseUrlExtension)

I investigated this. The Netscape base URL is a nonstandard
certificate extension. We should not support it. So let's not
add this useBaseUrlExtension argument. We should just remove
the code related to SEC_OID_NS_CERT_EXT_BASE_URL from this
function.

Comment 8

[Wan-Teh Chang]

Attachment: Remove Netscape base URL support from CERT_FindCertURLExtension

Kai, please merge this patch into your patch. Thanks.

IMPORTANT: I recommend removing the 'catag' argument as well.
Please look at how it's implemented. It calls
CERT_FindIssuerCertExtension, which determines the issuer
certificate by merely name matching. There is no signature
verification in that process. Do we really want to use that
function? It should not be hard to come up with an attack.
We should also remove that function.

Comment 9

[Kai Engert [:KaiE:]]

I agree to remove the automatic lookup of the base url, and the automatic lookup in an issuer cert.

The current functionality can be easily emulated by the application, by calling the API multiple times. If the application wants to retrieve a base url (wherever it might be stored in the certificat), the application can make a second call to the API with that base url tag.

Looking up tags in a issuer CA cert can also be handled by the application, by using other APIs to find the correct issuer cert, then querying that one for the URLs.

Comment 10

[Kai Engert [:KaiE:]]

Comment on attachment 8568332 [details] [diff] [review]
Remove Netscape base URL support from CERT_FindCertURLExtension

r=kaie and checked in:

https://hg.mozilla.org/projects/nss/rev/435c66fd2f90

Comment 11

[Kai Engert [:KaiE:]]

(In reply to Wan-Teh Chang from comment #7)
> But I won't object to setting error code if you think it is better.

I think it cannot hurt, and setting the error code is consistent with the other function, so I'll keep it.

Comment 12

[Kai Engert [:KaiE:]]

Attachment: patch v4

Comment 13

[Wan-Teh Chang]

Comment on attachment 8568838 [details] [diff] [review]
patch v4

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

r=wtc. Please make the two changes I suggested before checking
this in. Thanks.

::: lib/certdb/certv3.c
@@ +60,5 @@
>      
>      return(rv);
>  }
>  
>  /* find a URL extension in the cert or its CA

Please remove "or its CA" from this comment.

@@ -84,5 @@
>      rv = cert_FindExtension(cert->extensions, tag, &urlitem);
> -    if ( rv == SECSuccess ) {
> -    } else if ( catag ) {
> -	/* if the cert doesn't have the extensions, see if the issuer does */
> -	rv = CERT_FindIssuerCertExtension(cert, catag, &urlitem);

Please also remove the CERT_FindIssuerCertExtension function. It
is only called by this function:
http://mxr.mozilla.org/nss/ident?i=CERT_FindIssuerCertExtension

Remember to remove the declaration of CERT_FindIssuerCertExtension
from cert.h, too.

Comment 14

[Kai Engert [:KaiE:]]

Attachment: patch as checked in

Wan-Teh, thanks for being thorough. This is the patch as checked in.

https://hg.mozilla.org/projects/nss/rev/8777e4e52ea1

Comment 15

[Kai Engert [:KaiE:]]

reopening.

While working on the release notes, I noticed that we aren't declaring the newly exported SEC_GetCrlTimes function in any header yet.

Although we already have an NSS release candidate, we should fix this.

Comment 16

[Kai Engert [:KaiE:]]

Attachment: 1117897-declare-and-comment.patch

Comment 17

[Kai Engert [:KaiE:]]

Comment on attachment 8576241 [details] [diff] [review]
1117897-declare-and-comment.patch

Thanks Bob.

https://hg.mozilla.org/projects/nss/rev/4328ba1876aa

Comment 18

[Kai Engert [:KaiE:]]

Reopening this bug.

Thanks to Ryan Sleevi's questioning, we've discovered that the CERT_FindCertURLExtension API isn't sufficient for the need of the libreswan project.

Although I had already tagged a release, I suggest that we pull back that tag.

I suggest that we don't export that API.

I cannot find any internal user in NSS of this code, so I suggest we remove it altogether.

Comment 19

[Kai Engert [:KaiE:]]

Attachment: patch to remove CERT_FindCertURLExtension

Comment 20

[Robert Relyea]

Comment on attachment 8578276 [details] [diff] [review]
patch to remove CERT_FindCertURLExtension

r+ to backing this out.

Comment 21

[Kai Engert [:KaiE:]]

Comment on attachment 8578276 [details] [diff] [review]
patch to remove CERT_FindCertURLExtension

backed out the previous work on CERT_FindCertURLExtension and completely removed it

https://hg.mozilla.org/projects/nss/rev/c89e2dedb127