httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From William A Rowe Jr <wr...@rowe-clan.net>
Subject Re: svn commit: r1754556 - /httpd/httpd/trunk/server/protocol.c
Date Fri, 29 Jul 2016 17:42:18 GMT
On Fri, Jul 29, 2016 at 12:37 PM, <wrowe@apache.org> wrote:

> Author: wrowe
> Date: Fri Jul 29 17:37:41 2016
> New Revision: 1754556
>
> URL: http://svn.apache.org/viewvc?rev=1754556&view=rev
> Log:
> Introduce ap_scan_http_token / ap_scan_http_field_content for a much
> more efficient pass through the header text; rather than reparsing
> the strings over and over under the HTTP_CONFORMANCE_STRICT fules.
>
> Improve logic and legibility by eliminating multiple repetitive tests
> of the STRICT flag, and simply reorder 'classic' behavior first and
> this new parser second to simplify the diff. Because of the whitespace
> change (which I had wished to dodge), reading this --ignore-all-space
> is a whole lot easier. Particularly against 2.4.x branch, which is now
> identical in the 'classic' logic flow. Both of which I'll share with dev@
>

That diff to 2.4.x reads as;

--- ../../httpd-2.4/server/protocol.c 2016-07-29 12:05:52.560891394 -0500
+++ protocol.c 2016-07-29 12:17:29.333717519 -0500
@@ -825,6 +900,10 @@
                     return;
                 }

+                if (!(conf->http_conformance &
AP_HTTP_CONFORMANCE_STRICT)) {
+                {
+                    /* Not Strict, using the legacy parser */
+
                 if (!(value = strchr(last_field, ':'))) { /* Find ':' or
 */
                     r->status = HTTP_BAD_REQUEST;      /* abort bad
request */
                     apr_table_setn(r->notes, "error-notes",
@@ -862,6 +941,64 @@
                        && (*tmp_field == ' ' || *tmp_field == '\t')) {
                     *tmp_field-- = '\0';
                 }
+                }
+                else /* Using strict RFC7230 parsing */
+                {
+                    /* Ensure valid token chars before ':' per RFC 7230
3.2.4 */
+                    if (!(value = (char *)ap_scan_http_token(last_field))
+                            || *value != ':') {
+                        r->status = HTTP_BAD_REQUEST;
+                        apr_table_setn(r->notes, "error-notes",
+                            apr_psprintf(r->pool,
+                                         "Request header field name "
+                                         "is malformed.<br />\n"
+                                         "<pre>\n%.*s</pre>\n",
+                                         (int)LOG_NAME_MAX_LEN,
+                                         ap_escape_html(r->pool,
last_field)));
+                        ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
APLOGNO(02426)
+                                      "Request header field name is
malformed: "
+                                      "%.*s", (int)LOG_NAME_MAX_LEN,
last_field);
+                        return;
+                    }
+
+                    *value++ = '\0'; /* NUL-terminate last_field name at
':' */
+
+                    while (*value == ' ' || *value == '\t') {
+                        ++value;     /* Skip LWS of value */
+                    }
+
+                    /* Find invalid, non-HT ctrl char, or the trailing
NULL */
+                    tmp_field = (char *)ap_scan_http_field_content(value);
+
+                    /* Strip LWS after field-value, if string not empty */
+                    if (*value && (*tmp_field == '\0')) {
+                        tmp_field--;
+                        while (*tmp_field == ' ' || *tmp_field == '\t') {
+                            *tmp_field-- = '\0';
+                        }
+                        ++tmp_field;
+                    }
+
+                    /* Reject value for all garbage input (CTRLs excluding
HT)
+                     * e.g. only VCHAR / SP / HT / obs-text are allowed per
+                     * RFC7230 3.2.6 - leave all more explicit rule
enforcement
+                     * for specific header handler logic later in the cycle
+                     */
+                    if (*tmp_field != '\0') {
+                        r->status = HTTP_BAD_REQUEST;
+                        apr_table_setn(r->notes, "error-notes",
+                            apr_psprintf(r->pool,
+                                         "Request header value "
+                                         "is malformed.<br />\n"
+                                         "<pre>\n%.*s</pre>\n",
+                                         (int)LOG_NAME_MAX_LEN,
+                                         ap_escape_html(r->pool, value)));
+                        ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
APLOGNO(02427)
+                                      "Request header value is malformed: "
+                                      "%.*s", (int)LOG_NAME_MAX_LEN,
value);
+                        return;
+                    }
+                }

                 apr_table_addn(r->headers_in, last_field, value);


