diff mbox series

[Openvpn-devel] mbedtls: don't use API deprecated in mbed 2.7

Message ID 1518006166-14285-1-git-send-email-steffan.karger@fox-it.com
State Accepted
Headers
Series [Openvpn-devel] mbedtls: don't use API deprecated in mbed 2.7 |

Commit Message

Steffan Karger Feb. 7, 2018, 1:22 a.m. UTC 
  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(-)

Comments

Antonio Quartulli Feb. 22, 2018, 10:17 p.m. UTC | #1 
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,
Steffan Karger Feb. 23, 2018, 1:02 a.m. UTC | #2 
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
Antonio Quartulli Feb. 23, 2018, 1:09 a.m. UTC | #3 
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,
Gert Doering Feb. 26, 2018, 7:38 a.m. UTC | #4 
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 mbox series

Patch

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);