On Thu, Jun 18, 2020 at 6:03 PM Stefan Eissing
<stefan.eiss...@greenbytes.de> 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 (deferred_error == rrl_none)
deferred_error = rrl_missinguri;
- r->protocol = uri = "";
+ r->protocol = r->unparsed_uri = "";
goto rrl_done;
}
else if (strict && ll[0] && apr_isspace(ll[1])
@@ -793,19 +802,20 @@ AP_DECLARE(int) ap_parse_request_line(request_rec
/* Advance uri pointer over leading whitespace, NUL terminate the method
* If non-SP whitespace is encountered, mark as specific error
*/
- for (uri = ll; apr_isspace(*uri); ++uri)
- if (*uri != ' ' && deferred_error == rrl_none)
+ r->unparsed_uri = ll;
+ for (; apr_isspace(*r->unparsed_uri); ++r->unparsed_uri)
+ if (*r->unparsed_uri != ' ' && deferred_error == rrl_none)
deferred_error = rrl_badwhitespace;
*ll = '\0';
-
- if (!*uri && deferred_error == rrl_none)
+
+ if (!*r->unparsed_uri && deferred_error == rrl_none)
deferred_error = rrl_missinguri;
/* Scan the URI up to the next whitespace, ensure it contains no raw
* control characters, otherwise mark in error
*/
- ll = (char*) ap_scan_vchar_obstext(uri);
- if (ll == uri || (*ll && !apr_isspace(*ll))) {
+ ll = (char*) ap_scan_vchar_obstext(r->unparsed_uri);
+ if (ll == r->unparsed_uri || (*ll && !apr_isspace(*ll))) {
deferred_error = rrl_baduri;
ll = strpbrk(ll, "\t\n\v\f\r ");
}
@@ -858,7 +868,8 @@ rrl_done:
/* For internal integrity and palloc efficiency, reconstruct the_request
* in one palloc, using only single SP characters, per spec.
*/
- r->the_request = apr_pstrcat(r->pool, r->method, *uri ? " " : NULL, uri,
+ r->the_request = apr_pstrcat(r->pool, r->method,
+ *r->unparsed_uri ? " " : NULL, r->unparsed_uri,
*r->protocol ? " " : NULL, r->protocol, NULL);
if (len == 8
@@ -897,15 +908,6 @@ rrl_done:
r->proto_num = HTTP_VERSION(0, 9);
}
- /* Determine the method_number and parse the uri prior to invoking error
- * handling, such that these fields are available for substitution
- */
- r->method_number = ap_method_number_of(r->method);
- if (r->method_number == M_GET && r->method[0] == 'H')
- r->header_only = 1;
-
- ap_parse_uri(r, uri);
-
/* With the request understood, we can consider HTTP/0.9 specific errors */
if (r->proto_num == HTTP_VERSION(0, 9) && deferred_error == rrl_none) {
if (conf->http09_enable == AP_HTTP09_DISABLE)
@@ -954,10 +956,41 @@ rrl_done:
"HTTP Request Line; Unrecognized protocol '%.*s' "
"(perhaps whitespace was injected?)",
field_name_len(r->protocol), r->protocol);
+
+ if (r->proto_num == HTTP_VERSION(0, 9)) {
+ /* Send all parsing and protocol error response with 1.x behavior,
+ * and reserve 505 errors for actual HTTP protocols presented.
+ * As called out in RFC7230 3.5, any errors parsing the protocol
+ * from the request line are nearly always misencoded HTTP/1.x
+ * requests. Only a valid 0.9 request with no parsing errors
+ * at all may be treated as a simple request, if allowed.
+ */
+ r->assbackwards = 0;
+ r->connection->keepalive = AP_CONN_CLOSE;
+ r->proto_num = HTTP_VERSION(1, 0);
+ r->protocol = "HTTP/1.0";
+ }
r->status = HTTP_BAD_REQUEST;
- goto rrl_failed;
+ return 0;
}
+ return 1;
+}
+
+AP_DECLARE(int) ap_check_request_line(request_rec *r)
+{
+ core_server_config *conf = ap_get_core_module_config(r->server->module_config);
+ int strict = (conf->http_conformance != AP_HTTP_CONFORMANCE_UNSAFE);
+
+ /* Determine the method_number and parse the uri prior to invoking error
+ * handling, such that these fields are available for substitution
+ */
+ r->method_number = ap_method_number_of(r->method);
+ if (r->method_number == M_GET && r->method[0] == 'H')
+ r->header_only = 1;
+
+ ap_parse_uri(r, NULL);
+
if (conf->http_methods == AP_HTTP_METHODS_REGISTERED
&& r->method_number == M_INVALID) {
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02423)
@@ -1453,7 +1486,9 @@ request_rec *ap_read_request(conn_rec *conn)
ap_run_pre_read_request(r, conn);
/* Get the request... */
- if (!read_request_line(r, tmp_bb) || !ap_parse_request_line(r)) {
+ if (!read_request_line(r, tmp_bb)
+ || !ap_parse_request_line(r)
+ || !ap_check_request_line(r)) {
apr_brigade_cleanup(tmp_bb);
switch (r->status) {
case HTTP_REQUEST_URI_TOO_LARGE:
