OpenSSL 3.2.0: dane_tlsa_add(): tlsa_free() problem?

[Viktor Dukhovni]

On Sat, Nov 25, 2023 at 05:30:57PM +0000, Claus Assmann wrote:
> One of my regression tests crashes when using OpenSSL 3.2.0.  I've
> tracked it down to:
> 
> commit e4a94bcc77f3fda0f185e62a73a66d9b9b9388f5
>     Fix a possible memory leak in dane_tlsa_add
> diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
> index 5314e1ec0d..70d3b17c19 100644
> --- a/ssl/ssl_lib.c
> +++ b/ssl/ssl_lib.c
> @@ -339,6 +341,7 @@ static int dane_tlsa_add(SSL_DANE *dane,
>  
>              if ((DANETLS_USAGE_BIT(usage) & DANETLS_TA_MASK) == 0) {
>                  X509_free(cert);
> +                tlsa_free(t);
>                  break;
>              }
>  
> Now the question is: is this a bug in my application or in 3.2.0?
> Maybe someone who knows/understands the code can take a look?
> BTW: All the other added tlsa_free() calls are seemingly before
> a return statement. 
> 

Thanks for the report!

This is a freshly minted bug (just prior to the release) introducing a
double-free (rather than the intent of avoiding a memory leak):

    commit 7f943d40bda4539d63da34ecfbbc8556f2603fb3
    Author: Bernd Edlinger <bernd.edlinger at hotmail.de>
    Date:   Wed Nov 15 19:46:17 2023 +0100

    Fix a possible memory leak in dane_tlsa_add

    Several error cases leak either the X509 object
    or the pkey or the danetls_record object.

    Reviewed-by: Hugo Landau <hlandau at openssl.org>
    Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre at ncp-e.com>
    Reviewed-by: Richard Levitte <levitte at openssl.org>
    (Merged from https://github.com/openssl/openssl/pull/22743)

That tlsa_free(t) SHOULD NOT have been added, the record is still live
and used after the "break".  The reviewers should have checked more
carefully... :-(

-- 
    Viktor.