You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2020/05/05 13:23:18 UTC
Re: svn commit: r1877397 - in /httpd/httpd/trunk: CHANGES
modules/ssl/ssl_engine_init.c modules/ssl/ssl_engine_io.c
modules/ssl/ssl_engine_kernel.c modules/ssl/ssl_private.h
modules/ssl/ssl_util_ssl.c
On 5/5/20 2:40 PM, jorton@apache.org wrote:
> Author: jorton
> Date: Tue May 5 12:40:38 2020
> New Revision: 1877397
>
> URL: http://svn.apache.org/viewvc?rev=1877397&view=rev
> Log:
> mod_ssl: Switch to using SSL_OP_NO_RENEGOTATION (where available) to
> block client-initiated renegotiation with TLSv1.2 and earlier.
>
> * modules/ssl/ssl_private.h: Define modssl_reneg_state enum,
> modssl_set_reneg_state function.
>
> * modules/ssl/ssl_engine_io.c (bio_filter_out_write,
> bio_filter_in_read): #ifdef-out reneg protection if
> SSL_OP_NO_RENEGOTATION is defined.
>
> * modules/ssl/ssl_engine_init.c (ssl_init_ctx_protocol):
> Enable SSL_OP_NO_RENEGOTATION.
> (ssl_init_ctx_callbacks): Only enable the "info" callback if
> debug-level logging *or* OpenSSL doesn't support SSL_OP_NO_RENEGOTATION.
>
> * modules/ssl/ssl_engine_kernel.c (ssl_hook_Access_classic): Use
> modssl_set_reneg_state to set the reneg protection mode.
> (ssl_hook_Access_modern): Drop manipulation of the reneg mode which
> does nothing for TLSv1.3 already.
> (ssl_callback_Info): Only enable reneg protection if
> SSL_OP_NO_RENEGOTATION is *not* defined.
>
> * modules/ssl/ssl_util_ssl.c (modssl_set_reneg_state): New function.
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
> httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
> httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c
> httpd/httpd/trunk/modules/ssl/ssl_private.h
> httpd/httpd/trunk/modules/ssl/ssl_util_ssl.c
>
> Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c?rev=1877397&r1=1877396&r2=1877397&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c (original)
> +++ httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c Tue May 5 12:40:38 2020
> @@ -1182,8 +1182,6 @@ static int ssl_hook_Access_modern(reques
> return HTTP_FORBIDDEN;
> }
>
> - old_state = sslconn->reneg_state;
> - sslconn->reneg_state = RENEG_ALLOW;
> modssl_set_app_data2(ssl, r);
>
> SSL_do_handshake(ssl);
> @@ -1193,7 +1191,6 @@ static int ssl_hook_Access_modern(reques
> */
> SSL_peek(ssl, peekbuf, 0);
>
> - sslconn->reneg_state = old_state;
> modssl_set_app_data2(ssl, NULL);
>
> /*
I don't understand why this can be removed unconditionally.
Regards
Rüdiger
Re: svn commit: r1877397 - in /httpd/httpd/trunk: CHANGES
modules/ssl/ssl_engine_init.c modules/ssl/ssl_engine_io.c
modules/ssl/ssl_engine_kernel.c modules/ssl/ssl_private.h
modules/ssl/ssl_util_ssl.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 5/5/20 6:04 PM, Joe Orton wrote:
> On Tue, May 05, 2020 at 03:23:18PM +0200, Ruediger Pluem wrote:
>> On 5/5/20 2:40 PM, jorton@apache.org wrote:
>>> Author: jorton
>>> Date: Tue May 5 12:40:38 2020
>>> New Revision: 1877397
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1877397&view=rev
>>> Log:
>>> mod_ssl: Switch to using SSL_OP_NO_RENEGOTATION (where available) to
>>> block client-initiated renegotiation with TLSv1.2 and earlier.
> ...
>>> Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c
>>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c?rev=1877397&r1=1877396&r2=1877397&view=diff
>>> ==============================================================================
>>> --- httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c (original)
>>> +++ httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c Tue May 5 12:40:38 2020
>>
>>> @@ -1182,8 +1182,6 @@ static int ssl_hook_Access_modern(reques
>>> return HTTP_FORBIDDEN;
>>> }
>>>
>>> - old_state = sslconn->reneg_state;
>>> - sslconn->reneg_state = RENEG_ALLOW;
>>> modssl_set_app_data2(ssl, r);
>>>
>>> SSL_do_handshake(ssl);
>>> @@ -1193,7 +1191,6 @@ static int ssl_hook_Access_modern(reques
>>> */
>>> SSL_peek(ssl, peekbuf, 0);
>>>
>>> - sslconn->reneg_state = old_state;
>>> modssl_set_app_data2(ssl, NULL);
>>>
>>> /*
>>
>> I don't understand why this can be removed unconditionally.
>
> It is removed unconditionally in ssl_hook_Access_modern, which is used
> only for TLSv1.3. The aim of this code was to protect against
Thanks for the pointer. I missed the
#if SSL_HAVE_PROTOCOL_TLSV1_3
in front of ssl_hook_Access_modern.
>
> Does that make sense?
Yes :-)
Regards
Rüdiger
Re: svn commit: r1877397 - in /httpd/httpd/trunk: CHANGES
modules/ssl/ssl_engine_init.c modules/ssl/ssl_engine_io.c
modules/ssl/ssl_engine_kernel.c modules/ssl/ssl_private.h
modules/ssl/ssl_util_ssl.c
Posted by Joe Orton <jo...@redhat.com>.
On Tue, May 05, 2020 at 03:23:18PM +0200, Ruediger Pluem wrote:
> On 5/5/20 2:40 PM, jorton@apache.org wrote:
> > Author: jorton
> > Date: Tue May 5 12:40:38 2020
> > New Revision: 1877397
> >
> > URL: http://svn.apache.org/viewvc?rev=1877397&view=rev
> > Log:
> > mod_ssl: Switch to using SSL_OP_NO_RENEGOTATION (where available) to
> > block client-initiated renegotiation with TLSv1.2 and earlier.
...
> > Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c
> > URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c?rev=1877397&r1=1877396&r2=1877397&view=diff
> > ==============================================================================
> > --- httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c (original)
> > +++ httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c Tue May 5 12:40:38 2020
>
> > @@ -1182,8 +1182,6 @@ static int ssl_hook_Access_modern(reques
> > return HTTP_FORBIDDEN;
> > }
> >
> > - old_state = sslconn->reneg_state;
> > - sslconn->reneg_state = RENEG_ALLOW;
> > modssl_set_app_data2(ssl, r);
> >
> > SSL_do_handshake(ssl);
> > @@ -1193,7 +1191,6 @@ static int ssl_hook_Access_modern(reques
> > */
> > SSL_peek(ssl, peekbuf, 0);
> >
> > - sslconn->reneg_state = old_state;
> > modssl_set_app_data2(ssl, NULL);
> >
> > /*
>
> I don't understand why this can be removed unconditionally.
It is removed unconditionally in ssl_hook_Access_modern, which is used
only for TLSv1.3. The aim of this code was to protect against
ClientHello being sent unexpectedly and that is always illegal in
TLSv1.3. https://www.rfc-editor.org/rfc/rfc8446.html#page-28 -
Because TLS 1.3 forbids renegotiation, if a server has negotiated
TLS 1.3 and receives a ClientHello at any other time, it MUST
terminate the connection with an "unexpected_message" alert.
So the ->reneg_state setting already had no effect for TLSv1.3, and the
code in ssl_callback_Info *before* my commit already checked and was a
noop for the TLSv1.3 case, in line 2289 of the function:
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c?revision=1877349&view=markup&pathrev=1877397#l2273
Does that make sense?
Regards, Joe