Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

[Yann Ylavic]

On Thu, Jun 18, 2020 at 6:03 PM Stefan Eissing
 wrote:
>
> ap_parse_request_line() for example, checks the initial HTTP/1.1 request line 
> *and*
> the method names, uri, header_only and other request_rec fields.
>
> We can either copy the latter into mod_http2 and maintain it in two places or
> have a core function for it to be invoked by mod_http and mod_http2. That 
> seems
> to be the design decision to make.

How about this attached patch?

ap_parse_request_line() will only parse the request line (extract the
fields, which mod_h2 doesn't need), and ap_check_request_line() will
do the validation.

Since ap_parse_request_line() becomes local to "protocol.c" only, it
can be unexported (not in this patch to avoid a MAJOR bump and thus
ease backport).

Thoughts?
Index: include/ap_mmn.h
===
--- include/ap_mmn.h	(revision 1878986)
+++ include/ap_mmn.h	(working copy)
@@ -632,6 +632,7 @@
  * 20200420.1 (2.5.1-dev)  Add ap_filter_adopt_brigade()
  * 20200420.2 (2.5.1-dev)  Add ap_proxy_worker_can_upgrade()
  * 20200420.3 (2.5.1-dev)  Add ap_parse_strict_length()
+ * 20200420.4 (2.5.1-dev)  Add ap_check_request_line()
  */
 
 #define MODULE_MAGIC_COOKIE 0x41503235UL /* "AP25" */
@@ -639,7 +640,7 @@
 #ifndef MODULE_MAGIC_NUMBER_MAJOR
 #define MODULE_MAGIC_NUMBER_MAJOR 20200420
 #endif
-#define MODULE_MAGIC_NUMBER_MINOR 3/* 0...n */
+#define MODULE_MAGIC_NUMBER_MINOR 4/* 0...n */
 
 /**
  * Determine if the server's current MODULE_MAGIC_NUMBER is at least a
Index: include/http_protocol.h
===
--- include/http_protocol.h	(revision 1878986)
+++ include/http_protocol.h	(working copy)
@@ -68,13 +68,21 @@ AP_DECLARE(request_rec *) ap_create_request(conn_r
 request_rec *ap_read_request(conn_rec *c);
 
 /**
- * Parse and validate the request line.
+ * Parse the request line.
  * @param r The current request
  * @return 1 on success, 0 on failure
+ * TODO: unexport, used in trunk's protocol.c only
  */
 AP_DECLARE(int) ap_parse_request_line(request_rec *r);
 
 /**
+ * Validate the request line.
+ * @param r The current request
+ * @return 1 on success, 0 on failure
+ */
+AP_DECLARE(int) ap_check_request_line(request_rec *r);
+
+/**
  * Validate the request header and select vhost.
  * @param r The current request
  * @return 1 on success, 0 on failure
Index: modules/http2/h2_request.c
===
--- modules/http2/h2_request.c	(revision 1878986)
+++ modules/http2/h2_request.c	(working copy)
@@ -278,8 +278,14 @@ request_rec *h2_request_create_rec(const h2_reques
 
 /* Time to populate r with the data we have. */
 r->request_time = req->request_time;
-r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0", 
-  req->method, req->path ? req->path : "");
+r->proto_num = HTTP_VERSION(2, 0);
+r->protocol = apr_pstrdup(r->pool, "HTTP/2.0");
+r->method = apr_pstrdup(r->pool, req->method ? req->method : "");
+r->unparsed_uri = apr_pstrdup(r->pool, req->path ? req->path : "");
+r->the_request = apr_pstrcat(r->pool,
+ r->method, " ",
+ r->unparsed_uri, " ",
+ r->protocol, NULL);
 r->headers_in = apr_table_clone(r->pool, req->headers);
 
 /* Start with r->hostname = NULL, ap_check_request_header() will get it
@@ -288,7 +294,7 @@ request_rec *h2_request_create_rec(const h2_reques
 r->hostname = NULL;
 
 /* Validate HTTP/1 request and select vhost. */
