nginx: copies the req body chain to be processed instead of move

Felipe Zimmerle · Felipe Zimmerle · commit fa4c5fe2f1a6 · 2015-01-09T04:48:08.000-08:00
Add a check for the definition MOVE_REQUEST_CHAIN_TO_MODSEC, whenever it is set the chain will be moved into the brigade. If it was not set the chain will be only copied. Moving was causing segfaults on the following regression tests: #15 - SecRequestBodyInMemoryLimit #16 - SecRequestBodyInMemoryLimit (greater) #19 - SecRequestBodyLimitAction ProcessPartial (multipart/greater - chunked) (from: regression/config/10-request-directives.t)
diff --git a/nginx/modsecurity/apr_bucket_nginx.c b/nginx/modsecurity/apr_bucket_nginx.c
@@ -153,6 +153,36 @@ ngx_buf_t * apr_bucket_to_ngx_buf(apr_bucket *e, ngx_pool_t *pool) {
     return buf;
 }
 
+ngx_int_t
+copy_chain_to_brigade(ngx_chain_t *chain_orig, apr_bucket_brigade *bb, ngx_pool_t *pool, ngx_int_t last_buf) {
+    apr_bucket *e;
+
+    ngx_chain_t *chain = chain_orig;
+    while (chain) {
+        e = ngx_buf_to_apr_bucket(chain->buf, bb->p, bb->bucket_alloc);
+        if (e == NULL) {
+            return NGX_ERROR;
+        }
+
+        APR_BRIGADE_INSERT_TAIL(bb, e);
+        if (chain->buf->last_buf) {
+            e = apr_bucket_eos_create(bb->bucket_alloc);
+            APR_BRIGADE_INSERT_TAIL(bb, e);
+            return NGX_OK;
+        }
+        chain = chain->next;
+    }
+
+    if (last_buf) {
+        e = apr_bucket_eos_create(bb->bucket_alloc);
+        APR_BRIGADE_INSERT_TAIL(bb, e);
+        return NGX_OK;
+    }
+
+    return NGX_AGAIN;
+}
+
+
 ngx_int_t
 move_chain_to_brigade(ngx_chain_t *chain_orig, apr_bucket_brigade *bb, ngx_pool_t *pool, ngx_int_t last_buf) {
     apr_bucket         *e;
diff --git a/nginx/modsecurity/apr_bucket_nginx.h b/nginx/modsecurity/apr_bucket_nginx.h
@@ -13,6 +13,9 @@ apr_bucket * apr_bucket_nginx_make(apr_bucket *e, ngx_buf_t *buf,
 
 ngx_buf_t * apr_bucket_to_ngx_buf(apr_bucket *e, ngx_pool_t *pool);
 
+ngx_int_t copy_chain_to_brigade(ngx_chain_t *chain_orig,
+		apr_bucket_brigade *bb, ngx_pool_t *pool, ngx_int_t last_buf);
+
 ngx_int_t move_chain_to_brigade(ngx_chain_t *chain, apr_bucket_brigade *bb, ngx_pool_t *pool, ngx_int_t last_buf);
 ngx_int_t move_brigade_to_chain(apr_bucket_brigade *bb, ngx_chain_t **chain, ngx_pool_t *pool);
 
diff --git a/nginx/modsecurity/ngx_http_modsecurity.c b/nginx/modsecurity/ngx_http_modsecurity.c
@@ -18,7 +18,6 @@
 
 #include <apr_base64.h>
 
-
 /* Those are defined twice, lets keep it defined just once by `undef`
  * the first one.
  */
@@ -31,6 +30,29 @@
 #define NOTE_NGINX_REQUEST_CTX "nginx-ctx"
 #define TXID_SIZE 25
 
+/*
+ * If MOVE_REQUEST_CHAIN_TO_MODSEC is set, the chain will be moved into the
+ * ModSecurity brigade, after be processed by ModSecurity, it needs to be moved
+ * to nginx chains again, in order to be processed by other modules. It seems
+ * that this is not working well whenever the request body is delivered into
+ * chunks. Resulting in segfaults.
+ *
+ * If MOVE_REQUEST_CHAIN_TO_MODSEC is _not_ set, a copy of the request body
+ * will be delivered to ModSecurity and after processed it will be released,
+ * not modifying the nginx chain.
+ *
+ * The malfunctioning can be observer while running the following regression
+ * tests:
+ *  #15 - SecRequestBodyInMemoryLimit
+ *  #16 - SecRequestBodyInMemoryLimit (greater)
+ *  #19 - SecRequestBodyLimitAction ProcessPartial (multipart/greater - chunked)
+ *  (from: regression/config/10-request-directives.t)
+ */
+/*
+#define MOVE_REQUEST_CHAIN_TO_MODSEC
+*/
+
+
 #define tuxb
 
 /* 
@@ -492,8 +514,8 @@ ngx_http_modsecurity_save_headers_in_visitor(void *data, const char *key, const
 static ngx_inline ngx_int_t
 ngx_http_modsecurity_load_request_body(ngx_http_request_t *r)
 {
-    ngx_http_modsecurity_ctx_t    *ctx;
-
+    ngx_http_modsecurity_ctx_t *ctx;
+    ngx_chain_t *chain;
 
     ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
         "ModSec: loading request body.");
@@ -502,33 +524,47 @@ ngx_http_modsecurity_load_request_body(ngx_http_request_t *r)
 
     modsecSetBodyBrigade(ctx->req, ctx->brigade);
 
-    if (r->request_body == NULL || r->request_body->bufs == NULL) {
-        ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
-            "ModSec: request was null.");
-
-        return move_chain_to_brigade(NULL, ctx->brigade, r->pool, 1);
+    if (r->request_body == NULL) {
+        chain = NULL;
     }
+    else {
+        chain = r->request_body->bufs;
+    }
+
 
-    if (move_chain_to_brigade(r->request_body->bufs, ctx->brigade, r->pool, 1) != NGX_OK) {
+#ifdef MOVE_REQUEST_CHAIN_TO_MODSEC
+    if (move_chain_to_brigade(chain, ctx->brigade, r->pool, 1) != NGX_OK) {
         ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
             "ModSec: failed to move chain to brigade.");
 
         return NGX_ERROR;
     }
 
     r->request_body = NULL;
+#else
+    if (copy_chain_to_brigade(chain, ctx->brigade, r->pool, 1) != NGX_OK) {
+        ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
+            "ModSec: failed to copy chain to brigade.");
+
+        return NGX_ERROR;
+    }
+#endif
 
     return NGX_OK;
 }
 
 static ngx_inline ngx_int_t
 ngx_http_modsecurity_save_request_body(ngx_http_request_t *r)
 {
-    ngx_http_modsecurity_ctx_t    *ctx;
-    apr_off_t                      content_length;
-    ngx_buf_t                     *buf;
-    ctx = ngx_http_get_module_ctx(r, ngx_http_modsecurity);
+    ngx_http_modsecurity_ctx_t *ctx;
+#ifdef MOVE_REQUEST_CHAIN_TO_MODSEC
+    apr_off_t content_length;
+    ngx_buf_t *buf;
+#endif
+
+   ctx = ngx_http_get_module_ctx(r, ngx_http_modsecurity);
 
+#ifdef MOVE_REQUEST_CHAIN_TO_MODSEC
     apr_brigade_length(ctx->brigade, 0, &content_length);
 
     buf = ngx_create_temp_buf(ctx->r->pool, (size_t) content_length);
@@ -550,10 +586,15 @@ ngx_http_modsecurity_save_request_body(ngx_http_request_t *r)
 
     }
 
-
     r->headers_in.content_length_n = content_length;
 
-    ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "ModSec: Content length: %O, Content length n: %O", content_length, r->headers_in.content_length_n);
+    ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
+            "ModSec: Content length: %O, Content length n: %O", content_length,
+            r->headers_in.content_length_n);
+#else
+    apr_brigade_cleanup(ctx->brigade);
+#endif
+
     return NGX_OK;
 }
 
@@ -787,7 +828,7 @@ ngx_http_modsecurity_status(ngx_http_request_t *r, int status)
  * @param r pointer to ngx_http_request_t.
  *
  */
-static void
+ngx_int_t
 ngx_http_modsecurity_process_request(ngx_http_request_t *r)
 {
     ngx_int_t rc = 0;
@@ -802,23 +843,21 @@ ngx_http_modsecurity_process_request(ngx_http_request_t *r)
     if (ngx_http_modsecurity_load_request(r) != NGX_OK) {
         ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
                "ModSec: failed while loading the request.");
-        ngx_http_finalize_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR);
-        goto terminate;
+        return NGX_HTTP_INTERNAL_SERVER_ERROR;
     }
 
     if (ngx_http_modsecurity_load_headers_in(r) != NGX_OK) {
         ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
                "ModSec: failed while loading the headers.");
-        ngx_http_finalize_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR);
-        goto terminate;
+        return NGX_HTTP_INTERNAL_SERVER_ERROR;
     }
 
     rc = modsecProcessRequestHeaders(ctx->req);
     rc = ngx_http_modsecurity_status(r, rc);
     if (rc != NGX_DECLINED) {
         ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
                "ModSec: failed while processing the request headers");
