[Openvpn-devel,v3] Fix OpenSSL error stack handling of tls_ctx_add_extra_certs

Message ID 20200402103821.10347-1-arne@rfc2549.org
State Accepted
Headers show
Series
  • [Openvpn-devel,v3] Fix OpenSSL error stack handling of tls_ctx_add_extra_certs
Related show

Commit Message

Arne Schwabe April 2, 2020, 10:38 a.m.
Commit f67efa94 exposed that tls_ctx_add_extra_certs will always leave
an error of PEM_R_NO_START_LINE on the stack that will printed the next
time that the error is printed.

Fix this by discarding this error. Also clean up the logic to report
real error on other errors and also the no start line error if no
certificate can be found at all and it is required (--extra-certs
config option)

Patch V2: fix optional flag was flipped betwen --cert and --extra-certs
Patch V3: Make logic more easy to follow, no functional changes

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/ssl_openssl.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

Comments

Antonio Quartulli April 2, 2020, 1:15 p.m. | #1
Hi,

On 02/04/2020 12:38, Arne Schwabe wrote:
> Commit f67efa94 exposed that tls_ctx_add_extra_certs will always leave
> an error of PEM_R_NO_START_LINE on the stack that will printed the next
> time that the error is printed.
> 
> Fix this by discarding this error. Also clean up the logic to report
> real error on other errors and also the no start line error if no
> certificate can be found at all and it is required (--extra-certs
> config option)
> 
> Patch V2: fix optional flag was flipped betwen --cert and --extra-certs
> Patch V3: Make logic more easy to follow, no functional changes
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  src/openvpn/ssl_openssl.c | 30 +++++++++++++++++++++---------
>  1 file changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index 3f0031ff..1731474d 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -881,24 +881,36 @@ tls_ctx_load_cryptoapi(struct tls_root_ctx *ctx, const char *cryptoapi_cert)
>  #endif /* ENABLE_CRYPTOAPI */
>  
>  static void
> -tls_ctx_add_extra_certs(struct tls_root_ctx *ctx, BIO *bio)
> +tls_ctx_add_extra_certs(struct tls_root_ctx *ctx, BIO *bio, bool optional)
>  {
>      X509 *cert;
> -    for (;; )
> +    while (true)
>      {
>          cert = NULL;
> -        if (!PEM_read_bio_X509(bio, &cert, NULL, NULL)) /* takes ownership of cert */
> -        {
> -            break;
> -        }
> -        if (!cert)
> +        if (!PEM_read_bio_X509(bio, &cert, NULL, NULL))
>          {
> +            /*  Error indicates that no certificate is found in the buffer.

I would substitute "Error" with "PEM_R_NO_START_LINE" for clarity

> +             *  If loading more certificates is optional, break without
> +             *  raising an error */
> +

This empty line should be removed

> +            if (optional
> +                && ERR_GET_REASON(ERR_peek_error()) == PEM_R_NO_START_LINE)
> +            {
> +                /* remove that error from error stack */
> +                (void)ERR_get_error();
> +                break;
> +            }
> +
> +            /* Otherwise, bail out with error */
>              crypto_msg(M_FATAL, "Error reading extra certificate");
>          }
> +        /* takes ownership of cert like a set1 method */
>          if (SSL_CTX_add_extra_chain_cert(ctx->ctx, cert) != 1)
>          {
>              crypto_msg(M_FATAL, "Error adding extra certificate");
>          }
> +        /* We loaded at least one certificate, so loading more is optional */
> +        optional = true;
>      }
>  }
>  
> @@ -942,7 +954,7 @@ tls_ctx_load_cert_file(struct tls_root_ctx *ctx, const char *cert_file,
>      ret = SSL_CTX_use_certificate(ctx->ctx, x);
>      if (ret)
>      {
> -        tls_ctx_add_extra_certs(ctx, in);
> +        tls_ctx_add_extra_certs(ctx, in, true);
>      }
>  
>  end:
> @@ -1663,7 +1675,7 @@ tls_ctx_load_extra_certs(struct tls_root_ctx *ctx, const char *extra_certs_file,
>      }
>      else
>      {
> -        tls_ctx_add_extra_certs(ctx, in);
> +        tls_ctx_add_extra_certs(ctx, in, false);
>      }
>  
>      BIO_free(in);
> 


The rest looks good!

I reviewed the code and tested it with different combinations of files
provided to --cert and --extra-certs.

It all works as expected:
- an empty extra-certs file leads to a critical failure with an OpenSSL
error printed to screen;
- empty cert file leads to the same as above;
- proper cert and extra-certs files result to no error printed to screen
and everything works


Acked-by: Antonio Quartulli <a@unstable.cc>


Cheers,
Antonio Quartulli April 2, 2020, 1:21 p.m. | #2
Hi,

On 02/04/2020 15:15, Antonio Quartulli wrote:
> Hi,
> 
> On 02/04/2020 12:38, Arne Schwabe wrote:
>> Commit f67efa94 exposed that tls_ctx_add_extra_certs will always leave
>> an error of PEM_R_NO_START_LINE on the stack that will printed the next
>> time that the error is printed.
>>
>> Fix this by discarding this error. Also clean up the logic to report
>> real error on other errors and also the no start line error if no
>> certificate can be found at all and it is required (--extra-certs
>> config option)
>>
>> Patch V2: fix optional flag was flipped betwen --cert and --extra-certs
>> Patch V3: Make logic more easy to follow, no functional changes
>>
>> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
>> ---
>>  src/openvpn/ssl_openssl.c | 30 +++++++++++++++++++++---------
>>  1 file changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
>> index 3f0031ff..1731474d 100644
>> --- a/src/openvpn/ssl_openssl.c
>> +++ b/src/openvpn/ssl_openssl.c
>> @@ -881,24 +881,36 @@ tls_ctx_load_cryptoapi(struct tls_root_ctx *ctx, const char *cryptoapi_cert)
>>  #endif /* ENABLE_CRYPTOAPI */
>>  
>>  static void
>> -tls_ctx_add_extra_certs(struct tls_root_ctx *ctx, BIO *bio)
>> +tls_ctx_add_extra_certs(struct tls_root_ctx *ctx, BIO *bio, bool optional)
>>  {
>>      X509 *cert;
>> -    for (;; )
>> +    while (true)
>>      {
>>          cert = NULL;
>> -        if (!PEM_read_bio_X509(bio, &cert, NULL, NULL)) /* takes ownership of cert */
>> -        {
>> -            break;
>> -        }
>> -        if (!cert)
>> +        if (!PEM_read_bio_X509(bio, &cert, NULL, NULL))
>>          {
>> +            /*  Error indicates that no certificate is found in the buffer.

there is also a typ0 in the line above: certificateS -> certificate.
Gert Doering April 2, 2020, 6:55 p.m. | #3
Your patch has been applied to the master and release/2.4 branch (bugfix).

Looked at code and IRC discussion, makes all sense to me.  Tested a 
2.4 client build on linux and freebsd, mbedtls and openssl (build + 
t_client), just for extra sanity checking.

commit 3608d890583549dbdbefc40ed41bf617fa518aa1 (master)
commit 15bc476f80e66cee8e2bfba96879ef32e01380b5 (release/2.4)
Author: Arne Schwabe
Date:   Thu Apr 2 12:38:21 2020 +0200

     Fix OpenSSL error stack handling of tls_ctx_add_extra_certs

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20200402103821.10347-1-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19685.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 3f0031ff..1731474d 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -881,24 +881,36 @@  tls_ctx_load_cryptoapi(struct tls_root_ctx *ctx, const char *cryptoapi_cert)
 #endif /* ENABLE_CRYPTOAPI */
 
 static void
-tls_ctx_add_extra_certs(struct tls_root_ctx *ctx, BIO *bio)
+tls_ctx_add_extra_certs(struct tls_root_ctx *ctx, BIO *bio, bool optional)
 {
     X509 *cert;
-    for (;; )
+    while (true)
     {
         cert = NULL;
-        if (!PEM_read_bio_X509(bio, &cert, NULL, NULL)) /* takes ownership of cert */
-        {
-            break;
-        }
-        if (!cert)
+        if (!PEM_read_bio_X509(bio, &cert, NULL, NULL))
         {
+            /*  Error indicates that no certificate is found in the buffer.
+             *  If loading more certificates is optional, break without
+             *  raising an error */
+
+            if (optional
+                && ERR_GET_REASON(ERR_peek_error()) == PEM_R_NO_START_LINE)
+            {
+                /* remove that error from error stack */
+                (void)ERR_get_error();
+                break;
+            }
+
+            /* Otherwise, bail out with error */
             crypto_msg(M_FATAL, "Error reading extra certificate");
         }
+        /* takes ownership of cert like a set1 method */
         if (SSL_CTX_add_extra_chain_cert(ctx->ctx, cert) != 1)
         {
             crypto_msg(M_FATAL, "Error adding extra certificate");
         }
+        /* We loaded at least one certificate, so loading more is optional */
+        optional = true;
     }
 }
 
@@ -942,7 +954,7 @@  tls_ctx_load_cert_file(struct tls_root_ctx *ctx, const char *cert_file,
     ret = SSL_CTX_use_certificate(ctx->ctx, x);
     if (ret)
     {
-        tls_ctx_add_extra_certs(ctx, in);
+        tls_ctx_add_extra_certs(ctx, in, true);
     }
 
 end:
@@ -1663,7 +1675,7 @@  tls_ctx_load_extra_certs(struct tls_root_ctx *ctx, const char *extra_certs_file,
     }
     else
     {
-        tls_ctx_add_extra_certs(ctx, in);
+        tls_ctx_add_extra_certs(ctx, in, false);
     }
 
     BIO_free(in);