Mailing List Archive

Re: svn commit: r1893876 - /httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
On 10/4/21 12:26 PM, jorton@apache.org wrote:
> Author: jorton
> Date: Mon Oct 4 10:26:18 2021
> New Revision: 1893876
>
> URL: http://svn.apache.org/viewvc?rev=1893876&view=rev
> Log:
> * modules/ssl/ssl_engine_init.c (ssl_init_server_certs): For OpenSSL
> 1.1+, disable auto DH parameter selection if parameters have been
> manually configured. This fixes a regression in r1890067 after
> which manually configured parameters are ignored.
>
> Modified:
> httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
>
> Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_init.c?rev=1893876&r1=1893875&r2=1893876&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/ssl/ssl_engine_init.c (original)
> +++ httpd/httpd/trunk/modules/ssl/ssl_engine_init.c Mon Oct 4 10:26:18 2021
> @@ -1589,7 +1589,14 @@ static apr_status_t ssl_init_server_cert
> certfile = APR_ARRAY_IDX(mctx->pks->cert_files, 0, const char *);
> if (certfile && !modssl_is_engine_id(certfile)
> && (dh = ssl_dh_GetParamFromFile(certfile))) {
> + /* ### This should be replaced with SSL_CTX_set0_tmp_dh_pkey()
> + * for OpenSSL 3.0+. */
> SSL_CTX_set_tmp_dh(mctx->ssl_ctx, dh);
> +#if !MODSSL_USE_OPENSSL_PRE_1_1_API
> + /* OpenSSL ignores manually configured DH params if automatic
> + * selection if enabled, so disable auto selection here. */
> + SSL_CTX_set_dh_auto(mctx->ssl_ctx, 0);
> +#endif

Stupid question: Don't we need to disable it via SSL_CTX_set_dh_auto, before we do SSL_CTX_set_tmp_dh with custom parameters?
Hence is the order of both above correct?

Regards

RĂ¼diger
Re: svn commit: r1893876 - /httpd/httpd/trunk/modules/ssl/ssl_engine_init.c [ In reply to ]
On Thu, Oct 07, 2021 at 09:09:32AM +0200, Ruediger Pluem wrote:
> On 10/4/21 12:26 PM, jorton@apache.org wrote:
> > Author: jorton
> > Date: Mon Oct 4 10:26:18 2021
> > New Revision: 1893876
> >
> > URL: http://svn.apache.org/viewvc?rev=1893876&view=rev
...
> > +++ httpd/httpd/trunk/modules/ssl/ssl_engine_init.c Mon Oct 4 10:26:18 2021
> > @@ -1589,7 +1589,14 @@ static apr_status_t ssl_init_server_cert
> > certfile = APR_ARRAY_IDX(mctx->pks->cert_files, 0, const char *);
> > if (certfile && !modssl_is_engine_id(certfile)
> > && (dh = ssl_dh_GetParamFromFile(certfile))) {
> > + /* ### This should be replaced with SSL_CTX_set0_tmp_dh_pkey()
> > + * for OpenSSL 3.0+. */
> > SSL_CTX_set_tmp_dh(mctx->ssl_ctx, dh);
> > +#if !MODSSL_USE_OPENSSL_PRE_1_1_API
> > + /* OpenSSL ignores manually configured DH params if automatic
> > + * selection if enabled, so disable auto selection here. */
> > + SSL_CTX_set_dh_auto(mctx->ssl_ctx, 0);
> > +#endif
>
> Stupid question: Don't we need to disable it via SSL_CTX_set_dh_auto, before we do SSL_CTX_set_tmp_dh with custom parameters?
> Hence is the order of both above correct?

The order doesn't matter, it only gets checked later at runtime where
the logic is to honour the _auto setting over the configured params:

https://github.com/openssl/openssl/blob/openssl-3.0.0/ssl/statem/statem_srvr.c#L2458

but actually there is simpler code possible here, which is also
consistent with how the ECDH auto curve selection works, so -> r1893964
and thanks for the review.

Regards, Joe