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 2015/03/05 09:38:16 UTC
Re: svn commit: r1664205 - in /httpd/httpd/trunk: CHANGES
server/protocol.c
On 03/05/2015 03:33 AM, covener@apache.org wrote:
> Author: covener
> Date: Thu Mar 5 02:33:16 2015
> New Revision: 1664205
>
> URL: http://svn.apache.org/r1664205
> Log:
> *) SECURITY: CVE-2015-0253 (cve.mitre.org)
> core: Fix a crash introduced in with ErrorDocument 400 pointing
> to a local URL-path with the INCLUDES filter active, introduced
> in 2.4.11. PR 57531. [Yann Ylavic]
>
>
> Submitted By: ylavic
> Committed By: covener
>
>
>
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/server/protocol.c
>
> Modified: httpd/httpd/trunk/server/protocol.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=1664205&r1=1664204&r2=1664205&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/protocol.c (original)
> +++ httpd/httpd/trunk/server/protocol.c Thu Mar 5 02:33:16 2015
> @@ -606,8 +606,6 @@ static int read_request_line(request_rec
> */
> if (APR_STATUS_IS_ENOSPC(rv)) {
> r->status = HTTP_REQUEST_URI_TOO_LARGE;
> - r->proto_num = HTTP_VERSION(1,0);
> - r->protocol = apr_pstrdup(r->pool, "HTTP/1.0");
> }
> else if (APR_STATUS_IS_TIMEUP(rv)) {
> r->status = HTTP_REQUEST_TIME_OUT;
> @@ -615,6 +613,8 @@ static int read_request_line(request_rec
> else if (APR_STATUS_IS_EINVAL(rv)) {
> r->status = HTTP_BAD_REQUEST;
> }
> + r->proto_num = HTTP_VERSION(1,0);
> + r->protocol = apr_pstrdup(r->pool, "HTTP/1.0");
> return 0;
> }
> } while ((len <= 0) && (++num_blank_lines < max_blank_lines));
>
>
>
Don't we need to have the following in addition to avoid a crash in another path?
Index: protocol.c
===================================================================
--- protocol.c (revision 1664261)
+++ protocol.c (working copy)
@@ -674,6 +674,8 @@
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02418)
"Invalid protocol '%s'", r->protocol);
if (enforce_strict) {
+ r->protocol = apr_pstrdup(r->pool, "HTTP/1.0");
+ r->proto_num = HTTP_VERSION(1,0);
r->status = HTTP_BAD_REQUEST;
return 0;
}
Regards
Rüdiger
Re: svn commit: r1664205 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Mar 5, 2015 at 10:09 AM, Ruediger Pluem <rp...@apache.org> wrote:
>
>
> On 03/05/2015 10:01 AM, Yann Ylavic wrote:
>> On Thu, Mar 5, 2015 at 9:38 AM, Ruediger Pluem <rp...@apache.org> wrote:
>>>
>>> Don't we need to have the following in addition to avoid a crash in another path?
>>>
>>>
>>> Index: protocol.c
>>> ===================================================================
>>> --- protocol.c (revision 1664261)
>>> +++ protocol.c (working copy)
>>> @@ -674,6 +674,8 @@
>>> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02418)
>>> "Invalid protocol '%s'", r->protocol);
>>> if (enforce_strict) {
>>> + r->protocol = apr_pstrdup(r->pool, "HTTP/1.0");
>>> + r->proto_num = HTTP_VERSION(1,0);
>>> r->status = HTTP_BAD_REQUEST;
>>> return 0;
>>> }
>>>
>>
>> r->protocol is initialized above in any case (with an invalid value
>> but it is), so it won't crash.
>> But r->proto_num is still 0 (which, if I read output filter chain
>> correctly, will lead to a HTTP/1.1 response), though.
>> So we probably should keep the original value in r->protocol (for
>> logging), but force proto_num = HTTP_VERSION(1,0).
>
> I saw that r->protocol is initialized in this case, but I wanted to keep it consistent with r->proto_num in this case.
> Hence the reset to "HTTP/1.0". If we are interested in logging we might should increase the severity of the error
> message above from DEBUG to something more visible e.g. INFO at least in the enforce_strict case.
I meant *custom* logging of the original request fields, but
r->the_request will do the job in this case.
So it's indeed probably better to keep things consistent internally.
Re: svn commit: r1664205 - in /httpd/httpd/trunk: CHANGES
server/protocol.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 03/05/2015 10:01 AM, Yann Ylavic wrote:
> On Thu, Mar 5, 2015 at 9:38 AM, Ruediger Pluem <rp...@apache.org> wrote:
>>
>> Don't we need to have the following in addition to avoid a crash in another path?
>>
>>
>> Index: protocol.c
>> ===================================================================
>> --- protocol.c (revision 1664261)
>> +++ protocol.c (working copy)
>> @@ -674,6 +674,8 @@
>> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02418)
>> "Invalid protocol '%s'", r->protocol);
>> if (enforce_strict) {
>> + r->protocol = apr_pstrdup(r->pool, "HTTP/1.0");
>> + r->proto_num = HTTP_VERSION(1,0);
>> r->status = HTTP_BAD_REQUEST;
>> return 0;
>> }
>>
>
> r->protocol is initialized above in any case (with an invalid value
> but it is), so it won't crash.
> But r->proto_num is still 0 (which, if I read output filter chain
> correctly, will lead to a HTTP/1.1 response), though.
> So we probably should keep the original value in r->protocol (for
> logging), but force proto_num = HTTP_VERSION(1,0).
I saw that r->protocol is initialized in this case, but I wanted to keep it consistent with r->proto_num in this case.
Hence the reset to "HTTP/1.0". If we are interested in logging we might should increase the severity of the error
message above from DEBUG to something more visible e.g. INFO at least in the enforce_strict case.
Regards
Rüdiger
Re: svn commit: r1664205 - in /httpd/httpd/trunk: CHANGES server/protocol.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Mar 5, 2015 at 9:38 AM, Ruediger Pluem <rp...@apache.org> wrote:
>
> Don't we need to have the following in addition to avoid a crash in another path?
>
>
> Index: protocol.c
> ===================================================================
> --- protocol.c (revision 1664261)
> +++ protocol.c (working copy)
> @@ -674,6 +674,8 @@
> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02418)
> "Invalid protocol '%s'", r->protocol);
> if (enforce_strict) {
> + r->protocol = apr_pstrdup(r->pool, "HTTP/1.0");
> + r->proto_num = HTTP_VERSION(1,0);
> r->status = HTTP_BAD_REQUEST;
> return 0;
> }
>
r->protocol is initialized above in any case (with an invalid value
but it is), so it won't crash.
But r->proto_num is still 0 (which, if I read output filter chain
correctly, will lead to a HTTP/1.1 response), though.
So we probably should keep the original value in r->protocol (for
logging), but force proto_num = HTTP_VERSION(1,0).
Regards,
Yann.