[PATCH] Fix for ticket #106: Correctly handle multiple X-Forwarded-For headers

Ruslan Ermilov ru at nginx.com
Mon Jun 25 06:23:00 UTC 2012


On Tue, Jun 19, 2012 at 06:47:56PM -0700, Tribble, Alex wrote:
> When nginx gets multiple X-Forwarded-For headers in a single request, it
> only keeps the last one in r->headers_in (and thus in
> $http_x_forwarded_for, $proxy_add_x_forwarded_for). Reverse proxies behind
> an nginx instance sometimes need the entire X-Forwarded-For chain - part
> of which is discarded in this case.
> 
> Per RFC 2616, it's equivalent to concatenate each header value (separated
> by a comma) and send the concatenated value to the upstream:
> 4.2
> -snip-
>    Multiple message-header fields with the same field-name MAY be
>    present in a message if and only if the entire field-value for that
>    header field is defined as a comma-separated list [i.e., #(values)].
>    It MUST be possible to combine the multiple header fields into one
>    "field-name: field-value" pair, without changing the semantics of the
>    message, by appending each subsequent field-value to the first, each
>    separated by a comma. The order in which header fields with the same
>    field-name are received is therefore significant to the
>    interpretation of the combined field value, and thus a proxy MUST NOT
>    change the order of these field values when a message is forwarded.
> -snip-
> 
> Attached is a patch that does exactly this, in the case of multiple headers.
> Please let me know if you have any comments about this patch - I'm happy
> to make any changes you suggest.
> 
> Relevant bug report:
> http://trac.nginx.org/nginx/ticket/106

How's this patch instead?

%%%
Index: src/http/ngx_http_request.h
===================================================================
--- src/http/ngx_http_request.h	(revision 4698)
+++ src/http/ngx_http_request.h	(working copy)
@@ -193,7 +193,7 @@ typedef struct {
     ngx_table_elt_t                  *keep_alive;
 
 #if (NGX_HTTP_X_FORWARDED_FOR)
-    ngx_table_elt_t                  *x_forwarded_for;
+    ngx_array_t                       x_forwarded_for;
 #endif
 
 #if (NGX_HTTP_REALIP)
Index: src/http/ngx_http_request.c
===================================================================
--- src/http/ngx_http_request.c	(revision 4698)
+++ src/http/ngx_http_request.c	(working copy)
@@ -21,14 +21,14 @@ static ngx_int_t ngx_http_process_header
     ngx_table_elt_t *h, ngx_uint_t offset);
 static ngx_int_t ngx_http_process_unique_header_line(ngx_http_request_t *r,
     ngx_table_elt_t *h, ngx_uint_t offset);
+static ngx_int_t ngx_http_process_multi_header_lines(ngx_http_request_t *r,
+    ngx_table_elt_t *h, ngx_uint_t offset);
 static ngx_int_t ngx_http_process_host(ngx_http_request_t *r,
     ngx_table_elt_t *h, ngx_uint_t offset);
 static ngx_int_t ngx_http_process_connection(ngx_http_request_t *r,
     ngx_table_elt_t *h, ngx_uint_t offset);
 static ngx_int_t ngx_http_process_user_agent(ngx_http_request_t *r,
     ngx_table_elt_t *h, ngx_uint_t offset);
-static ngx_int_t ngx_http_process_cookie(ngx_http_request_t *r,
-    ngx_table_elt_t *h, ngx_uint_t offset);
 
 static ngx_int_t ngx_http_process_request_header(ngx_http_request_t *r);
 static void ngx_http_process_request(ngx_http_request_t *r);
@@ -141,7 +141,7 @@ ngx_http_header_t  ngx_http_headers_in[]
 #if (NGX_HTTP_X_FORWARDED_FOR)
     { ngx_string("X-Forwarded-For"),
                  offsetof(ngx_http_headers_in_t, x_forwarded_for),
-                 ngx_http_process_header_line },
+                 ngx_http_process_multi_header_lines },
 #endif
 
 #if (NGX_HTTP_REALIP)
@@ -173,7 +173,8 @@ ngx_http_header_t  ngx_http_headers_in[]
                  ngx_http_process_header_line },
 #endif
 
-    { ngx_string("Cookie"), 0, ngx_http_process_cookie },
+    { ngx_string("Cookie"), offsetof(ngx_http_headers_in_t, cookies),
+                 ngx_http_process_multi_header_lines },
 
     { ngx_null_string, 0, NULL }
 };
@@ -917,15 +918,6 @@ ngx_http_process_request_line(ngx_event_
                 return;
             }
 
