Fix response body #5
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
While here, fixed msc_set_connector_info() to use consistent version string. This fixes owasp-modsecurity#85.
intervention.log is being allocated via strdup() here: https://github.com/SpiderLabs/ModSecurity/blob/v3/master/src/transaction.cc#L1362 and should be freed by connector.
A number of improvements in README
Fix incorrect handling of request/response body data chain of ngx_buf_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.
…_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 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.
Changed placement of ngx_http_modsecurity_module in nginx load order of modules to come after ngx_http_gzip_filter_module so that we could read a response body before it gets compressed. Prior to this fix ngx_http_modsecurity_body_filter was called after gzip filter so that msc_append_response_body was fed with compressed body bytes thus effectively making all further response body processing meaningless.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.