Fix memory leak in x509_verify_cert_ku()

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 <[email protected]...>

---
 Changes.rst                      | 9 +++++++++
 src/openvpn/ssl_verify_openssl.c | 1 +
 2 files changed, 10 insertions(+)

Changes.rst +9 -0

index c1583b3..3dba7e0 100644
@@ -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.

src/openvpn/ssl_verify_openssl.c +0 -0

index 54eadbd..337729c 100644

On 07/05/17 13:01, Steffan Karger wrote:
--------------------------------------------------------------------
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 ?