| Message ID | 20210810061644.20353-1-maximilian.fillinger@foxcrypto.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel] Replace deprecated mbedtls DRBG update function | expand |
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
> 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
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
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>
> 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!
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,
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>
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
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 --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)); } }
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(-)