-
-            if (ngx_array_init(&r->headers_in.cookies, r->pool, 2,
-                               sizeof(ngx_table_elt_t *))
-                != NGX_OK)
-            {
-                ngx_http_close_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR);
-                return;
-            }
-
             c->log->action = "reading client request headers";
 
             rev->handler = ngx_http_process_request_headers;
@@ -1526,20 +1518,31 @@ ngx_http_process_user_agent(ngx_http_req
 
 
 static ngx_int_t
-ngx_http_process_cookie(ngx_http_request_t *r, ngx_table_elt_t *h,
+ngx_http_process_multi_header_lines(ngx_http_request_t *r, ngx_table_elt_t *h,
     ngx_uint_t offset)
 {
-    ngx_table_elt_t  **cookie;
+    ngx_array_t      *headers;
+    ngx_table_elt_t  **ph;
 
-    cookie = ngx_array_push(&r->headers_in.cookies);
-    if (cookie) {
-        *cookie = h;
-        return NGX_OK;
+    headers = (ngx_array_t *) ((char *) &r->headers_in + offset);
+
+    if (headers->elts == NULL) {
+        if (ngx_array_init(headers, r->pool, 1, sizeof(ngx_table_elt_t *))
+            != NGX_OK)
+        {
+            ngx_http_close_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR);
+            return NGX_ERROR;
+        }
     }
 
-    ngx_http_close_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR);
+    ph = ngx_array_push(headers);
+    if (ph == NULL) {
+        ngx_http_close_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR);
+        return NGX_ERROR;
+    }
 
-    return NGX_ERROR;
+    *ph = h;
+    return NGX_OK;
 }
 
 
Index: src/http/ngx_http_core_module.h
===================================================================
--- src/http/ngx_http_core_module.h	(revision 4696)
+++ src/http/ngx_http_core_module.h	(working copy)
@@ -514,7 +514,8 @@ ngx_int_t ngx_http_set_disable_symlinks(
     ngx_http_core_loc_conf_t *clcf, ngx_str_t *path, ngx_open_file_info_t *of);
 
 ngx_int_t ngx_http_get_forwarded_addr(ngx_http_request_t *r, ngx_addr_t *addr,
-    u_char *xff, size_t xfflen, ngx_array_t *proxies, int recursive);
+    ngx_array_t *headers, ngx_str_t *hvalue, ngx_array_t *proxies,
+    int recursive);
 
 
 extern ngx_module_t  ngx_http_core_module;
Index: src/http/ngx_http_core_module.c
===================================================================
--- src/http/ngx_http_core_module.c	(revision 4696)
+++ src/http/ngx_http_core_module.c	(working copy)
@@ -2699,12 +2699,13 @@ ngx_http_set_disable_symlinks(ngx_http_r
 }
 
 
