Fix incorrect handling of request/response body data chain of ngx_buf_t buffers.

[turchanov]

The documentation [http://nginx.org/en/docs/dev/development_guide.html#buffer]
clearly states that .pos, .last must be used to reference actual data
contained by the buffer. Whereas .start, .end denote the boundaries of the memory
block allocated for the buffer (in case of dynamically allocated data) or just
NULL (when .pos, .last reference a static memory location - one can see that
kind of usage in ngx_http_gzip_filter_module.c:ngx_http_gzip_filter_gzheader()).
To back up my words I invite to examine
ngx_http_charset_filter_module.c:ngx_http_charset_recode() as an example of
iteration over data contained in data buffer.

Without this fix ngx_http_modsecurity_body_filter feeds random bytes from
memory pointed by .start, .end range to msc_append_response_body. In my case
is was 8KB of data instead of 10 bytes when referenced by (.pos, .last).
That is this vulnerability may disclose sensitive data like passwords or
whatever from nginx heap.

The fix for ngx_http_modsecurity_pre_access_handler is to use .pos not .start to
reference data as they may differ in general case.

[zimmerle]

Thank you @turchanov, i will have a look together with @defanator.

[defanator]

@turchanov how did you test your changes given that currently this module explicitly requires request body in temp. file: https://github.com/SpiderLabs/ModSecurity-nginx/blob/master/src/ngx_http_modsecurity_pre_access.c#L105-L106 ?

I'm seeing REQBODY_ERROR with "JSON parsing error: parse error: premature EOF\x0a" when I'm trying to POST valid JSON data with your patch applied and without hard requirements of using temp. files.

[defanator]

@turchanov please disregard the #104 (review) comment here, messed up things a bit. Will provide more information later.

[defanator]

@turchanov this change looks good, though I'd use the ngx_buf_size() macro here. Also, it would be better to move chain assignment closer to the cycle in ngx_http_modsecurity_pre_access_handler() for better readability (I'd even love to use "standard" for loop as it is commonly used across the code and dev guide).

Consider the following patch on top of your one:

diff --git a/src/ngx_http_modsecurity_body_filter.c b/src/ngx_http_modsecurity_body_filter.c
index edf44f6..690190c 100644
--- a/src/ngx_http_modsecurity_body_filter.c
+++ b/src/ngx_http_modsecurity_body_filter.c
@@ -152,7 +152,8 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in)
         {
             u_char *data = chain->buf->pos;
 
-            msc_append_response_body(ctx->modsec_transaction, data, chain->buf->last - data);
+            msc_append_response_body(ctx->modsec_transaction, data,
+                                     ngx_buf_size(chain->buf));
             ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r);
             if (ret > 0) {
                 return ngx_http_filter_finalize_request(r,
diff --git a/src/ngx_http_modsecurity_pre_access.c b/src/ngx_http_modsecurity_pre_access.c
index 6f4cbcb..3abfacc 100644
--- a/src/ngx_http_modsecurity_pre_access.c
+++ b/src/ngx_http_modsecurity_pre_access.c
@@ -131,8 +131,6 @@ ngx_http_modsecurity_pre_access_handler(ngx_http_request_t *r)
 
         dd("request body is ready to be processed");
 
-        ngx_chain_t *chain = r->request_body->bufs;
-
         /**
          * TODO: Speed up the analysis by sending chunk while they arrive.
          *
@@ -161,12 +159,13 @@ ngx_http_modsecurity_pre_access_handler(ngx_http_request_t *r)
             dd("inspection request body in memory.");
         }
 
+        ngx_chain_t *chain = r->request_body->bufs;
         while (chain && !already_inspected)
         {
             u_char *data = chain->buf->pos;
 
             msc_append_request_body(ctx->modsec_transaction, data,
-                chain->buf->last - data);
+                                    ngx_buf_size(chain->buf));
 
             if (chain->buf->last_buf) {
                 break;

[zimmerle]

dev/pull104 is now on our build bots.

[turchanov]

I voiced some of my concerns about ngx_buf_size in my comment in #105. I thought that you would answer them...

[zimmerle]

Hi @turchanov,

I did not merged to main line yet. Place it in this new branch. Waiting for @defanator's comments.

[turchanov]

I did not merged to main line yet. Place it in this new branch. ....

I beg you pardon, I do not understand what do you want me to do?
Do I need to re-post "my concerns abount ngx_buf_size" from #105 to this pull request?

[turchanov]

If you did ask to re-post my comments from #105, I can do it, but @defanator already answered them in #105. I'am sorry if I caused such a confusion. So what do I need to do?

[zimmerle]

Merged. Thanks ;)