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

Message ID 20200401215052.3489613-1-wgh@torlan.ru
State Changes Requested
Headers show
Series
  • [Openvpn-devel] OpenSSL: Fix --crl-verify not loading multiple CRLs in one file
Related show

Commit Message

WGH April 1, 2020, 9:50 p.m.
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 backends 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 | 41 +++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 12 deletions(-)

Comments

Arne Schwabe April 1, 2020, 10:28 p.m. | #1
Am 01.04.20 um 23:50 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].
> 
> mbedtls crypto backends 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.

Feature-ACK but some changes to the code required.

>  {
> -    X509_CRL *crl = NULL;
> +    int i = 0;

We can use C99 style definition. The init on top is just because we just
to have C89 style

>      BIO *in = NULL;
>  
>      X509_STORE *store = SSL_CTX_get_cert_store(ssl_ctx->ctx);
> @@ -1079,21 +1079,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)
> -    {
> -        msg(M_WARN, "CRL: cannot read CRL from file %s", crl_file);
> -        goto end;
> -    }
> +    for (i = 0;; i++) {

This for loop is a bit odd. Also the use of goto with this kind of for
loop is not the best to understand. Also since i is not really an index,
I would prefer a better named variable like num_crls_loaded or something
similar.


> +        X509_CRL *crl = PEM_read_bio_X509_CRL(in, NULL, NULL, NULL);
> +        if (crl == NULL)
> +        {
> +            unsigned long err = ERR_get_error();

I think we should use ERR_peek_error here, so we do not drop the error
message if the error is something else.

> +            char buf[256];
> +
> +            if (ERR_GET_REASON(err) == PEM_R_NO_START_LINE && i > 0) {

flip condition if (i > 0 && err == ...) sounds more logical to me

> +                // PEM_R_NO_START_LINE can be considered equivalent to EOF.
> +                //
> +                // A file without any CRLs should still be considered an error,
> +                // though. Hence i > 0.

Style: OpenVPN does not use C99/C++ style comments.

Also comment does not match what the code does. This is just a warning
and not an error.

> +                goto end;
> +            }
>  
> -    if (!X509_STORE_add_crl(store, crl))
> -    {
> -        msg(M_WARN, "CRL: cannot add %s to store", crl_file);
> -        goto end;
> +            ERR_error_string_n(err, buf, sizeof(buf));
> +

> +            msg(M_WARN, "CRL: cannot read CRL from file %s: %s", crl_file, buf);

Use crypto_msg that handles the openssl error stack printing.

> +            goto end;
> +        }
> +
> +        if (!X509_STORE_add_crl(store, crl))
> +        {
> +            msg(M_WARN, "CRL: cannot add %s to store", crl_file);

Same here.

> +            X509_CRL_free(crl);
> +            goto end;
> +        }
> +        X509_CRL_free(crl);
>      }
>  
>  end:
> -    X509_CRL_free(crl);
> +    msg(M_INFO, "CRL: loaded %d CRLs from file %s", i, crl_file);
>      BIO_free(in);
>  }
>  
>
WGH April 2, 2020, 2:09 p.m. | #2
On 4/2/20 1:28 AM, Arne Schwabe wrote:
> Am 01.04.20 um 23:50 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].
>>
>> mbedtls crypto backends 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.
>
> Feature-ACK but some changes to the code required.
Thank you for the feedback! How should I send a new patch version? As a reply (with [PATCH v2]) to this thread, or as an independent message? I'm new to contributing patches through e-mail.
>> +        X509_CRL *crl = PEM_read_bio_X509_CRL(in, NULL, NULL, NULL);
>> +        if (crl == NULL)
>> +        {
>> +            unsigned long err = ERR_get_error();
>
> I think we should use ERR_peek_error here, so we do not drop the error
> message if the error is something else.

backend_tls_ctx_reload_crl doesn't return an error (as it's void), and its caller never checks OpenSSL error stack. So as this function is, I think it should handle all errors itself, and leave the error stack clear.

Of course, it would make sense to refactor this part, but that would be a different patch.

