On 6/8/20 10:05 PM, Yann Ylavic wrote:
> On Mon, Jun 8, 2020 at 9:30 PM Ruediger Pluem <rpl...@apache.org> wrote:
>>
>> On 6/8/20 4:59 PM, Yann Ylavic wrote:
>>> On Mon, Jun 8, 2020 at 9:56 AM Ruediger Pluem <rpl...@apache.org> wrote:
>>>>
>>>> I came across the question if we should not reject HTTP protocols >= 2.0 
>>>> in the request line when we parse it
>>>> in ap_parse_request_line.
>>>
>>> Why not >= 1.2 ?
>>>
>>>> A possible patch could look like the following (which rejects such 
>>>> requests with a HTTP_VERSION_NOT_SUPPORTED status code):
>>>
>>> Looks good.
>>>
>>>
>>> In the same category, could we reject invalid URI paths earlier
>>> (request-target per RFC-7230 5.3)?
>>> Today it fails in ap_core_translate(), but possibly the below would be 
>>> better:
>>
>> I think we could, but I am not sure if we have ap_parse_uri callers in other 
>> parts of the code that do not pass absolute URI's
> 
> This patch works with absolute URIs too since apr_parse_uri will split
> it in r->parsed_uri and we consider r->parsed_uri.path only.

I guess I was not precise enough with my concern. I meant that I haven't 
checked if there are callers which pass in relative URI's.

> 
>>
>>>
>>> Index: server/protocol.c
>>> ===================================================================
>>> --- server/protocol.c    (revision 1878328)
>>> +++ server/protocol.c    (working copy)
>>> @@ -627,6 +627,14 @@ AP_CORE_DECLARE(void) ap_parse_uri(request_rec *r,
>>>      }
>>>      else {
>>>          status = apr_uri_parse(r->pool, uri, &r->parsed_uri);
>>> +        if (status == APR_SUCCESS
>>> +            && (r->parsed_uri.path != NULL)
>>> +            && (r->parsed_uri.path[0] != '/')
>>> +            && (r->method_number != M_OPTIONS
>>> +                || strcmp(r->parsed_uri.path, "*") != 0)) {
>>> +            /* Invalid request-target per rfc7230#section-5.3 */
>>> +            status = APR_EINVAL;
>>> +        }
>>>      }
>>>
>>>      if (status == APR_SUCCESS) {
>>
>> Don't we miss in  server/protocol.c:
>>
>> @@ -906,6 +911,12 @@
>>
>>      ap_parse_uri(r, uri);
>>
>> +    if (strict && deferred_error == rrl_none
>> +        && r->status == HTTP_BAD_REQUEST) {
>> +        deferred_error = rrl_invaliduri
>> +    }
> 
> Somehow r->status != HTTP_OK is caught below all the deferred_error
> cases (which we want to report in priority?), and then jumps to the
> rrl_failed label.

Fair enough :-) I missed this one.

Regards

RĂ¼diger

Reply via email to