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 2023/03/30 17:19:31 UTC
Re: svn commit: r1908805 - /httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c
On 3/30/23 7:09 PM, gbechis@apache.org wrote:
> Author: gbechis
> Date: Thu Mar 30 17:09:09 2023
> New Revision: 1908805
>
> URL: http://svn.apache.org/viewvc?rev=1908805&view=rev
> Log:
> check for more possible SSL failures
> bz #66225
>
> Modified:
> httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.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=1908805&r1=1908804&r2=1908805&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c (original)
> +++ httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c Thu Mar 30 17:09:09 2023
> @@ -997,10 +997,7 @@ static int ssl_hook_Access_classic(reque
> * handshake to proceed. */
> modssl_set_reneg_state(sslconn, RENEG_ALLOW);
>
> - SSL_renegotiate(ssl);
> - SSL_do_handshake(ssl);
> -
> - if (!SSL_is_init_finished(ssl)) {
> + if(!SSL_renegotiate(ssl) || !SSL_do_handshake(ssl) || !SSL_is_init_finished(ssl)) {
Wouldn't
if (!(SSL_renegotiate(ssl) && SSL_do_handshake(ssl) && SSL_is_init_finished(ssl))) {
be better as it would stop the calls as soon as one fails (due to boolean shortcuts)?
Or is it mandatory that SSL_do_handshake and / or SSL_is_init_finished get executed if one of steps before fails?
> ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02225)
> "Re-negotiation request failed");
> ssl_log_ssl_error(SSLLOG_MARK, APLOG_ERR, r->server);
>
>
>
Regards
Rüdiger
Re: svn commit: r1908805 - /httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c
Posted by Giovanni Bechis <gi...@paclan.it>.
On Thu, Mar 30, 2023 at 07:28:20PM +0200, Ruediger Pluem wrote:
>
>
> On 3/30/23 7:19 PM, Ruediger Pluem wrote:
> >
> >
> > On 3/30/23 7:09 PM, gbechis@apache.org wrote:
> >> Author: gbechis
> >> Date: Thu Mar 30 17:09:09 2023
> >> New Revision: 1908805
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1908805&view=rev
> >> Log:
> >> check for more possible SSL failures
> >> bz #66225
> >>
> >> Modified:
> >> httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.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=1908805&r1=1908804&r2=1908805&view=diff
> >> ==============================================================================
> >> --- httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c (original)
> >> +++ httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c Thu Mar 30 17:09:09 2023
> >> @@ -997,10 +997,7 @@ static int ssl_hook_Access_classic(reque
> >> * handshake to proceed. */
> >> modssl_set_reneg_state(sslconn, RENEG_ALLOW);
> >>
> >> - SSL_renegotiate(ssl);
> >> - SSL_do_handshake(ssl);
> >> -
> >> - if (!SSL_is_init_finished(ssl)) {
> >> + if(!SSL_renegotiate(ssl) || !SSL_do_handshake(ssl) || !SSL_is_init_finished(ssl)) {
> >
> > Wouldn't
> >
> > if (!(SSL_renegotiate(ssl) && SSL_do_handshake(ssl) && SSL_is_init_finished(ssl))) {
> >
> > be better as it would stop the calls as soon as one fails (due to boolean shortcuts)?
> > Or is it mandatory that SSL_do_handshake and / or SSL_is_init_finished get executed if one of steps before fails?
>
> Scratch the above. This was already true for the expression in the commit.
> But for SSL_do_handshake only the return value 1 indicates success, all values <= 0 indicate failure.
> https://www.openssl.org/docs/man1.1.1/man3/SSL_do_handshake.html
> Hence the proposal would be
>
> if (!SSL_renegotiate(ssl) || (SSL_do_handshake(ssl) != 1) || !SSL_is_init_finished(ssl))
>
good catch, thanks.
Giovanni
> >
> >> ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02225)
> >> "Re-negotiation request failed");
> >> ssl_log_ssl_error(SSLLOG_MARK, APLOG_ERR, r->server);
> >>
> >>
> >>
> >
>
> Regards
>
> Rüdiger
>
Re: svn commit: r1908805 - /httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 4/3/23 8:45 AM, Ruediger Pluem wrote:
>
>
> On 4/3/23 8:37 AM, giovanni@paclan.it wrote:
>> On 3/30/23 19:28, Ruediger Pluem wrote:
>>>
>>>
>>> On 3/30/23 7:19 PM, Ruediger Pluem wrote:
>>>>
>>>>
>>>> On 3/30/23 7:09 PM, gbechis@apache.org wrote:
>>>>> Author: gbechis
>
>>> Scratch the above. This was already true for the expression in the commit.
>>> But for SSL_do_handshake only the return value 1 indicates success, all values <= 0 indicate failure.
>>> https://www.openssl.org/docs/man1.1.1/man3/SSL_do_handshake.html
>>> Hence the proposal would be
>>>
>>> if (!SSL_renegotiate(ssl) || (SSL_do_handshake(ssl) != 1) || !SSL_is_init_finished(ssl))
>>>
>> will you commit it ? Otherwise I will take care of this before it get lost.
>
> I would leave this to you.
>
> Regards
>
> Rüdiger
>
Re: svn commit: r1908805 - /httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 3/30/23 7:19 PM, Ruediger Pluem wrote:
>
>
> On 3/30/23 7:09 PM, gbechis@apache.org wrote:
>> Author: gbechis
>> Date: Thu Mar 30 17:09:09 2023
>> New Revision: 1908805
>>
>> URL: http://svn.apache.org/viewvc?rev=1908805&view=rev
>> Log:
>> check for more possible SSL failures
>> bz #66225
>>
>> Modified:
>> httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.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=1908805&r1=1908804&r2=1908805&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c (original)
>> +++ httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c Thu Mar 30 17:09:09 2023
>> @@ -997,10 +997,7 @@ static int ssl_hook_Access_classic(reque
>> * handshake to proceed. */
>> modssl_set_reneg_state(sslconn, RENEG_ALLOW);
>>
>> - SSL_renegotiate(ssl);
>> - SSL_do_handshake(ssl);
>> -
>> - if (!SSL_is_init_finished(ssl)) {
>> + if(!SSL_renegotiate(ssl) || !SSL_do_handshake(ssl) || !SSL_is_init_finished(ssl)) {
>
> Wouldn't
>
> if (!(SSL_renegotiate(ssl) && SSL_do_handshake(ssl) && SSL_is_init_finished(ssl))) {
>
> be better as it would stop the calls as soon as one fails (due to boolean shortcuts)?
> Or is it mandatory that SSL_do_handshake and / or SSL_is_init_finished get executed if one of steps before fails?
Scratch the above. This was already true for the expression in the commit.
But for SSL_do_handshake only the return value 1 indicates success, all values <= 0 indicate failure.
https://www.openssl.org/docs/man1.1.1/man3/SSL_do_handshake.html
Hence the proposal would be
if (!SSL_renegotiate(ssl) || (SSL_do_handshake(ssl) != 1) || !SSL_is_init_finished(ssl))
>
>> ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02225)
>> "Re-negotiation request failed");
>> ssl_log_ssl_error(SSLLOG_MARK, APLOG_ERR, r->server);
>>
>>
>>
>
Regards
Rüdiger