diff --git a/src/core/ngx_inet.c b/src/core/ngx_inet.c index 2233e617b..7d01560d5 100644 --- a/src/core/ngx_inet.c +++ b/src/core/ngx_inet.c @@ -688,11 +688,18 @@ ngx_int_t ngx_parse_url(ngx_pool_t *pool, ngx_url_t *u) { u_char *p; - size_t len; + size_t len, i; p = u->url.data; len = u->url.len; + for (i = 0; i < len; ++i) { + if (p[i] <= 0x20 || p[i] == 0x7F) { + u->err = "Control character in URL (possible CRLF injection attack?)"; + return NGX_ERROR; + } + } + if (len >= 5 && ngx_strncasecmp(p, (u_char *) "unix:", 5) == 0) { return ngx_parse_unix_domain_url(pool, u); } diff --git a/src/http/modules/ngx_http_headers_filter_module.c b/src/http/modules/ngx_http_headers_filter_module.c index 90e6da8b3..5666da406 100644 --- a/src/http/modules/ngx_http_headers_filter_module.c +++ b/src/http/modules/ngx_http_headers_filter_module.c @@ -221,6 +221,14 @@ ngx_http_headers_filter(ngx_http_request_t *r) return NGX_ERROR; } + if (ngx_http_valid_header_value(value) != NGX_OK) { + ngx_log_error(NGX_LOG_ERR, r->connection->log, 0, + "Header value contains forbidden characters " + "(do you use variables in header values that " + "can contain CR or LF?)"); + return NGX_ERROR; + } + if (h[i].handler(r, &h[i], &value) != NGX_OK) { return NGX_ERROR; } @@ -306,6 +314,14 @@ ngx_http_trailers_filter(ngx_http_request_t *r, ngx_chain_t *in) } if (value.len) { + if (ngx_http_valid_header_value(value) != NGX_OK) { + ngx_log_error(NGX_LOG_ERR, r->connection->log, 0, + "Header value contains forbidden characters " + "(do you use variables in header values that " + "can contain CR or LF?)"); + return NGX_ERROR; + } + t = ngx_list_push(&r->headers_out.trailers); if (t == NULL) { return NGX_ERROR; @@ -785,6 +801,11 @@ ngx_http_headers_add(ngx_conf_t *cf, ngx_command_t *cmd, void *conf) value = cf->args->elts; + if (ngx_conf_check_field_name_and_strip_value(cf, value, value + 1, + value + 2) != NGX_OK) { + return NGX_CONF_ERROR; + } + headers = (ngx_array_t **) ((char *) hcf + cmd->offset); if (*headers == NULL) { diff --git a/src/http/modules/ngx_http_proxy_module.c b/src/http/modules/ngx_http_proxy_module.c index d4c5abf62..11b10b10c 100644 --- a/src/http/modules/ngx_http_proxy_module.c +++ b/src/http/modules/ngx_http_proxy_module.c @@ -206,6 +206,8 @@ static char *ngx_http_proxy_merge_loc_conf(ngx_conf_t *cf, static ngx_int_t ngx_http_proxy_init_headers(ngx_conf_t *cf, ngx_http_proxy_loc_conf_t *conf, ngx_http_proxy_headers_t *headers, ngx_keyval_t *default_headers); +static char *ngx_http_proxy_set_header(ngx_conf_t *cf, + ngx_command_t *cmd, void *conf); static char *ngx_http_proxy_pass(ngx_conf_t *cf, ngx_command_t *cmd, void *conf); @@ -416,7 +418,7 @@ static ngx_command_t ngx_http_proxy_commands[] = { { ngx_string("proxy_set_header"), NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE2, - ngx_conf_set_keyval_slot, + ngx_http_proxy_set_header, NGX_HTTP_LOC_CONF_OFFSET, offsetof(ngx_http_proxy_loc_conf_t, headers_source), NULL }, @@ -1259,7 +1261,7 @@ ngx_http_proxy_create_request(ngx_http_request_t *r) key_len, val_len; uintptr_t escape; ngx_buf_t *b; - ngx_str_t method; + ngx_str_t method, tmp; ngx_uint_t i, unparsed_uri; ngx_chain_t *cl, *body; ngx_list_part_t *part; @@ -1409,6 +1411,15 @@ ngx_http_proxy_create_request(ngx_http_request_t *r) continue; } + if (ngx_http_valid_header_name(header[i].key) != NGX_OK + || ngx_http_valid_header_value(header[i].value) != NGX_OK) { + /* oops, request smuggling */ + ngx_log_error(NGX_LOG_ALERT, r->connection->log, 0, + "Internal malformed header name or value " + "detected (this is a bug!!!!"); + return NGX_ERROR; + } + len += header[i].key.len + sizeof(": ") - 1 + header[i].value.len + sizeof(CRLF) - 1; } @@ -1426,6 +1437,7 @@ ngx_http_proxy_create_request(ngx_http_request_t *r) } cl->buf = b; + cl->next = NULL; /* the request line */ @@ -1463,6 +1475,12 @@ ngx_http_proxy_create_request(ngx_http_request_t *r) } u->uri.len = b->last - u->uri.data; + if (ngx_http_valid_header_value(u->uri) != NGX_OK) { + ngx_log_error(NGX_LOG_ALERT, r->connection->log, 0, + "URI contains control characters " + "(possible CRLF injection?"); + return NGX_ERROR; + } if (plcf->http_version == NGX_HTTP_VERSION_11) { b->last = ngx_cpymem(b->last, ngx_http_proxy_version_11, @@ -1484,6 +1502,7 @@ ngx_http_proxy_create_request(ngx_http_request_t *r) while (*(uintptr_t *) le.ip) { + tmp.data = e.pos; lcode = *(ngx_http_script_len_code_pt *) le.ip; (void) lcode(&le); @@ -1508,14 +1527,30 @@ ngx_http_proxy_create_request(ngx_http_request_t *r) code = *(ngx_http_script_code_pt *) e.ip; code((ngx_http_script_engine_t *) &e); + tmp.len = (size_t)(e.pos - tmp.data); + if (ngx_http_valid_header_name(tmp) != NGX_OK) { + ngx_log_error(NGX_LOG_ALERT, r->connection->log, 0, + "Header name contains forbidden characters " + "(do you use header names with variables?)"); + return NGX_ERROR; + } *e.pos++ = ':'; *e.pos++ = ' '; + tmp.data = e.pos; while (*(uintptr_t *) e.ip) { code = *(ngx_http_script_code_pt *) e.ip; code((ngx_http_script_engine_t *) &e); } e.ip += sizeof(uintptr_t); + tmp.len = (size_t)(e.pos - tmp.data); + if (ngx_http_valid_header_value(tmp) != NGX_OK) { + ngx_log_error(NGX_LOG_ALERT, r->connection->log, 0, + "Header value contains forbidden characters " + "(do you use variables in header values that " + "can contain CR or LF?)"); + return NGX_ERROR; + } *e.pos++ = CR; *e.pos++ = LF; } @@ -4166,6 +4201,25 @@ ngx_http_proxy_merge_loc_conf(ngx_conf_t *cf, void *parent, void *child) return NGX_CONF_OK; } +static char * +ngx_http_proxy_set_header(ngx_conf_t *cf, ngx_command_t *cmd, void *conf) +{ + char *rc; + ngx_str_t *value; + ngx_str_t unstripped_value; + + value = cf->args->elts; + unstripped_value = value[2]; + if (ngx_conf_check_field_name_and_strip_value(cf, value, value + 1, + value + 2) != NGX_OK) { + value[2] = unstripped_value; + return NGX_CONF_ERROR; + } + + rc = ngx_conf_set_keyval_slot(cf, cmd, conf); + value[2] = unstripped_value; + return rc; +} static ngx_int_t ngx_http_proxy_init_headers(ngx_conf_t *cf, ngx_http_proxy_loc_conf_t *conf, diff --git a/src/http/ngx_http.c b/src/http/ngx_http.c index d835f896e..1e187a954 100644 --- a/src/http/ngx_http.c +++ b/src/http/ngx_http.c @@ -2196,3 +2196,80 @@ ngx_http_set_default_types(ngx_conf_t *cf, ngx_array_t **types, return NGX_OK; } + +ngx_int_t +ngx_conf_check_field_name_and_strip_value(ngx_conf_t *cf, + const ngx_str_t *cmd_name, const ngx_str_t *name, ngx_str_t *value) +{ + u_char *out_cursor, *cursor, *end; + + if (ngx_http_valid_header_name(*name) != NGX_OK) { + ngx_conf_log_error(NGX_LOG_EMERG, cf, 0, + "%V: Invalid HTTP field name", + (ngx_str_t *)cmd_name); + return NGX_ERROR; + } + + if (ngx_http_conf_forbidden_header(*name)) { + ngx_conf_log_error(NGX_LOG_EMERG, cf, 0, + "%V: cannot set %V as it would break HTTP/1.x " + "framing", (ngx_str_t *)cmd_name, + (ngx_str_t *)name); + return NGX_ERROR; + } + +#define ISSPACE(c) ((c) == ' ' || (c) == '\t') + while (value->len > 0) { + if (!ISSPACE(value->data[0])) { + break; + } + value->len--; + value->data++; + } + + while (value->len > 0) { + size_t last = value->len - 1; + if (!ISSPACE(value->data[last])) { + break; + } + value->len--; + } + + end = value->len + value->data; + out_cursor = cursor = value->data; + + for ( /* void */; cursor < end; ++cursor) { + if (cursor[0] == 0) { + goto fail; + } + + if (cursor[0] != CR && cursor[0] != LF) { + *out_cursor++ = *cursor; + continue; + } + + int iscr = cursor[0] == CR; + if (end - cursor >= iscr + 2 && cursor[iscr] == LF + && ISSPACE(cursor[iscr + 1])) { + while (out_cursor > value->data && ISSPACE(out_cursor[-1])) { + out_cursor--; + } + cursor += iscr + 2; + while (end > cursor && ISSPACE(*cursor)) { + cursor++; + } + *out_cursor++ = ' '; + continue; + } + goto fail; + } + if (out_cursor < end) { + memset(out_cursor, '\0', (size_t)(end - out_cursor)); + value->len -= (size_t)(end - out_cursor); + } + return NGX_OK; +fail: + ngx_conf_log_error(NGX_LOG_EMERG, cf, 0, + "%V: Invalid HTTP field value", (ngx_str_t *)cmd_name); + return NGX_ERROR; +} diff --git a/src/http/ngx_http.h b/src/http/ngx_http.h index cb4a1e68a..72c0f1b3d 100644 --- a/src/http/ngx_http.h +++ b/src/http/ngx_http.h @@ -119,6 +119,19 @@ void ngx_http_split_args(ngx_http_request_t *r, ngx_str_t *uri, ngx_int_t ngx_http_parse_chunked(ngx_http_request_t *r, ngx_buf_t *b, ngx_http_chunked_t *ctx, ngx_uint_t keep_trailers); +/** Check if value is a valid HTTP field name. */ +ngx_int_t ngx_http_valid_header_name(ngx_str_t value); +/** Check if value is a valid HTTP field value. */ +ngx_int_t ngx_http_valid_header_value(ngx_str_t value); +/** Check if value is equal (case insensitive) to Transfer-Encoding or + * Content-Length. NGINX does not allow manually overriding these fields + * as it would make proper request framing impossible. */ +ngx_int_t ngx_http_conf_forbidden_header(ngx_str_t key); +/** Check if the field name and value are valid for an HTTP field name-value + * pair. Leading and trailing ' ' and '\t' are stripped from the field value + * before the value is checked. The value is updated in-place. */ +ngx_int_t ngx_conf_check_field_name_and_strip_value(ngx_conf_t *cf, + const ngx_str_t *cmd_name, const ngx_str_t *name, ngx_str_t *value); ngx_http_request_t *ngx_http_create_request(ngx_connection_t *c); ngx_int_t ngx_http_process_request_uri(ngx_http_request_t *r); diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c index a45c04554..d8e4bc3bd 100644 --- a/src/http/ngx_http_parse.c +++ b/src/http/ngx_http_parse.c @@ -2413,3 +2413,80 @@ invalid: return NGX_ERROR; } + + +ngx_int_t +ngx_http_valid_header_value(ngx_str_t value) +{ + size_t j; + + /* TODO: this might break configurations that are invalid, + * but happen to work right now because the leading and/or + * trailing whitespace gets stripped. It should be turned on + * if NGINX ever has a major version bump, or if a mechanism + * is added for the user to opt-in. Sending a message with + * leading or trailing space is much less serious than CRLF + * injection. */ + if (0) { + if (value.len < 1) { + return NGX_OK; + } + + if (value.data[0] == ' ' || value.data[0] == '\t' + || value.data[value.len - 1] == ' ' + || value.data[value.len - 1] == '\t') { + return NGX_ERROR; + } + } + + for (j = 0; j < value.len; ++j) { + if (value.data[j] == '\0' || value.data[j] == CR + || value.data[j] == LF) { + return NGX_ERROR; + } + } + + return NGX_OK; +} + + +ngx_int_t +ngx_http_valid_header_name(ngx_str_t value) +{ + size_t j; + + if (value.len < 1) { + return NGX_ERROR; + } + + for (j = 0; j < value.len; ++j) { + /* todo: check that this is an HTTP TOKEN */ + if (value.data[j] <= 0x20 || value.data[j] == 0x7F + || value.data[j] == ':') { + return NGX_ERROR; + } + } + + return NGX_OK; +} + +ngx_int_t +ngx_http_conf_forbidden_header(ngx_str_t key) +{ + switch (key.len) { + case sizeof("Transfer-Encoding") - 1: + if (ngx_strncasecmp(key.data, (u_char *)"Transfer-Encoding", + key.len) == 0) { + return NGX_ERROR; + } + return NGX_OK; + case sizeof("Content-Length") - 1: + if (ngx_strncasecmp(key.data, (u_char *)"Content-Length", + key.len) == 0) { + return NGX_ERROR; + } + return NGX_OK; + default: + return NGX_OK; + } +}