Dropped the never-released ap_has_cntrls() as it had very limited

and inefficient application at that, added ap_scan_vchar_obstext()
to accomplish a similar purpose.

Dropped HttpProtocolOptions StrictURL option, this will be better
handled in the future with a specific directive and perhaps multiple
levels of scrutiny, use ap_scan_vchar_obstext() to simply ensure there
are no control characters or whitespace within the URI.

Changed the scanning of the response header table by check_headers()
to follow the same rulesets as reading request headers. Disallow any
CTL character within a response header value, and any CTL or whitespace
in response header field name, even in strict mode.

Apply HttpProtocolOptions Strict to chunk header parsing, invalid
whitespace is invalid, line termination must follow CRLF convention.
Submitted by: wrowe
Backport: r1764961,1765112-1765115 

When redrawing the parser, ap_get_http_token looked to be useful, but there's
no application for this yet in httpd, so hold off adding this function when
we backport the enhancements. ap_scan_http_token was entirely sufficient.
If the community wants this new function, we can add it when backporting
work is complete.

This patch, and the earlier patches Friday actually demanded an mmn major
bump due to struct member changes. In any final backport, new members must
be added to the end of the struct to retain an mmn minor designation.
Submitted by: wrowe
Backport: r1765451


git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x-merge-http-strict@1769672 13f79535-47bb-0310-9956-ffa450edef68

9 files changed, 109 insertions, 158 deletions

diff --git a/docs/manual/mod/core.xml b/docs/manual/mod/core.xml
index 3c68b5922c..7888f47adf 100644
--- a/docs/manual/mod/core.xml
+++ b/docs/manual/mod/core.xml
@@ -1241,9 +1241,9 @@ EnableSendfile On
 <directivesynopsis>
 <name>HttpProtocolOptions</name>
 <description>Modify restrictions on HTTP Request Messages</description>
-<syntax>HttpProtocolOptions [Strict|Unsafe] [StrictURL|UnsafeURL]
- [RegisteredMethods|LenientMethods] [Allow0.9|Require1.0]</syntax>
-<default>HttpProtocolOptions Strict StrictURL LenientMethods Allow0.9</default>
+<syntax>HttpProtocolOptions [Strict|Unsafe] [RegisteredMethods|LenientMethods]
+ [Allow0.9|Require1.0]</syntax>
+<default>HttpProtocolOptions Strict LenientMethods Allow0.9</default>
 <contextlist><context>server config</context>
 <context>virtual host</context></contextlist>
 <compatibility>2.2.32 or 2.4.24 and later</compatibility>
@@ -1255,11 +1255,11 @@ EnableSendfile On
     (<a href="https://tools.ietf.org/html/rfc7230#section-3.2"
       >RFC 7230 &sect;3.2</a>), which are now applied by default or using
     the <code>Strict</code> option. Due to legacy modules, applications or
-    custom user-agents which must be deperecated, <code>Unsafe</code>
-    and <code>UnsafeURL</code> options have been added to revert to the legacy
-    behaviors. These rules are applied prior to request processing, so must be
-    configured at the global or default (first) matching virtual host section,
-    by IP/port interface and not by name, to be honored.</p>
+    custom user-agents which must be deperecated the <code>Unsafe</code>
+    option has been added to revert to the legacy behaviors. These rules
+    are applied prior to request processing, so must be configured at the
+    global or default (first) matching virtual host section, by IP/port
+    interface (and not by name) to be honored.</p>
 
     <p>Prior to the introduction of this directive, the Apache HTTP Server
     request message parsers were tolerant of a number of forms of input
@@ -1277,21 +1277,12 @@ EnableSendfile On
     mode, and the strict whitespace suggested by section 3.5 is enforced
     and cannot be relaxed.</p>
 
-    <p><a href="https://tools.ietf.org/html/rfc3986#section-2.2"
-         >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>
-    or <code>UnsafeURI</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>
+    mode 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 the Unsafe option only on a specific virtual host configured
+    to service their internal private network.</p>
 
     <p>Reviewing the messages logged to the <directive>ErrorLog</directive>,
     configured with <directive>LogLevel</directive> <code>debug</code> level,
diff --git a/include/ap_mmn.h b/include/ap_mmn.h
index 01267ff146..124057ca7d 100644
--- a/include/ap_mmn.h
+++ b/include/ap_mmn.h
@@ -489,8 +489,9 @@
  *                          ap_proxy_check_connection().
  * 20120211.67 (2.4.24-dev) Add http09_enable, http_conformance, and
  *                          http_methods to core_server_config
