On 07/05/17 13:01, Steffan Karger wrote:
> If keyUsage was only required to be present, but no specific value was
> required, we would omit to free the extracted string. This happens as of
> 2.4.1, if --remote-cert-tls is used. In that case we leak a bit of
> memory on each TLS (re)negotiation.
>
> Signed-off-by: Steffan Karger <stef...@karger.me>
> ---
> Changes.rst | 9 +++++++++
> src/openvpn/ssl_verify_openssl.c | 1 +
> 2 files changed, 10 insertions(+)
>
> diff --git a/Changes.rst b/Changes.rst
> index c1583b3..3dba7e0 100644
> --- a/Changes.rst
> +++ b/Changes.rst
> @@ -318,3 +318,12 @@ Version 2.4.1
> ``--remote-cert-tls`` uses the far more common keyUsage and
> extendedKeyUsage
> extension instead. Make sure your certificates carry these to be able to
> use ``--remote-cert-tls``.
> +
> +
> +Version 2.4.2
> +=============
> +
> +Bugfixes
> +--------
> +- Fix memory leak introduced in 2.4.1: if --remote-cert-tls is used, we
> leaked
> + some memory on each TLS (re)negotiation.
> diff --git a/src/openvpn/ssl_verify_openssl.c
> b/src/openvpn/ssl_verify_openssl.c
> index 54eadbd..337729c 100644
> --- a/src/openvpn/ssl_verify_openssl.c
> +++ b/src/openvpn/ssl_verify_openssl.c
> @@ -599,6 +599,7 @@ x509_verify_cert_ku(X509 *x509, const unsigned *const
> expected_ku,
> if (expected_ku[0] == OPENVPN_KU_REQUIRED)
> {
> /* Extension required, value checked by TLS library */
> + ASN1_BIT_STRING_free(ku);
> return SUCCESS;
> }
>
Just some nit-pick from me. Below is a larger fragment of this function
(I've done some word-wrapping to make it slim enough for the mail).
--------------------------------------------------------------------
result_t
x509_verify_cert_ku(X509 *x509, const unsigned *const expected_ku,
int expected_len)
{
ASN1_BIT_STRING *ku = X509_get_ext_d2i(x509, NID_key_usage,
NULL, NULL);
if (ku == NULL)
{
msg(D_TLS_ERRORS, "Certificate does not have key "
"usage extension");
return FAILURE;
}
if (expected_ku[0] == OPENVPN_KU_REQUIRED)
{
/* Extension required, value checked by TLS library */
ASN1_BIT_STRING_free(ku); /** new line **/
return SUCCESS;
}
unsigned nku = 0;
for (size_t i = 0; i < 8; i++)
--------------------------------------------------------------------
Wouldn't it make more sense to rather move the
`if (expected_ku[0] == OPENVPN_KU_REQUIRED)` block above the
`ASN1_BIT_STRING *ku` declaration? Like this:
--------------------------------------------------------------------
result_t
x509_verify_cert_ku(X509 *x509, const unsigned *const expected_ku,
int expected_len)
{
if (expected_ku[0] == OPENVPN_KU_REQUIRED)
{
/* Extension required, value checked by TLS library */
return SUCCESS;
}
ASN1_BIT_STRING *ku = X509_get_ext_d2i(x509, NID_key_usage,
NULL, NULL);
if (ku == NULL)
{
msg(D_TLS_ERRORS, "Certificate does not have key "
"usage extension");
return FAILURE;
}
unsigned nku = 0;
for (size_t i = 0; i < 8; i++)
--------------------------------------------------------------------
Or is the implicit check that ku != NULL important when checking
OPENVPN_KU_REQUIRED ?
Except of that, it looks fine and I'm willing to ACK it.
--
kind regards,
David Sommerseth
OpenVPN Technologies, Inc
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
