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@xxxxxxxxxxx> Date: Thu Oct 15 16:45:54 2020 +0100 Implement a replacement for SSL_set_tmp_dh() -- Viktor.