- *                          Add ap_has_cntrl(), ap_get_http_token()
- *                          Add ap_scan_http_field_[content|token]()
+ *                          Add ap_scan_http_field_token(),
+ *                          ap_scan_http_field_content(),
+ *                          and ap_scan_vchar_obstext()
  *                          Replaced fold boolean with with multiple bit flags
  *                          to ap_[r]getline()
  */
diff --git a/include/http_core.h b/include/http_core.h
index 836e814717..35df5dc960 100644
--- a/include/http_core.h
+++ b/include/http_core.h
@@ -741,11 +741,6 @@ typedef struct {
 #define AP_HTTP_METHODS_REGISTERED    2
     char http_methods;
 
-#define AP_HTTP_URI_UNSET             0
-#define AP_HTTP_URI_UNSAFE            1
-#define AP_HTTP_URI_STRICT            2
-    char http_stricturi;
-
 } core_server_config;
 
 /* for AddOutputFiltersByType in core.c */
diff --git a/include/httpd.h b/include/httpd.h
index e2d4a4e2c5..eb18563348 100644
--- a/include/httpd.h
+++ b/include/httpd.h
@@ -1600,22 +1600,12 @@ AP_DECLARE(const char *) ap_scan_http_field_content(const char *ptr);
  */
 AP_DECLARE(const char *) ap_scan_http_token(const char *ptr);
 
-/* Scan a string for valid URI characters per RFC3986, and 
- * return a pointer to the first non-URI character encountered.
+/* Scan a string for visible ASCII (0x21-0x7E) or obstext (0x80+)
+ * and return a pointer to the first SP/CTL/NUL character encountered.
  * @param ptr The string to scan
- * @return A pointer to the first non-token character.
- */
-AP_DECLARE(const char *) ap_scan_http_uri_safe(const char *ptr);
-
-/* Retrieve a token, advancing the pointer to the first non-token character
- * and returning a copy of the token string.
- * @param ptr The string to scan. On return, this points to the first non-token
- *  character encountered, or NULL if *ptr was not a token character
- * @return A copy of the token string
- * @note The caller must handle leading and trailing whitespace as applicable
- *  and evaluate the terminating character.
+ * @return A pointer to the first SP/CTL character.
  */
-AP_DECLARE(char *) ap_get_http_token(apr_pool_t *p, const char **ptr);
+AP_DECLARE(const char *) ap_scan_vchar_obstext(const char *ptr);
 
 /**
  * Retrieve an array of tokens in the format "1#token" defined in RFC2616. Only
diff --git a/modules/http/http_filters.c b/modules/http/http_filters.c
index cf26bee1e6..94595b4113 100644
--- a/modules/http/http_filters.c
+++ b/modules/http/http_filters.c
@@ -126,14 +126,15 @@ static apr_status_t bail_out_on_error(http_ctx_t *ctx,
 
 /**
  * Parse a chunk line with optional extension, detect overflow.
- * There are two error cases:
+ * There are several error cases:
+ *  1) If the chunk link is misformatted, APR_EINVAL is returned.
  *  1) If the conversion would require too many bits, APR_EGENERAL is returned.
  *  2) If the conversion used the correct number of bits, but an overflow
  *     caused only the sign bit to flip, then APR_ENOSPC is returned.
- * In general, any negative number can be considered an overflow error.
+ * A negative chunk length always indicates an overflow error.
  */
 static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer,