-if (!ap_parse_request_line(r) || !ap_check_request_header(r)) {
+if (!ap_check_request_line(r) || !ap_check_request_header(r)) {
 /* we may have switched to another server still */
 r->per_dir_config = r->server->lookup_defaults;
 access_status = r->status;
Index: server/protocol.c
===
--- server/protocol.c	(revision 1878986)
+++ server/protocol.c	(working copy)
@@ -610,7 +610,16 @@ AP_CORE_DECLARE(void) ap_parse_uri(request_rec *r,
 {
 int status = HTTP_OK;
 
-r->unparsed_uri = apr_pstrdup(r->pool, uri);
+if (uri) {
+r->unparsed_uri = apr_pstrdup(r->pool, uri);
+}
+else {
+uri = r->unparsed_uri;
+if (!uri) {
+r->status = HTTP_INTERNAL_SERVER_ERROR;
+return;
+}
+}
 
 /* http://issues.apache.org/bugzilla/show_bug.cgi?id=31875
  * http://issues.apache.org/bugzilla/show_bug.cgi?id=28450
@@ -751,7 +760,7 @@ AP_DECLARE(int) ap_parse_request_line(request_rec
 rrl_badmethod09, rrl_reject09
 } deferred_error = rrl_none;
 apr_size_t len = 0;
-char *uri, *ll;
+char *ll;
 
 r->method = r->the_request;
 
@@ -782,7 +791,7 @@ AP_DECLARE(int) ap_parse_request_line(request_rec
 if (!ll) {
 if 

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

[Stefan Eissing]

Thanks!

> Am 19.06.2020 um 14:02 schrieb Ruediger Pluem :
> 
> 
> 
> On 6/18/20 9:58 PM, Ruediger Pluem wrote:
> 
>> 
>> Provided that my above understanding is correct I see no real benefit any 
>> longer in returning a 505 and I would
>> revert r1878708 and all the follow ups (from r1878926 only the changes to 
>> modules/http2/h2_request.c and probably CHANGES as the
>> remaining changes are no functional changes to my understanding).
> 
> Done as r1878984 and r1878985
> 
> Regards
> 
> Rüdiger
> 

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

[Ruediger Pluem]

On 6/18/20 9:58 PM, Ruediger Pluem wrote:

> 
> Provided that my above understanding is correct I see no real benefit any 
> longer in returning a 505 and I would
> revert r1878708 and all the follow ups (from r1878926 only the changes to 
> modules/http2/h2_request.c and probably CHANGES as the
> remaining changes are no functional changes to my understanding).

Done as r1878984 and r1878985

Regards

Rüdiger

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

[Ruediger Pluem]

On 6/18/20 8:55 PM, Roy T. Fielding wrote:
>> On Jun 18, 2020, at 9:03 AM, Stefan Eissing > > wrote:
>>> Am 18.06.2020 um 16:51 schrieb William A Rowe Jr >> >:
>>>
>>>
>>> On 6/18/20 12:09 AM, Roy T. Fielding wrote:
> On Jun 8, 2020, at 12:56 AM, Ruediger Pluem  > 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.
> This does not affect mod_http2 if loaded as HTTP/2.0 connections 
> itself are not parsed via ap_parse_request_line
> and sending a
>
> GET /something HTTP/2.0
>
> as request line is not a valid way to start a HTTP 2.0 connection and 
> I doubt that it will be for future major versions.
>>>
>>> Correct, it starts an HTTP/1.1 connection, and the response should reflect 
>>> HTTP/1.1.
>>>
 That isn't how these things typically work. New protocols are
 advanced with either deliberate backwards-compat or deliberate
 backwards-break, with an expectation that it will either do
 something useful on an older-protocol server or cause a safe
 error in an expected way.

 Hence, we might still see an HTTP/4.0 that is designed to be
 parsed like HTTP/1.1 (by an old server) while at the same time
 work perfectly for a new server. That would be some hefty magic,
 but it remains possible. Likewise, we might want to deploy a
 version of h2 or HTTP/3 that works on unix domain sockets or
 localhost over non-Internet TCP.

 This is why the existing code did not error on protocols >= 2.0.
 Doing so is both unnecessary and counterproductive. If parsing
 fails for some other reason, we want that other reason to be
 in the response (because that's what the new protocol will be
 expecting from an old protocol server). If it doesn't fail, we
 want to provide the successful response because the request
 was deliberately crafted that way to save a round trip.

 Note that the incoming request protocol version should always
 be distinct from any forwarded request protocol or response
 protocol versions.
>>>
>>> Precisely. If mod_http2 or quic/mod_http3 can do something with the 
>>> connection
>>> based on the request line, it's up to them through the hook facility to 
>>> take ownership
>>> of the connection.

What I get out of all of this is the following:

If we get a

METHOD /somepath HTTP/x.y

request line on the wire.

We could return a 505 (Version not supported) if we know that we do not support 
that major version x (e.g. HTTP/3
as mentioned below seem to be finished soon, but we do not have support for it 
yet). The 505 answer would be a
HTTP/1.1 response.
But we could also accept this request as long as it is a valid HTTP/1.1 request
and just respond with the desired HTTP/1.1 response for this resource as if 
this was sent with HTTP/1.1.
If it violates the HTTP/1.1 syntax somehow we would respond with the same 
status code we use for this case
on a HTTP/1.1 request. That second approach is IMHO the behavior we had before 
r1878708.

Provided that my above understanding is correct I see no real benefit any 
longer in returning a 505 and I would
revert r1878708 and all the follow ups (from r1878926 only the changes to 
modules/http2/h2_request.c and probably CHANGES as the
remaining changes are no functional changes to my understanding).

The positive thing I get out of this is that it revealed that we probably 
should think about splitting some functions
that do some version agnostic HTTP processing and some HTTP/1.1 specific 
processing in one place to make this distinction more
clear and the version agnostic HTTP processing code more reusable.

Many thanks to all for this constructive and focused discussion that taught me 
new things.

>>
>> That is not issue. That works well. 
>>
>>> If they cannot/do not, then the core http1 connection/request processors 
>>> remain
>>> in place and in response to "please speak in HTTP/4.0" this server will 
>>> respond,
>>> "sure, here is your HTTP/1.1 response" as expected and defined by the RFC.
>>
>> There are now several RFCs and they differentiate between HTTP/1.1 transport 
>> and
>> the pure HTTP semantics. This split is not reflected in our code, yet. We 
>> have 
>> functions that mix both. Not as a mistake, it's historical.
>>
>> ap_parse_request_line() for example, checks the initial HTTP/1.1 request 
>> line *and*
>> the method names, uri, header_only and other request_rec fields.
>>
>> We can either copy the latter into mod_http2 and maintain it in two places or
>> have a core function for it to be invoked by mod_http and mod_http2. That 
>> seems 
>> to be the design 

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

[Roy T. Fielding]

> On Jun 18, 2020, at 9:03 AM, Stefan Eissing  
> wrote:
>> Am 18.06.2020 um 16:51 schrieb William A Rowe Jr :
>> 
>> 
>> On 6/18/20 12:09 AM, Roy T. Fielding wrote:
 On Jun 8, 2020, at 12:56 AM, Ruediger Pluem  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.
 This does not affect mod_http2 if loaded as HTTP/2.0 connections 
 itself are not parsed via ap_parse_request_line
 and sending a
 
 GET /something HTTP/2.0
 
 as request line is not a valid way to start a HTTP 2.0 connection and 
 I doubt that it will be for future major versions.
>> 
>> Correct, it starts an HTTP/1.1 connection, and the response should reflect 
>> HTTP/1.1.
>> 
>>> That isn't how these things typically work. New protocols are
>>> advanced with either deliberate backwards-compat or deliberate
>>> backwards-break, with an expectation that it will either do
>>> something useful on an older-protocol server or cause a safe
>>> error in an expected way.
>>> 
>>> Hence, we might still see an HTTP/4.0 that is designed to be
>>> parsed like HTTP/1.1 (by an old server) while at the same time
>>> work perfectly for a new server. That would be some hefty magic,
>>> but it remains possible. Likewise, we might want to deploy a
>>> version of h2 or HTTP/3 that works on unix domain sockets or
>>> localhost over non-Internet TCP.
>>> 
>>> This is why the existing code did not error on protocols >= 2.0.
>>> Doing so is both unnecessary and counterproductive. If parsing
>>> fails for some other reason, we want that other reason to be
>>> in the response (because that's what the new protocol will be
>>> expecting from an old protocol server). If it doesn't fail, we
>>> want to provide the successful response because the request
>>> was deliberately crafted that way to save a round trip.
>>> 
>>> Note that the incoming request protocol version should always
>>> be distinct from any forwarded request protocol or response
>>> protocol versions.
>> 
>> Precisely. If mod_http2 or quic/mod_http3 can do something with the 
>> connection
>> based on the request line, it's up to them through the hook facility to take 
>> ownership
>> of the connection.
> 
> That is not issue. That works well. 
> 
>> If they cannot/do not, then the core http1 connection/request processors 
>> remain
>> in place and in response to "please speak in HTTP/4.0" this server will 
>> respond,
>> "sure, here is your HTTP/1.1 response" as expected and defined by the RFC.
> 
> There are now several RFCs and they differentiate between HTTP/1.1 transport 
> and
> the pure HTTP semantics. This split is not reflected in our code, yet. We 
> have 
> functions that mix both. Not as a mistake, it's historical.
> 
> ap_parse_request_line() for example, checks the initial HTTP/1.1 request line 
> *and*
> the method names, uri, header_only and other request_rec fields.
> 
> We can either copy the latter into mod_http2 and maintain it in two places or
> have a core function for it to be invoked by mod_http and mod_http2. That 
> seems 
> to be the design decision to make.

I think that is the right way forward, though we have always tried
to minimize overhead for the common case. I guess the extra cycles
are irrelevant now.

> I used ap_parse_request_line() from mod_http2 to *not* duplicate the other 
> code,
> and someone added checks in trunk that imply it only ever gets called for 
> HTTP/1.x.
> So, now it pukes. Which is good, as it brought up this discussion.

Yep. The new RFCs should be "finished" by next month due to HTTP/3
being in WGLC this month.  If anyone's interested:

   https://github.com/httpwg/http-core 
   https://github.com/quicwg/base-drafts 

...Roy

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

[Stefan Eissing]

Stefan Eissing

bytes GmbH
Hafenweg 16
48155 Münster
www.greenbytes.de

> Am 18.06.2020 um 16:51 schrieb William A Rowe Jr :
> 
>  
>  On 6/18/20 12:09 AM, Roy T. Fielding wrote:
> >> On Jun 8, 2020, at 12:56 AM, Ruediger Pluem  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.
> >> This does not affect mod_http2 if loaded as HTTP/2.0 connections 
> >> itself are not parsed via ap_parse_request_line
> >> and sending a
> >>
> >> GET /something HTTP/2.0
> >>
> >> as request line is not a valid way to start a HTTP 2.0 connection and 
> >> I doubt that it will be for future major versions.
> 
> Correct, it starts an HTTP/1.1 connection, and the response should reflect 
> HTTP/1.1.
> 
> > That isn't how these things typically work. New protocols are
> > advanced with either deliberate backwards-compat or deliberate
> > backwards-break, with an expectation that it will either do
> > something useful on an older-protocol server or cause a safe
> > error in an expected way.
> >
> > Hence, we might still see an HTTP/4.0 that is designed to be
> > parsed like HTTP/1.1 (by an old server) while at the same time
> > work perfectly for a new server. That would be some hefty magic,
> > but it remains possible. Likewise, we might want to deploy a
> > version of h2 or HTTP/3 that works on unix domain sockets or
> > localhost over non-Internet TCP.
> >
> > This is why the existing code did not error on protocols >= 2.0.
> > Doing so is both unnecessary and counterproductive. If parsing
> > fails for some other reason, we want that other reason to be
> > in the response (because that's what the new protocol will be
> > expecting from an old protocol server). If it doesn't fail, we
> > want to provide the successful response because the request
> > was deliberately crafted that way to save a round trip.
> >
> > Note that the incoming request protocol version should always
> > be distinct from any forwarded request protocol or response
> > protocol versions.
>  
> Precisely. If mod_http2 or quic/mod_http3 can do something with the connection
> based on the request line, it's up to them through the hook facility to take 
> ownership
> of the connection.

That is not issue. That works well. 

> If they cannot/do not, then the core http1 connection/request processors 
> remain
> in place and in response to "please speak in HTTP/4.0" this server will 
> respond,
> "sure, here is your HTTP/1.1 response" as expected and defined by the RFC.

There are now several RFCs and they differentiate between HTTP/1.1 transport and
the pure HTTP semantics. This split is not reflected in our code, yet. We have 
functions that mix both. Not as a mistake, it's historical.

ap_parse_request_line() for example, checks the initial HTTP/1.1 request line 
*and*
the method names, uri, header_only and other request_rec fields.

We can either copy the latter into mod_http2 and maintain it in two places or
have a core function for it to be invoked by mod_http and mod_http2. That seems 
to be the design decision to make.

I used ap_parse_request_line() from mod_http2 to *not* duplicate the other code,
and someone added checks in trunk that imply it only ever gets called for 
HTTP/1.x.
So, now it pukes. Which is good, as it brought up this discussion.

- Stefan

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

[William A Rowe Jr]

>  On 6/18/20 12:09 AM, Roy T. Fielding wrote:
> >> On Jun 8, 2020, at 12:56 AM, Ruediger Pluem 
> 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.
> >> This does not affect mod_http2 if loaded as HTTP/2.0 connections
> itself are not parsed via ap_parse_request_line
> >> and sending a
> >>
> >> GET /something HTTP/2.0
> >>
> >> as request line is not a valid way to start a HTTP 2.0 connection
> and I doubt that it will be for future major versions.
>

Correct, it starts an HTTP/1.1 connection, and the response should reflect
HTTP/1.1.

> That isn't how these things typically work. New protocols are
> > advanced with either deliberate backwards-compat or deliberate
> > backwards-break, with an expectation that it will either do
> > something useful on an older-protocol server or cause a safe
> > error in an expected way.
> >
> > Hence, we might still see an HTTP/4.0 that is designed to be
> > parsed like HTTP/1.1 (by an old server) while at the same time
> > work perfectly for a new server. That would be some hefty magic,
> > but it remains possible. Likewise, we might want to deploy a
> > version of h2 or HTTP/3 that works on unix domain sockets or
> > localhost over non-Internet TCP.
> >
> > This is why the existing code did not error on protocols >= 2.0.
> > Doing so is both unnecessary and counterproductive. If parsing
> > fails for some other reason, we want that other reason to be
> > in the response (because that's what the new protocol will be
> > expecting from an old protocol server). If it doesn't fail, we
> > want to provide the successful response because the request
> > was deliberately crafted that way to save a round trip.
> >
> > Note that the incoming request protocol version should always
> > be distinct from any forwarded request protocol or response
> > protocol versions.
>

Precisely. If mod_http2 or quic/mod_http3 can do something with the
connection
based on the request line, it's up to them through the hook facility to
take ownership
of the connection.

If they cannot/do not, then the core http1 connection/request processors
remain
in place and in response to "please speak in HTTP/4.0" this server will
respond,
"sure, here is your HTTP/1.1 response" as expected and defined by the RFC.

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

[Ruediger Pluem]

On 6/18/20 12:21 PM, Stefan Eissing wrote:
>> Am 18.06.2020 um 11:49 schrieb Ruediger Pluem :
>>
>>
>>
>> On 6/18/20 10:37 AM, Stefan Eissing wrote:
>>>
>>> Stefan Eissing
>>>
>>> bytes GmbH
>>> Hafenweg 16
>>> 48155 Münster
>>> www.greenbytes.de
>>>
 Am 18.06.2020 um 09:48 schrieb Ruediger Pluem :



 On 6/18/20 12:09 AM, Roy T. Fielding wrote:
>> On Jun 8, 2020, at 12:56 AM, Ruediger Pluem  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.
>> This does not affect mod_http2 if loaded as HTTP/2.0 connections itself 
>> are not parsed via ap_parse_request_line
>> and sending a
>>
>> GET /something HTTP/2.0
>>
>> as request line is not a valid way to start a HTTP 2.0 connection and I 
>> doubt that it will be for future major versions.
>
> That isn't how these things typically work. New protocols are
> advanced with either deliberate backwards-compat or deliberate
> backwards-break, with an expectation that it will either do
> something useful on an older-protocol server or cause a safe
> error in an expected way.
>
> Hence, we might still see an HTTP/4.0 that is designed to be
> parsed like HTTP/1.1 (by an old server) while at the same time
> work perfectly for a new server. That would be some hefty magic,
> but it remains possible. Likewise, we might want to deploy a
> version of h2 or HTTP/3 that works on unix domain sockets or
> localhost over non-Internet TCP.
>
> This is why the existing code did not error on protocols >= 2.0.
> Doing so is both unnecessary and counterproductive. If parsing
> fails for some other reason, we want that other reason to be
> in the response (because that's what the new protocol will be
> expecting from an old protocol server). If it doesn't fail, we
> want to provide the successful response because the request
> was deliberately crafted that way to save a round trip.
>
> Note that the incoming request protocol version should always
> be distinct from any forwarded request protocol or response
> protocol versions.

 As always many thanks for the insights. I see two ways forward now:

 1. Roll back r1878708 and all the follow ups and be done.
>>>
>>> +1
>>>
>>> If we want the HTTP/1.x protocol handler to balk at higher protocol 
>>> versions, this
>>> check should be done in ap_read_request() (which should be placed in 
>>> modules/http
>>
>> This is an interesting proposal, but I think that the two points I mentioned 
>> below in 2.
>> would apply as well if the check is done there.
>>
>>> (which should then be named modules/http1)).
>>
>> Given Roy's comments above I think that at least in theory the stuff in 
>> modules/http could
>> still be used for protocols > HTTP/2.x. So I am not sure if this rename is 
>> justified.
> 
> The pre-condition was that *if* modules/http code refuses processing >= 
> HTTP/2.0, it should be named modules/http1.

Fair enough. I somehow missed this if. I will wait how this discussion evolves 
to see what next steps are the best ones.

Regards

Rüdiger

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

[Stefan Eissing]

> Am 18.06.2020 um 11:49 schrieb Ruediger Pluem :
> 
> 
> 
> On 6/18/20 10:37 AM, Stefan Eissing wrote:
>> 
>> Stefan Eissing
>> 
>> bytes GmbH
>> Hafenweg 16
>> 48155 Münster
>> www.greenbytes.de
>> 
>>> Am 18.06.2020 um 09:48 schrieb Ruediger Pluem :
>>> 
>>> 
>>> 
>>> On 6/18/20 12:09 AM, Roy T. Fielding wrote:
> On Jun 8, 2020, at 12:56 AM, Ruediger Pluem  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.
> This does not affect mod_http2 if loaded as HTTP/2.0 connections itself 
> are not parsed via ap_parse_request_line
> and sending a
> 
> GET /something HTTP/2.0
> 
> as request line is not a valid way to start a HTTP 2.0 connection and I 
> doubt that it will be for future major versions.
 
 That isn't how these things typically work. New protocols are
 advanced with either deliberate backwards-compat or deliberate
 backwards-break, with an expectation that it will either do
 something useful on an older-protocol server or cause a safe
 error in an expected way.
 
 Hence, we might still see an HTTP/4.0 that is designed to be
 parsed like HTTP/1.1 (by an old server) while at the same time
 work perfectly for a new server. That would be some hefty magic,
 but it remains possible. Likewise, we might want to deploy a
 version of h2 or HTTP/3 that works on unix domain sockets or
 localhost over non-Internet TCP.
 
 This is why the existing code did not error on protocols >= 2.0.
 Doing so is both unnecessary and counterproductive. If parsing
 fails for some other reason, we want that other reason to be
 in the response (because that's what the new protocol will be
 expecting from an old protocol server). If it doesn't fail, we
 want to provide the successful response because the request
 was deliberately crafted that way to save a round trip.
 
 Note that the incoming request protocol version should always
 be distinct from any forwarded request protocol or response
 protocol versions.
>>> 
>>> As always many thanks for the insights. I see two ways forward now:
>>> 
>>> 1. Roll back r1878708 and all the follow ups and be done.
>> 
>> +1
>> 
>> If we want the HTTP/1.x protocol handler to balk at higher protocol 
>> versions, this
>> check should be done in ap_read_request() (which should be placed in 
>> modules/http
> 
> This is an interesting proposal, but I think that the two points I mentioned 
> below in 2.
> would apply as well if the check is done there.
> 
>> (which should then be named modules/http1)).
> 
> Given Roy's comments above I think that at least in theory the stuff in 
> modules/http could
> still be used for protocols > HTTP/2.x. So I am not sure if this rename is 
> justified.

The pre-condition was that *if* modules/http code refuses processing >= 
HTTP/2.0, it should be named modules/http1.

But I agree that there is mostly stuff in there that we want to use for *any* 
http request.

>> 
>> 
>>> 2. Keep r1878708 and all the follow ups but adjust the code in 
>>> ap_parse_request_line
>>>  to be more specific when to sent a 505. Given today's state I think a 505 
>>> would still be
>>>  correct in case of
>> 
>>> 
>>>  1. Protocol >= HTTP/3.0. Of course this would need to be adjusted if an 
>>> implementation of
>>> HTTP/x.y with x > 2 pops up even if provided by a 3rd party module.
>>>  2. If HTTP/2.0 is sent over the wire like a HTTP/1.x request as HTTP/2 has 
>>> that
>>> deliberate backwards-break as I understand it. OTOH you could argue 
>>> that with
>>> mod_http2 present 505 is the wrong reply since the server supports 
>>> HTTP/2 but not the
>>> way it was tried and hence another code would be the correct response 
>>> (400 ??) if we see a
>>> HTTP/1.x request with Protocol HTTP/2.0.
>>> 
> 
> Regards
> 
> Rüdiger

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

[Ruediger Pluem]

On 6/18/20 10:37 AM, Stefan Eissing wrote:
> 
> Stefan Eissing
> 
> bytes GmbH
> Hafenweg 16
> 48155 Münster
> www.greenbytes.de
> 
>> Am 18.06.2020 um 09:48 schrieb Ruediger Pluem :
>>
>>
>>
>> On 6/18/20 12:09 AM, Roy T. Fielding wrote:
 On Jun 8, 2020, at 12:56 AM, Ruediger Pluem  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.
 This does not affect mod_http2 if loaded as HTTP/2.0 connections itself 
 are not parsed via ap_parse_request_line
 and sending a

 GET /something HTTP/2.0

 as request line is not a valid way to start a HTTP 2.0 connection and I 
 doubt that it will be for future major versions.
>>>
>>> That isn't how these things typically work. New protocols are
>>> advanced with either deliberate backwards-compat or deliberate
>>> backwards-break, with an expectation that it will either do
>>> something useful on an older-protocol server or cause a safe
>>> error in an expected way.
>>>
>>> Hence, we might still see an HTTP/4.0 that is designed to be
>>> parsed like HTTP/1.1 (by an old server) while at the same time
>>> work perfectly for a new server. That would be some hefty magic,
>>> but it remains possible. Likewise, we might want to deploy a
>>> version of h2 or HTTP/3 that works on unix domain sockets or
>>> localhost over non-Internet TCP.
>>>
>>> This is why the existing code did not error on protocols >= 2.0.
>>> Doing so is both unnecessary and counterproductive. If parsing
>>> fails for some other reason, we want that other reason to be
>>> in the response (because that's what the new protocol will be
>>> expecting from an old protocol server). If it doesn't fail, we
>>> want to provide the successful response because the request
>>> was deliberately crafted that way to save a round trip.
>>>
>>> Note that the incoming request protocol version should always
>>> be distinct from any forwarded request protocol or response
>>> protocol versions.
>>
>> As always many thanks for the insights. I see two ways forward now:
>>
>> 1. Roll back r1878708 and all the follow ups and be done.
> 
> +1
> 
> If we want the HTTP/1.x protocol handler to balk at higher protocol versions, 
> this
> check should be done in ap_read_request() (which should be placed in 
> modules/http

This is an interesting proposal, but I think that the two points I mentioned 
below in 2.
would apply as well if the check is done there.

> (which should then be named modules/http1)).

Given Roy's comments above I think that at least in theory the stuff in 
modules/http could
still be used for protocols > HTTP/2.x. So I am not sure if this rename is 
justified.

> 
> 
>> 2. Keep r1878708 and all the follow ups but adjust the code in 
>> ap_parse_request_line
>>   to be more specific when to sent a 505. Given today's state I think a 505 
>> would still be
>>   correct in case of
> 
>>
>>   1. Protocol >= HTTP/3.0. Of course this would need to be adjusted if an 
>> implementation of
>>  HTTP/x.y with x > 2 pops up even if provided by a 3rd party module.
>>   2. If HTTP/2.0 is sent over the wire like a HTTP/1.x request as HTTP/2 has 
>> that
>>  deliberate backwards-break as I understand it. OTOH you could argue 
>> that with
>>  mod_http2 present 505 is the wrong reply since the server supports 
>> HTTP/2 but not the
>>  way it was tried and hence another code would be the correct response 
>> (400 ??) if we see a
>>  HTTP/1.x request with Protocol HTTP/2.0.
>>

Regards

Rüdiger

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

[Stefan Eissing]

Stefan Eissing

bytes GmbH
Hafenweg 16
48155 Münster
www.greenbytes.de

> Am 18.06.2020 um 09:48 schrieb Ruediger Pluem :
> 
> 
> 
> On 6/18/20 12:09 AM, Roy T. Fielding wrote:
>>> On Jun 8, 2020, at 12:56 AM, Ruediger Pluem  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.
>>> This does not affect mod_http2 if loaded as HTTP/2.0 connections itself are 
>>> not parsed via ap_parse_request_line
>>> and sending a
>>> 
>>> GET /something HTTP/2.0
>>> 
>>> as request line is not a valid way to start a HTTP 2.0 connection and I 
>>> doubt that it will be for future major versions.
>> 
>> That isn't how these things typically work. New protocols are
>> advanced with either deliberate backwards-compat or deliberate
>> backwards-break, with an expectation that it will either do
>> something useful on an older-protocol server or cause a safe
>> error in an expected way.
>> 
>> Hence, we might still see an HTTP/4.0 that is designed to be
>> parsed like HTTP/1.1 (by an old server) while at the same time
>> work perfectly for a new server. That would be some hefty magic,
>> but it remains possible. Likewise, we might want to deploy a
>> version of h2 or HTTP/3 that works on unix domain sockets or
>> localhost over non-Internet TCP.
>> 
>> This is why the existing code did not error on protocols >= 2.0.
>> Doing so is both unnecessary and counterproductive. If parsing
>> fails for some other reason, we want that other reason to be
>> in the response (because that's what the new protocol will be
>> expecting from an old protocol server). If it doesn't fail, we
>> want to provide the successful response because the request
>> was deliberately crafted that way to save a round trip.
>> 
>> Note that the incoming request protocol version should always
>> be distinct from any forwarded request protocol or response
>> protocol versions.
> 
> As always many thanks for the insights. I see two ways forward now:
> 
> 1. Roll back r1878708 and all the follow ups and be done.

+1

If we want the HTTP/1.x protocol handler to balk at higher protocol versions, 
this
check should be done in ap_read_request() (which should be placed in 
modules/http
(which should then be named modules/http1)).


> 2. Keep r1878708 and all the follow ups but adjust the code in 
> ap_parse_request_line
>   to be more specific when to sent a 505. Given today's state I think a 505 
> would still be
>   correct in case of

> 
>   1. Protocol >= HTTP/3.0. Of course this would need to be adjusted if an 
> implementation of
>  HTTP/x.y with x > 2 pops up even if provided by a 3rd party module.
>   2. If HTTP/2.0 is sent over the wire like a HTTP/1.x request as HTTP/2 has 
> that
>  deliberate backwards-break as I understand it. OTOH you could argue that 
> with
>  mod_http2 present 505 is the wrong reply since the server supports 
> HTTP/2 but not the
>  way it was tried and hence another code would be the correct response 
> (400 ??) if we see a
>  HTTP/1.x request with Protocol HTTP/2.0.
> 
> Regards
> 
> Rüdiger

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

[Ruediger Pluem]

On 6/18/20 12:09 AM, Roy T. Fielding wrote:
>> On Jun 8, 2020, at 12:56 AM, Ruediger Pluem  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.
>> This does not affect mod_http2 if loaded as HTTP/2.0 connections itself are 
>> not parsed via ap_parse_request_line
>> and sending a
>>
>> GET /something HTTP/2.0
>>
>> as request line is not a valid way to start a HTTP 2.0 connection and I 
>> doubt that it will be for future major versions.
> 
> That isn't how these things typically work. New protocols are
> advanced with either deliberate backwards-compat or deliberate
> backwards-break, with an expectation that it will either do
> something useful on an older-protocol server or cause a safe
> error in an expected way.
> 
> Hence, we might still see an HTTP/4.0 that is designed to be
> parsed like HTTP/1.1 (by an old server) while at the same time
> work perfectly for a new server. That would be some hefty magic,
> but it remains possible. Likewise, we might want to deploy a
> version of h2 or HTTP/3 that works on unix domain sockets or
> localhost over non-Internet TCP.
> 
> This is why the existing code did not error on protocols >= 2.0.
> Doing so is both unnecessary and counterproductive. If parsing
> fails for some other reason, we want that other reason to be
> in the response (because that's what the new protocol will be
> expecting from an old protocol server). If it doesn't fail, we
> want to provide the successful response because the request
> was deliberately crafted that way to save a round trip.
> 
> Note that the incoming request protocol version should always
> be distinct from any forwarded request protocol or response
> protocol versions.

As always many thanks for the insights. I see two ways forward now:

1. Roll back r1878708 and all the follow ups and be done.
2. Keep r1878708 and all the follow ups but adjust the code in 
ap_parse_request_line
   to be more specific when to sent a 505. Given today's state I think a 505 
would still be
   correct in case of

   1. Protocol >= HTTP/3.0. Of course this would need to be adjusted if an 
implementation of
  HTTP/x.y with x > 2 pops up even if provided by a 3rd party module.
   2. If HTTP/2.0 is sent over the wire like a HTTP/1.x request as HTTP/2 has 
that
  deliberate backwards-break as I understand it. OTOH you could argue that 
with
  mod_http2 present 505 is the wrong reply since the server supports HTTP/2 
but not the
  way it was tried and hence another code would be the correct response 
(400 ??) if we see a
  HTTP/1.x request with Protocol HTTP/2.0.

Regards

Rüdiger

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

[Roy T. Fielding]

> On Jun 8, 2020, at 12:56 AM, Ruediger Pluem  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.
> This does not affect mod_http2 if loaded as HTTP/2.0 connections itself are 
> not parsed via ap_parse_request_line
> and sending a
> 
> GET /something HTTP/2.0
> 
> as request line is not a valid way to start a HTTP 2.0 connection and I doubt 
> that it will be for future major versions.

That isn't how these things typically work. New protocols are
advanced with either deliberate backwards-compat or deliberate
backwards-break, with an expectation that it will either do
something useful on an older-protocol server or cause a safe
error in an expected way.

Hence, we might still see an HTTP/4.0 that is designed to be
parsed like HTTP/1.1 (by an old server) while at the same time
work perfectly for a new server. That would be some hefty magic,
but it remains possible. Likewise, we might want to deploy a
version of h2 or HTTP/3 that works on unix domain sockets or
localhost over non-Internet TCP.

This is why the existing code did not error on protocols >= 2.0.
Doing so is both unnecessary and counterproductive. If parsing
fails for some other reason, we want that other reason to be
in the response (because that's what the new protocol will be
expecting from an old protocol server). If it doesn't fail, we
want to provide the successful response because the request
was deliberately crafted that way to save a round trip.

Note that the incoming request protocol version should always
be distinct from any forwarded request protocol or response
protocol versions.

Roy

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

[Yann Ylavic]

On Mon, Jun 8, 2020 at 8:38 PM Ruediger Pluem  wrote:
>
> On 6/8/20 6:06 PM, Yann Ylavic wrote:
> > On Mon, Jun 8, 2020 at 5:43 PM Julian Reschke  wrote:
> >>
> >> On 08.06.2020 16:59, Yann Ylavic wrote:
> >>> On Mon, Jun 8, 2020 at 9:56 AM Ruediger Pluem  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 ?
> >>
> >> In *theory*, there could a future HTTP/1.2 version that shares the wire
> >> format with 1.0 and 1.1.
>
> Exactly this was my reason for not >= 1.2 as this case is IMHO already 
> handled in a compliant way by the current code.
> https://tools.ietf.org/html/rfc7230#section-2.6 states:
>
> The interpretation of a header field does not change between minor
>versions of the same major HTTP version, though the default behavior
>of a recipient in the absence of such a field can change.  Unless
>specified otherwise, header fields defined in HTTP/1.1 are defined
>for all versions of HTTP/1.x.  In particular, the Host and Connection
>header fields ought to be implemented by all HTTP/1.x implementations
>whether or not they advertise conformance with HTTP/1.1.
>
>New header fields can be introduced without changing the protocol
>version if their defined semantics allow them to be safely ignored by
>recipients that do not recognize them.  Header field extensibility is
>discussed in Section 3.2.1.
>
>
> I interpret this that if we handle a potential HTTP 1.2 request as if
>
> - headers already known in HTTP 1.1 are handled in the same way as if the 
> request was HTTP 1.1
> - headers unknown in HTTP 1.2 and added and in HTTP 1.2 are ignored
>
> we are on the safe side if we sent back a HTTP 1.1 response.
>
> Furthermore I get from https://tools.ietf.org/html/rfc7231#section-6.6.6 that
> 505 indicates that we reject the processing of the major version which would 
> be wrong in the HTTP 1.2 case
> as we will process HTTP 1.0 and HTTP 1.1.

Fair enough ;)

Regards;
Yann.

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

[Yann Ylavic]

On Mon, Jun 8, 2020 at 10:12 PM Ruediger Pluem  wrote:
>
> On 6/8/20 10:05 PM, Yann Ylavic wrote:
> > On Mon, Jun 8, 2020 at 9:30 PM Ruediger Pluem  wrote:
> >>
> >> On 6/8/20 4:59 PM, Yann Ylavic wrote:
> >>> On Mon, Jun 8, 2020 at 9:56 AM Ruediger Pluem  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.

Oh no, actually _I_ missed the "do _not_ pass" :)

I don't think ap_parse_uri() should accept an URI-path which is not
HTTP compliant (i.e. does not start with '/'), that's where we
initialize the request_rec after all.. So yes apr_uri_parse() accepts
that and sets a relative path in r->parsed_uri.path, but
ap_parse_uri() if for HTTP parsing IMHO.

Regards;
Yann.

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

[Ruediger Pluem]

On 6/8/20 10:05 PM, Yann Ylavic wrote:
> On Mon, Jun 8, 2020 at 9:30 PM Ruediger Pluem  wrote:
>>
>> On 6/8/20 4:59 PM, Yann Ylavic wrote:
>>> On Mon, Jun 8, 2020 at 9:56 AM Ruediger Pluem  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, >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

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

[Yann Ylavic]

On Mon, Jun 8, 2020 at 10:05 PM Yann Ylavic  wrote:
>
> On Mon, Jun 8, 2020 at 9:30 PM Ruediger Pluem  wrote:
> >
> > 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.

But to be complete on that side, and handle the proxy case of the
asterisk-form (section 5.3.4 [1]), I think we also need this hunk:

Index: server/protocol.c
===
--- server/protocol.c(revision 1878328)
+++ server/protocol.c(working copy)
@@ -640,8 +648,15 @@ AP_CORE_DECLARE(void) ap_parse_uri(request_rec *r,
 }

 r->args = r->parsed_uri.query;
-r->uri = r->parsed_uri.path ? r->parsed_uri.path
- : apr_pstrdup(r->pool, "/");
+if (r->parsed_uri.path) {
+r->uri = r->parsed_uri.path;
+}
+else if (r->method_number == M_OPTIONS) {
+r->uri = apr_pstrdup(r->pool, "*");
+}
+else {
+r->uri = apr_pstrdup(r->pool, "/");
+}

 #if defined(OS2) || defined(WIN32)
 /* Handle path translations for OS/2 and plug security hole.
--

[1] https://tools.ietf.org/html/rfc7230#section-5.3.4

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

[Yann Ylavic]

On Mon, Jun 8, 2020 at 9:30 PM Ruediger Pluem  wrote:
>
> On 6/8/20 4:59 PM, Yann Ylavic wrote:
> > On Mon, Jun 8, 2020 at 9:56 AM Ruediger Pluem  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.

>
> >
> > 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, >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.


Regards;
Yann.

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

[Ruediger Pluem]

On 6/8/20 4:59 PM, Yann Ylavic wrote:
> On Mon, Jun 8, 2020 at 9:56 AM Ruediger Pluem  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

> 
> 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, >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
+}
+
+

Plus adding rrl_invaliduri to the enum and handling later on?

Regards

Rüdiger

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

[Ruediger Pluem]

On 6/8/20 6:06 PM, Yann Ylavic wrote:
> On Mon, Jun 8, 2020 at 5:43 PM Julian Reschke  wrote:
>>
>> On 08.06.2020 16:59, Yann Ylavic wrote:
>>> On Mon, Jun 8, 2020 at 9:56 AM Ruediger Pluem  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 ?
>>
>> In *theory*, there could a future HTTP/1.2 version that shares the wire
>> format with 1.0 and 1.1.

Exactly this was my reason for not >= 1.2 as this case is IMHO already handled 
in a compliant way by the current code.
https://tools.ietf.org/html/rfc7230#section-2.6 states:

The interpretation of a header field does not change between minor
   versions of the same major HTTP version, though the default behavior
   of a recipient in the absence of such a field can change.  Unless
   specified otherwise, header fields defined in HTTP/1.1 are defined
   for all versions of HTTP/1.x.  In particular, the Host and Connection
   header fields ought to be implemented by all HTTP/1.x implementations
   whether or not they advertise conformance with HTTP/1.1.

   New header fields can be introduced without changing the protocol
   version if their defined semantics allow them to be safely ignored by
   recipients that do not recognize them.  Header field extensibility is
   discussed in Section 3.2.1.


I interpret this that if we handle a potential HTTP 1.2 request as if

- headers already known in HTTP 1.1 are handled in the same way as if the 
request was HTTP 1.1
- headers unknown in HTTP 1.2 and added and in HTTP 1.2 are ignored

we are on the safe side if we sent back a HTTP 1.1 response.

Furthermore I get from https://tools.ietf.org/html/rfc7231#section-6.6.6 that
505 indicates that we reject the processing of the major version which would be 
wrong in the HTTP 1.2 case
as we will process HTTP 1.0 and HTTP 1.1.

Regards

Rüdiger

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

[Yann Ylavic]

On Mon, Jun 8, 2020 at 5:43 PM Julian Reschke  wrote:
>
> On 08.06.2020 16:59, Yann Ylavic wrote:
> > On Mon, Jun 8, 2020 at 9:56 AM Ruediger Pluem  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 ?
>
> In *theory*, there could a future HTTP/1.2 version that shares the wire
> format with 1.0 and 1.1.

Sure, but we have quite some time to adapt the check then :)


Regards;
Yann.

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

[Julian Reschke]

On 08.06.2020 16:59, Yann Ylavic wrote:

On Mon, Jun 8, 2020 at 9:56 AM Ruediger Pluem  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 ?


In *theory*, there could a future HTTP/1.2 version that shares the wire
format with 1.0 and 1.1.


...


Best regards, Julian

Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?

[Yann Ylavic]

On Mon, Jun 8, 2020 at 9:56 AM Ruediger Pluem  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:

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, >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) {
--

Regards;
Yann.