[PATCH RFC] Use certificate pinning to allow otherwise invalid certs

[Dan Williams]

On Tue, 2015-03-03 at 19:49 -0600, Rohit Agrawal wrote:
> Hi all,
> 
> wpa_supplicant allows one to specify the sha256 hash of a certificate,
> which is currently used (in src/crypto/tls_openssl.c lines 1506-1540)
> to do the following:
> * if a leaf certificate is valid and the certificate is pinned, don't
> check the full chain (lines 1506-1507)
> * if the leaf certificate is valid and the provided certificate
> doesn't match the pinned one, reject it (lines 1518-1541).
> 
> I would like to propose that the behavior be modified slightly to add
> the following:
> * if openssl reports that the leaf certificate is _invalid_ but it
> matches the pinned certificate, accept it
> 
> My use case is connecting to a RADIUS server I do not have control
> over with certificate chain problems, but due to other out-of-band
> reasons I trust the leaf certificate. Currently, even if I pin the
> certificate, wpa_supplicant rejects it because openssl reports that
> the certificate is invalid, and wpa_supplicant then ignores the fact
> that I specified a pinned cert.
> 
> If this is acceptable, the following patch implements the behavior change:
> 
> From a568b8dacf2ba3c534a278217cb22719bda73c41 Mon Sep 17 00:00:00 2001
> From: Rohit Agrawal <rohit.agrawal.mn at gmail.com>
> Date: Tue, 3 Mar 2015 19:44:52 -0600
> Subject: [PATCH] Pinned certificates are always accepted
> 
> If OpenSSL reports that a presented leaf certificate is invalid
> but it has been explicitly pinned, accept it anyway.
> 
> Signed-off-by: Rohit Agrawal <rohit.agrawal.mn at gmail.com>
> ---
>  src/crypto/tls_openssl.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/src/crypto/tls_openssl.c b/src/crypto/tls_openssl.c
> index 46c4a46..f4b7620 100644
> --- a/src/crypto/tls_openssl.c
> +++ b/src/crypto/tls_openssl.c
> @@ -1516,7 +1516,9 @@ static int tls_verify_cb(int preverify_ok,
> X509_STORE_CTX *x509_ctx)
>   err_str = X509_verify_cert_error_string(err);
> 
>  #ifdef CONFIG_SHA256
> - if (preverify_ok && depth == 0 && conn->server_cert_only) {
> + // don't require preverify_ok so we can explicity allow otherwise
> + // invalid pinned certs

Use "C" style comments, not C++ style ones...

> + if (depth == 0 && conn->server_cert_only) {

I think your mailer mangled the whitespace in this patch.  If it's got a
"preformatted" option, try that when pasting in the patch itself?

Dan

>   struct wpabuf *cert;
>   cert = get_x509_cert(err_cert);
>   if (!cert) {
> @@ -1534,6 +1536,10 @@ static int tls_verify_cb(int preverify_ok,
> X509_STORE_CTX *x509_ctx)
>   err_str = "Server certificate mismatch";
>   err = X509_V_ERR_SELF_SIGNED_CERT_IN_CHAIN;
>   preverify_ok = 0;
> + } else {
> + // certificate matches pinned certificate, allow
> + // regardless of other problems
> + preverify_ok = 1;
>   }
>   wpabuf_free(cert);
>   }