| Message ID | 1518006166-14285-1-git-send-email-steffan.karger@fox-it.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel] mbedtls: don't use API deprecated in mbed 2.7 | expand |
Hi, On 07/02/18 20:22, Steffan Karger wrote: > - mbedtls_sha256(cert->tbs.p, cert->tbs.len, sha256_hash, false); > + if (0 != md_full(sha256_kt, cert->tbs.p, cert->tbs.len, sha256_hash)) Why not using mbedtls_sha256_ret() since we are already in mbedtls-specific code here? Any advantage in using a wrapper for another wrapper? :-P (mbedtls_sha256_ret() is also the suggested replacement for mbedtls_sha256()) Moreover, SHA256 is statically selected, therefore using mbedtls_sha256_ret() would also avoid the md_kt_t local variable. > + { > + msg(M_WARN, "WARNING: failed to personalise random"); > + } > + Since we now have a reason for the failure, may it make sense to print a proper description based on the return value? (even though I think mbedtls_sha256_ret() can't really return something different from 0) > if (0 != memcmp(old_sha256_hash, sha256_hash, sizeof(sha256_hash))) > { > mbedtls_ctr_drbg_update(cd_ctx, sha256_hash, 32); > Cheers,
Hi, Thanks for reviewing! On 23-02-18 10:17, Antonio Quartulli wrote: > On 07/02/18 20:22, Steffan Karger wrote: >> - mbedtls_sha256(cert->tbs.p, cert->tbs.len, sha256_hash, false); >> + if (0 != md_full(sha256_kt, cert->tbs.p, cert->tbs.len, sha256_hash)) > > Why not using mbedtls_sha256_ret() since we are already in > mbedtls-specific code here? > Any advantage in using a wrapper for another wrapper? :-P > > (mbedtls_sha256_ret() is also the suggested replacement for > mbedtls_sha256()) > > > Moreover, SHA256 is statically selected, therefore using > mbedtls_sha256_ret() would also avoid the md_kt_t local variable. This was the only place in the code where we took a (direct) dependency on functions from sha256.h. By using our internal wrapper, I could also do: @@ -60,7 +60,6 @@ #include <mbedtls/oid.h> #include <mbedtls/pem.h> -#include <mbedtls/sha256.h> Since reducing dependencies usually reduces maintenance burden, I prefer this solution. >> + { >> + msg(M_WARN, "WARNING: failed to personalise random"); >> + } >> + > > Since we now have a reason for the failure, may it make sense to print a > proper description based on the return value? (even though I think > mbedtls_sha256_ret() can't really return something different from 0) md_full returns true/false, but you're right we can improve error reporting. Are you satisfied if I add an mbed_ok() wrapper *inside* md_full() to print the internal mbed error? I think that should be sufficient information. -Steffan ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On 23/02/18 20:02, Steffan Karger wrote: > Since reducing dependencies usually reduces maintenance burden, I prefer > this solution. You're right here. Ok, let's keep md_full() then > >>> + { >>> + msg(M_WARN, "WARNING: failed to personalise random"); >>> + } >>> + >> >> Since we now have a reason for the failure, may it make sense to print a >> proper description based on the return value? (even though I think >> mbedtls_sha256_ret() can't really return something different from 0) > > md_full returns true/false, but you're right we can improve error > reporting. Are you satisfied if I add an mbed_ok() wrapper *inside* > md_full() to print the internal mbed error? I think that should be > sufficient information. Yeah, that would be nice. Thanks I guess this patch fine as it is then. The mbed_ok() can be introduced with another patch. Acked-by: Antonio Quartulli <a@unstable.cc> Cheers,
Your patch has been applied to the master and release/2.4 branch.
(release/2.4 due to "long term compatibility" reasoning)
Smoke-tested 2.4 vs. mbedTLS 2.7 on Gentoo ("works").
commit f22e89bd2311d3cab511e574746c6f82f1fa1a54 (master)
commit 7c913600105505bd7f539d6b4206d358b6811943 (release/2.4)
Author: Steffan Karger
Date: Wed Feb 7 13:22:46 2018 +0100
mbedtls: don't use API deprecated in mbed 2.7
Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
Acked-by: Antonio Quartulli <a@unstable.cc>
Message-Id: <1518006166-14285-1-git-send-email-steffan.karger@fox-it.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg16445.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c index 2a1215d..3906cd5 100644 --- a/src/openvpn/ssl_mbedtls.c +++ b/src/openvpn/ssl_mbedtls.c @@ -60,7 +60,6 @@ #include <mbedtls/oid.h> #include <mbedtls/pem.h> -#include <mbedtls/sha256.h> static const mbedtls_x509_crt_profile openvpn_x509_crt_profile_legacy = { @@ -851,9 +850,14 @@ tls_ctx_personalise_random(struct tls_root_ctx *ctx) if (NULL != ctx->crt_chain) { + const md_kt_t *sha256_kt = md_kt_get("SHA256"); mbedtls_x509_crt *cert = ctx->crt_chain; - mbedtls_sha256(cert->tbs.p, cert->tbs.len, sha256_hash, false); + if (0 != md_full(sha256_kt, cert->tbs.p, cert->tbs.len, sha256_hash)) + { + msg(M_WARN, "WARNING: failed to personalise random"); + } + if (0 != memcmp(old_sha256_hash, sha256_hash, sizeof(sha256_hash))) { mbedtls_ctr_drbg_update(cd_ctx, sha256_hash, 32);
The void-returning mbedtls_sha256() was deprecated in mbed TLS 2.7. Use our own md_full() abstraction instead. (The new function can theoretically fail, but only in case of highly unlikely digest function failures. The personalisation on random using the certificate is a best-effort measure, so we simply log a warning and skip the personalisation if such highly unlikely errors occur.) Signed-off-by: Steffan Karger <steffan.karger@fox-it.com> --- src/openvpn/ssl_mbedtls.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)