>> +                goto end;
>> +            }
>>  
>> -    if (!X509_STORE_add_crl(store, crl))
>> -    {
>> -        msg(M_WARN, "CRL: cannot add %s to store", crl_file);
>> -        goto end;
>> +            ERR_error_string_n(err, buf, sizeof(buf));
>> +
>
>> +            msg(M_WARN, "CRL: cannot read CRL from file %s: %s", crl_file, buf);
>
> Use crypto_msg that handles the openssl error stack printing.
>
>> +            goto end;
>> +        }
>> +
>> +        if (!X509_STORE_add_crl(store, crl))
>> +        {
>> +            msg(M_WARN, "CRL: cannot add %s to store", crl_file);
>
> Same here.

I can do this, but at the same time I'd like to note that output
becomes worse:

    Thu Apr  2 17:04:24 2020 Diffie-Hellman initialized with 2048 bit key
    Thu Apr  2 17:04:24 2020 OpenSSL: error:0909006C:PEM routines:get_name:no start line
    Thu Apr  2 17:04:24 2020 OpenSSL: error:0908F066:PEM routines:get_header_and_data:bad end line
    Thu Apr  2 17:04:24 2020 CRL: cannot read CRL from file combined_crl.pem
    Thu Apr  2 17:04:24 2020 CRL: loaded 1 CRLs from file combined_crl.pem
    Thu Apr  2 17:04:24 2020 Failed to extract curve from certificate (UNDEF), using secp384r1 instead

The first error comes god knows where from, and the second error is
actually related to CRL (I corrupted the END X509 CRL line on purpose).
It's hard to figure out what the errors printed by crypto_msg are
actually related to.

Compare with how it was in current edition of the patch:

    Thu Apr  2 17:07:13 2020 Diffie-Hellman initialized with 2048 bit key
    Thu Apr  2 17:07:13 2020 OpenSSL: error:0909006C:PEM routines:get_name:no start line
    Thu Apr  2 17:07:13 2020 CRL: cannot read CRL from file combined_crl.pem: error:0908F066:PEM routines:get_header_and_data:bad end line
    Thu Apr  2 17:07:13 2020 CRL: loaded 1 CRLs from file combined_crl.pem
Arne Schwabe April 2, 2020, 2:25 p.m. | #3
>> Feature-ACK but some changes to the code required.
> Thank you for the feedback! How should I send a new patch version? As a reply (with [PATCH v2]) to this thread, or as an independent message? I'm new to contributing patches through e-mail.
>>> +        X509_CRL *crl = PEM_read_bio_X509_CRL(in, NULL, NULL, NULL);
>>> +        if (crl == NULL)
>>> +        {
>>> +            unsigned long err = ERR_get_error();
>>
>> I think we should use ERR_peek_error here, so we do not drop the error
>> message if the error is something else.
> 
> backend_tls_ctx_reload_crl doesn't return an error (as it's void), and its caller never checks OpenSSL error stack. So as this function is, I think it should handle all errors itself, and leave the error stack clear.

Yes. But your patch throws that error code away. If there an error that
is not PEM_R_NO_START_LINE it will be silenty discarded. That is my
problem with using ERR_get_error there.

>>> +        }
>>> +
>>> +        if (!X509_STORE_add_crl(store, crl))
>>> +        {
>>> +            msg(M_WARN, "CRL: cannot add %s to store", crl_file);
>>
>> Same here.
> 
> I can do this, but at the same time I'd like to note that output

Yeah. I think I my comment about ERR_get vs ERR_peek was a bit
misleading. I wanted to say that you should only consume error there if
it is the header error.

> becomes worse:
> 
>     Thu Apr  2 17:04:24 2020 Diffie-Hellman initialized with 2048 bit key
>     Thu Apr  2 17:04:24 2020 OpenSSL: error:0909006C:PEM routines:get_name:no start line
>     Thu Apr  2 17:04:24 2020 OpenSSL: error:0908F066:PEM routines:get_header_and_data:bad end line
>     Thu Apr  2 17:04:24 2020 CRL: cannot read CRL from file combined_crl.pem
>     Thu Apr  2 17:04:24 2020 CRL: loaded 1 CRLs from file combined_crl.pem
>     Thu Apr  2 17:04:24 2020 Failed to extract curve from certificate (UNDEF), using secp384r1 instead
> 
> The first error comes god knows where from, and the second error is
> actually related to CRL (I corrupted the END X509 CRL line on purpose).
> It's hard to figure out what the errors printed by crypto_msg are
> actually related to.

That is a very recent regression. It should not have been there. See
this (not yet commited) fix: https://patchwork.openvpn.net/patch/1071/

After reviewing your patch and writing that fix, I also realised how
similar the two pieces of code are. You can probably almost copy and
paste the (new) tls_ctx_add_extra_certs to the crl loading function.


> Compare with how it was in current edition of the patch:
> 
>     Thu Apr  2 17:07:13 2020 Diffie-Hellman initialized with 2048 bit key
>     Thu Apr  2 17:07:13 2020 OpenSSL: error:0909006C:PEM routines:get_name:no start line
>     Thu Apr  2 17:07:13 2020 CRL: cannot read CRL from file combined_crl.pem: error:0908F066:PEM routines:get_header_and_data:bad end line
>     Thu Apr  2 17:07:13 2020 CRL: loaded 1 CRLs from file combined_crl.pem
> 


Yeah but *always* having a scary error that translates to "No more crls
found in the file" is not very user friendly since most people will not
understand that this is to be expected.

Arne
Arne Schwabe April 7, 2020, 5:40 a.m. | #4
Am 07.04.20 um 03:28 schrieb WGH:
> I think there has been some misunderstanding about the error handling in my patch.
> 
> On 4/2/20 5:25 PM, Arne Schwabe wrote:
>>> backend_tls_ctx_reload_crl doesn't return an error (as it's void), and its caller never checks OpenSSL error stack. So as this function is, I think it should handle all errors itself, and leave the error stack clear.
>> Yes. But your patch throws that error code away. If there an error that
>> is not PEM_R_NO_START_LINE it will be silenty discarded. That is my
>> problem with using ERR_get_error there.
> Please look again: the patch doesn't throw it away, it's printed with ERR_error_string_n/msg combo.
>>> Compare with how it was in current edition of the patch:
>>>
>>>     Thu Apr  2 17:07:13 2020 Diffie-Hellman initialized with 2048 bit key
>>>     Thu Apr  2 17:07:13 2020 OpenSSL: error:0909006C:PEM routines:get_name:no start line
>>>     Thu Apr  2 17:07:13 2020 CRL: cannot read CRL from file combined_crl.pem: error:0908F066:PEM routines:get_header_and_data:bad end line
>>>     Thu Apr  2 17:07:13 2020 CRL: loaded 1 CRLs from file combined_crl.pem
>>>
>> Yeah but *always* having a scary error that translates to "No more crls
>> found in the file" is not very user friendly since most people will not
>> understand that this is to be expected.
> 
> Perhaps you glanced over and didn't notice that it says "bad _end_ line"? Here I simulated a corrupt file, causing this error, to highlight the difference in how errors are printed by crypto_msg and by my ad-hoc code. My code does a better job at indicating that the error comes from the CRL file, where messages printed by crypto_msg may blend with other bogus errors and warnings, like it happened with that bogus warning you fixed in the aforementioned patch.
> 