-        ngx_http_finalize_request(r, rc);
+        return rc;
     }
 
     /* Here we check if ModSecurity is enabled or disabled again.
@@ -841,12 +880,15 @@ ngx_http_modsecurity_process_request(ngx_http_request_t *r)
     }
 
     if (load_request_body == 1) {
+        ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
+            "ModSec: loading request body...");
+
         rc = ngx_http_modsecurity_load_request_body(r);
         if (rc != NGX_OK)
         {
             ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
                 "ModSec: failed while loading the request body.");
-            ngx_http_finalize_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR);
+            return NGX_HTTP_INTERNAL_SERVER_ERROR;
         }
 
 
@@ -862,24 +904,23 @@ ngx_http_modsecurity_process_request(ngx_http_request_t *r)
             "ModSec: finalizing the connection after process the " \
             "request body.");
 
-        ngx_http_finalize_request(r, rc);
+        return rc;
     }
     if (load_request_body == 1) {
         if (ngx_http_modsecurity_save_request_body(r) != NGX_OK)
         {
             ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
                 "ModSec: failed while saving the request body");
-            ngx_http_finalize_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR);
+            return NGX_HTTP_INTERNAL_SERVER_ERROR;
         }
         if (ngx_http_modsecurity_save_headers_in(r) != NGX_OK)
         {
             ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
                 "ModSec: failed while saving the headers in");
-            ngx_http_finalize_request(r, NGX_HTTP_INTERNAL_SERVER_ERROR);
+            return NGX_HTTP_INTERNAL_SERVER_ERROR;
         }
     }
-terminate:
-    return;
+    return NGX_OK;
 }
 
 
@@ -1102,14 +1143,22 @@ ngx_http_modsecurity_handler(ngx_http_request_t *r) {
 
         ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
             "ModSec: request is ready to be processed.");
-        ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, 
-                "ModSec: chuncked? %d", r->chunked);
-        ngx_http_modsecurity_process_request(r);
+        rc = ngx_http_modsecurity_process_request(r);
         ctx->request_processed = 1;
+
+        if (rc == NGX_ERROR || rc >= NGX_HTTP_SPECIAL_RESPONSE) {
+            ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
+                "ModSec: returning a special response after process " \
+                "a request: %d", rc);
+
+           return rc;
+        }
+
+
     }
 
     ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
-        "ModSec: returning NGX_OK. Count++ :P" );
+        "ModSec: returning NGX_DECLINED." );
 
     return NGX_DECLINED;
 }
