diff mbox series

[Openvpn-devel] Replace deprecated mbedtls DRBG update function

Message ID 20210810061644.20353-1-maximilian.fillinger@foxcrypto.com
State Accepted
Headers
Series [Openvpn-devel] Replace deprecated mbedtls DRBG update function |

Commit Message

Maximilian Fillinger Aug. 9, 2021, 8:16 p.m. UTC 
  The function mbedtls_ctr_drbg_update is deprecated as of mbedtls 2.16
and is superseded by mbedtls_ctr_drbg_update_ret, which returns an error
code. This commit replaces the call to the deprecated function with the
new one and logs a warning in case of an error.

For older versions of mbedtls, we add a compatibility function that runs
mbedtls_ctr_drbg_update and returns 0.

Signed-off-by: Max Fillinger <maximilian.fillinger@foxcrypto.com>
---
 src/openvpn/ssl_mbedtls.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

Comments

Arne Schwabe Aug. 10, 2021, 12:11 a.m. UTC | #1 
Am 10.08.21 um 08:16 schrieb Max Fillinger:
> +#if MBEDTLS_VERSION_NUMBER < 0x02100000

Is that really 2.16? Looking at the API doc
(https://tls.mbed.org/api/version_8h.html#adb4f54ebb33fd1a25e2c4d4480cf4936)
it sounds like there should be a 16 in that number.

Arne
Maximilian Fillinger Aug. 10, 2021, 12:17 a.m. UTC | #2 
> From: Arne Schwabe [mailto:arne@rfc2549.org]
> Sent: dinsdag 10 augustus 2021 12:12
> To: Maximilian Fillinger <maximilian.fillinger@foxcrypto.com>; openvpn-
> devel@lists.sourceforge.net
> Subject: Re: [Openvpn-devel] [PATCH] Replace deprecated mbedtls DRBG
> update function
> 
> Am 10.08.21 um 08:16 schrieb Max Fillinger:
> > +#if MBEDTLS_VERSION_NUMBER < 0x02100000
> 
> Is that really 2.16? Looking at the API doc
> (https://tls.mbed.org/api/version_8h.html#adb4f54ebb33fd1a25e2c4d4480cf4
> 936)
> it sounds like there should be a 16 in that number.

It's a hexadecimal 16. Version 2.26.0 for example does

#define MBEDTLS_VERSION_NUMBER         0x021A0000
Arne Schwabe Aug. 10, 2021, 12:23 a.m. UTC | #3 
Am 10.08.21 um 12:17 schrieb Maximilian Fillinger:
>> From: Arne Schwabe [mailto:arne@rfc2549.org]
>> Sent: dinsdag 10 augustus 2021 12:12
>> To: Maximilian Fillinger <maximilian.fillinger@foxcrypto.com>; openvpn-
>> devel@lists.sourceforge.net
>> Subject: Re: [Openvpn-devel] [PATCH] Replace deprecated mbedtls DRBG
>> update function
>>
>> Am 10.08.21 um 08:16 schrieb Max Fillinger:
>>> +#if MBEDTLS_VERSION_NUMBER < 0x02100000
>>
>> Is that really 2.16? Looking at the API doc
>> (https://tls.mbed.org/api/version_8h.html#adb4f54ebb33fd1a25e2c4d4480cf4
>> 936)
>> it sounds like there should be a 16 in that number.
> 
> It's a hexadecimal 16. Version 2.26.0 for example does
> 
> #define MBEDTLS_VERSION_NUMBER         0x021A0000
> 
Oh well that is confusing. Thanks for clearing that up.

Arne
Arne Schwabe Aug. 10, 2021, 2:44 a.m. UTC | #4 
Am 10.08.21 um 08:16 schrieb Max Fillinger:
> The function mbedtls_ctr_drbg_update is deprecated as of mbedtls 2.16
> and is superseded by mbedtls_ctr_drbg_update_ret, which returns an error
> code. This commit replaces the call to the deprecated function with the
> new one and logs a warning in case of an error.
> 
> For older versions of mbedtls, we add a compatibility function that runs
> mbedtls_ctr_drbg_update and returns 0.
> 
> Signed-off-by: Max Fillinger <maximilian.fillinger@foxcrypto.com>
> ---

Normally we have patch v2 here and also a patch v2 in the subject (use
-v 2 when doing git format-patch) but for this small patch it is not a
problem.

>  src/openvpn/ssl_mbedtls.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
> index 265ea36f..1853335e 100644
> --- a/src/openvpn/ssl_mbedtls.c
> +++ b/src/openvpn/ssl_mbedtls.c
> @@ -62,6 +62,21 @@
>  #include <mbedtls/oid.h>
>  #include <mbedtls/pem.h>
>  
> +/**
> + * Compatibility: mbedtls_ctr_drbg_update was deprecated in mbedtls 2.16 and
> + * replaced with mbedtls_ctr_drbg_update_ret, which returns an error code.
> + * For older versions, we call mbedtls_ctr_drbg_update and return 0 (success).
> + */
> +#if MBEDTLS_VERSION_NUMBER < 0x02100000
> +static int mbedtls_ctr_drbg_update_ret(mbedtls_ctr_drbg_context *ctx,
> +                                       const unsigned char *additional,
> +                                       size_t add_len)
> +{
> +    mbedtls_ctr_drbg_update(ctx, additional, add_len);
> +    return 0;
> +}
> +#endif
> +
>  static const mbedtls_x509_crt_profile openvpn_x509_crt_profile_legacy =
>  {
>      /* Hashes from SHA-1 and above */
> @@ -950,7 +965,10 @@ tls_ctx_personalise_random(struct tls_root_ctx *ctx)
>  
>          if (0 != memcmp(old_sha256_hash, sha256_hash, sizeof(sha256_hash)))
>          {
> -            mbedtls_ctr_drbg_update(cd_ctx, sha256_hash, 32);
> +            if (!mbed_ok(mbedtls_ctr_drbg_update_ret(cd_ctx, sha256_hash, 32)))
> +            {
> +                msg(M_WARN, "WARNING: failed to personalise random, could not update CTR_DRBG");
> +            }
>              memcpy(old_sha256_hash, sha256_hash, sizeof(old_sha256_hash));
>          }
>      }
> 

Apart from the fact that we might want to abort (M_FATAL) if this fails
instead basically ignoring the error and just log it, the change is
fine. Considering the return status was ignored before, this patch is
otherwise good. But failing also does not have any really bad impact... So:

Acked-By: Arne Schwabe <arne@rfc2549.org>
Maximilian Fillinger Aug. 10, 2021, 2:51 a.m. UTC | #5 
> Normally we have patch v2 here and also a patch v2 in the subject (use
> -v 2 when doing git format-patch) but for this small patch it is not a
> problem.

I'll keep it in mind for next time!

> Apart from the fact that we might want to abort (M_FATAL) if this fails
> instead basically ignoring the error and just log it, the change is
> fine. Considering the return status was ignored before, this patch is
> otherwise good. But failing also does not have any really bad impact...

I was thinking about making it fatal, but there's another place where
personalizing random can fail that also only gives a warning. So that's what
I went with. (I wouldn't be opposed to making them both fatal, though.)

Anyway, thanks for the ACK!
Antonio Quartulli Aug. 10, 2021, 2:55 a.m. UTC | #6 
On 10/08/2021 14:51, Maximilian Fillinger wrote:
>> Normally we have patch v2 here and also a patch v2 in the subject (use
>> -v 2 when doing git format-patch) but for this small patch it is not a
>> problem.
> 
> I'll keep it in mind for next time!
> 
>> Apart from the fact that we might want to abort (M_FATAL) if this fails
>> instead basically ignoring the error and just log it, the change is
>> fine. Considering the return status was ignored before, this patch is
>> otherwise good. But failing also does not have any really bad impact...
> 
> I was thinking about making it fatal, but there's another place where
> personalizing random can fail that also only gives a warning. So that's what
> I went with. (I wouldn't be opposed to making them both fatal, though.)
> 

IMHO it's ok to keep WARN for now because, like pointed out, other parts
of the code path just warn in case of issues.

Just a few line above Max's change there is another failure treated with
a WARN only.


Therefore I'd stick to WARN unless a complete revamp is performed on the
entire codepath.

Regards,
Antonio Quartulli Aug. 11, 2021, 2:22 a.m. UTC | #7 
Hi,

On 10/08/2021 08:16, Max Fillinger wrote:
> The function mbedtls_ctr_drbg_update is deprecated as of mbedtls 2.16
> and is superseded by mbedtls_ctr_drbg_update_ret, which returns an error
> code. This commit replaces the call to the deprecated function with the
> new one and logs a warning in case of an error.
> 
> For older versions of mbedtls, we add a compatibility function that runs
> mbedtls_ctr_drbg_update and returns 0.
> 
> Signed-off-by: Max Fillinger <maximilian.fillinger@foxcrypto.com>

I also gave this patch a go and compiled it against:
* mbedTLS 2.26.0
* mbedTLS 2.20.0
* mbedTLS 2.10.0

all good.
Patch looks good to me too.

Acked-by: Antonio Quartulli <antonio@openvpn.net>
Gert Doering Aug. 11, 2021, 2:41 a.m. UTC | #8 
I have only done a cursory look ("it's like v1 with the compat function"),
and done a minimal test compile (Linux, mbedTLS 2.26.0, and FreeBSD,
mbedTLS 2.16.11) + t_client test run.  No surprises.

Your patch has been applied to the master branch.

commit b99fa3fd4fc41862354be709edb9877aae3e138c
Author: Max Fillinger
Date:   Tue Aug 10 08:16:44 2021 +0200

     Replace deprecated mbedtls DRBG update function

     Signed-off-by: Max Fillinger <maximilian.fillinger@foxcrypto.com>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20210810061644.20353-1-maximilian.fillinger@foxcrypto.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22711.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Gert Doering Aug. 11, 2021, 6:55 a.m. UTC | #9 
Hi,

On Tue, Aug 10, 2021 at 08:16:44AM +0200, Max Fillinger wrote:
> The function mbedtls_ctr_drbg_update is deprecated as of mbedtls 2.16
> and is superseded by mbedtls_ctr_drbg_update_ret, which returns an error
> code. This commit replaces the call to the deprecated function with the
> new one and logs a warning in case of an error.
> 
> For older versions of mbedtls, we add a compatibility function that runs
> mbedtls_ctr_drbg_update and returns 0.

For the record, this breaks 2.14.*1* - which does have this function,
and the static-vs-non-static prototyping causes a compile error.

So, followup patch needed.  

2.14.0 does not have it, and from what I understand, 2.15.x does not
have it either.  But maybe we do not care for 2.15

gert
diff mbox series

Patch

diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
index 265ea36f..1853335e 100644
--- a/src/openvpn/ssl_mbedtls.c
+++ b/src/openvpn/ssl_mbedtls.c
@@ -62,6 +62,21 @@ 
 #include <mbedtls/oid.h>
 #include <mbedtls/pem.h>
 
+/**
+ * Compatibility: mbedtls_ctr_drbg_update was deprecated in mbedtls 2.16 and
+ * replaced with mbedtls_ctr_drbg_update_ret, which returns an error code.
+ * For older versions, we call mbedtls_ctr_drbg_update and return 0 (success).
+ */
+#if MBEDTLS_VERSION_NUMBER < 0x02100000
+static int mbedtls_ctr_drbg_update_ret(mbedtls_ctr_drbg_context *ctx,
+                                       const unsigned char *additional,
+                                       size_t add_len)
+{
+    mbedtls_ctr_drbg_update(ctx, additional, add_len);
+    return 0;
+}
+#endif
+
 static const mbedtls_x509_crt_profile openvpn_x509_crt_profile_legacy =
 {
     /* Hashes from SHA-1 and above */
@@ -950,7 +965,10 @@  tls_ctx_personalise_random(struct tls_root_ctx *ctx)
 
         if (0 != memcmp(old_sha256_hash, sha256_hash, sizeof(sha256_hash)))
         {
-            mbedtls_ctr_drbg_update(cd_ctx, sha256_hash, 32);
+            if (!mbed_ok(mbedtls_ctr_drbg_update_ret(cd_ctx, sha256_hash, 32)))
+            {
+                msg(M_WARN, "WARNING: failed to personalise random, could not update CTR_DRBG");
+            }
             memcpy(old_sha256_hash, sha256_hash, sizeof(old_sha256_hash));
         }
     }