realip module broken?

[Igor Sysoev]

On Wed, Aug 13, 2008 at 10:23:27AM +0200, Spil Games wrote:

> Igor Sysoev wrote:
> > I will look how to resolve the issue. Right now you may disable 
> > keepalive on nginx side.
> 
> I'll keep it at 0.0.0.0/0 for now. The 'set_real_ip_from' directive 
> doesn't add much security anyway:
> 
> a) The loadbalancer overwrites any existing X-Real-IP headers.
> 
> b) Even if a) would not be done, the header would be accepted because 
> all requests come from the loadbalancer IP.
> 
> It would be nice if the realip module could be fixed though. It's a 
> matter of semantics, but I believe an X-Real-IP (or X-Forwarded-For) 
> header should only influence the request, not the entire connection.

The attached patch should fix the issue.


-- 
Igor Sysoev
http://sysoev.ru/en/
-------------- next part --------------
Index: src/http/ngx_http_request.h
===================================================================
--- src/http/ngx_http_request.h	(revision 1494)
+++ src/http/ngx_http_request.h	(working copy)
@@ -436,17 +436,7 @@
     unsigned                          bypass_cache:1;
     unsigned                          no_cache:1;
 
-#if (NGX_HTTP_REALIP)
-
     /*
-     * instead of using the request context data in ngx_http_realip_module
-     * we use the single bit in the request structure
-     */
-    unsigned                          realip_set:1;
-
-#endif
-
-    /*
      * instead of using the request context data in ngx_http_limit_zone_module
      * we use the single bit in the request structure
      */
Index: src/http/modules/ngx_http_realip_module.c
===================================================================
--- src/http/modules/ngx_http_realip_module.c	(revision 1494)
+++ src/http/modules/ngx_http_realip_module.c	(working copy)
@@ -12,19 +12,26 @@
 /* AF_INET only */
 
 typedef struct {
-    in_addr_t     mask;
-    in_addr_t     addr;
+    in_addr_t          mask;
+    in_addr_t          addr;
 } ngx_http_realip_from_t;
 
 
 typedef struct {
-    ngx_array_t  *from;     /* array of ngx_http_realip_from_t */
-
-    ngx_uint_t    xfwd;
+    ngx_array_t       *from;     /* array of ngx_http_realip_from_t */
+    ngx_uint_t         xfwd;
 } ngx_http_realip_loc_conf_t;
 
 
+typedef struct {
+    ngx_connection_t  *connection;
+    in_addr_t          addr;
+    ngx_str_t          addr_text;
+} ngx_http_realip_ctx_t;
+
+
 static ngx_int_t ngx_http_realip_handler(ngx_http_request_t *r);
+static void ngx_http_realip_cleanup(void *data);
 static char *ngx_http_realip_from(ngx_conf_t *cf, ngx_command_t *cmd,
     void *conf);
 static void *ngx_http_realip_create_loc_conf(ngx_conf_t *cf);
@@ -100,13 +107,23 @@
     in_addr_t                    addr;
     ngx_uint_t                   i;
     struct sockaddr_in          *sin;
+    ngx_connection_t            *c;
+    ngx_pool_cleanup_t          *cln;
+    ngx_http_realip_ctx_t       *ctx;
     ngx_http_realip_from_t      *from;
     ngx_http_realip_loc_conf_t  *rlcf;
 
-    if (r->realip_set) {
+    ctx = ngx_http_get_module_ctx(r, ngx_http_realip_module);
+
+    if (ctx) {
         return NGX_DECLINED;
     }
 
+    cln = ngx_pool_cleanup_add(r->pool, sizeof(ngx_http_realip_ctx_t));
+    if (cln == NULL) {
+        return NGX_HTTP_INTERNAL_SERVER_ERROR;
+    }
+
     rlcf = ngx_http_get_module_loc_conf(r, ngx_http_realip_module);
 
     if (rlcf->from == NULL) {
@@ -139,41 +156,50 @@
         }
     }
 
-    ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
-                   "realip: \"%s\"", ip);
+    c = r->connection;
 
+    ngx_log_debug1(NGX_LOG_DEBUG_HTTP, c->log, 0, "realip: \"%s\"", ip);
+
     /* AF_INET only */
 
-    sin = (struct sockaddr_in *) r->connection->sockaddr;
+    sin = (struct sockaddr_in *) c->sockaddr;
 
     from = rlcf->from->elts;
     for (i = 0; i < rlcf->from->nelts; i++) {
 
-        ngx_log_debug3(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
+        ngx_log_debug3(NGX_LOG_DEBUG_HTTP, c->log, 0,
                        "realip: %08XD %08XD %08XD",
                        sin->sin_addr.s_addr, from[i].mask, from[i].addr);
 
         if ((sin->sin_addr.s_addr & from[i].mask) == from[i].addr) {
 
-            r->realip_set = 1;
+            ctx = cln->data;
 
+            ngx_http_set_ctx(r, ctx, ngx_http_realip_module);
+
             addr = inet_addr((char *) ip);
 
             if (addr == INADDR_NONE) {
                 return NGX_DECLINED;
             }
 
-            p = ngx_pnalloc(r->connection->pool, len);
+            p = ngx_pnalloc(c->pool, len);
             if (p == NULL) {
                 return NGX_HTTP_INTERNAL_SERVER_ERROR;
             }
 
             ngx_memcpy(p, ip, len);
 
+            cln->handler = ngx_http_realip_cleanup;
+
+            ctx->connection = c;
+            ctx->addr = sin->sin_addr.s_addr;
+            ctx->addr_text = c->addr_text;
+
             sin->sin_addr.s_addr = addr;
 
-            r->connection->addr_text.len = len;
-            r->connection->addr_text.data = p;
+            c->addr_text.len = len;
+            c->addr_text.data = p;
 
             return NGX_DECLINED;
         }
@@ -183,6 +209,23 @@
 }
 
 
+static void
+ngx_http_realip_cleanup(void *data)
+{
+    ngx_http_realip_ctx_t *ctx = data;
+
+    ngx_connection_t    *c;
+    struct sockaddr_in  *sin;
+
+    c = ctx->connection;
+
+    sin = (struct sockaddr_in *) c->sockaddr;
+    sin->sin_addr.s_addr = ctx->addr;
+
+    c->addr_text = ctx->addr_text;
+}
+
+
 static char *
 ngx_http_realip_from(ngx_conf_t *cf, ngx_command_t *cmd, void *conf)
 {