On 2/4/22 5:05 PM, Stefan Eissing wrote:
>
>
>> Am 04.02.2022 um 16:18 schrieb Yann Ylavic <ylavic....@gmail.com>:
>>
>> On Fri, Feb 4, 2022 at 1:22 PM <ic...@apache.org> 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;
}
}
Regards
Rüdiger
