From: Martin Kraemer Date: Tue, 21 May 2002 13:03:56 +0000 (+0000) Subject: Apply a stricter check to the request line syntax, in order to prevent X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2963e526a67ab11e1443753d58fde1c982e0c8c1;p=thirdparty%2Fapache%2Fhttpd.git Apply a stricter check to the request line syntax, in order to prevent arbitrary user input to end up (unescaped) in the access_log and error_log files. Until now, garbage could be injected to spoof accesses to nonexistent (or inaccessible) resources -- of course without the client actually getting access to them. Now anything but whitespace following the " HTTP/x.y" request line is disallowed, and special characters in the request are escaped in the log. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/1.3.x@95205 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/src/CHANGES b/src/CHANGES index 26c2ddd6f0f..9eeef575112 100644 --- a/src/CHANGES +++ b/src/CHANGES @@ -1,5 +1,13 @@ Changes with Apache 1.3.25 + *) Disallow anything but whitespace on the request line after the + HTTP/x.y protocol string. That prevents arbitrary user input + from ending up in the access_log and error_log. Also, special + characters (especially control characters) are escaped in the + log file now, to make a clear distinction between client-supplied + strings (with special characters) and server-side strings. + [Martin Kraemer] + *) Get rid of DEFAULT_XFERLOG as it is not used anywhere. It was preserved by the build system, printed with "httpd -V", but apart from that completely ignored: the default transfer log diff --git a/src/include/httpd.h b/src/include/httpd.h index eb461002bd9..045b9e5d18d 100644 --- a/src/include/httpd.h +++ b/src/include/httpd.h @@ -1023,6 +1023,7 @@ API_EXPORT(char *) ap_os_escape_path(pool *p, const char *path, int partial); API_EXPORT(char *) ap_escape_html(pool *p, const char *s); API_EXPORT(char *) ap_construct_server(pool *p, const char *hostname, unsigned port, const request_rec *r); +API_EXPORT(char *) ap_escape_logitem(pool *p, const char *str); API_EXPORT(char *) ap_escape_shell_cmd(pool *p, const char *s); API_EXPORT(int) ap_count_dirs(const char *path); diff --git a/src/main/gen_test_char.c b/src/main/gen_test_char.c index fae84664442..d310dc55624 100644 --- a/src/main/gen_test_char.c +++ b/src/main/gen_test_char.c @@ -8,6 +8,7 @@ #define T_ESCAPE_PATH_SEGMENT (0x02) #define T_OS_ESCAPE_PATH (0x04) #define T_HTTP_TOKEN_STOP (0x08) +#define T_ESCAPE_LOGITEM (0x10) int main(int argc, char *argv[]) { @@ -16,25 +17,27 @@ int main(int argc, char *argv[]) printf( "/* this file is automatically generated by gen_test_char, do not edit */\n" -"#define T_ESCAPE_SHELL_CMD (%u)\n" -"#define T_ESCAPE_PATH_SEGMENT (%u)\n" -"#define T_OS_ESCAPE_PATH (%u)\n" -"#define T_HTTP_TOKEN_STOP (%u)\n" -"\n" -"static const unsigned char test_char_table[256] = {\n" -" 0,", +"#define T_ESCAPE_SHELL_CMD 0x%02x /* chars with special meaning in the shell */\n" +"#define T_ESCAPE_PATH_SEGMENT 0x%02x /* find path segment, as defined in RFC1808 */\n" +"#define T_OS_ESCAPE_PATH 0x%02x /* escape characters in a path or uri */\n" +"#define T_HTTP_TOKEN_STOP 0x%02x /* find http tokens, as defined in RFC2616 */\n" +"#define T_ESCAPE_LOGITEM 0x%02x /* filter what should go in the log file */\n" +"\n", T_ESCAPE_SHELL_CMD, T_ESCAPE_PATH_SEGMENT, T_OS_ESCAPE_PATH, - T_HTTP_TOKEN_STOP); + T_HTTP_TOKEN_STOP, + T_ESCAPE_LOGITEM + ); /* we explicitly dealt with NUL above * in case some strchr() do bogosity with it */ + printf("static const unsigned char test_char_table[256] = {\n" + " 0x00, "); /* print initial item */ + for (c = 1; c < 256; ++c) { flags = 0; - if (c % 20 == 0) - printf("\n "); /* escape_shell_cmd */ #if defined(WIN32) || defined(OS2) @@ -67,8 +70,19 @@ int main(int argc, char *argv[]) if (ap_iscntrl(c) || strchr(" \t()<>@,;:\\/[]?={}", c)) { flags |= T_HTTP_TOKEN_STOP; } - printf("%u%c", flags, (c < 255) ? ',' : ' '); + /* For logging, escape all control characters, + * double quotes (because they delimit the request in the log file) + * backslashes (because we use backslash for escaping) + * and 8-bit chars with the high bit set + */ + if (!ap_isprint(c) || c == '"' || c == '\\' || ap_iscntrl(c)) { + flags |= T_ESCAPE_LOGITEM; + } + printf("0x%02x%s", flags, (c < 255) ? ", " : " "); + + if ((c % 8) == 7) + printf(" /*0x%02x...0x%02x*/\n ", c-7, c); } printf("\n};\n"); diff --git a/src/main/http_protocol.c b/src/main/http_protocol.c index 64ab0f970db..281427834f8 100644 --- a/src/main/http_protocol.c +++ b/src/main/http_protocol.c @@ -983,7 +983,7 @@ static int read_request_line(request_rec *r) const char *uri; conn_rec *conn = r->connection; unsigned int major = 1, minor = 0; /* Assume HTTP/1.0 if non-"HTTP" protocol */ - int len; + int len, n; /* Read past empty lines until we get a real request line, * a read error, the connection closes (EOF), or we timeout. @@ -1045,12 +1045,26 @@ static int read_request_line(request_rec *r) r->assbackwards = (ll[0] == '\0'); r->protocol = ap_pstrdup(r->pool, ll[0] ? ll : "HTTP/0.9"); - if (2 == sscanf(r->protocol, "HTTP/%u.%u", &major, &minor) + if (2 == sscanf(r->protocol, "HTTP/%u.%u%n", &major, &minor, &n) && minor < HTTP_VERSION(1,0)) /* don't allow HTTP/0.1000 */ r->proto_num = HTTP_VERSION(major, minor); else r->proto_num = HTTP_VERSION(1,0); + /* Check for a valid protocol, and disallow everything but whitespace + * after the protocol string */ + while (ap_isspace(r->protocol[n])) + ++n; + if (r->protocol[n] != '\0') { + r->status = HTTP_BAD_REQUEST; + r->proto_num = HTTP_VERSION(1,0); + r->protocol = ap_pstrdup(r->pool, "HTTP/1.0"); + ap_table_setn(r->notes, "error-notes", + "The request line contained invalid characters " + "following the protocol string.

