From: William A. Rowe Jr Date: Thu, 25 Aug 2016 01:46:20 +0000 (+0000) Subject: Rename LenientWhitespace to UnsafeWhitespace and change StrictWhitespace X-Git-Tag: 2.5.0-alpha~1210 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6cae6f2487a1cf2f41c712a126e1b4bb370cbd57;p=thirdparty%2Fapache%2Fhttpd.git Rename LenientWhitespace to UnsafeWhitespace and change StrictWhitespace to the default behavior, after discussion with fielding et al about the purpose of section 3.5. Update the documentation to clarify this. This patch removes whitespace considerations from the Strict|Unsafe toggle and consolidates them all in the StrictWhitespace|UnsafeWhitespace toggle. Added a bunch of logic comments to read_request_line parsing. Dropped the badwhitespace list for an all-or-nothing toggle in rrl. Leading space before the method is optimized to be evaluated only once. Toggled the request from HTTP/0.9 to HTTP/1.0 for more BAD_REQUEST cases. Moved s/[\n\v\f\r]/ / cleanup logic earlier in the cycle, to operate on each individual line read, and catch bad whitespace errors earlier. This changes the obs-fold to more efficiently condense whitespace and forces concatinatination with a single SP, always. Overrides are not necessary since obs-fold is clearly deprecated. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1757589 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/docs/manual/mod/core.xml b/docs/manual/mod/core.xml index 5be625a447b..2bc7730b672 100644 --- a/docs/manual/mod/core.xml +++ b/docs/manual/mod/core.xml @@ -1253,9 +1253,9 @@ EnableSendfile On HttpProtocolOptions Modify restrictions on HTTP Request Messages HttpProtocolOptions [Strict|Unsafe] [StrictURL|UnsafeURL] - [StrictWhitespace|LenientWhitespace] [RegisteredMethods|LenientMethods] + [StrictWhitespace|UnsafeWhitespace] [RegisteredMethods|LenientMethods] [Allow0.9|Require1.0] -HttpProtocolOptions Strict StrictURL LenientWhitespace +HttpProtocolOptions Strict StrictURL StrictWhitespace LenientMethods Allow0.9 server config virtual host @@ -1286,19 +1286,32 @@ LenientMethods Allow0.9 the default Strict operating mode.

RFC 7230 §2.2 and 2.3 define "Reserved Characters" and + >RFC 3986 §2.2 and 2.3 define "Reserved Characters" and "Unreserved Characters". All other character octets are required to be %XX encoded under this spec, and RFC7230 defers to these requirements. By default the StrictURI option will reject all requests containing invalid characters. This rule can be relaxed with the UnsafeURI option to support badly written user-agents.

-

Users are strongly cautioned against toggling the Unsafe - and UnsafeURI modes of operation, most especially on - outward-facing, publicly accessible server deployments. If an interface - is required of faulty monitoring or other custom software running only - on an intranet, users should consider toggling these only for a specific - virtual host configured on their private subnet.

+

RFC 7230 §3.5 "Message Parsing Robustness" permits, and + identifies potential risks of parsing messages containing non-space + character whitespace. While the spec defines that exactly one space + seperates the URI from the method, and the protocol from the URI, and + only space and horizontal tab characters are allowed in request header + field contents, the Apache HTTP Server was traditionally lenient in + accepting other whitespace. The default StrictWhitespace + option will now reject non-conforming requests. The administrator may + toggle the UnsafeWhitespace option to continue to honor + non-conforming requests, with considerable risk of proxy interactions.

+ +

Users are strongly cautioned against toggling the Unsafe, + UnsafeURI or UnsafeWhitespace modes of operation + particularly on outward-facing, publicly accessible server deployments. + If an interface is required for faulty monitoring or other custom service + consumers running on an intranet, users should toggle only those Unsafe + options which are necessary, and only on a specific virtual host configured + to service only their internal private network.

Reviewing the messages logged to the ErrorLog, configured with LogLevel info level, @@ -1306,19 +1319,6 @@ LenientMethods Allow0.9 Users should pay particular attention to any 400 responses in the access log for indiciations that valid requests are unexpectedly rejected.

-

RFC 7230 §3.5 "Message Parsing Robustness" permits, and - identifies potential risks of parsing messages containing non-space - character whitespace. While the spec defines that exactly one space - seperates the URI from the method, and the protocol from the URI, the - Apache HTTP Server has traditionally been lenient in accepting other - whitespace including one or more horizontal-tab or space characters. - The default LenientWhitespace continues to accept such - requests from non-conforming user-agents, but the administrator may toggle - the StrictWhitespace option to insist on precisely two spaces - in the request line. Other whitespace including vertical-tab, form-feed, - and carriage-return characters are rejected and cannot be supported.

