Block CRLF injection attacks if NGINX is misconfigured

This protects servers running with the following vulnerable
configurations:

    location /b {
        # $uri can contain a newline
        proxy_pass http://127.0.0.1:8080/$uri;
    }

    location /b {
        # $uri can contain a newline
        add_header X-Original-Path $uri;
        proxy_pass http://127.0.0.1:8080;
    }

    location /b {
        # $uri can contain a newline
        add_trailer X-Original-Path $uri;
        proxy_pass http://127.0.0.1:8080;
    }

    location /b {
        # $uri can contain a newline
        proxy_set_header X-Normalized-URI $uri;
        proxy_pass http://127.0.0.1:8080;
    }

Previously, these would be vulnerable to a CRLF injection attack,
allowing request smuggling.  With this patch, the attack is foiled,
and NGINX will return a 500 Internal Server Error response to the
client.  These configurations are still incorrect, which is why NGINX
returns a 500 response instead of a 4xx response.  Also, there will
still be control characters injected into the error log, which is a
bug.

This change also causes NGINX to fail to start if a add_header,
add_trailer, or proxy_set_header directive specifies an invalid field
name, rather than sending an invalid field name at runtime.
Transfer-Encoding and Content-Length are considered invalid, as they are
used for HTTP/1.x framing and so cannot safely be overridden.
Multi-line header values in configuration files are explicitly detected
and allowed, provided that the multi-line value uses the HTTP/1.x
obs-fold syntax (with leading whitespace in the second and subsequent
lines) and that the newlines are in the literal header value (as opposed
to a variable).  The obs-fold syntax is replaced by a single space
during configuration parsing, so clients and servers receive well-formed
data.

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);
     }

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) {

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,

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;
+}

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);

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;
+    }
+}