while the diff to -rPREV on trunk as --ignore-all-space reads as;

--- protocol.c.orig 2016-07-29 12:16:59.453082289 -0500
+++ protocol.c 2016-07-29 12:17:29.333717519 -0500
@@ -900,6 +900,10 @@
                     return;
                 }

+                if (!(conf->http_conformance &
AP_HTTP_CONFORMANCE_STRICT)) {
+                {
+                    /* Not Strict, using the legacy parser */
+
                 if (!(value = strchr(last_field, ':'))) { /* Find ':' or
 */
                     r->status = HTTP_BAD_REQUEST;      /* abort bad
request */
                     apr_table_setn(r->notes, "error-notes",
@@ -937,34 +941,65 @@
                        && (*tmp_field == ' ' || *tmp_field == '\t')) {
                     *tmp_field-- = '\0';
                 }
+                }
+                else /* Using strict RFC7230 parsing */
+                {
+                    /* Ensure valid token chars before ':' per RFC 7230
3.2.4 */
+                    if (!(value = (char *)ap_scan_http_token(last_field))
+                            || *value != ':') {
+                        r->status = HTTP_BAD_REQUEST;
+                        apr_table_setn(r->notes, "error-notes",
+                            apr_psprintf(r->pool,
+                                         "Request header field name "
+                                         "is malformed.<br />\n"
+                                         "<pre>\n%.*s</pre>\n",
+                                         (int)LOG_NAME_MAX_LEN,
+                                         ap_escape_html(r->pool,
last_field)));
+                        ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
APLOGNO(02426)
+                                      "Request header field name is
malformed: "
+                                      "%.*s", (int)LOG_NAME_MAX_LEN,
last_field);
+                        return;
+                    }

-                if (conf->http_conformance & AP_HTTP_CONFORMANCE_STRICT) {
-                    int err = 0;
+                    *value++ = '\0'; /* NUL-terminate last_field name at
':' */

-                    if (*last_field == '\0') {
-                        err = HTTP_BAD_REQUEST;
-                        ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
APLOGNO(02425)
-                                      "Empty request header field name not
allowed");
+                    while (*value == ' ' || *value == '\t') {
+                        ++value;     /* Skip LWS of value */
                     }
-                    else if (ap_has_cntrl(last_field)) {
-                        err = HTTP_BAD_REQUEST;
-                        ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
APLOGNO(02426)
-                                      "[HTTP strict] Request header field
name contains "
-                                      "control character: %.*s",
-                                      (int)LOG_NAME_MAX_LEN, last_field);
+
+                    /* Find invalid, non-HT ctrl char, or the trailing
NULL */
+                    tmp_field = (char *)ap_scan_http_field_content(value);
+
+                    /* Strip LWS after field-value, if string not empty */
+                    if (*value && (*tmp_field == '\0')) {
+                        tmp_field--;
+                        while (*tmp_field == ' ' || *tmp_field == '\t') {
+                            *tmp_field-- = '\0';
                     }
-                    else if (ap_has_cntrl(value)) {
-                        err = HTTP_BAD_REQUEST;
-                        ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
APLOGNO(02427)
-                                      "Request header field '%.*s'
contains "
-                                      "control character",
(int)LOG_NAME_MAX_LEN,
-                                      last_field);
+                        ++tmp_field;
                     }
-                    if (err && !(conf->http_conformance &
AP_HTTP_CONFORMANCE_LOGONLY)) {
-                        r->status = err;
+
+                    /* Reject value for all garbage input (CTRLs excluding
HT)
+                     * e.g. only VCHAR / SP / HT / obs-text are allowed per
+                     * RFC7230 3.2.6 - leave all more explicit rule
enforcement
+                     * for specific header handler logic later in the cycle
+                     */
+                    if (*tmp_field != '\0') {
+                        r->status = HTTP_BAD_REQUEST;
+                        apr_table_setn(r->notes, "error-notes",
+                            apr_psprintf(r->pool,
+                                         "Request header value "
+                                         "is malformed.<br />\n"
+                                         "<pre>\n%.*s</pre>\n",
+                                         (int)LOG_NAME_MAX_LEN,
+                                         ap_escape_html(r->pool, value)));
+                        ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
APLOGNO(02427)
+                                      "Request header value is malformed: "
+                                      "%.*s", (int)LOG_NAME_MAX_LEN,
value);
                         return;
                     }
                 }
+
                 apr_table_addn(r->headers_in, last_field, value);

                 /* reset the alloc_len so that we'll allocate a new

Mime
View raw message