-                                     apr_size_t len, int linelimit)
+                                     apr_size_t len, int linelimit, int strict)
 {
     apr_size_t i = 0;
 
@@ -146,6 +147,12 @@ static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer,
         if (ctx->state == BODY_CHUNK_END
                 || ctx->state == BODY_CHUNK_END_LF) {
             if (c == LF) {
+                if (strict && (ctx->state != BODY_CHUNK_END_LF)) {
+                    /*
+                     * CR missing before LF.
+                     */
+                    return APR_EINVAL;
+                }
                 ctx->state = BODY_CHUNK;
             }
             else if (c == CR && ctx->state == BODY_CHUNK_END) {
@@ -153,7 +160,7 @@ static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer,
             }
             else {
                 /*
-                 * LF expected.
+                 * CRLF expected.
                  */
                 return APR_EINVAL;
             }
@@ -180,6 +187,12 @@ static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer,
         }
 
         if (c == LF) {
+            if (strict && (ctx->state != BODY_CHUNK_LF)) {
+                /*
+                 * CR missing before LF.
+                 */
+                return APR_EINVAL;
+            }
             if (ctx->remaining) {
                 ctx->state = BODY_CHUNK_DATA;
             }
@@ -201,13 +214,14 @@ static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer,
         }
         else if (ctx->state == BODY_CHUNK_EXT) {
             /*
-             * Control chars (but tabs) are invalid.
+             * Control chars (excluding tabs) are invalid.
+             * TODO: more precisely limit input
              */
             if (c != '\t' && apr_iscntrl(c)) {
                 return APR_EINVAL;
             }
         }
-        else if (c == ' ' || c == '\t') {
+        else if (!strict && (c == ' ' || c == '\t')) {
             /* Be lenient up to 10 BWS (term from rfc7230 - 3.2.3).
              */
             ctx->state = BODY_CHUNK_CR;
@@ -324,7 +338,10 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
                             ap_input_mode_t mode, apr_read_type_e block,
                             apr_off_t readbytes)
 {
-    core_server_config *conf;
+    core_server_config *conf =
+        (core_server_config *) ap_get_module_config(f->r->server->module_config,
+                                                    &core_module);
+    int strict = (conf->http_conformance != AP_HTTP_CONFORMANCE_UNSAFE);
     apr_bucket *e;
     http_ctx_t *ctx = f->ctx;
     apr_status_t rv;
@@ -332,9 +349,6 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
     apr_bucket_brigade *bb;
     int again;
 
-    conf = (core_server_config *)
-        ap_get_module_config(f->r->server->module_config, &core_module);
-
     /* just get out of the way of things we don't want. */
     if (mode != AP_MODE_READBYTES && mode != AP_MODE_GETLINE) {
         return ap_get_brigade(f->next, b, mode, block, readbytes);
@@ -526,7 +540,7 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
                     if (rv == APR_SUCCESS) {
                         parsing = 1;
                         rv = parse_chunk_size(ctx, buffer, len,
-                                f->r->server->limit_req_fieldsize);
+                                f->r->server->limit_req_fieldsize, strict);
                     }
                     if (rv != APR_SUCCESS) {
                         ap_log_rerror(APLOG_MARK, APLOG_INFO, rv, f->r, APLOGNO(01590)
@@ -670,32 +684,49 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b,
 
 struct check_header_ctx {
     request_rec *r;
-    int error;
+    int strict;
 };
 
 /* check a single header, to be used with apr_table_do() */
 static int check_header(void *arg, const char *name, const char *val)
 {
     struct check_header_ctx *ctx = arg;
+    const char *test;
+
     if (name[0] == '\0') {
-        ctx->error = 1;
         ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, ctx->r, APLOGNO(02428)
                       "Empty response header name, aborting request");
         return 0;
     }
-    if (ap_has_cntrl(name)) {
-        ctx->error = 1;
+
+    if (ctx->strict) { 
+        test = ap_scan_http_token(name);
+    }
+    else {
+        test = ap_scan_vchar_obstext(name);
+    }
+    if (*test) {
         ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, ctx->r, APLOGNO(02429)
-                      "Response header name '%s' contains control "
+                      "Response header name '%s' contains invalid "
                       "characters, aborting request",
                       name);
         return 0;
     }
-    if (ap_has_cntrl(val)) {
-        ctx->error = 1;
+
+    if (ctx->strict) { 
+        test = ap_scan_http_field_content(val);
+    }
+    else {
+        /* Simply terminate scanning on a CTL char, allowing whitespace */
+        test = val;
+        do {
+            test = ap_scan_vchar_obstext(test);
+        } while (*test == ' ' || *test == '\t');
+    }
+    if (*test) {
         ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, ctx->r, APLOGNO(02430)
-                      "Response header '%s' contains control characters, "
-                      "aborting request: %s",
+                      "Response header '%s' value of '%s' contains invalid "
+                      "characters, aborting request",
                       name, val);
         return 0;
     }
@@ -709,11 +740,13 @@ static int check_header(void *arg, const char *name, const char *val)
 static APR_INLINE int check_headers(request_rec *r)
 {
     const char *loc;
-    struct check_header_ctx ctx = { 0, 0 };
+    struct check_header_ctx ctx;
+    core_server_config *conf =
+            ap_get_core_module_config(r->server->module_config);
 
     ctx.r = r;
-    apr_table_do(check_header, &ctx, r->headers_out, NULL);
-    if (ctx.error)
+    ctx.strict = (conf->http_conformance != AP_HTTP_CONFORMANCE_UNSAFE);
+    if (!apr_table_do(check_header, &ctx, r->headers_out, NULL))
         return 0; /* problem has been logged by check_header() */
 
     if ((loc = apr_table_get(r->headers_out, "Location")) != NULL) {
@@ -1245,7 +1278,6 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f,
     header_filter_ctx *ctx = f->ctx;
     const char *ctype;
     ap_bucket_error *eb = NULL;
-    core_server_config *conf;
 
     AP_DEBUG_ASSERT(!r->main);
 
@@ -1301,13 +1333,9 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f,
                                            r->headers_out);
     }
 
-    conf = ap_get_core_module_config(r->server->module_config);
-    if (conf->http_conformance != AP_HTTP_CONFORMANCE_UNSAFE) {
-        int ok = check_headers(r);
-        if (!ok) {
-            ap_die(HTTP_INTERNAL_SERVER_ERROR, r);
-            return AP_FILTER_ERROR;
-        }
+    if (!check_headers(r)) {
+        ap_die(HTTP_INTERNAL_SERVER_ERROR, r);
+        return AP_FILTER_ERROR;
     }
 
     /*
diff --git a/server/core.c b/server/core.c
index 01a0a936ff..8a92964508 100644
--- a/server/core.c
+++ b/server/core.c
@@ -525,9 +525,6 @@ static void *merge_core_server_configs(apr_pool_t *p, void *basev, void *virtv)
     if (virt->http_conformance != AP_HTTP_CONFORMANCE_UNSET)
         conf->http_conformance = virt->http_conformance;
 
-    if (virt->http_stricturi != AP_HTTP_URI_UNSET)
-        conf->http_stricturi = virt->http_stricturi;
-
     if (virt->http_methods != AP_HTTP_METHODS_UNSET)
         conf->http_methods = virt->http_methods;
 
@@ -3921,10 +3918,6 @@ static const char *set_http_protocol_options(cmd_parms *cmd, void *dummy,
         conf->http_conformance |= AP_HTTP_CONFORMANCE_STRICT;
     else if (strcasecmp(arg, "unsafe") == 0)
         conf->http_conformance |= AP_HTTP_CONFORMANCE_UNSAFE;
-    else if (strcasecmp(arg, "stricturi") == 0)
-        conf->http_stricturi |= AP_HTTP_URI_STRICT;
-    else if (strcasecmp(arg, "unsafeuri") == 0)
-        conf->http_stricturi |= AP_HTTP_URI_UNSAFE;
     else if (strcasecmp(arg, "registeredmethods") == 0)
         conf->http_methods |= AP_HTTP_METHODS_REGISTERED;
     else if (strcasecmp(arg, "lenientmethods") == 0)
@@ -3932,7 +3925,6 @@ static const char *set_http_protocol_options(cmd_parms *cmd, void *dummy,
     else
         return "HttpProtocolOptions accepts "
                "'Unsafe' or 'Strict' (default), "
-               "'UnsafeURI' or 'StrictURI' (default), "
                "'RegisteredMethods' or 'LenientMethods' (default), and "
                "'Require1.0' or 'Allow0.9' (default)";
 
@@ -3941,11 +3933,6 @@ static const char *set_http_protocol_options(cmd_parms *cmd, void *dummy,
         return "HttpProtocolOptions 'Allow0.9' and 'Require1.0'"
                " are mutually exclusive";
 
-    if ((conf->http_stricturi & AP_HTTP_URI_STRICT)
-            && (conf->http_stricturi & AP_HTTP_URI_UNSAFE))
-        return "HttpProtocolOptions 'StrictURI' and 'UnsafeURI'"
-               " are mutually exclusive";
-
     if ((conf->http_conformance & AP_HTTP_CONFORMANCE_STRICT)
             && (conf->http_conformance & AP_HTTP_CONFORMANCE_UNSAFE))
         return "HttpProtocolOptions 'Strict' and 'Unsafe'"
@@ -4493,8 +4480,9 @@ AP_INIT_TAKE1("ProtocolsHonorOrder", set_protocols_honor_order, NULL, RSRC_CONF,
               "'off' (default) or 'on' to respect given order of protocols, "
               "by default the client specified order determines selection"),
 AP_INIT_ITERATE("HttpProtocolOptions", set_http_protocol_options, NULL, RSRC_CONF,
-              "'Allow0.9' or 'Require1.0' (default) to allow or deny HTTP/0.9; "
-              "'Unsafe' or 'Strict' (default) to process incorrect requests"),
+                "'Allow0.9' or 'Require1.0' (default); "
+                "'RegisteredMethods' or 'LenientMethods' (default); "
+                "'Unsafe' or 'Strict' (default). Sets HTTP acceptance rules"),
 AP_INIT_ITERATE("RegisterHttpMethod", set_http_method, NULL, RSRC_CONF,
                 "Registers non-standard HTTP methods"),
 { NULL }
diff --git a/server/gen_test_char.c b/server/gen_test_char.c
index 251e7fb2e3..48ae6f47d0 100644
--- a/server/gen_test_char.c
+++ b/server/gen_test_char.c
@@ -53,7 +53,7 @@
 #define T_ESCAPE_FORENSIC     (0x20)
 #define T_ESCAPE_URLENCODED   (0x40)
 #define T_HTTP_CTRLS          (0x80)
-#define T_URI_RFC3986        (0x100)
+#define T_VCHAR_OBSTEXT      (0x100)
 
 int main(int argc, char *argv[])
 {
@@ -70,7 +70,7 @@ int main(int argc, char *argv[])
            "#define T_ESCAPE_FORENSIC      (%u)\n"
            "#define T_ESCAPE_URLENCODED    (%u)\n"
            "#define T_HTTP_CTRLS           (%u)\n"
-           "#define T_URI_RFC3986          (%u)\n"
+           "#define T_VCHAR_OBSTEXT        (%u)\n"
            "\n"
            "static const unsigned short test_char_table[256] = {",
            T_ESCAPE_SHELL_CMD,
@@ -81,7 +81,7 @@ int main(int argc, char *argv[])
            T_ESCAPE_FORENSIC,
            T_ESCAPE_URLENCODED,
            T_HTTP_CTRLS,
-           T_URI_RFC3986);
+           T_VCHAR_OBSTEXT);
 
     for (c = 0; c < 256; ++c) {
         flags = 0;
@@ -143,11 +143,8 @@ int main(int argc, char *argv[])
          * and unreserved (2.3) that are possible somewhere within a URI.
          * Spec requires all others to be %XX encoded, including obs-text.
          */
-        if (c && (strchr("%"                              /* pct-encode */
-                         ":/?#[]@"                        /* gen-delims */ 
-                         "!$&'()*+,;="                    /* sub-delims */
-                         "-._~", c) || apr_isalnum(c))) { /* unreserved */
-            flags |= T_URI_RFC3986;
+        if (c && !apr_iscntrl(c) && c != ' ') {
+            flags |= T_VCHAR_OBSTEXT;
         }
 
         /* For logging, escape all control characters,
diff --git a/server/protocol.c b/server/protocol.c
index 0dd66cae5e..3b297bd0f1 100644
--- a/server/protocol.c
+++ b/server/protocol.c
@@ -591,7 +591,6 @@ 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);
-    int stricturi = (conf->http_stricturi != AP_HTTP_URI_UNSAFE);
 
     /* Read past empty lines until we get a real request line,
      * a read error, the connection closes (EOF), or we timeout.
@@ -662,14 +661,15 @@ static int read_request_line(request_rec *r, apr_bucket_brigade *bb)
      */
     if (strict) {
         ll = (char*) ap_scan_http_token(r->method);
-        if (((ll == r->method) || (*ll && !apr_isspace(*ll)))
-                && deferred_error == rrl_none) {
-            deferred_error = rrl_badmethod;
-            ll = strpbrk(ll, "\t\n\v\f\r ");
-        }
     }
     else {
-        ll = strpbrk(r->method, "\t\n\v\f\r ");
+        ll = (char*) ap_scan_vchar_obstext(r->method);
+    }
+
+    if (((ll == r->method) || (*ll && !apr_isspace(*ll)))
+            && deferred_error == rrl_none) {
+        deferred_error = rrl_badmethod;
+        ll = strpbrk(ll, "\t\n\v\f\r ");
     }
 
     /* Verify method terminated with a single SP, or mark as specific error */
@@ -697,18 +697,13 @@ static int read_request_line(request_rec *r, apr_bucket_brigade *bb)
     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
+    /* Scan the URI up to the next whitespace, ensure it contains no raw
+     * control characters, otherwise mark in error
      */
-    if (stricturi) {
-        ll = (char*) ap_scan_http_uri_safe(uri);
-        if (ll == uri || (*ll && !apr_isspace(*ll))) {
-            deferred_error = rrl_baduri;
-            ll = strpbrk(ll, "\t\n\v\f\r ");
-        }
-    }
-    else {
-        ll = strpbrk(uri, "\t\n\v\f\r ");
+    ll = (char*) ap_scan_vchar_obstext(uri);
+    if (ll == uri || (*ll && !apr_isspace(*ll))) {
+        deferred_error = rrl_baduri;
+        ll = strpbrk(ll, "\t\n\v\f\r ");
     }
 
     /* Verify method terminated with a single SP, or mark as specific error */
@@ -732,7 +727,7 @@ static int read_request_line(request_rec *r, apr_bucket_brigade *bb)
     *ll = '\0';
 
     /* Scan the protocol up to the next whitespace, validation comes later */
-    if (!(ll = strpbrk(r->protocol, " \t\n\v\f\r"))) {
+    if (!(ll = (char*) ap_scan_vchar_obstext(r->protocol))) {
         len = strlen(r->protocol);
         goto rrl_done;
     }
@@ -742,7 +737,10 @@ static int read_request_line(request_rec *r, apr_bucket_brigade *bb)
      * determine if trailing text is found, unconditionally mark in error,
      * finally NUL terminate the protocol string
      */
-    if (strict && *ll) {
+    if (*ll && !apr_isspace(*ll)) {
+        deferred_error = rrl_badprotocol;
+    }
+    else if (strict && *ll) {
         deferred_error = rrl_excesswhitespace;
     }
     else {
@@ -881,14 +879,6 @@ rrl_done:
     }
 
     if (strict) {
-        /* No sense re-testing here for what was evaulated above */
-        if (!stricturi && ap_has_cntrl(r->the_request)) {
-            ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02420)
-                          "HTTP Request Line; URI must not contain control"
-                          " characters");
-            r->status = HTTP_BAD_REQUEST;
-            goto rrl_failed;
-        }
         if (r->parsed_uri.fragment) {
             /* RFC3986 3.5: no fragment */
             ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02421)
diff --git a/server/util.c b/server/util.c
index e8d4a5a783..fba34bde1a 100644
--- a/server/util.c
+++ b/server/util.c
@@ -1614,31 +1614,12 @@ AP_DECLARE(const char *) ap_scan_http_token(const char *ptr)
     return ptr;
 }
 
-/* Retrieve a token, advancing the pointer to the first non-token character
- * and returning a copy of the token string.
- * The caller must handle whitespace and determine the meaning of the
- * terminating character. Returns NULL if the character at **ptr is not
- * a valid token character.
+/* Scan a string for visible ASCII (0x21-0x7E) or obstext (0x80+)
+ * and return a pointer to the first ctrl/space character encountered.
  */
-AP_DECLARE(char *) ap_get_http_token(apr_pool_t *p, const char **ptr)
+AP_DECLARE(const char *) ap_scan_vchar_obstext(const char *ptr)
 {
-    const char *tok_end = ap_scan_http_token(*ptr);
-    char *tok;
-
-    if (tok_end == *ptr)
-        return NULL;
-
-    tok = apr_pstrmemdup(p, *ptr, tok_end - *ptr);
-    *ptr = tok_end;
-    return tok;
-}
-
-/* Scan a string for valid URI characters per RFC3986, and 
- * return a pointer to the first non-URI character encountered.
- */
-AP_DECLARE(const char *) ap_scan_http_uri_safe(const char *ptr)
-{
-    for ( ; TEST_CHAR(*ptr, T_URI_RFC3986); ++ptr) ;
+    for ( ; TEST_CHAR(*ptr, T_VCHAR_OBSTEXT); ++ptr) ;
 
     return ptr;
 }
@@ -2239,16 +2220,6 @@ AP_DECLARE(void) ap_bin2hex(const void *src, apr_size_t srclen, char *dest)
     *dest = '\0';
 }
 
-AP_DECLARE(int) ap_has_cntrl(const char *str)
-{
-    while (*str) {
-        if (apr_iscntrl(*str))
-            return 1;
-        str++;
-    }
-    return 0;
-}
-
 AP_DECLARE(int) ap_is_directory(apr_pool_t *p, const char *path)
 {
     apr_finfo_t finfo;