| 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 | expand |
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); > } > >
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
>> 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
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
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); }
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(-)