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

[Viktor Dukhovni]

On Sat, Nov 25, 2023 at 01:19:54PM -0500, Viktor Dukhovni wrote:

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

Proposed fix:

commit 2518d166d4671478072428aacbfcdbf32a876cd2
Author: Viktor Dukhovni <openssl-users at dukhovni.org>
Date:   Sat Nov 25 13:26:20 2023 -0500

    Fix freshly introduced double-free.
    
    We don't need the decoded X.509 Full(0) certificate for the EE usages 1 and 3,
    because the leaf certificate is always part of the presented chain, so the
    certificate is only validated as well-formed, and then discarded, but the
    TLSA record is of course still used after the validation step.
    
    Reported by Claus Assmann.

--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -340,8 +340,19 @@ static int dane_tlsa_add(SSL_DANE *dane,
             }
 
             if ((DANETLS_USAGE_BIT(usage) & DANETLS_TA_MASK) == 0) {
+                /*
+                 * The Full(0) certificate decodes to a seemingly valid X.509
+                 * object with a plausible key, so the TLSA record is well
+                 * formed.  However, we don't actually need the certifiate for
+                 * usages PKIX-EE(1) or DANE-EE(3), because at least the EE
+                 * certificate is always presented by the peer.  We discard the
+                 * certificate, and just use the TLSA data as an opaque blob
+                 * for matching the raw presented DER octets.
+                 *
+                 * DO NOT FREE `t` here, it will be added to the TLSA record
+                 * list below!
+                 */
                 X509_free(cert);
-                tlsa_free(t);
                 break;
             }
 

-- 
    Viktor.