-ngx_int_t
-ngx_http_get_forwarded_addr(ngx_http_request_t *r, ngx_addr_t *addr,
+static ngx_int_t
+ngx_http_get_forwarded_addr_iter(ngx_http_request_t *r, ngx_addr_t *addr,
     u_char *xff, size_t xfflen, ngx_array_t *proxies, int recursive)
 {
     u_char           *p;
     in_addr_t         inaddr;
+    ngx_int_t         rc;
     ngx_addr_t        paddr;
     ngx_cidr_t       *cidr;
     ngx_uint_t        family, i;
@@ -2788,8 +2789,15 @@ ngx_http_get_forwarded_addr(ngx_http_req
         *addr = paddr;
 
         if (recursive && p > xff) {
-            (void) ngx_http_get_forwarded_addr(r, addr, xff, p - 1 - xff,
-                                               proxies, 1);
+            rc = ngx_http_get_forwarded_addr_iter(r, addr, xff, p - 1 - xff,
+                                                  proxies, 1);
+
+            if (rc == NGX_DECLINED) {
+                return NGX_DONE;
+            }
+
+            /* rc == NGX_OK || rc == NGX_DONE  */
+            return rc;
         }
 
         return NGX_OK;
@@ -2802,6 +2810,45 @@ ngx_http_get_forwarded_addr(ngx_http_req
 }
 
 
+ngx_int_t
+ngx_http_get_forwarded_addr(ngx_http_request_t *r, ngx_addr_t *addr,
+    ngx_array_t *headers, ngx_str_t *hvalue, ngx_array_t *proxies,
+    int recursive)
+{
+    ngx_int_t          rc;
+    ngx_uint_t         i;
+    ngx_table_elt_t  **h;
+
+    if (headers == NULL) {
+        return ngx_http_get_forwarded_addr_iter(r, addr, hvalue->data,
+                                                hvalue->len, proxies,
+                                                recursive);
+    }
+
+    i = headers->nelts;
+    h = headers->elts;
+
+    rc = NGX_DECLINED;
+
+    for ( ;; ) {
+        if (i == 0) {
+            break;
+        }
+        i--;
+
+        rc = ngx_http_get_forwarded_addr_iter(r, addr, h[i]->value.data,
+                                              h[i]->value.len, proxies,
+                                              recursive);
+
+        if (!recursive || rc != NGX_OK) {
+            break;
+        }
+    }
+
+    return rc;
+}
+
+
 static char *
 ngx_http_core_server(ngx_conf_t *cf, ngx_command_t *cmd, void *dummy)
 {
Index: src/http/ngx_http_variables.c
===================================================================
--- src/http/ngx_http_variables.c	(revision 4698)
+++ src/http/ngx_http_variables.c	(working copy)
@@ -21,8 +21,14 @@ static void ngx_http_variable_request_se
     ngx_http_variable_value_t *v, uintptr_t data);
 static ngx_int_t ngx_http_variable_header(ngx_http_request_t *r,
     ngx_http_variable_value_t *v, uintptr_t data);
-static ngx_int_t ngx_http_variable_headers(ngx_http_request_t *r,
+static ngx_int_t ngx_http_variable_headers_sep(ngx_http_request_t *r,
+    ngx_http_variable_value_t *v, uintptr_t data, u_char sep);
+static ngx_int_t ngx_http_variable_headers_semi(ngx_http_request_t *r,
     ngx_http_variable_value_t *v, uintptr_t data);
+#if (NGX_HTTP_X_FORWARDED_FOR)
+static ngx_int_t ngx_http_variable_headers_comma(ngx_http_request_t *r,
+    ngx_http_variable_value_t *v, uintptr_t data);
+#endif
 
 static ngx_int_t ngx_http_variable_unknown_header_in(ngx_http_request_t *r,
     ngx_http_variable_value_t *v, uintptr_t data);
@@ -112,8 +118,8 @@ static ngx_int_t ngx_http_variable_pid(n
  */
 
 /*
- * the $http_host, $http_user_agent, $http_referer, $http_via,
- * and $http_x_forwarded_for variables may be handled by generic
+ * the $http_host, $http_user_agent, $http_referer, and $http_via
+ * variables may be handled by generic
  * ngx_http_variable_unknown_header_in(), but for performance reasons
  * they are handled using dedicated entries
  */
@@ -135,11 +141,11 @@ static ngx_http_variable_t  ngx_http_cor
 #endif
 
 #if (NGX_HTTP_X_FORWARDED_FOR)
-    { ngx_string("http_x_forwarded_for"), NULL, ngx_http_variable_header,
+    { ngx_string("http_x_forwarded_for"), NULL, ngx_http_variable_headers_comma,
       offsetof(ngx_http_request_t, headers_in.x_forwarded_for), 0, 0 },
 #endif
 
-    { ngx_string("http_cookie"), NULL, ngx_http_variable_headers,
+    { ngx_string("http_cookie"), NULL, ngx_http_variable_headers_semi,
       offsetof(ngx_http_request_t, headers_in.cookies), 0, 0 },
 
     { ngx_string("content_length"), NULL, ngx_http_variable_header,
@@ -252,7 +258,8 @@ static ngx_http_variable_t  ngx_http_cor
     { ngx_string("sent_http_transfer_encoding"), NULL,
       ngx_http_variable_sent_transfer_encoding, 0, 0, 0 },
 
-    { ngx_string("sent_http_cache_control"), NULL, ngx_http_variable_headers,
+    { ngx_string("sent_http_cache_control"), NULL,
+      ngx_http_variable_headers_semi,
       offsetof(ngx_http_request_t, headers_out.cache_control), 0, 0 },
 
     { ngx_string("limit_rate"), ngx_http_variable_request_set_size,
@@ -666,8 +673,8 @@ ngx_http_variable_header(ngx_http_reques
 
 
 static ngx_int_t
-ngx_http_variable_headers(ngx_http_request_t *r, ngx_http_variable_value_t *v,
-    uintptr_t data)
+ngx_http_variable_headers_sep(ngx_http_request_t *r,
+    ngx_http_variable_value_t *v, uintptr_t data, u_char sep)
 {
     size_t             len;
     u_char            *p, *end;
@@ -688,7 +695,7 @@ ngx_http_variable_headers(ngx_http_reque
             continue;
         }
 
-        len += h[i]->value.len + sizeof("; ") - 1;
+        len += h[i]->value.len + 2;
     }
 
     if (len == 0) {
@@ -696,7 +703,7 @@ ngx_http_variable_headers(ngx_http_reque
         return NGX_OK;
     }
 
-    len -= sizeof("; ") - 1;
+    len -= 2;
 
     v->valid = 1;
     v->no_cacheable = 0;
@@ -731,7 +738,7 @@ ngx_http_variable_headers(ngx_http_reque
             break;
         }
 
-        *p++ = ';'; *p++ = ' ';
+        *p++ = sep; *p++ = ' ';
     }
 
     return NGX_OK;
@@ -739,6 +746,26 @@ ngx_http_variable_headers(ngx_http_reque
 
 
 static ngx_int_t
+ngx_http_variable_headers_semi(ngx_http_request_t *r,
+    ngx_http_variable_value_t *v, uintptr_t data)
+{
+    return ngx_http_variable_headers_sep(r, v, data, ';');
+}
+
+
+#if (NGX_HTTP_X_FORWARDED_FOR)
+
+static ngx_int_t
+ngx_http_variable_headers_comma(ngx_http_request_t *r,
+    ngx_http_variable_value_t *v, uintptr_t data)
+{
+    return ngx_http_variable_headers_sep(r, v, data, ',');
+}
+
+#endif
+
+
+static ngx_int_t
 ngx_http_variable_unknown_header_in(ngx_http_request_t *r,
     ngx_http_variable_value_t *v, uintptr_t data)
 {
Index: src/http/modules/ngx_http_geo_module.c
===================================================================
--- src/http/modules/ngx_http_geo_module.c	(revision 4696)
+++ src/http/modules/ngx_http_geo_module.c	(working copy)
@@ -215,19 +215,18 @@ static in_addr_t
 ngx_http_geo_addr(ngx_http_request_t *r, ngx_http_geo_ctx_t *ctx)
 {
     ngx_addr_t           addr;
-    ngx_table_elt_t     *xfwd;
+    ngx_array_t         *xfwd;
     struct sockaddr_in  *sin;
 
     if (ngx_http_geo_real_addr(r, ctx, &addr) != NGX_OK) {
         return INADDR_NONE;
     }
 
-    xfwd = r->headers_in.x_forwarded_for;
+    xfwd = &r->headers_in.x_forwarded_for;
 
-    if (xfwd != NULL && ctx->proxies != NULL) {
-        (void) ngx_http_get_forwarded_addr(r, &addr, xfwd->value.data,
-                                           xfwd->value.len, ctx->proxies,
-                                           ctx->proxy_recursive);
+    if (xfwd->elts != NULL && ctx->proxies != NULL) {
+        (void) ngx_http_get_forwarded_addr(r, &addr, xfwd, NULL,
+                                           ctx->proxies, ctx->proxy_recursive);
     }
 
 #if (NGX_HAVE_INET6)
Index: src/http/modules/ngx_http_geoip_module.c
===================================================================
--- src/http/modules/ngx_http_geoip_module.c	(revision 4696)
+++ src/http/modules/ngx_http_geoip_module.c	(working copy)
@@ -208,19 +208,18 @@ static u_long
 ngx_http_geoip_addr(ngx_http_request_t *r, ngx_http_geoip_conf_t *gcf)
 {
     ngx_addr_t           addr;
-    ngx_table_elt_t     *xfwd;
+    ngx_array_t         *xfwd;
     struct sockaddr_in  *sin;
 
     addr.sockaddr = r->connection->sockaddr;
     addr.socklen = r->connection->socklen;
     /* addr.name = r->connection->addr_text; */
 
-    xfwd = r->headers_in.x_forwarded_for;
+    xfwd = &r->headers_in.x_forwarded_for;
 
-    if (xfwd != NULL && gcf->proxies != NULL) {
-        (void) ngx_http_get_forwarded_addr(r, &addr, xfwd->value.data,
-                                           xfwd->value.len, gcf->proxies,
-                                           gcf->proxy_recursive);
+    if (xfwd->elts != NULL && gcf->proxies != NULL) {
+        (void) ngx_http_get_forwarded_addr(r, &addr, xfwd, NULL,
+                                           gcf->proxies, gcf->proxy_recursive);
     }
 
 #if (NGX_HAVE_INET6)
Index: src/http/modules/ngx_http_proxy_module.c
===================================================================
--- src/http/modules/ngx_http_proxy_module.c	(revision 4696)
+++ src/http/modules/ngx_http_proxy_module.c	(working copy)
@@ -2260,32 +2260,55 @@ static ngx_int_t
 ngx_http_proxy_add_x_forwarded_for_variable(ngx_http_request_t *r,
     ngx_http_variable_value_t *v, uintptr_t data)
 {
-    u_char  *p;
+    size_t             len;
+    u_char            *p;
+    ngx_uint_t         i, n;
+    ngx_table_elt_t  **h;
 
     v->valid = 1;
     v->no_cacheable = 0;
     v->not_found = 0;
 
-    if (r->headers_in.x_forwarded_for == NULL) {
+    n = r->headers_in.x_forwarded_for.nelts;
+    h = r->headers_in.x_forwarded_for.elts;
+
+    len = 0;
+
+    for (i = 0; i < n; i++) {
+
+        if (h[i]->hash == 0) {
+            continue;
+        }
+
+        len += h[i]->value.len + sizeof(", ") - 1;
+    }
+
+    if (len == 0) {
         v->len = r->connection->addr_text.len;
         v->data = r->connection->addr_text.data;
         return NGX_OK;
     }
 
-    v->len = r->headers_in.x_forwarded_for->value.len
-             + sizeof(", ") - 1 + r->connection->addr_text.len;
+    len += r->connection->addr_text.len;
 
-    p = ngx_pnalloc(r->pool, v->len);
+    p = ngx_pnalloc(r->pool, len);
     if (p == NULL) {
         return NGX_ERROR;
     }
 
+    v->len = len;
     v->data = p;
 
-    p = ngx_copy(p, r->headers_in.x_forwarded_for->value.data,
-                 r->headers_in.x_forwarded_for->value.len);
+    for (i = 0; i < n; i++) {
+
+        if (h[i]->hash == 0) {
+            continue;
+        }
+
+        p = ngx_copy(p, h[i]->value.data, h[i]->value.len);
 
-    *p++ = ','; *p++ = ' ';
+        *p++ = ','; *p++ = ' ';
+    }
 
     ngx_memcpy(p, r->connection->addr_text.data, r->connection->addr_text.len);
 
Index: src/http/modules/ngx_http_realip_module.c
===================================================================
--- src/http/modules/ngx_http_realip_module.c	(revision 4696)
+++ src/http/modules/ngx_http_realip_module.c	(working copy)
@@ -107,10 +107,12 @@ ngx_module_t  ngx_http_realip_module = {
 static ngx_int_t
 ngx_http_realip_handler(ngx_http_request_t *r)
 {
-    u_char                      *ip, *p;
+    u_char                      *p;
     size_t                       len;
+    ngx_str_t                   *hvalue;
     ngx_uint_t                   i, hash;
     ngx_addr_t                   addr;
+    ngx_array_t                 *xfwd;
     ngx_list_part_t             *part;
     ngx_table_elt_t             *header;
     ngx_connection_t            *c;
@@ -137,19 +139,20 @@ ngx_http_realip_handler(ngx_http_request
             return NGX_DECLINED;
         }
 
-        len = r->headers_in.x_real_ip->value.len;
-        ip = r->headers_in.x_real_ip->value.data;
+        hvalue = &r->headers_in.x_real_ip->value;
+        xfwd = NULL;
 
         break;
 
     case NGX_HTTP_REALIP_XFWD:
 
-        if (r->headers_in.x_forwarded_for == NULL) {
+        xfwd = &r->headers_in.x_forwarded_for;
+
+        if (xfwd->elts == NULL) {
             return NGX_DECLINED;
         }
 
-        len = r->headers_in.x_forwarded_for->value.len;
-        ip = r->headers_in.x_forwarded_for->value.data;
+        hvalue = NULL;
 
         break;
 
@@ -178,8 +181,8 @@ ngx_http_realip_handler(ngx_http_request
                 && len == header[i].key.len
                 && ngx_strncmp(p, header[i].lowcase_key, len) == 0)
             {
-                len = header[i].value.len;
-                ip = header[i].value.data;
+                hvalue = &header[i].value;
+                xfwd = NULL;
 
                 goto found;
             }
@@ -192,15 +195,13 @@ found:
 
     c = r->connection;
 
-    ngx_log_debug1(NGX_LOG_DEBUG_HTTP, c->log, 0, "realip: \"%s\"", ip);
-
     addr.sockaddr = c->sockaddr;
     addr.socklen = c->socklen;
     /* addr.name = c->addr_text; */
 
-    if (ngx_http_get_forwarded_addr(r, &addr, ip, len, rlcf->from,
+    if (ngx_http_get_forwarded_addr(r, &addr, xfwd, hvalue, rlcf->from,
                                     rlcf->recursive)
-        == NGX_OK)
+        != NGX_DECLINED)
     {
         return ngx_http_realip_set_addr(r, &addr);
     }
%%%



More information about the nginx-devel mailing list