Re: svn commit: r1897760 - in /httpd/httpd/trunk: ./ changes-entries/ include/ modules/generators/ modules/ssl/ server/mpm/event/ support/ test/modules/http2/

On 8/12/25 11:12 AM, Ruediger Pluem wrote:
> 
> 
> On 2/4/22 5:05 PM, Stefan Eissing wrote:
>>
>>
>>> Am 04.02.2022 um 16:18 schrieb Yann Ylavic <[email protected]>:
>>>
>>> On Fri, Feb 4, 2022 at 1:22 PM <[email protected]> wrote:
>>>>
>>>> Modified: httpd/httpd/trunk/modules/ssl/mod_ssl.c
>>>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/mod_ssl.c?rev=1897760&r1=1897759&r2=1897760&view=diff
>>>> ==============================================================================
>>>> --- httpd/httpd/trunk/modules/ssl/mod_ssl.c (original)
>>>> +++ httpd/httpd/trunk/modules/ssl/mod_ssl.c Fri Feb  4 12:22:26 2022
>>>> @@ -690,80 +689,31 @@ static int ssl_hook_process_connection(c
>>>> {
>>> []
>>>>         temp = apr_brigade_create(c->pool, c->bucket_alloc);
>>>> +        rv = ap_get_brigade(c->input_filters, temp,
>>>> +                            AP_MODE_INIT, APR_BLOCK_READ, 0);
>>>> +        apr_brigade_destroy(temp);
>>>>
>>> []
>>>> +        if (APR_SUCCESS != APR_SUCCESS) {
>>>
>>> This one's unlikely to trigger :)
>>>
>>>> +            if (c->cs) {
>>>> +                c->cs->state = CONN_STATE_LINGER;
>>>>             }
>>>> +            ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c, APLOGNO(10373)
>>>> +                          "SSL handshake was not completed, "
>>>> +                          "closing connection");
>>>> +            return OK;
>>>>         }
>>> []
>>>> +
>>>> +    return DECLINED;
>>>> }
>>
>> That was is my mistakes. I tried to rescue Graham's check on a failed handshake. Maybe you can come up with an improvement on that...
> 
> Reviving this old topic again. Someone reported the above condition in PR 69773
> (https://bz.apache.org/bugzilla/show_bug.cgi?id=69773). When I tried the obvious and proposed fix by the reporter,
> replacing one APR_SUCCESS with rv, 4 tests failed:
> 
> t/security/CVE-2005-3357.t        (Wstat: 0 Tests: 3 Failed: 2)
>   Failed tests:  1, 3
> t/ssl/http.t                      (Wstat: 0 Tests: 2 Failed: 2)
>   Failed tests:  1-2
> 
> They fail because they try to speak plain HTTP to a TLS server and expect a 400 response with the error message
> "speaking plain HTTP to an SSL-enabled server port" , which is generated later in the connection / request life cycle.
> Letting the above error out causes the connection to be closed before the response can be send. I assume we want to keep this
> behavior. Hence I think we should either
> 
> * Document why this strange code is here and that it is work in progress. Probably we could also change one APR_SUCCESS to rv
>   already and put the whole if block in #if 0 / #endif
> * Remove the if block
> * Use the following patch which causes the tests to pass again and restores the return of the error message while at the same time
>   closing the connection early if we do not have the "Plain HTTP" scenario.
> 
> Index: modules/ssl/mod_ssl.c
> ===================================================================
> --- modules/ssl/mod_ssl.c	(revision 1927749)
> +++ modules/ssl/mod_ssl.c	(working copy)
> @@ -709,14 +709,23 @@
>                              AP_MODE_INIT, APR_BLOCK_READ, 0);
>          apr_brigade_destroy(temp);
> 
> -        if (APR_SUCCESS != APR_SUCCESS) {
> -            if (c->cs) {
> -                c->cs->state = CONN_STATE_LINGER;
> +        if (rv != APR_SUCCESS) {
> +            /*
> +             * We handle NON_SSL_SEND_REQLINE and NON_SSL_SEND_HDR_SEP
> +             * later again in the input filter and in ssl_hook_ReadReq
> +             * to return an appropriate error message to the client.
> +             * Hence do not close the connection here.
> +             */
> +            if ((sslconn->non_ssl_request != NON_SSL_SEND_REQLINE) &&
> +                (sslconn->non_ssl_request != NON_SSL_SEND_HDR_SEP)) {
> +                if (c->cs) {
> +                    c->cs->state = CONN_STATE_LINGER;
> +                }
> +                ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c, APLOGNO(10373)
> +                              "SSL handshake was not completed, "
> +                              "closing connection");
> +                return OK;
>              }
> -            ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c, APLOGNO(10373)
> -                          "SSL handshake was not completed, "
> -                          "closing connection");
> -            return OK;
>          }
>      }
> 
> 
> 

Commited as r1927880.

Regards

Rüdiger