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
