Hello,

Previous patches (still against 7.18.1), updated with your inputs and
with 2 additional patches for curl_easy_setopt() man page. See comments
inline below.

Rob Crittenden <rcritten_at_redhat.com> writes:

> In nss_load_crl() you'll want to free the slot even if there is no
> crl. I'd move the PK11_FreeSlot(slot) above the if
> (!crl)... statement. Otherwise NSS_Shutdown() will fail.

Obviously. Thanks.

> You probably also want to logically-or CRL_DECODE_DEFAULT_OPTIONS with
> CRL_DECODE_DONT_COPY_DER in the call to
> PK11_ImportCRL(). CRL_DECODE_DEFAULT_OPTIONS is currently 0x0 but I
> suppose it could change at some point.

Good catch. thanks.

> And if the import fails there is no indication of why it failed or
> where. Perhaps some new curl error messages are needed? For example,
> the CRL may be failing to import because it has bad BEGIN/END
> formatting ...

The CURLE_SSL_CRL_BADFILE error we introduced tells that the CRL file
could not be imported (permissions, format, ...). We could indeed make
different versions, i.e. provide a finer granularity but this is not
done for other elements handled by curl. If someone find it annoying,
she will provide a patch.

> ... or it could be expired. There would be no way to tell.

From RFC 5280, next update field "indicates the date by which the next
CRL will be issued". I don't think a CRL with a Next Update value before
current time should result in an error. It just means it is probably
not the latest but in all cases (99.999...%), the information it
contains is valid and you want to take it into account. The only
counterexample I see would be the case of a certificate that has been
put in hold in the CRL and made valid again in an up-to-date version. I
do not think it is critical (AFAICT, it is not used).

> I think the last two lines here need to be reversed to prevent a
> memory leak:
> + rv = ATOB_ConvertAsciiToItem(&crlDER,body);
> + if (rv) return 0;
> + PORT_Free(filedata.data);

yep. thanks.

>> NSS version has comments. It tries to mimic the behaviour of OpenSSL
>> X509_check_issued() function.
>
> You call this code twice:
>
> + proto_win = SSL_RevealPinArg(sock);
> + issuer = NULL;
> + issuer = PK11_FindCertFromNickname(issuer_nickname, proto_win);

In fact, the second call was in a big comment, i.e. not called. I
removed it in the new version (should have done that before).

> This function could fail very early and yet it continues to perform
> the checks. Should it have a way to bail out early (e.g. a goto loser:
> where the certificates are freed)?

That should be ok and easier to read now.

If everything is ok, next round will be against 7.18.2.

Regards,

a+