Refactor CRL handling

This patch refactors the CRL handling to rely more on the implementation of the crypto library. It will insert the CRL at the correct time to keep it up to date, but all additional verification logic is removed from ssl_verify_<backend>.c. "Less code of our own, less bugs of our own."

In practice, this means extra checks will be performed on the CRL, such as checking it validBefore and validAfter fields.

This patch was originally written by Ivo Manca, and then molded by Steffan before sending to the list. All bugs are Steffan's fault.

Thanks also go to Antonio Quartulli for useful feedback. He'll send follow-up patches to improve CRL handling performance.

---
 Changes.rst                      |  6 +++
 src/openvpn/ssl.c                | 15 +++++++
 src/openvpn/ssl_backend.h        | 11 +++++
 src/openvpn/ssl_mbedtls.c        | 43 +++++++++++++++++-
 src/openvpn/ssl_mbedtls.h        |  1 +
 src/openvpn/ssl_openssl.c        | 58 ++++++++++++++++++++++++
 src/openvpn/ssl_verify.c         | 19 ++++----
 src/openvpn/ssl_verify_backend.h | 19 +++-----
 src/openvpn/ssl_verify_mbedtls.c | 56 +++--------------------
 src/openvpn/ssl_verify_openssl.c | 96 ++++++++++++++++------------------------
 10 files changed, 193 insertions(+), 131 deletions(-)

index 0118e78..3df35a7 100644
@@ -173,6 +173,12 @@ User-visible Changes
   capable. The ``--tun-ipv6`` option is ignored (behaves like it is always
   on).
 
+- CRLs are now handled by the crypto library (OpenSSL or mbed TLS), instead of
+  inside OpenVPN itself.  The crypto library implementations are more strict
+  than the OpenVPN implementation was.  This might reject peer certificates
+  that would previously be accepted.  If this occurs, OpenVPN will log the
+  crypto library's error description.
+
 
 Maintainer-visible changes
 --------------------------

index 7f370b7..1307569 100644
@@ -608,6 +608,12 @@ init_ssl (const struct options *options, struct tls_root_ctx *new_ctx)
   /* Check certificate notBefore and notAfter */
   tls_ctx_check_cert_time(new_ctx);
 
+  /* Read CRL */
+  if (options->crl_file && !(options->ssl_flags & SSLF_CRL_VERIFY_DIR))
+    {
+      tls_ctx_reload_crl(new_ctx, options->crl_file, options->crl_file_inline);
+    }
+
   /* Once keys and cert are loaded, load ECDH parameters */
   if (options->tls_server)
     tls_ctx_load_ecdh_params(new_ctx, options->ecdh_curve);

