Fixed processing of response body chunks ... #105
Conversation
…_t buffers. 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.
… can be
called by Nginx several times during request processing. And each time with
it own unique set of chained buf pointers.
For example, suppose a complete response consists of this chain of data:
A->B->C->D->E
Ngix may (and actually does, as verified by me in gdb) call body filter two
times like this:
handler(r, in = A->B->C)
handler(r, in = D->E), E has last_buf set
Current implementation delays feeding chain->buf to msc_append_response_body
until it comes upon a chain with buf->last_buf set. So we loose chain containing
A->B->C sequence. We must process body bufs as soon as we see them in body
handler otherwise we will not see them again.
N.B. You have PR owasp-modsecurity#84 pending. It goes further and fixes the problem when
a blocking decision is made after headers were sent. I intentionally retained
current (buggy) behavior to make my patch less intrusive and easier to review.
Besides owasp-modsecurity#84 impose an excessive memory usage due to a complete copy of all
bufs passed through body filter (we have sometimes 500K and more replies in our
applications) - I will elaborate on this in code review for owasp-modsecurity#84.
| u_char *data = chain->buf->pos; | ||
| int ret; | ||
|
|
||
| msc_append_response_body(ctx->modsec_transaction, data, chain->buf->last - data); |
There was a problem hiding this comment.
Let's use ngx_buf_size() here.
There was a problem hiding this comment.
ngx_buf_size is defined as
#define ngx_buf_size(b) \
(ngx_buf_in_memory(b) ? (off_t) (b->last - b->pos): \
(b->file_last - b->file_pos))
If somehow we happend to receive a buf which is completely in a file (i.e., buf->in_file == true but ngx_buf_in_memory(buf) == false) then .pos = .last = 0 but .file_last - .file_post > 0, so we will try to dereference a non-zero length block at NULL pointer .pos. But with current .last - .pos we would erroneously assume that we have no data but won't segfault at least.
But from the other hand I did a quick testing with gdb, even when a response body is buffered to a temporary file, nginx supplies a buf which has both .in_file and .temporary set (ngx_buf_in_memory(buf) == true => .pos, .last are valid).
ngx_http_charset_filter_module also references .pos/.last in ngx_http_charset_recode_{to,from}_utf8 without worries . So I think it is guaranteed that response body filters get chain bufs with ngx_buf_in_memory(buf) == true ?
One more thing, I did a quick grep of Nginx sources and see that ngx_buf_size is not used that much, whereas .last - .pos is much more common... maybe we should stick to the latter?
There was a problem hiding this comment.
@turchanov well ok - let's keep .last - .pos.
| @@ -163,10 +163,10 @@ ngx_http_modsecurity_pre_access_handler(ngx_http_request_t *r) | |||
|
|
|||
| while (chain && !already_inspected) | |||
| { | |||
| u_char *data = chain->buf->start; | |||
| u_char *data = chain->buf->pos; | |||
|
|
|||
| msc_append_request_body(ctx->modsec_transaction, data, | |||
There was a problem hiding this comment.
Ditto (it seems like the commit from #104 though, I left the same comment there).
|
Merged. Thanks ;) |
in ngx_http_modsecurity_body_filter.
A body filter function (ngx_http_modsecurity_body_filter in our case) can be
called by Nginx several times during request processing. And each time with
it own unique set of chained buf pointers.
For example, suppose a complete response consists of this chain of data:
A->B->C->D->E
Ngix may (and actually does, as verified by me in gdb) call body filter two
times like this:
handler(r, in = A->B->C)
handler(r, in = D->E), E has last_buf set
Current implementation delays feeding chain->buf to msc_append_response_body
until it comes upon a chain with buf->last_buf set. So we loose chain containing
A->B->C sequence. We must process body bufs as soon as we see them in body
handler otherwise we will not see them again.
N.B. You have PR #84 pending. It goes further and fixes the problem when
a blocking decision is made after headers were sent. I intentionally retained
current (buggy) behavior to make my patch less intrusive and easier to review.
Besides #84 impose an excessive memory usage due to a complete copy of all
bufs passed through body filter (we have sometimes 500K and more replies in our
applications) - I will elaborate on this in code review for #84.
This PR depends on #104