Arnaud Ebalard wrote:
> Hi,
>
> Please find attached for discussion a set of patches developed by
> Axel (in Cc, but not on the list, so please keep him in Cc) and I
> adding functionalities related to SSL/TLS support in libcrul:
>
> - support for CRL
> - support for Issuer check
>
> They are against version 7.18.1 and apply one on top of each other.
>
> The main reason for developing the patches is a need for another set
> of patches we have against Debian APT https method (provided by
> apt-transport-https package), but it might be useful to others.
>
> Because we used our modified APT https method (based on libcurl-gnutls)
> for the tests, we can only provide feedback for the behavior of the
> gnutls flavour even if our patches also have [untested] code for OpenSSL
> and NSS flavours.
>
> Comments are welcome. Patches are described below.
>
>
> ## crl_support.patch
>
> The first patch (crl_support.patch) adds support of CRL for gnutls,
> openssl and NSS flavours of libcurl.
>
> More precisely, it adds CURLOPT_CRLFILE option that allows specifying a
> file containing CRL information in PEM format (multiple CRL can be
> concatenated together).
>
> For OpenSSL, X509_V_FLAG_CRL_CHECK and X509_V_FLAG_CRL_CHECK_AL flags
> are both set, requiring CRL check and for the whole chain if a CRL
> file is passed.
>
> For gnutls and NSS, there is no way to influence the behavior w.r.t
> the status of CRL checks.
>
> It has been tested against a https server with different CRL files
> (not provided, revoked server certificate, non-revoked server
> certficate) and works ok AFAICT.
>
> If there is some simple way to test the OpenSSL and NSS support, just
> tell us.

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.

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.

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 or
it could be expired. There would be no way to tell.

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);

>
>
> ## issuer_check.patch
>
> In multi-levels PKI, there is sometimes a need to limit/check the
> issuer of the server certificate by providing a hint. Some apps allow
> passing the DN of the expected issuer as a string or more simply by
> passing the expected issuer certificate.
>
> This patch adds support for CURLOPT_ISSUERCERT option, which allows
> passing the expected issuer certificate file in PEM format. If the
> option is provided, then, after the usual certificate check procedure
> (done as usual), provided issuer certificate is compared with the
> information in the peer certificate.
>
> For gnutls, it uses gnutls_x509_crt_check_issuer() against the two
> certs.
>
> For OpenSSL, it uses X509_check_issued() against the two
> certificates.
>
> 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);

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)?

> It has been tested against a https server with two different
> certificates (issuer one and another one) and works ok AFAICT.
>
> Again, if there is some simple way to test the OpenSSL and NSS
> support, just tell us.
>
>
> Cheers,
>
> a+
>
>

Thanks very much for including an NSS version in your patches!

rob