(OpenSSL bug please fix) Re: Need Replacement for Deprecated function.

[Viktor Dukhovni]

On Fri, Dec 03, 2021 at 07:05:43PM +0000, Jeremy Harris wrote:

> > EVP_PKEY_get_bits() should be equivalent to DH_bits() (for a DH
> > file). I would definitely double-check that you are not mis-loading
> > something.
> 
> OK; this was indeed my fault.

Actually, no, not your fault at all.  The implementation in libssl is
borked.  Please open a ticket.

> One minor docs item:
>    https://www.openssl.org/docs/manmaster/man3/SSL_CTX_set0_tmp_dh_pkey.html
> 
> says
>    "Ownership of the dhpkey value is passed to the SSL_CTX or SSL
>    object as a result of this call, and so the caller should not free
>    it if the function call is succesful."

Yes, ***if the call is successful**.  Unsuccessfull calls should not
ever take ownership or have any side effects other than reporting
failure.

The implementation is:

    int SSL_set0_tmp_dh_pkey(SSL *s, EVP_PKEY *dhpkey)
    {
        if (!ssl_security(s, SSL_SECOP_TMP_DH,
                          EVP_PKEY_get_security_bits(dhpkey), 0, dhpkey)) {
            ERR_raise(ERR_LIB_SSL, SSL_R_DH_KEY_TOO_SMALL);
--Wrong!--> EVP_PKEY_free(dhpkey);
            return 0;
        }
        EVP_PKEY_free(s->cert->dh_tmp);
        s->cert->dh_tmp = dhpkey;
        return 1;
    }

> It's not quite clear what the onwership for a failing call is.
> Experiment shows that an EVP_free() after a fail causes a crash,
> at least for a "dh key too small" error.

This is a booby trap and needs to be fixed.  You're not the only
the one to bit by this.  This even affects existing code in OpenSSL:

    ssl/ssl_conf.c:cmd_DHParameters()

        ...
        if (cctx->ctx != NULL) {
            if ((rv = SSL_CTX_set0_tmp_dh_pkey(cctx->ctx, dhpkey)) > 0)
                dhpkey = NULL;
        }
        if (cctx->ssl != NULL) {
            if ((rv = SSL_set0_tmp_dh_pkey(cctx->ssl, dhpkey)) > 0)
                dhpkey = NULL;
        }
     end:
        EVP_PKEY_free(dhpkey);
        BIO_free(in);
        return rv > 0;
    }

The key is freed when the call fails.  This is a bug in:

    commit 163f6dc1f70f30de46a68137c36e70cae4d95cd8
    Author: Matt Caswell <matt at openssl.org>
    Date:   Thu Oct 15 16:45:54 2020 +0100

        Implement a replacement for SSL_set_tmp_dh()

-- 
    Viktor.