-

RFC 7231 §4.1 "Request Methods" "Overview" requires that origin servers shall respond with an error when an unsupported method diff --git a/include/http_core.h b/include/http_core.h index 7bbdde51dba..cac1f3b303d 100644 --- a/include/http_core.h +++ b/include/http_core.h @@ -740,7 +740,7 @@ typedef struct { char http_conformance; #define AP_HTTP_WHITESPACE_UNSET 0 -#define AP_HTTP_WHITESPACE_LENIENT 1 +#define AP_HTTP_WHITESPACE_UNSAFE 1 #define AP_HTTP_WHITESPACE_STRICT 2 char http_whitespace; diff --git a/server/core.c b/server/core.c index 0af04c461b9..4280204354b 100644 --- a/server/core.c +++ b/server/core.c @@ -4040,19 +4040,19 @@ static const char *set_http_protocol_options(cmd_parms *cmd, void *dummy, conf->http_stricturi |= AP_HTTP_URI_UNSAFE; else if (strcasecmp(arg, "strictwhitespace") == 0) conf->http_whitespace |= AP_HTTP_WHITESPACE_STRICT; - else if (strcasecmp(arg, "lenientwhitespace") == 0) - conf->http_whitespace |= AP_HTTP_WHITESPACE_LENIENT; + else if (strcasecmp(arg, "unsafewhitespace") == 0) + conf->http_whitespace |= AP_HTTP_WHITESPACE_UNSAFE; else if (strcasecmp(arg, "registeredmethods") == 0) conf->http_methods |= AP_HTTP_METHODS_REGISTERED; else if (strcasecmp(arg, "lenientmethods") == 0) conf->http_methods |= AP_HTTP_METHODS_LENIENT; else - return "HttpProtocolOptions accepts " + return "HttpProtocolOptions accepts " "'Unsafe' or 'Strict' (default), " "'UnsafeURI' or 'StrictURI' (default), " - "'Require1.0' or 'Allow0.9' (default), " - "'StrictWhitespace' or 'LenientWhitespace (default), and " - "'RegisteredMethods' or 'LenientMethods (default)"; + "'UnsafeWhitespace' or 'StrictWhitespace' (default), " + "'RegisteredMethods' or 'LenientMethods' (default), and " + "'Require1.0' or 'Allow0.9' (default)"; if ((conf->http09_enable & AP_HTTP09_ENABLE) && (conf->http09_enable & AP_HTTP09_DISABLE)) @@ -4070,8 +4070,8 @@ static const char *set_http_protocol_options(cmd_parms *cmd, void *dummy, " are mutually exclusive"; if ((conf->http_whitespace & AP_HTTP_WHITESPACE_STRICT) - && (conf->http_whitespace & AP_HTTP_WHITESPACE_LENIENT)) - return "HttpProtocolOptions 'StrictWhitespace' and 'LenientWhitespace'" + && (conf->http_whitespace & AP_HTTP_WHITESPACE_UNSAFE)) + return "HttpProtocolOptions 'StrictWhitespace' and 'UnsafeWhitespace'" " are mutually exclusive"; if ((conf->http_methods & AP_HTTP_METHODS_REGISTERED) diff --git a/server/protocol.c b/server/protocol.c index f45b928f297..9eab47bb545 100644 --- a/server/protocol.c +++ b/server/protocol.c @@ -582,9 +582,8 @@ static int read_request_line(request_rec *r, apr_bucket_brigade *bb) int num_blank_lines = DEFAULT_LIMIT_BLANK_LINES; core_server_config *conf = ap_get_core_module_config(r->server->module_config); int strict = (conf->http_conformance != AP_HTTP_CONFORMANCE_UNSAFE); - const char *badwhitespace = strict ? "\t\n\v\f\r" : "\n\v\f\r"; - int strictspaces = (conf->http_whitespace == AP_HTTP_WHITESPACE_STRICT); int stricturi = (conf->http_stricturi != AP_HTTP_URI_UNSAFE); + int strictspaces = (conf->http_whitespace != AP_HTTP_WHITESPACE_UNSAFE); /* Read past empty lines until we get a real request line, * a read error, the connection closes (EOF), or we timeout. @@ -642,11 +641,17 @@ static int read_request_line(request_rec *r, apr_bucket_brigade *bb) r->request_time = apr_time_now(); r->method = r->the_request; - if (apr_isspace(*r->method)) + + /* If there is whitespace before a method, skip it and mark in error */ + if (apr_isspace(*r->method)) { deferred_error = rrl_excesswhitespace; - for ( ; apr_isspace(*r->method); ++r->method) - ; + for ( ; apr_isspace(*r->method); ++r->method) + ; + } + /* Scan the method up to the next whitespace, ensure it contains only + * valid http-token characters, otherwise mark in error + */ if (strict) { ll = (char*) ap_scan_http_token(r->method); if ((ll == r->method) || (*ll && !apr_isspace(*ll))) { @@ -665,17 +670,27 @@ static int read_request_line(request_rec *r, apr_bucket_brigade *bb) goto rrl_done; } + /* Verify method terminated with a single SP, otherwise mark in error */ if (strictspaces && ll[0] && (ll[0] != ' ' || apr_isspace(ll[1])) && deferred_error == rrl_none) { deferred_error = rrl_excesswhitespace; } + + /* Advance uri pointer over leading whitespace, + * then NUL terminate the method string + */ for (uri = ll; apr_isspace(*uri); ++uri) - if (ap_strchr_c(badwhitespace, *uri) && deferred_error == rrl_none) + if (strictspaces && ap_strchr_c("\t\n\v\f\r", *uri) + && deferred_error == rrl_none) deferred_error = rrl_badwhitespace; *ll = '\0'; + if (!*uri && deferred_error == rrl_none) deferred_error = rrl_missinguri; + /* Scan the URI up to the next whitespace, ensure it contains only + * valid RFC3986 characters, otherwise mark in error + */ if (stricturi) { ll = (char*) ap_scan_http_uri_safe(uri); if (ll == uri || (*ll && !apr_isspace(*ll))) { @@ -687,23 +702,38 @@ static int read_request_line(request_rec *r, apr_bucket_brigade *bb) ll = strpbrk(uri, "\t\n\v\f\r "); } if (!ll) { - r->protocol = ""; - len = 0; - goto rrl_done; + r->protocol = ""; + len = 0; + goto rrl_done; } + + /* Advance protocol pointer over leading whitespace, + * then NUL terminate the uri string + */ for (r->protocol = ll; apr_isspace(*r->protocol); ++r->protocol) - if (ap_strchr_c(badwhitespace, *r->protocol) && deferred_error == rrl_none) + if (strictspaces && ap_strchr_c("\t\n\v\f\r", *r->protocol) + && deferred_error == rrl_none) deferred_error = rrl_badwhitespace; *ll = '\0'; + + /* Scan the protocol up to the next whitespace, validation happens + * further below + */ if (!(ll = strpbrk(r->protocol, " \t\n\v\f\r"))) { len = strlen(r->protocol); goto rrl_done; } len = ll - r->protocol; + + /* Advance over trailing whitespace, if found mark in error, + * determine if trailing text is found, unconditionally mark in error, + * finally NUL terminate the protocol string + */ if (strictspaces && *ll) deferred_error = rrl_excesswhitespace; for ( ; apr_isspace(*ll); ++ll) - if (ap_strchr_c(badwhitespace, *ll) && deferred_error == rrl_none) + if (strictspaces && ap_strchr_c("\t\n\v\f\r", *ll) + && deferred_error == rrl_none) deferred_error = rrl_badwhitespace; if (*ll && deferred_error == rrl_none) deferred_error = rrl_trailingtext; @@ -711,10 +741,8 @@ static int read_request_line(request_rec *r, apr_bucket_brigade *bb) *ll = '\0'; rrl_done: - /* For internal integrety, reconstruct the_request - * using only single SP characters, per spec. - * Once the method, uri and protocol are processed, - * we can then resume deferred error reporting + /* For internal integrety 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->protocol ? " " : NULL, r->protocol, NULL); @@ -758,7 +786,7 @@ rrl_done: r->proto_num = HTTP_VERSION(0, 9); } - /* Satisfy the method_number and uri fields prior to invoking error + /* Determine the method_number and parse the uri prior to invoking error * handling, such that these fields are available for subsitution */ r->method_number = ap_method_number_of(r->method); @@ -767,13 +795,17 @@ rrl_done: 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) deferred_error = rrl_reject09; - else if (strict && strcmp(r->method, "GET")) + else if (strict && (r->method_number != M_GET || r->header_only)) deferred_error = rrl_badmethod09; } + /* Now that the method, uri and protocol are all processed, + * we can safely resume any deferred error reporting + */ if (deferred_error != rrl_none) { if (deferred_error == rrl_badmethod) ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(03445) @@ -811,21 +843,8 @@ 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 should be treated as a simple request, when 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; - return 0; + goto rrl_failed; } if (conf->http_methods == AP_HTTP_METHODS_REGISTERED @@ -835,13 +854,14 @@ rrl_done: "(disallowed by RegisteredMethods)", field_name_len(r->method), r->method); r->status = HTTP_NOT_IMPLEMENTED; + /* This can't happen in an HTTP/0.9 request, we verified GET above */ return 0; } if (r->status != HTTP_OK) { ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(03450) "HTTP Request Line; URI parsing failed"); - return 0; + goto rrl_failed; } if (strict) { @@ -851,25 +871,41 @@ rrl_done: "HTTP Request Line; URI must not contain control" " characters"); r->status = HTTP_BAD_REQUEST; - return 0; + goto rrl_failed; } if (r->parsed_uri.fragment) { /* RFC3986 3.5: no fragment */ ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02421) "HTTP Request Line; URI must not contain a fragment"); r->status = HTTP_BAD_REQUEST; - return 0; + goto rrl_failed; } if (r->parsed_uri.user || r->parsed_uri.password) { ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02422) "HTTP Request Line; URI must not contain a " "username/password"); r->status = HTTP_BAD_REQUEST; - return 0; + goto rrl_failed; } } return 1; + +rrl_failed: + 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"; + } + return 0; } static int table_do_fn_check_lengths(void *r_, const char *key, @@ -900,7 +936,7 @@ AP_DECLARE(void) ap_get_mime_headers_core(request_rec *r, apr_bucket_brigade *bb char *tmp_field; core_server_config *conf = ap_get_core_module_config(r->server->module_config); int strict = (conf->http_conformance != AP_HTTP_CONFORMANCE_UNSAFE); - int strictspaces = (conf->http_whitespace == AP_HTTP_WHITESPACE_STRICT); + int strictspaces = (conf->http_whitespace != AP_HTTP_WHITESPACE_UNSAFE); /* * Read header lines until we get the empty separator line, a read error, @@ -939,6 +975,23 @@ AP_DECLARE(void) ap_get_mime_headers_core(request_rec *r, apr_bucket_brigade *bb return; } + if (strictspaces && strpbrk(field, "\n\v\f\r")) { + r->status = HTTP_BAD_REQUEST; + ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(03451) + "Request header line presented bad whitespace " + "(disallowed by StrictWhitespace)"); + return; + } + else { + /* Ensure no unusual whitespace is present in the resulting + * header field input line, even in unsafe mode, by replacing + * bad whitespace with SP before collapsing whitespace + */ + char *ll = field; + while ((ll = strpbrk(ll, "\n\v\f\r"))) + *(ll++) = ' '; + } + /* For all header values, and all obs-fold lines, the presence of * additional whitespace is a no-op, so collapse trailing whitespace * to save buffer allocation and optimize copy operations. @@ -1011,9 +1064,7 @@ AP_DECLARE(void) ap_get_mime_headers_core(request_rec *r, apr_bucket_brigade *bb } memcpy(last_field + last_len, field, len +1); /* +1 for nul */ /* Replace obs-fold w/ SP per RFC 7230 3.2.4 */ - if (strict || strictspaces) { - last_field[last_len] = ' '; - } + last_field[last_len] = ' '; last_len += len; /* We've appended this obs-fold line to last_len, proceed to @@ -1044,19 +1095,6 @@ AP_DECLARE(void) ap_get_mime_headers_core(request_rec *r, apr_bucket_brigade *bb { /* Not Strict ('Unsafe' mode), using the legacy parser */ - if (strictspaces && strpbrk(last_field, "\n\v\f\r")) { - r->status = HTTP_BAD_REQUEST; - ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(03451) - "Request header presented bad whitespace " - "(disallowed by StrictWhitespace)"); - return; - } - else { - char *ll = last_field; - while ((ll = strpbrk(ll, "\n\v\f\r"))) - *(ll++) = ' '; - } - if (!(value = strchr(last_field, ':'))) { /* Find ':' or */ r->status = HTTP_BAD_REQUEST; /* abort bad request */ ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00564)