mbedtls: correctly check return value in pkcs11_certificate_dn()
PATCH 1/1 mbedtls: correctly check return value in pkcs11_certificate_dn()
mbedtls_x509_dn_gets() would not always return -1 error, which could cause us to incorrectly continue after the function call failed. To fix this, just call our own x509_get_subject(), which does all the neccesary error checking correctly.
pkcs11_certificate_dn() is only called by show_pkcs11_ids(), to list the certificates on the pkcs11 token. Therefor, this mistake did not have a security impact.
This issue was found by Quarkslab during the OSTIF-founded security audit (issue 5.3).
--- src/openvpn/pkcs11_mbedtls.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
src/openvpn/pkcs11_mbedtls.c +2 -5
@@ -39,6 +39,7 @@#include "errlevel.h" #include "pkcs11_backend.h" +#include "ssl_verify_backend.h" #include <mbedtls/pkcs11.h> #include <mbedtls/x509.h>
@@ -82,8 +83,6 @@ char * pkcs11_certificate_dn(pkcs11h_certificate_t cert, struct gc_arena *gc) { char *ret = NULL; -char dn[1024 ] = {0 }; -mbedtls_x509_crt mbed_crt = {0 };if (mbedtls_pkcs11_x509_cert_bind (&mbed_crt, cert))
@@ -92,14 +91,12 @@ pkcs11_certificate_dn(pkcs11h_certificate_t cert, struct gc_arena *gc) goto cleanup; } -if (-1 ==mbedtls_x509_dn_gets (dn,sizeof (dn), &mbed_crt.subject)) +if (!(ret =x509_get_subject (&mbed_crt, gc))) {msg (M_FATAL ,"PKCS#11: mbed TLS cannot parse subject" ); goto cleanup; } - ret =string_alloc (dn, gc); - cleanup:mbedtls_x509_crt_free (&mbed_crt);
Signature · 1 line
2.7.4
Hi,
src/openvpn/pkcs11_mbedtls.c pkcs11_certificate_dn(pkcs11h_certificate_t cert, struct gc_arena *gc) · line 102
Quoted 12 lines · Steffan Karger
goto cleanup; }src/openvpn/pkcs11_mbedtls.c +1
-3-if (-1 ==mbedtls_x509_dn_gets (dn,sizeof (dn), &mbed_crt.subject)) +if (!(ret =x509_get_subject (&mbed_crt, gc))) {msg (M_FATAL ,"PKCS#11: mbed TLS cannot parse subject" ); goto cleanup; } - ret =string_alloc (dn, gc); -
ACK.
x509_get_subject() does exactly the same thing, just with error handling for the case "mbedtls_x509_dn_gets() returns 0". So - less code, and error case handled :-)
(Note: cleanup patch expected, to remove the "goto cleanup" calls after a msg(M_FATAL, ...) line which is guaranteed to never return)
gert
Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany [email protected]...
fax: +49-89-35655025 [email protected]...-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Your patch has been applied to the following branches
commit 423bb16e8a8fe22a907f469074a25533208fa0bc (master) commit 1ebd3ade5f3fcdefa40790f2e9d16c473bac370a (release/2.4)
Author: Steffan Karger Date: Tue May 9 10:12:43 2017 +0200
mbedtls: correctly check return value in pkcs11_certificate_dn()
Signed-off-by: Steffan Karger <[email protected]...>
Acked-by: Gert Doering <[email protected]...>
Message-Id: <[email protected]...>
URL: https://www.mail-archive.com/[email protected]/msg14591.html
Signed-off-by: David Sommerseth <[email protected]...>- --
kind regards,
David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
iQIcBAEBCAAGBQJZEZ5dAAoJEIbPlEyWcf3yaV8P/A/PASUpOs7zxM52wIErQUZ9 H+Wos+U7URoJpnFIwP1J0X6n3bn/OJML4Vu7c/v+ZpOA8gQ4WS4cNPG3iLipgee8 Otpgxv7nJZM47DYUC//plxjFKxhpTQF4ogmTY3LoB/1yFJdxAy6I9GWWdIZvCxBx ttpiQt78XzVAHHIcqzH7zAf926by1aoPUJaQrHwG1DfV4+CG6LXd7WWLg3y5+/CI sO8MxLIomIHhogCJDIY8ISbqgXZ3VJsf3/l/NjqpPx3OWS3A4CgSriWDzI4aKtH5 jYilBu7NmQQnm8yQf4kQQ7XbMsAZIAU+vRkXvJLy9p0Z3z8QWB9m4+Ql8m39Yd2c R0yr+0YtcSagCc6ipCKatiNEWBLkRQqRRwGsL+dSkVLVaOUmg3k//pBWERnS3JrR hRdM6nsoyM//solAO0oYP9DXn3vCgJAXiSjceWI9O4IH2MGD5OArOKLZE0XVlVIa Cl18A7tAC0u5H8BXo88NF478F8RWbwDE0VG0Eu2cu8V/jHRemk4iwlKDdjcRpSaO s982RfTleJbekm+eSB16lNwMfjeK6vMH9l4RxHYs1nAhOzgOyB06Et17K31+R8cj z4ZSjhVEmZxA6e3lQVOe6I6RbGz0DUKllVGWUHSmFN2iOegDXoZIzbo40GYw9u7V ugOT5E0twiES4NsqLP+q
=gu5r
-----END PGP SIGNATURE-----