\n"); + return 0; + } + return 1; } @@ -1169,6 +1183,14 @@ API_EXPORT(request_rec *) ap_read_request(conn_rec *conn) ap_log_transaction(r); return r; } + else if (r->status == HTTP_BAD_REQUEST) { + ap_log_rerror(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, r, + "request failed: erroneous characters after protocol string: %s", + ap_escape_logitem(r->pool, r->the_request)); + ap_send_error_response(r, 0); + ap_log_transaction(r); + return r; + } return NULL; } if (!r->assbackwards) { diff --git a/src/main/util.c b/src/main/util.c index fe4beb64427..b29148657a2 100644 --- a/src/main/util.c +++ b/src/main/util.c @@ -1446,6 +1446,59 @@ API_EXPORT(int) ap_find_last_token(pool *p, const char *line, const char *tok) return (strncasecmp(&line[lidx], tok, tlen) == 0); } + +/* escape a string for logging */ +API_EXPORT(char *) ap_escape_logitem(pool *p, const char *str) +{ + char *ret; + unsigned char *d; + const unsigned char *s; + + if (str == NULL) + return NULL; + + ret = ap_palloc(p, 4 * strlen(str) + 1); /* Be safe */ + d = (unsigned char *)ret; + s = (const unsigned char *)str; + for (; *s; ++s) { + + if (TEST_CHAR(*s, T_ESCAPE_LOGITEM)) { + *d++ = '\\'; + switch(*s) { + case '\b': + *d++ = 'b'; + break; + case '\n': + *d++ = 'n'; + break; + case '\r': + *d++ = 'r'; + break; + case '\t': + *d++ = 't'; + break; + case '\v': + *d++ = 'v'; + break; + case '\\': + case '"': + *d++ = *s; + break; + default: + c2x(*s, d); + *d = 'x'; + d += 3; + } + } + else + *d++ = *s; + } + *d = '\0'; + + return ret; +} + + API_EXPORT(char *) ap_escape_shell_cmd(pool *p, const char *str) { char *cmd; diff --git a/src/modules/standard/mod_log_config.c b/src/modules/standard/mod_log_config.c index 5869fa6eaf0..078a060253d 100644 --- a/src/modules/standard/mod_log_config.c +++ b/src/modules/standard/mod_log_config.c @@ -291,8 +291,8 @@ static const char *constant_item(request_rec *dummy, char *stuff) static const char *log_remote_host(request_rec *r, char *a) { - return ap_get_remote_host(r->connection, r->per_dir_config, - REMOTE_NAME); + return ap_escape_logitem(r->pool, ap_get_remote_host(r->connection, r->per_dir_config, + REMOTE_NAME)); } static const char *log_remote_address(request_rec *r, char *a) @@ -307,7 +307,7 @@ static const char *log_local_address(request_rec *r, char *a) static const char *log_remote_logname(request_rec *r, char *a) { - return ap_get_remote_logname(r); + return ap_escape_logitem(r->pool, ap_get_remote_logname(r)); } static const char *log_remote_user(request_rec *r, char *a) @@ -320,6 +320,8 @@ static const char *log_remote_user(request_rec *r, char *a) else if (strlen(rvalue) == 0) { rvalue = "\"\""; } + else + rvalue = ap_escape_logitem(r->pool, rvalue); return rvalue; } @@ -330,10 +332,12 @@ static const char *log_request_line(request_rec *r, char *a) * (note the truncation before the protocol string for HTTP/0.9 requests) * (note also that r->the_request contains the unmodified request) */ - return (r->parsed_uri.password) ? ap_pstrcat(r->pool, r->method, " ", + return ap_escape_logitem(r->pool, + (r->parsed_uri.password) ? ap_pstrcat(r->pool, r->method, " ", ap_unparse_uri_components(r->pool, &r->parsed_uri, 0), r->assbackwards ? NULL : " ", r->protocol, NULL) - : r->the_request; + : r->the_request + ); } static const char *log_request_file(request_rec *r, char *a) @@ -342,19 +346,20 @@ static const char *log_request_file(request_rec *r, char *a) } static const char *log_request_uri(request_rec *r, char *a) { - return r->uri; + return ap_escape_logitem(r->pool, r->uri); } static const char *log_request_method(request_rec *r, char *a) { - return r->method; + return ap_escape_logitem(r->pool, r->method); } static const char *log_request_protocol(request_rec *r, char *a) { - return r->protocol; + return ap_escape_logitem(r->pool, r->protocol); } static const char *log_request_query(request_rec *r, char *a) { - return (r->args != NULL) ? ap_pstrcat(r->pool, "?", r->args, NULL) + return (r->args != NULL) ? ap_pstrcat(r->pool, "?", + ap_escape_logitem(r->pool, r->args), NULL) : ""; } static const char *log_status(request_rec *r, char *a) @@ -389,7 +394,7 @@ static const char *log_bytes_sent(request_rec *r, char *a) static const char *log_header_in(request_rec *r, char *a) { - return ap_table_get(r->headers_in, a); + return ap_escape_logitem(r->pool, ap_table_get(r->headers_in, a)); } static const char *log_header_out(request_rec *r, char *a) @@ -470,6 +475,7 @@ static const char *log_child_pid(request_rec *r, char *a) { return ap_psprintf(r->pool, "%ld", (long) getpid()); } + static const char *log_connection_status(request_rec *r, char *a) { if (r->connection->aborted) @@ -482,6 +488,7 @@ static const char *log_connection_status(request_rec *r, char *a) return "-"; } + /***************************************************************** * * Parsing the log format string diff --git a/src/support/httpd.exp b/src/support/httpd.exp index c2ae5a76ac6..cea2e954c3b 100644 --- a/src/support/httpd.exp +++ b/src/support/httpd.exp @@ -106,6 +106,7 @@ ap_dummy_mutex ap_each_byterange ap_error_log2stderr ap_escape_html +ap_escape_logitem ap_escape_path_segment ap_escape_quotes ap_escape_shell_cmd