@@ -2496,6 +2502,15 @@ tls_process (struct tls_multi *multi,
 	{
 	  ks->state = S_START;
 	  state_change = true;
+
+	  /* Reload the CRL before TLS negotiation */
+	  if (session->opt->crl_file &&
+	      !(session->opt->ssl_flags & SSLF_CRL_VERIFY_DIR))
+	    {
+	      tls_ctx_reload_crl(&session->opt->ssl_ctx,
+		  session->opt->crl_file, session->opt->crl_file_inline);
+	    }
+
 	  dmsg (D_TLS_DEBUG_MED, "STATE S_START");
 	}
 

index 542c373..a393a0c 100644
@@ -344,6 +344,17 @@ void key_state_ssl_init(struct key_state_ssl *ks_ssl,
 void key_state_ssl_free(struct key_state_ssl *ks_ssl);
 
 /**
+ * Reload the Certificate Revocation List for the SSL channel
+ *
+ * @param ssl_ctx       The TLS context to use when reloading the CRL
+ * @param crl_file      The file name to load the CRL from, or
+ *                      "[[INLINE]]" in the case of inline files.
+ * @param crl_inline    A string containing the CRL
+ */
+void tls_ctx_reload_crl(struct tls_root_ctx *ssl_ctx,
+    const char *crl_file, const char *crl_inline);
+
+/**
  * Keying Material Exporters [RFC 5705] allows additional keying material to be
  * derived from existing TLS channel. This exported keying material can then be
  * used for a variety of purposes.

index 6f778ef..3edeedc 100644
@@ -73,6 +73,7 @@ struct tls_root_ctx {
     mbedtls_x509_crt *crt_chain;	/**< Local Certificate chain */
     mbedtls_x509_crt *ca_chain;		/**< CA chain for remote verification */
     mbedtls_pk_context *priv_key;	/**< Local private key */
+    mbedtls_x509_crl *crl;              /**< Certificate Revocation List */
 #if defined(ENABLE_PKCS11)
     mbedtls_pkcs11_context *priv_key_pkcs11;	/**< PKCS11 private key */
 #endif

index d0c22b8..745b125 100644
@@ -669,15 +669,18 @@ verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep
   if (opt->crl_file)
     {
       if (opt->ssl_flags & SSLF_CRL_VERIFY_DIR)
-      {
-	if (SUCCESS != verify_check_crl_dir(opt->crl_file, cert))
-	  goto cleanup;
-      }
+	{
+	  if (SUCCESS != verify_check_crl_dir(opt->crl_file, cert))
+	    goto cleanup;
+	}
       else
-      {
-	if (SUCCESS != x509_verify_crl(opt->crl_file, opt->crl_file_inline, cert, subject))
-	  goto cleanup;
-      }
+	{
+	  if (tls_verify_crl_missing (opt))
+	    {
+	      msg (D_TLS_ERRORS, "VERIFY ERROR: CRL not loaded");
+	      goto cleanup;
+	    }
+	}
     }
 
   msg (D_HANDSHAKE, "VERIFY OK: depth=%d, %s", cert_depth, subject);

index 91e6ec9..de304b9 100644
@@ -252,19 +252,12 @@ result_t x509_verify_cert_eku (openvpn_x509_cert_t *x509, const char * const exp
  */
 result_t x509_write_pem(FILE *peercert_file, openvpn_x509_cert_t *peercert);
 
-/*
- * Check the certificate against a CRL file.
- *
- * @param crl_file	File name of the CRL file
- * @param cert		Certificate to verify
- * @param crl_inline	Contents of the crl file if it is inlined
- * @param subject	Subject of the given certificate
- *
- * @return 		\c SUCCESS if the CRL was not signed by the issuer of the
- * 			certificate or does not contain an entry for it.
- * 			\c FAILURE otherwise.
+/**
+ * Return true iff a CRL is configured, but is not loaded.  This can be caused
+ * by e.g. a CRL parsing error, a missing CRL file or CRL file permission
+ * errors.  (These conditions are checked upon startup, but the CRL might be
+ * updated and reloaded during runtime.)
  */
-result_t x509_verify_crl(const char *crl_file, const char *crl_inline,
-                         openvpn_x509_cert_t *cert, const char *subject);
+bool tls_verify_crl_missing(const struct tls_options *opt);
 
 #endif /* SSL_VERIFY_BACKEND_H_ */

2.7.4

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

This is a bit scary. I have nothing to complain about in the patch, so I feel like I'm not seeing the large neon-pink elephant in the room.

I've run lots of tests, both with OpenSSL and mbedTLS. Testing with and without valgrind --leak-check=full. Tested configurations without
- --crl, with --crl ca.crl and --crl crldir dir.

Every thing works as expected, no memory leaks detected. It passes 'make check' including t_client.sh runs. Traffic is passed over the tunnel where certificate is not listed on a CRL.

There is one detail, which is not a fault of this patch ... but I'll just mention it here. The client does not exit gracefully on rejections due to CRL. It's not too odd, but should probably look into if it would be possible to send an "AUTH_FAILED,REVOKED:
Certificate revoked" message back to the client. But that's for a different patch.

I can definitely give this one a Feature-ACK. I can also support a full ACK too if one more developer does a thorough code review.

- --
kind regards,

David Sommerseth
OpenVPN Technologies, Inc

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJYG4BCAAoJEIbPlEyWcf3yahUP/jP1fh4L+y4Ct2u61OVVJLIg 7g5nxw/S3j87tbqf5cyvP/Pw8GvB9klDqMt3MHPZLfG1fRpiJuOwPB/ad53WNdOf BDN2NqlDpDFNT1Zv+VxM4B1SJlefMQGRvKS67PM3VESDdCVZ60p//AwvTyczi6lB lgk+Wp/vOypdky3L1nxvH7iMsOJWdkE/DcvYFrryI+vFCBQjxp88V4X6MHxQKqRp g2l5rSHXv0gh7A5BmnzZLIbUjJBpuCDZ4/TpkNAgScXeqGOGLyzT+S8kvNEdgVUG jOkDQunJ6hcN1Hf3g1xltj+zoDoYh4iRSGqAHpURh4hgKVue1ElLXWGqS2vNzxRQ vV0Nmt/4R6LTiCgKow7WX8kJxWfB83SKhpB9XCg1xri2tqwqwy7pNwVFhqcBjb9F qHLb/z9vddMsgvTjuMwOH0DyQbXi9vyNTB+KadhkN9BSYBJaR4CHAddAdzOnEh+0 bjGNIbd9UVVbf+Bo8sqbjV3VVLJgV2gZgPK4HpxAY6DGno1Bfh0IYGNngR4w2+T7 QetPKsbOaa9w6k+Sc7sGuytZBhgJ4jtjPRBgZ13Q+cUpyVgl8UjxPS9YyxwHg0Mt S02uLZvhqA8NMd3s4X7VGjPle4or1voMxiWGUonvrHR8PsoAVfEBfNK5xrvfkf8m xjpM785uFGjPvbQkisTN
=mk0U
-----END PGP SIGNATURE-----

Hi,

What probably makes a difference here is that this patch was not originally written by me ;) Seriously though, the patch was written by my colleague, reviewed and slightly adjusted by me, then got some comments from Antonio and then finally molded to it's current form. So it already got quite some off-list review. Also, I ran our OpenVPN-NL test suite against (the mbed TLS version of) this patch, which verifies correct behaviour for a number of more advanced CA setups (including intermediate CAs, revoked intermediates, invalid CRLS, etc...).

Still, this does change behaviour (e.g. rejecting expired CRLS), so will probably reject some connections that earlier versions would accept. But I'd consider that a fix, not a bug.

We can't send AUTH_FAILED, because we have no control channel to send it over yet (see other mail thread).

Thanks!

-Steffan

Hi,

I've reviewed the code and found a few nag bits, so you're just not critical enough :-)

None of the nag bits are relevant enough to NAK the code - so, combined with your detailed review of the SSL library side...

... I think this is good enough. So, ACK.

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Your patch has been applied to the master branch

commit 160504a2955c4478cd2c0323452929e07016a336

Author: Steffan Karger
Date:   Fri Oct 28 17:54:47 2016 +0200

     Refactor CRL handling

     Signed-off-by: Ivo Manca <[email protected]...>
     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: David Sommerseth <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg12809.html
     Signed-off-by: David Sommerseth <[email protected]...>

- --
kind regards,

David Sommerseth

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJYLHoHAAoJEIbPlEyWcf3yf7oP/RXZQqn6tr/DGqZbsB8Ub4LQ rHvcUBx9xLhbxVzCJglP42xkarzlRPEuDDvRh04qavNFF8ymLaL5l9tcgFmDAfev 7e97Rz88MnGiTW1MvFjILcVRl7ezdRtzRkJ1nnTq4uxGRY2hrqOGubkKVKoOU68c ifeKqjBEGCmKFrsDB5Uplkd5Q8gfw+GTPVlvRyoEDHe5NPUE+FF/mp1z/mopgV2r Mzv1K8KbqcLLJIHXeOPi5/hnU6noIpohjdCHNyBrloVVxO3KBkj7LOpSXu4AM47/ ZthQ4TAzjBNNiqGzpoQIH7X/gHvSRORWIV85HH3lmXM4101o8zjJGToCeXjJP4Cm lCX7gOjFZdyusL6FUY38n6noDjTjMfrQBF7LpzG1bTdQ2LuDPZDlPMNXud8Jyx30 8BXIXG0NQ65auci+Hafm5Nvo9RJcqwXBpTxUnaioqsd13qzaFWuMlKRQB3liFmH1 0wwh/LVgstAEk1wzO0kdQr4fJ9Yx/qis3iyU1/m/KHds/YEE/OFPyxEDoNVjU/D8 +p9WQabaChRAKCJatXZerZZsYqHNAi3KRiIZ1he95+jXrj89y6qxfF7EJVjT8dpo vuibHM+20jE9481QsSOSzr7O1dL1kFN9sNDjCEKXy6W3vD2T53g6Z65eRo9fhSsJ 6aIAM4Qz47w9sJ+c6/XQ
=3xBv
-----END PGP SIGNATURE-----