You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by wr...@apache.org on 2016/07/29 17:37:41 UTC
svn commit: r1754556 - /httpd/httpd/trunk/server/protocol.c
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@
Modified:
httpd/httpd/trunk/server/protocol.c
Modified: httpd/httpd/trunk/server/protocol.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=1754556&r1=1754555&r2=1754556&view=diff
==============================================================================
--- httpd/httpd/trunk/server/protocol.c (original)
+++ httpd/httpd/trunk/server/protocol.c Fri Jul 29 17:37:41 2016
@@ -900,71 +900,106 @@ AP_DECLARE(void) ap_get_mime_headers_cor
return;
}
- if (!(value = strchr(last_field, ':'))) { /* Find ':' or */
- r->status = HTTP_BAD_REQUEST; /* abort bad request */
- apr_table_setn(r->notes, "error-notes",
- apr_psprintf(r->pool,
- "Request header field is "
- "missing ':' separator.<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(00564)
- "Request header field is missing ':' "
- "separator: %.*s", (int)LOG_NAME_MAX_LEN,
- last_field);
- 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",
+ apr_psprintf(r->pool,
+ "Request header field is "
+ "missing ':' separator.<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(00564)
+ "Request header field is missing ':' "
+ "separator: %.*s", (int)LOG_NAME_MAX_LEN,
+ last_field);
+ return;
+ }
- tmp_field = value - 1; /* last character of field-name */
+ tmp_field = value - 1; /* last character of field-name */
- *value++ = '\0'; /* NUL-terminate at colon */
+ *value++ = '\0'; /* NUL-terminate at colon */
- while (*value == ' ' || *value == '\t') {
- ++value; /* Skip to start of value */
- }
+ while (*value == ' ' || *value == '\t') {
+ ++value; /* Skip to start of value */
+ }
- /* Strip LWS after field-name: */
- while (tmp_field > last_field
- && (*tmp_field == ' ' || *tmp_field == '\t')) {
- *tmp_field-- = '\0';
- }
+ /* Strip LWS after field-name: */
+ while (tmp_field > last_field
+ && (*tmp_field == ' ' || *tmp_field == '\t')) {
+ *tmp_field-- = '\0';
+ }
- /* Strip LWS after field-value: */
- tmp_field = last_field + last_len - 1;
- while (tmp_field > value
- && (*tmp_field == ' ' || *tmp_field == '\t')) {
- *tmp_field-- = '\0';
+ /* Strip LWS after field-value: */
+ tmp_field = last_field + last_len - 1;
+ while (tmp_field > value
+ && (*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';
+ }
+ ++tmp_field;
}
- else if (ap_has_cntrl(value)) {
- err = HTTP_BAD_REQUEST;
+
+ /* 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 field '%.*s' contains "
- "control character", (int)LOG_NAME_MAX_LEN,
- last_field);
- }
- if (err && !(conf->http_conformance & AP_HTTP_CONFORMANCE_LOGONLY)) {
- r->status = err;
+ "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
Re: svn commit: r1754556 - /httpd/httpd/trunk/server/protocol.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Fri, Jul 29, 2016 at 1:49 PM, Ruediger Pluem <rp...@apache.org> wrote:
>
> > + 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))
>
> Hm, how could ap_scan_http_token ever return NULL?
>
You are correct, I'd started with ap_get_http_token, but there was no
reason for an extra copy. Thanks for both catches!
Re: svn commit: r1754556 - /httpd/httpd/trunk/server/protocol.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 07/29/2016 07: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@
>
>
> Modified:
> httpd/httpd/trunk/server/protocol.c
>
> Modified: httpd/httpd/trunk/server/protocol.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=1754556&r1=1754555&r2=1754556&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/protocol.c (original)
> +++ httpd/httpd/trunk/server/protocol.c Fri Jul 29 17:37:41 2016
> @@ -900,71 +900,106 @@ AP_DECLARE(void) ap_get_mime_headers_cor
> return;
> }
>
> - if (!(value = strchr(last_field, ':'))) { /* Find ':' or */
> - r->status = HTTP_BAD_REQUEST; /* abort bad request */
> - apr_table_setn(r->notes, "error-notes",
> - apr_psprintf(r->pool,
> - "Request header field is "
> - "missing ':' separator.<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(00564)
> - "Request header field is missing ':' "
> - "separator: %.*s", (int)LOG_NAME_MAX_LEN,
> - last_field);
> - 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",
> + apr_psprintf(r->pool,
> + "Request header field is "
> + "missing ':' separator.<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(00564)
> + "Request header field is missing ':' "
> + "separator: %.*s", (int)LOG_NAME_MAX_LEN,
> + last_field);
> + return;
> + }
>
> - tmp_field = value - 1; /* last character of field-name */
> + tmp_field = value - 1; /* last character of field-name */
>
> - *value++ = '\0'; /* NUL-terminate at colon */
> + *value++ = '\0'; /* NUL-terminate at colon */
>
> - while (*value == ' ' || *value == '\t') {
> - ++value; /* Skip to start of value */
> - }
> + while (*value == ' ' || *value == '\t') {
> + ++value; /* Skip to start of value */
> + }
>
> - /* Strip LWS after field-name: */
> - while (tmp_field > last_field
> - && (*tmp_field == ' ' || *tmp_field == '\t')) {
> - *tmp_field-- = '\0';
> - }
> + /* Strip LWS after field-name: */
> + while (tmp_field > last_field
> + && (*tmp_field == ' ' || *tmp_field == '\t')) {
> + *tmp_field-- = '\0';
> + }
>
> - /* Strip LWS after field-value: */
> - tmp_field = last_field + last_len - 1;
> - while (tmp_field > value
> - && (*tmp_field == ' ' || *tmp_field == '\t')) {
> - *tmp_field-- = '\0';
> + /* Strip LWS after field-value: */
> + tmp_field = last_field + last_len - 1;
> + while (tmp_field > value
> + && (*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))
Hm, how could ap_scan_http_token ever return NULL?
> + || *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;
> + }
Regards
R�diger
Re: svn commit: r1754556 - /httpd/httpd/trunk/server/protocol.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Fri, Jul 29, 2016 at 12:37 PM, <wr...@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