Re: Reject HTTP protocols >= 2.0 in ap_parse_request_line?
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?
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?
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?
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?
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?
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?
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?
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?
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?
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.
Re: RFC: Documenting changes in the CHANGES file
> On 1 Jun 2020, at 13:33, Graham Leggett wrote: > > On 29 May 2020, at 21:30, Ruediger Pluem wrote: > >> changes-fragments/ >>2.4.41/ >>2.4.42/ >>2.4.43/ >>2.4.44/ And a current/ as symlink? > I’m keen for a simpler version of this that doesn't create additional steps > for people. Not sure that's simpler, though it too has potential. Delete-after-append seems tidier than keeping forever-fragments. I wonder if this could be hooked into SVN? Something like an @CHANGES tag in a svn commit message? -- Nick Kew
Re: RFC: Documenting changes in the CHANGES file
On 6/2/20 2:17 PM, Stefan Eissing wrote: > >> Am 02.06.2020 um 14:11 schrieb Daniel Ruggeri : >> >> On 6/1/2020 6:23 AM, Yann Ylavic wrote: >>> On Fri, May 29, 2020 at 9:30 PM Ruediger Pluem >>> wrote: >>> Reviewing our backport process I noticed that in many cases a clean merge via svn merge fails due to conflicts in CHANGES. While these are easy to solve it puts IMHO unnecessary extra work on the backport process, both for reviewing and for actually doing the backport. How about if we change the way we document changes the following way: 1. We create a changes-fragments directory (name to be determined) at the top level. 2. For each release we create a subdirectory such that we end up with the following structure: changes-fragments/ 2.4.41/ 2.4.42/ 2.4.43/ 2.4.44/ 3. Each directory contains the changes for each release and each change entry is a single file. 4. We have a script that builds our current CHANGES file from the content in changes-fragments directories with the help of a template or at least some sort of header / footer that is static. 5. This script can be called either manually and we commit the resulting CHANGES file as we like just like the x-forms commits for documentation plus this script is called by the release scripts from Daniel as part of the preparation of rolling a release. >>> +1 from me, I don't volonteer for the scripts though :) >>> >>> Regards; >>> Yann. >>> >> Hi, Yann; >> >> I'm open to whatever... and don't mind writing or tweaking scripts once we >> decide on an approach :-) >> >> While we are discussing ideas in this neighborhood, one thing to keep in >> mind is that during release of security fixes, sometimes there are items >> added to CHANGES and sometimes CHANGES is modified to add CVE information. >> There have been minor bumps in the road where these patches don't always >> apply cleanly. So, if possible, it would be great to consider. There may be >> nothing to do, though, since that happens way after backport. >> > > > +1 from me as well. CHANGES is annoying atm, any automation appreciated. > Thanks for all the feedback. I try to work out something more detailed aka patch that we can discuss then. Regards Rüdiger
Reject HTTP protocols >= 2.0 in ap_parse_request_line?
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.
A possible patch could look like the following (which rejects such requests
with a HTTP_VERSION_NOT_SUPPORTED status code):
Index: server/protocol.c
===
--- server/protocol.c (revision 1878470)
+++ server/protocol.c (working copy)
@@ -748,7 +748,7 @@ AP_DECLARE(int) ap_parse_request_line(request_rec
enum {
rrl_none, rrl_badmethod, rrl_badwhitespace, rrl_excesswhitespace,
rrl_missinguri, rrl_baduri, rrl_badprotocol, rrl_trailingtext,
-rrl_badmethod09, rrl_reject09
+rrl_badmethod09, rrl_reject09, rrl_versionnotsupported
} deferred_error = rrl_none;
apr_size_t len = 0;
char *uri, *ll;
@@ -897,6 +897,11 @@ rrl_done:
r->proto_num = HTTP_VERSION(0, 9);
}
+if (strict && deferred_error == rrl_none
+&& r->proto_num >= HTTP_VERSION(2, 0)) {
+deferred_error = rrl_versionnotsupported;
+}
+
/* Determine the method_number and parse the uri prior to invoking error
* handling, such that these fields are available for substitution
*/
@@ -918,6 +923,7 @@ rrl_done:
* we can safely resume any deferred error reporting
*/
if (deferred_error != rrl_none) {
+r->status = HTTP_BAD_REQUEST;
if (deferred_error == rrl_badmethod)
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03445)
"HTTP Request Line; Invalid method token: '%.*s'",
@@ -954,7 +960,13 @@ rrl_done:
"HTTP Request Line; Unrecognized protocol '%.*s' "
"(perhaps whitespace was injected?)",
field_name_len(r->protocol), r->protocol);
-r->status = HTTP_BAD_REQUEST;
+else if (deferred_error == rrl_versionnotsupported) {
+ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO()
+ "HTTP Request Line; Protocol '%.*s' >= HTTP/2.0 not"
+ " supported", field_name_len(r->protocol),
+ r->protocol);
+r->status = HTTP_VERSION_NOT_SUPPORTED;
+}
goto rrl_failed;
}
Regards
Rüdiger
