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/11/14 17:59:10 UTC
svn commit: r1769669 [2/2] - in
/httpd/httpd/branches/2.4.x-merge-http-strict: ./ docs/manual/
docs/manual/mod/ include/ server/
Modified: httpd/httpd/branches/2.4.x-merge-http-strict/server/gen_test_char.c
URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x-merge-http-strict/server/gen_test_char.c?rev=1769669&r1=1769668&r2=1769669&view=diff
==============================================================================
--- httpd/httpd/branches/2.4.x-merge-http-strict/server/gen_test_char.c (original)
+++ httpd/httpd/branches/2.4.x-merge-http-strict/server/gen_test_char.c Mon Nov 14 17:59:10 2016
@@ -53,11 +53,12 @@
#define T_ESCAPE_FORENSIC (0x20)
#define T_ESCAPE_URLENCODED (0x40)
#define T_HTTP_CTRLS (0x80)
+#define T_URI_RFC3986 (0x100)
int main(int argc, char *argv[])
{
unsigned c;
- unsigned char flags;
+ unsigned short flags;
printf("/* this file is automatically generated by gen_test_char, "
"do not edit */\n"
@@ -69,8 +70,9 @@ 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"
"\n"
- "static const unsigned char test_char_table[256] = {",
+ "static const unsigned short test_char_table[256] = {",
T_ESCAPE_SHELL_CMD,
T_ESCAPE_PATH_SEGMENT,
T_OS_ESCAPE_PATH,
@@ -78,7 +80,8 @@ int main(int argc, char *argv[])
T_ESCAPE_LOGITEM,
T_ESCAPE_FORENSIC,
T_ESCAPE_URLENCODED,
- T_HTTP_CTRLS);
+ T_HTTP_CTRLS,
+ T_URI_RFC3986);
for (c = 0; c < 256; ++c) {
flags = 0;
@@ -110,7 +113,7 @@ int main(int argc, char *argv[])
flags |= T_ESCAPE_PATH_SEGMENT;
}
- if (!apr_isalnum(c) && !strchr("$-_.+!*'(),:@&=/~", c)) {
+ if (!apr_isalnum(c) && !strchr("$-_.+!*'(),:;@&=/~", c)) {
flags |= T_OS_ESCAPE_PATH;
}
@@ -122,7 +125,7 @@ int main(int argc, char *argv[])
* and "tspecials" (RFC2068) a.k.a. "separators" (RFC2616), which
* is easer to express as characters remaining in the ASCII token set
*/
- if (!(apr_isalnum(c) || strchr("!#$%&'*+-.^_`|~", c))) {
+ if (!c || !(apr_isalnum(c) || strchr("!#$%&'*+-.^_`|~", c))) {
flags |= T_HTTP_TOKEN_STOP;
}
@@ -136,6 +139,17 @@ int main(int argc, char *argv[])
flags |= T_HTTP_CTRLS;
}
+ /* From RFC3986, the specific sets of gen-delims, sub-delims (2.2),
+ * 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;
+ }
+
/* For logging, escape all control characters,
* double quotes (because they delimit the request in the log file)
* backslashes (because we use backslash for escaping)
@@ -153,7 +167,7 @@ int main(int argc, char *argv[])
flags |= T_ESCAPE_FORENSIC;
}
- printf("0x%02x%c", flags, (c < 255) ? ',' : ' ');
+ printf("0x%03x%c", flags, (c < 255) ? ',' : ' ');
}
printf("\n};\n");
Modified: httpd/httpd/branches/2.4.x-merge-http-strict/server/protocol.c
URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x-merge-http-strict/server/protocol.c?rev=1769669&r1=1769668&r2=1769669&view=diff
==============================================================================
--- httpd/httpd/branches/2.4.x-merge-http-strict/server/protocol.c (original)
+++ httpd/httpd/branches/2.4.x-merge-http-strict/server/protocol.c Mon Nov 14 17:59:10 2016
@@ -204,7 +204,7 @@ AP_DECLARE(apr_time_t) ap_rationalize_mt
* APR_ENOSPC is returned if there is not enough buffer space.
* Other errors may be returned on other errors.
*
- * The LF is *not* returned in the buffer. Therefore, a *read of 0
+ * The [CR]LF are *not* returned in the buffer. Therefore, a *read of 0
* indicates that an empty line was read.
*
* Notes: Because the buffer uses 1 char for NUL, the most we can return is
@@ -215,13 +215,15 @@ AP_DECLARE(apr_time_t) ap_rationalize_mt
*/
AP_DECLARE(apr_status_t) ap_rgetline_core(char **s, apr_size_t n,
apr_size_t *read, request_rec *r,
- int fold, apr_bucket_brigade *bb)
+ int flags, apr_bucket_brigade *bb)
{
apr_status_t rv;
apr_bucket *e;
apr_size_t bytes_handled = 0, current_alloc = 0;
char *pos, *last_char = *s;
int do_alloc = (*s == NULL), saw_eos = 0;
+ int fold = flags & AP_GETLINE_FOLD;
+ int crlf = flags & AP_GETLINE_CRLF;
/*
* Initialize last_char as otherwise a random value will be compared
@@ -327,6 +329,13 @@ AP_DECLARE(apr_status_t) ap_rgetline_cor
}
}
+ if (crlf && (last_char <= *s || last_char[-1] != APR_ASCII_CR)) {
+ *last_char = '\0';
+ bytes_handled = last_char - *s;
+ *read = bytes_handled;
+ return APR_EINVAL;
+ }
+
/* Now NUL-terminate the string at the end of the line;
* if the last-but-one character is a CR, terminate there */
if (last_char > *s && last_char[-1] == APR_ASCII_CR) {
@@ -471,7 +480,7 @@ AP_DECLARE(apr_status_t) ap_rgetline(cha
}
#endif
-AP_DECLARE(int) ap_getline(char *s, int n, request_rec *r, int fold)
+AP_DECLARE(int) ap_getline(char *s, int n, request_rec *r, int flags)
{
char *tmp_s = s;
apr_status_t rv;
@@ -479,7 +488,7 @@ AP_DECLARE(int) ap_getline(char *s, int
apr_bucket_brigade *tmp_bb;
tmp_bb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
- rv = ap_rgetline(&tmp_s, n, &len, r, fold, tmp_bb);
+ rv = ap_rgetline(&tmp_s, n, &len, r, flags, tmp_bb);
apr_brigade_destroy(tmp_bb);
/* Map the out-of-space condition to the old API. */
@@ -573,7 +582,8 @@ static int read_request_line(request_rec
{
enum {
rrl_none, rrl_badmethod, rrl_badwhitespace, rrl_excesswhitespace,
- rrl_missinguri, rrl_badprotocol, rrl_trailingtext
+ rrl_missinguri, rrl_baduri, rrl_badprotocol, rrl_trailingtext,
+ rrl_badmethod09, rrl_reject09
} deferred_error = rrl_none;
char *ll;
char *uri;
@@ -581,8 +591,7 @@ 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);
/* Read past empty lines until we get a real request line,
* a read error, the connection closes (EOF), or we timeout.
@@ -607,7 +616,7 @@ static int read_request_line(request_rec
*/
r->the_request = NULL;
rv = ap_rgetline(&(r->the_request), (apr_size_t)(r->server->limit_req_line + 2),
- &len, r, 0, bb);
+ &len, r, strict ? AP_GETLINE_CRLF : 0, bb);
if (rv != APR_SUCCESS) {
r->request_time = apr_time_now();
@@ -640,14 +649,21 @@ static int read_request_line(request_rec
r->request_time = apr_time_now();
r->method = r->the_request;
- if (apr_isspace(*r->method))
- deferred_error = rrl_excesswhitespace;
- for ( ; apr_isspace(*r->method); ++r->method)
- ;
+ /* If there is whitespace before a method, skip it and mark in error */
+ if (apr_isspace(*r->method)) {
+ deferred_error = rrl_badwhitespace;
+ 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))) {
+ if (((ll == r->method) || (*ll && !apr_isspace(*ll)))
+ && deferred_error == rrl_none) {
deferred_error = rrl_badmethod;
ll = strpbrk(ll, "\t\n\v\f\r ");
}
@@ -655,6 +671,8 @@ static int read_request_line(request_rec
else {
ll = strpbrk(r->method, "\t\n\v\f\r ");
}
+
+ /* Verify method terminated with a single SP, or mark as specific error */
if (!ll) {
if (deferred_error == rrl_none)
deferred_error = rrl_missinguri;
@@ -662,47 +680,84 @@ static int read_request_line(request_rec
len = 0;
goto rrl_done;
}
-
- if (strictspaces && ll[0] && (ll[0] != ' ' || apr_isspace(ll[1]))
- && deferred_error == rrl_none) {
+ else if (strict && ll[0] && (ll[0] != ' ' || apr_isspace(ll[1]))
+ && deferred_error == rrl_none) {
deferred_error = rrl_excesswhitespace;
}
+
+ /* Advance uri pointer over leading whitespace, NUL terminate the method
+ * If non-SP whitespace is encountered, mark as specific error
+ */
for (uri = ll; apr_isspace(*uri); ++uri)
- if (ap_strchr_c(badwhitespace, *uri) && deferred_error == rrl_none)
+ if (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;
- if (!(ll = strpbrk(uri, " \t\n\v\f\r"))) {
+ /* 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))) {
+ deferred_error = rrl_baduri;
+ ll = strpbrk(ll, "\t\n\v\f\r ");
+ }
+ }
+ else {
+ ll = strpbrk(uri, "\t\n\v\f\r ");
+ }
+
+ /* Verify method terminated with a single SP, or mark as specific error */
+ if (!ll) {
r->protocol = "";
len = 0;
goto rrl_done;
}
+ else if (strict && ll[0] && (ll[0] != ' ' || apr_isspace(ll[1]))
+ && deferred_error == rrl_none) {
+ deferred_error = rrl_excesswhitespace;
+ }
+
+ /* Advance protocol pointer over leading whitespace, NUL terminate the uri
+ * If non-SP whitespace is encountered, mark as specific error
+ */
for (r->protocol = ll; apr_isspace(*r->protocol); ++r->protocol)
- if (ap_strchr_c(badwhitespace, *r->protocol) && deferred_error == rrl_none)
+ if (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 comes later */
if (!(ll = strpbrk(r->protocol, " \t\n\v\f\r"))) {
len = strlen(r->protocol);
goto rrl_done;
}
len = ll - r->protocol;
- if (strictspaces && *ll)
- deferred_error = rrl_excesswhitespace;
- for ( ; apr_isspace(*ll); ++ll)
- if (ap_strchr_c(badwhitespace, *ll) && deferred_error == rrl_none)
- deferred_error = rrl_badwhitespace;
- if (*ll && deferred_error == rrl_none)
- deferred_error = rrl_trailingtext;
- ll = (char *)r->protocol + len;
- *ll = '\0';
+
+ /* 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 (strict && *ll) {
+ deferred_error = rrl_excesswhitespace;
+ }
+ else {
+ for ( ; apr_isspace(*ll); ++ll)
+ if (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;
+ }
+ *((char *)r->protocol + len) = '\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);
@@ -746,7 +801,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);
@@ -755,99 +810,118 @@ rrl_done:
ap_parse_uri(r, uri);
- if (r->proto_num == HTTP_VERSION(0, 9)) {
- if (conf->http09_enable == AP_HTTP09_DISABLE
- && deferred_error == rrl_none) {
- /* If we deny 0.9, send error message with 1.x behavior */
- r->assbackwards = 0;
- r->connection->keepalive = AP_CONN_CLOSE;
- ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02401)
- "HTTP Request Line; Rejected HTTP/0.9 request");
- r->status = HTTP_VERSION_NOT_SUPPORTED;
- return 0;
- }
- if (strict && strcmp(r->method, "GET") && deferred_error == rrl_none) {
- ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(03444)
- "HTTP Request Line; Invalid method token: '%.*s'"
- " (only GET is allowed for HTTP/0.9 requests)",
- field_name_len(r->method), r->method);
- r->status = HTTP_BAD_REQUEST;
- return 0;
- }
+ /* 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 && (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)
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03445)
"HTTP Request Line; Invalid method token: '%.*s'",
field_name_len(r->method), r->method);
+ else if (deferred_error == rrl_badmethod09)
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03444)
+ "HTTP Request Line; Invalid method token: '%.*s'"
+ " (only GET is allowed for HTTP/0.9 requests)",
+ field_name_len(r->method), r->method);
else if (deferred_error == rrl_missinguri)
- ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(03446)
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03446)
"HTTP Request Line; Missing URI");
+ else if (deferred_error == rrl_baduri)
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03454)
+ "HTTP Request Line; URI incorrectly encoded: '%.*s'",
+ field_name_len(r->uri), r->uri);
else if (deferred_error == rrl_badwhitespace)
- ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(03447)
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03447)
"HTTP Request Line; Invalid whitespace");
else if (deferred_error == rrl_excesswhitespace)
- ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(03448)
- "HTTP Request Line; Inappropriate whitespace "
- "(disallowed by StrictWhitespace");
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03448)
+ "HTTP Request Line; Excess whitespace "
+ "(disallowed by HttpProtocolOptions Strict");
else if (deferred_error == rrl_trailingtext)
- ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(03449)
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03449)
"HTTP Request Line; Extraneous text found '%.*s' "
"(perhaps whitespace was injected?)",
field_name_len(ll), ll);
- else if (deferred_error == rrl_badprotocol) {
- ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02418)
+ else if (deferred_error == rrl_reject09)
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02401)
+ "HTTP Request Line; Rejected HTTP/0.9 request");
+ else if (deferred_error == rrl_badprotocol)
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02418)
"HTTP Request Line; Unrecognized protocol '%.*s' "
"(perhaps whitespace was injected?)",
field_name_len(r->protocol), r->protocol);
- r->protocol = "HTTP/1.0";
- }
r->status = HTTP_BAD_REQUEST;
- return 0;
+ goto rrl_failed;
}
if (conf->http_methods == AP_HTTP_METHODS_REGISTERED
&& r->method_number == M_INVALID) {
- ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02423)
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02423)
"HTTP Request Line; Unrecognized HTTP method: '%.*s' "
"(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;
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03450)
+ "HTTP Request Line; Unable to parse URI: '%.*s'",
+ field_name_len(r->uri), r->uri);
+ goto rrl_failed;
}
if (strict) {
- if (ap_has_cntrl(r->the_request)) {
- ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02420)
+ /* 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;
- 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)
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 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)
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 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,
@@ -878,7 +952,6 @@ 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);
/*
* Read header lines until we get the empty separator line, a read error,
@@ -889,7 +962,7 @@ AP_DECLARE(void) ap_get_mime_headers_cor
field = NULL;
rv = ap_rgetline(&field, r->server->limit_req_fieldsize + 2,
- &len, r, 0, bb);
+ &len, r, strict ? AP_GETLINE_CRLF : 0, bb);
if (rv != APR_SUCCESS) {
if (APR_STATUS_IS_TIMEUP(rv)) {
@@ -937,7 +1010,7 @@ AP_DECLARE(void) ap_get_mime_headers_cor
if (last_field == NULL) {
r->status = HTTP_BAD_REQUEST;
- ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(03442)
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03442)
"Line folding encountered before first"
" header line");
return;
@@ -945,7 +1018,7 @@ AP_DECLARE(void) ap_get_mime_headers_cor
if (field[1] == '\0') {
r->status = HTTP_BAD_REQUEST;
- ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(03443)
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03443)
"Empty folded line encountered");
return;
}
@@ -991,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
@@ -1024,22 +1095,9 @@ 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)
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00564)
"Request header field is missing ':' "
"separator: %.*s", (int)LOG_NAME_MAX_LEN,
last_field);
@@ -1051,11 +1109,11 @@ AP_DECLARE(void) ap_get_mime_headers_cor
*value++ = '\0'; /* NUL-terminate at colon */
- if (strictspaces && strpbrk(last_field, " \t")) {
+ if (strpbrk(last_field, "\t\n\v\f\r ")) {
r->status = HTTP_BAD_REQUEST;
- ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(03452)
- "Request header field name with whitespace "
- "(disallowed by StrictWhitespace)");
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03452)
+ "Request header field name presented"
+ " invalid whitespace");
return;
}
@@ -1063,15 +1121,17 @@ AP_DECLARE(void) ap_get_mime_headers_cor
++value; /* Skip to start of value */
}
- /* Strip LWS after field-name: */
- while (tmp_field > last_field
- && (*tmp_field == ' ' || *tmp_field == '\t')) {
- *(tmp_field--) = '\0';
+ if (strpbrk(value, "\n\v\f\r")) {
+ r->status = HTTP_BAD_REQUEST;
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03451)
+ "Request header field value presented"
+ " bad whitespace");
+ return;
}
if (tmp_field == last_field) {
r->status = HTTP_BAD_REQUEST;
- ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(03453)
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03453)
"Request header field name was empty");
return;
}
@@ -1082,7 +1142,7 @@ AP_DECLARE(void) ap_get_mime_headers_cor
value = (char *)ap_scan_http_token(last_field);
if ((value == last_field) || *value != ':') {
r->status = HTTP_BAD_REQUEST;
- ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02426)
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02426)
"Request header field name is malformed: "
"%.*s", (int)LOG_NAME_MAX_LEN, last_field);
return;
@@ -1104,7 +1164,7 @@ AP_DECLARE(void) ap_get_mime_headers_cor
*/
if (*tmp_field != '\0') {
r->status = HTTP_BAD_REQUEST;
- ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02427)
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02427)
"Request header value is malformed: "
"%.*s", (int)LOG_NAME_MAX_LEN, value);
return;
@@ -1225,7 +1285,7 @@ request_rec *ap_read_request(conn_rec *c
r->server->limit_req_line);
}
else if (r->method == NULL) {
- ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00566)
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00566)
"request failed: malformed request line");
}
access_status = r->status;
@@ -1265,7 +1325,7 @@ request_rec *ap_read_request(conn_rec *c
ap_get_mime_headers_core(r, tmp_bb);
if (r->status != HTTP_OK) {
- ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00567)
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00567)
"request failed: error reading the headers");
ap_send_error_response(r, 0);
ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, r);
@@ -1284,7 +1344,7 @@ request_rec *ap_read_request(conn_rec *c
*/
if (!(strcasecmp(tenc, "chunked") == 0 /* fast path */
|| ap_find_last_token(r->pool, tenc, "chunked"))) {
- ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02539)
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02539)
"client sent unknown Transfer-Encoding "
"(%s): %s", tenc, r->uri);
r->status = HTTP_BAD_REQUEST;
@@ -1305,25 +1365,6 @@ request_rec *ap_read_request(conn_rec *c
apr_table_unset(r->headers_in, "Content-Length");
}
}
- else {
- if (r->header_only) {
- /*
- * Client asked for headers only with HTTP/0.9, which doesn't send
- * headers! Have to dink things just to make sure the error message
- * comes through...
- */
- ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00568)
- "client sent invalid HTTP/0.9 request: HEAD %s",
- r->uri);
- r->header_only = 0;
- r->status = HTTP_BAD_REQUEST;
- ap_send_error_response(r, 0);
- ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, r);
- ap_run_log_transaction(r);
- apr_brigade_destroy(tmp_bb);
- goto traceout;
- }
- }
apr_brigade_destroy(tmp_bb);
@@ -1355,7 +1396,7 @@ request_rec *ap_read_request(conn_rec *c
* a Host: header, and the server MUST respond with 400 if it doesn't.
*/
access_status = HTTP_BAD_REQUEST;
- ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00569)
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00569)
"client sent HTTP/1.1 request without hostname "
"(see RFC2616 section 14.23): %s", r->uri);
}
Modified: httpd/httpd/branches/2.4.x-merge-http-strict/server/util.c
URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x-merge-http-strict/server/util.c?rev=1769669&r1=1769668&r2=1769669&view=diff
==============================================================================
--- httpd/httpd/branches/2.4.x-merge-http-strict/server/util.c (original)
+++ httpd/httpd/branches/2.4.x-merge-http-strict/server/util.c Mon Nov 14 17:59:10 2016
@@ -1633,6 +1633,16 @@ AP_DECLARE(char *) ap_get_http_token(apr
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) ;
+
+ return ptr;
+}
+
/* Retrieve a token, spacing over it and returning a pointer to
* the first non-white byte afterwards. Note that these tokens
* are delimited by semis and commas; and can also be delimited
Re: svn commit: r1769669 [2/2] - in /httpd/httpd/branches/2.4.x-merge-http-strict:
./ docs/manual/ docs/manual/mod/ include/ server/
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Thu, Dec 22, 2016 at 11:37 AM, Rainer Jung <ra...@kippdata.de>
wrote:
> Am 22.12.2016 um 18:25 schrieb William A Rowe Jr:
>
>> On Thu, Dec 22, 2016 at 9:29 AM, Eric Covener <covener@gmail.com
>> <ma...@gmail.com>> wrote:
>>
>> I think the log severity changes below could use some eyes, especially
>> in context of 2.2. Are these lowered because they're redundant? I
>> haven't yet looked.
>>
>> I am tempted to leave the old severities for 2.2 and wait and see if
>> it's confusing in 2.4 (should not have to enable DEBUG to see the
>> cause of a 400 error)
>>
>>
>> Log severity is lowered because successfully throwing off a (generally
>> deliberately) bad request is of no interest to an operator at loglevel
>> error/warn, any more than any successful request is of interest.
>>
>> This is just log pollution. There is no action for the operator to take.
>>
>
> Hmmm, but if it isn't about a deliberately bad request but a bad client,
> the admin could consider setting "HttpProtocolOptions Unsafe" (and now get
> what he asked for. I must admit, I didn't check whether the log lines in
> question are in code areas which are controlled by HttpProtocolOptions or
> not.
>
That's true. However, every admin/operator knows that if the app dev team
or users are complaining that the request doesn't work, the first thing any
admin does is enable loglevel debug to see what's going on.
The CTL's aren't override-able (see the security reports 2.4 detailed
description) by our group concensus, and my UnsafeWhitespace proposal was
rejected. But there is still information at debug level to know what sort
of problem occurred without breaking out wireshark first.
Once that is sorted, it's still noise and needs to be disabled again. The
broken clients will be the very odd exception, not a rule.
Re: svn commit: r1769669 [2/2] - in
/httpd/httpd/branches/2.4.x-merge-http-strict: ./ docs/manual/
docs/manual/mod/ include/ server/
Posted by Rainer Jung <ra...@kippdata.de>.
Am 22.12.2016 um 18:25 schrieb William A Rowe Jr:
> On Thu, Dec 22, 2016 at 9:29 AM, Eric Covener <covener@gmail.com
> <ma...@gmail.com>> wrote:
>
> I think the log severity changes below could use some eyes, especially
> in context of 2.2. Are these lowered because they're redundant? I
> haven't yet looked.
>
> I am tempted to leave the old severities for 2.2 and wait and see if
> it's confusing in 2.4 (should not have to enable DEBUG to see the
> cause of a 400 error)
>
>
> Log severity is lowered because successfully throwing off a (generally
> deliberately) bad request is of no interest to an operator at loglevel
> error/warn, any more than any successful request is of interest.
>
> This is just log pollution. There is no action for the operator to take.
Hmmm, but if it isn't about a deliberately bad request but a bad client,
the admin could consider setting "HttpProtocolOptions Unsafe" (and now
get what he asked for. I must admit, I didn't check whether the log
lines in question are in code areas which are controlled by
HttpProtocolOptions or not.
Regards,
Rainer
Re: svn commit: r1769669 [2/2] - in /httpd/httpd/branches/2.4.x-merge-http-strict:
./ docs/manual/ docs/manual/mod/ include/ server/
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Thu, Dec 22, 2016 at 9:29 AM, Eric Covener <co...@gmail.com> wrote:
> I think the log severity changes below could use some eyes, especially
> in context of 2.2. Are these lowered because they're redundant? I
> haven't yet looked.
>
> I am tempted to leave the old severities for 2.2 and wait and see if
> it's confusing in 2.4 (should not have to enable DEBUG to see the
> cause of a 400 error)
Log severity is lowered because successfully throwing off a (generally
deliberately) bad request is of no interest to an operator at loglevel
error/warn, any more than any successful request is of interest.
This is just log pollution. There is no action for the operator to take.
Re: svn commit: r1769669 [2/2] - in /httpd/httpd/branches/2.4.x-merge-http-strict:
./ docs/manual/ docs/manual/mod/ include/ server/
Posted by Eric Covener <co...@gmail.com>.
I think the log severity changes below could use some eyes, especially
in context of 2.2. Are these lowered because they're redundant? I
haven't yet looked.
I am tempted to leave the old severities for 2.2 and wait and see if
it's confusing in 2.4 (should not have to enable DEBUG to see the
cause of a 400 error)
> @@ -937,7 +1010,7 @@ AP_DECLARE(void) ap_get_mime_headers_cor
>
> if (last_field == NULL) {
> r->status = HTTP_BAD_REQUEST;
> - ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(03442)
> + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03442)
> "Line folding encountered before first"
> " header line");
> return;
> @@ -945,7 +1018,7 @@ AP_DECLARE(void) ap_get_mime_headers_cor
>
> if (field[1] == '\0') {
> r->status = HTTP_BAD_REQUEST;
> - ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(03443)
> + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03443)
> "Empty folded line encountered");
> return;
> }
> @@ -991,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
> @@ -1024,22 +1095,9 @@ 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)
> + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00564)
> "Request header field is missing ':' "
> "separator: %.*s", (int)LOG_NAME_MAX_LEN,
> last_field);
> @@ -1051,11 +1109,11 @@ AP_DECLARE(void) ap_get_mime_headers_cor
>
> *value++ = '\0'; /* NUL-terminate at colon */
>
> - if (strictspaces && strpbrk(last_field, " \t")) {
> + if (strpbrk(last_field, "\t\n\v\f\r ")) {
> r->status = HTTP_BAD_REQUEST;
> - ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(03452)
> - "Request header field name with whitespace "
> - "(disallowed by StrictWhitespace)");
> + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03452)
> + "Request header field name presented"
> + " invalid whitespace");
> return;
> }
>
> @@ -1063,15 +1121,17 @@ AP_DECLARE(void) ap_get_mime_headers_cor
> ++value; /* Skip to start of value */
> }
>
> - /* Strip LWS after field-name: */
> - while (tmp_field > last_field
> - && (*tmp_field == ' ' || *tmp_field == '\t')) {
> - *(tmp_field--) = '\0';
> + if (strpbrk(value, "\n\v\f\r")) {
> + r->status = HTTP_BAD_REQUEST;
> + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03451)
> + "Request header field value presented"
> + " bad whitespace");
> + return;
> }
>
> if (tmp_field == last_field) {
> r->status = HTTP_BAD_REQUEST;
> - ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(03453)
> + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(03453)
> "Request header field name was empty");
> return;
> }
> @@ -1082,7 +1142,7 @@ AP_DECLARE(void) ap_get_mime_headers_cor
> value = (char *)ap_scan_http_token(last_field);
> if ((value == last_field) || *value != ':') {
> r->status = HTTP_BAD_REQUEST;
> - ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02426)
> + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02426)
> "Request header field name is malformed: "
> "%.*s", (int)LOG_NAME_MAX_LEN, last_field);
> return;
> @@ -1104,7 +1164,7 @@ AP_DECLARE(void) ap_get_mime_headers_cor
> */
> if (*tmp_field != '\0') {
> r->status = HTTP_BAD_REQUEST;
> - ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02427)
> + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02427)
> "Request header value is malformed: "
> "%.*s", (int)LOG_NAME_MAX_LEN, value);
> return;
> @@ -1225,7 +1285,7 @@ request_rec *ap_read_request(conn_rec *c
> r->server->limit_req_line);
> }
> else if (r->method == NULL) {
> - ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00566)
> + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00566)
> "request failed: malformed request line");
> }
> access_status = r->status;
> @@ -1265,7 +1325,7 @@ request_rec *ap_read_request(conn_rec *c
>
> ap_get_mime_headers_core(r, tmp_bb);
> if (r->status != HTTP_OK) {
> - ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00567)
> + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00567)
> "request failed: error reading the headers");
> ap_send_error_response(r, 0);
> ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, r);
> @@ -1284,7 +1344,7 @@ request_rec *ap_read_request(conn_rec *c
> */
> if (!(strcasecmp(tenc, "chunked") == 0 /* fast path */
> || ap_find_last_token(r->pool, tenc, "chunked"))) {
> - ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02539)
> + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02539)
> "client sent unknown Transfer-Encoding "
> "(%s): %s", tenc, r->uri);
> r->status = HTTP_BAD_REQUEST;
> @@ -1305,25 +1365,6 @@ request_rec *ap_read_request(conn_rec *c
> apr_table_unset(r->headers_in, "Content-Length");
> }
> }
> - else {
> - if (r->header_only) {
> - /*
> - * Client asked for headers only with HTTP/0.9, which doesn't send
> - * headers! Have to dink things just to make sure the error message
> - * comes through...
> - */
> - ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00568)
> - "client sent invalid HTTP/0.9 request: HEAD %s",
> - r->uri);
> - r->header_only = 0;
> - r->status = HTTP_BAD_REQUEST;
> - ap_send_error_response(r, 0);
> - ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, r);
> - ap_run_log_transaction(r);
> - apr_brigade_destroy(tmp_bb);
> - goto traceout;
> - }
> - }
>
> apr_brigade_destroy(tmp_bb);
>
> @@ -1355,7 +1396,7 @@ request_rec *ap_read_request(conn_rec *c
> * a Host: header, and the server MUST respond with 400 if it doesn't.
> */
> access_status = HTTP_BAD_REQUEST;
> - ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00569)
> + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00569)
> "client sent HTTP/1.1 request without hostname "
> "(see RFC2616 section 14.23): %s", r->uri);
> }