Yeah sorry, the formatting of the patch made me believe the control flow
was different from what it really is. But this bogus error message was a
one off mistake in OpenVPN. In general printing an error before the
message isn't a big problem.


> Also, my patch doesn't print "no start line" error unless there were no valid CRLs inside the specified file. Printing a scary error in the latter case is IMHO justfified: file is either empty due to some administrative mistake (e.g. buggy updating scripts) or corrupt.
> 
> I do not insist on leaving my logging in place, though, and will replace it with crypto_msg as you suggested. After all, crypto_msg is used all over the place, and making its errors more understandable is something out of scope of this patch.
> 

I feel consistency in code is important. And crypto_msg will also print
the whole error stack instead of just the last message (which probably
does not matter in this case). Since crypto_msg is used all over the
place, its printing should be improved instead of having different error
message handling/printing all over the place. For your one-off patch it
might seem superior but for maintaining the code it is not.

Arne

Patch

diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 3f0031ff..a5502a5b 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -1038,7 +1038,7 @@  void
 backend_tls_ctx_reload_crl(struct tls_root_ctx *ssl_ctx, const char *crl_file,
                            const char *crl_inline)
 {
-    X509_CRL *crl = NULL;
+    int i = 0;
     BIO *in = NULL;
 
     X509_STORE *store = SSL_CTX_get_cert_store(ssl_ctx->ctx);
@@ -1079,21 +1079,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)
-    {
-        msg(M_WARN, "CRL: cannot read CRL from file %s", crl_file);
-        goto end;
-    }
+    for (i = 0;; i++) {
+        X509_CRL *crl = PEM_read_bio_X509_CRL(in, NULL, NULL, NULL);
+        if (crl == NULL)
+        {
+            unsigned long err = ERR_get_error();
+            char buf[256];
+
+            if (ERR_GET_REASON(err) == PEM_R_NO_START_LINE && i > 0) {
+                // PEM_R_NO_START_LINE can be considered equivalent to EOF.
+                //
+                // A file without any CRLs should still be considered an error,
+                // though. Hence i > 0.
+                goto end;
+            }
 
-    if (!X509_STORE_add_crl(store, crl))
-    {
-        msg(M_WARN, "CRL: cannot add %s to store", crl_file);
-        goto end;
+            ERR_error_string_n(err, buf, sizeof(buf));
+
+            msg(M_WARN, "CRL: cannot read CRL from file %s: %s", crl_file, buf);
+            goto end;
+        }
+
+        if (!X509_STORE_add_crl(store, crl))
+        {
+            msg(M_WARN, "CRL: cannot add %s to store", crl_file);
+            X509_CRL_free(crl);
+            goto end;
+        }
+        X509_CRL_free(crl);
     }
 
 end:
-    X509_CRL_free(crl);
+    msg(M_INFO, "CRL: loaded %d CRLs from file %s", i, crl_file);
     BIO_free(in);
 }