diff mbox series

[Openvpn-devel,v2] OpenSSL: Fix --crl-verify not loading multiple CRLs in one file

Message ID 20200407174436.238933-1-wgh@torlan.ru
State Accepted
Headers
Series [Openvpn-devel,v2] OpenSSL: Fix --crl-verify not loading multiple CRLs in one file |

Commit Message

WGH April 7, 2020, 7:44 a.m. UTC 
  From: Maxim Plotnikov <wgh@torlan.ru>

Lack of this led people accepting multiple CAs to use capath,
which already supports multiple CRLs. But capath mode itself
is somewhat ugly: you have to create new file/symlink every time
CRL is updated, and there's no good way to clean them up without
restarting OpenVPN, since any gap in the sequence would cause it
to lose sync[1].

mbedtls crypto backend already loads multiple CRLs as is, so
it doesn't need this fix.

The patch also includes some logging changes which I think are useful.

If you wish to test the patch, here is prepared configuration
files: https://wgh.torlan.ru/openvpn-crl-fix-ca.tar.gz.
The client_ca2_revoked.ovpn config uses a revoked certificate
issued by the second CA, and is accepted by unpatched server,
but rightfully rejected with this patch (or when using mbedtls backend).

[1] https://community.openvpn.net/openvpn/ticket/623#comment:7
---
 src/openvpn/ssl_openssl.c | 40 +++++++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 12 deletions(-)

Comments

Arne Schwabe April 7, 2020, 12:38 p.m. UTC | #1 
Am 07.04.20 um 19:44 schrieb wgh@torlan.ru:
> From: Maxim Plotnikov <wgh@torlan.ru>
> 
> Lack of this led people accepting multiple CAs to use capath,
> which already supports multiple CRLs. But capath mode itself
> is somewhat ugly: you have to create new file/symlink every time
> CRL is updated, and there's no good way to clean them up without
> restarting OpenVPN, since any gap in the sequence would cause it
> to lose sync[1].
> 


>  
> -    crl = PEM_read_bio_X509_CRL(in, NULL, NULL, NULL);
> -    if (crl == NULL)
> +    int num_crls_loaded = 0;
> +    while (true)
>      {
> -        msg(M_WARN, "CRL: cannot read CRL from file %s", crl_file);
> -        goto end;
> -    }
> +        X509_CRL *crl = PEM_read_bio_X509_CRL(in, NULL, NULL, NULL);
> +        if (crl == NULL)
> +        {
> +            /*
> +             * PEM_R_NO_START_LINE can be considered equivalent to EOF.
> +            */

Minor whitespace problem.

> +            bool eof = ERR_GET_REASON(ERR_peek_error()) == PEM_R_NO_START_LINE;
> +            /* but warn if no CRLs have been loaded */
> +            if (num_crls_loaded > 0 && eof) {
> +                /* remove that error from error stack */
> +                (void)ERR_get_error();
> +                break;
> +            }

Minor style problem. { should be on the next line.

Acked-By: Arne Schwabe <arne@rfc2549.org>

I leave it to Gert/David if they want a rev3 or fix the minor
style/whitespace problems on commit.

Arne
Gert Doering April 10, 2020, 10:25 a.m. UTC | #2 
Your patch has been applied to the master and release/2.4 branch
(sort-of bugfix / featurette with very localized impact).

Did a very basic compile/client side test, no CRLs involved.  Code change
looks reasonable and similar to what Arne did with the CAs just recently.

commit 05229fb5923f43a502bf0ca731d9ba3106c259e8 (master)
commit ed925c0a8d3e6aa8bc26de8c0e7ed79a47e5c7d6 (release/2.4)
Author: Maxim Plotnikov
Date:   Tue Apr 7 20:44:36 2020 +0300

     OpenSSL: Fix --crl-verify not loading multiple CRLs in one file

     Acked-by: Arne Schwabe <arne@rfc2549.org>
     Message-Id: <20200407174436.238933-1-wgh@torlan.ru>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19710.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 15959a90..dd818175 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -1050,7 +1050,6 @@  void
 backend_tls_ctx_reload_crl(struct tls_root_ctx *ssl_ctx, const char *crl_file,
                            const char *crl_inline)
 {
-    X509_CRL *crl = NULL;
     BIO *in = NULL;
 
     X509_STORE *store = SSL_CTX_get_cert_store(ssl_ctx->ctx);
@@ -1091,21 +1090,38 @@  backend_tls_ctx_reload_crl(struct tls_root_ctx *ssl_ctx, const char *crl_file,
         goto end;
     }
 
-    crl = PEM_read_bio_X509_CRL(in, NULL, NULL, NULL);
-    if (crl == NULL)
+    int num_crls_loaded = 0;
+    while (true)
     {
-        msg(M_WARN, "CRL: cannot read CRL from file %s", crl_file);
-        goto end;
-    }
+        X509_CRL *crl = PEM_read_bio_X509_CRL(in, NULL, NULL, NULL);
+        if (crl == NULL)
+        {
+            /*
+             * PEM_R_NO_START_LINE can be considered equivalent to EOF.
+            */
+            bool eof = ERR_GET_REASON(ERR_peek_error()) == PEM_R_NO_START_LINE;
+            /* but warn if no CRLs have been loaded */
+            if (num_crls_loaded > 0 && eof) {
+                /* remove that error from error stack */
+                (void)ERR_get_error();
+                break;
+            }
 
-    if (!X509_STORE_add_crl(store, crl))
-    {
-        msg(M_WARN, "CRL: cannot add %s to store", crl_file);
-        goto end;
-    }
+            crypto_msg(M_WARN, "CRL: cannot read CRL from file %s", crl_file);
+            break;
+        }
 
+        if (!X509_STORE_add_crl(store, crl))
+        {
+            X509_CRL_free(crl);
+            crypto_msg(M_WARN, "CRL: cannot add %s to store", crl_file);
+            break;
+        }
+        X509_CRL_free(crl);
+        num_crls_loaded++;
+    }
+    msg(M_INFO, "CRL: loaded %d CRLs from file %s", num_crls_loaded, crl_file);
 end:
-    X509_CRL_free(crl);
     BIO_free(in);
 }