httpd-cvs mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From wr...@apache.org
Subject svn commit: r1757589 - in /httpd/httpd/trunk: docs/manual/mod/core.xml include/http_core.h server/core.c server/protocol.c
Date Thu, 25 Aug 2016 01:46:20 GMT
Author: wrowe
Date: Thu Aug 25 01:46:20 2016
New Revision: 1757589

URL: http://svn.apache.org/viewvc?rev=1757589&view=rev
Log:
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.





Modified:
    httpd/httpd/trunk/docs/manual/mod/core.xml
    httpd/httpd/trunk/include/http_core.h
    httpd/httpd/trunk/server/core.c
    httpd/httpd/trunk/server/protocol.c

Modified: httpd/httpd/trunk/docs/manual/mod/core.xml
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/docs/manual/mod/core.xml?rev=1757589&r1=1757588&r2=1757589&view=diff
==============================================================================
--- httpd/httpd/trunk/docs/manual/mod/core.xml (original)
+++ httpd/httpd/trunk/docs/manual/mod/core.xml Thu Aug 25 01:46:20 2016
@@ -1253,9 +1253,9 @@ EnableSendfile On
 <name>HttpProtocolOptions</name>
 <description>Modify restrictions on HTTP Request Messages</description>
 <syntax>HttpProtocolOptions [Strict|Unsafe] [StrictURL|UnsafeURL]
- [StrictWhitespace|LenientWhitespace] [RegisteredMethods|LenientMethods]
+ [StrictWhitespace|UnsafeWhitespace] [RegisteredMethods|LenientMethods]
  [Allow0.9|Require1.0]</syntax>
-<default>HttpProtocolOptions Strict StrictURL LenientWhitespace 
+<default>HttpProtocolOptions Strict StrictURL StrictWhitespace 
 LenientMethods Allow0.9</default>
 <contextlist><context>server config</context>
 <context>virtual host</context></contextlist>
@@ -1286,19 +1286,32 @@ LenientMethods Allow0.9</default>
     the default <code>Strict</code> operating mode.</p>
 
     <p><a href="https://tools.ietf.org/html/rfc3986#section-2.2"
-         >RFC 7230 &sect;2.2 and 2.3</a> define "Reserved Characters" and
+         >RFC 3986 &sect;2.2 and 2.3</a> 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 <code>StrictURI</code> option will reject all requests 
     containing invalid characters. This rule can be relaxed with the
     <code>UnsafeURI</code> option to support badly written user-agents.</p>
     
-    <p>Users are strongly cautioned against toggling the <code>Unsafe</code>
-    and <code>UnsafeURI</code> 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.</p>
+    <p><a href="https://tools.ietf.org/html/rfc7230#section-3.5"
+         >RFC 7230 &sect;3.5</a> "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 <code>StrictWhitespace</code>
+    option will now reject non-conforming requests. The administrator may
+    toggle the <code>UnsafeWhitespace</code> option to continue to honor
+    non-conforming requests, with considerable risk of proxy interactions.</p>
+
+    <p>Users are strongly cautioned against toggling the <code>Unsafe</code>,
+    <code>UnsafeURI</code> or <code>UnsafeWhitespace</code> 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.</p>
 
     <p>Reviewing the messages logged to the <directive>ErrorLog</directive>,
     configured with <directive>LogLevel</directive> <code>info</code>
level,
@@ -1306,19 +1319,6 @@ LenientMethods Allow0.9</default>
     Users should pay particular attention to any 400 responses in the access
     log for indiciations that valid requests are unexpectedly rejected.</p>
 
-    <p><a href="https://tools.ietf.org/html/rfc7230#section-3.5"
-         >RFC 7230 &sect;3.5</a> "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 <code>LenientWhitespace</code> continues to accept such
-    requests from non-conforming user-agents, but the administrator may toggle
-    the <code>StrictWhitespace</code> 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.</p>
-
     <p><a href="https://tools.ietf.org/html/rfc7231#section-4.1"
          >RFC 7231 &sect;4.1</a> "Request Methods" "Overview" requires that
     origin servers shall respond with an error when an unsupported method

Modified: httpd/httpd/trunk/include/http_core.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/http_core.h?rev=1757589&r1=1757588&r2=1757589&view=diff
==============================================================================
--- httpd/httpd/trunk/include/http_core.h (original)
+++ httpd/httpd/trunk/include/http_core.h Thu Aug 25 01:46:20 2016
@@ -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;
 

Modified: httpd/httpd/trunk/server/core.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/core.c?rev=1757589&r1=1757588&r2=1757589&view=diff
==============================================================================
--- httpd/httpd/trunk/server/core.c (original)
+++ httpd/httpd/trunk/server/core.c Thu Aug 25 01:46:20 2016
@@ -4040,19 +4040,19 @@ static const char *set_http_protocol_opt
         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_opt
                " 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)

Modified: httpd/httpd/trunk/server/protocol.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=1757589&r1=1757588&r2=1757589&view=diff
==============================================================================
--- httpd/httpd/trunk/server/protocol.c (original)
+++ httpd/httpd/trunk/server/protocol.c Thu Aug 25 01:46:20 2016
@@ -582,9 +582,8 @@ static int read_request_line(request_rec
     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->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
         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
         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
     *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_cor
     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_cor
             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_cor
             }
             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_cor
             {
                 /* 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)



Mime
View raw message