Skip to content

Commit 146054f

Browse files
turchanovFelipe Zimmerle
authored and
Felipe Zimmerle
committed
Fixed processing of response body chunks 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 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.
1 parent 3d82ee2 commit 146054f

File tree

1 file changed

+25
-30
lines changed

1 file changed

+25
-30
lines changed

Diff for: src/ngx_http_modsecurity_body_filter.c

+25-30
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@ ngx_http_modsecurity_body_filter_init(void)
3535
ngx_int_t
3636
ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in)
3737
{
38-
int buffer_fully_loadead = 0;
3938
ngx_chain_t *chain = in;
4039
ngx_http_modsecurity_ctx_t *ctx = NULL;
4140
#if defined(MODSECURITY_SANITY_CHECKS) && (MODSECURITY_SANITY_CHECKS)
@@ -135,47 +134,43 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in)
135134
}
136135
#endif
137136

137+
int is_request_processed = 0;
138138
for (; chain != NULL; chain = chain->next)
139139
{
140-
/* XXX: chain->buf->last_buf || chain->buf->last_in_chain */
141-
if (chain->buf->last_buf) {
142-
buffer_fully_loadead = 1;
140+
u_char *data = chain->buf->pos;
141+
int ret;
142+
143+
msc_append_response_body(ctx->modsec_transaction, data, chain->buf->last - data);
144+
ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r);
145+
if (ret > 0) {
146+
return ngx_http_filter_finalize_request(r,
147+
&ngx_http_modsecurity_module, ret);
143148
}
144-
}
145149

146-
if (buffer_fully_loadead == 1)
147-
{
148-
int ret;
149-
ngx_pool_t *old_pool;
150+
/* XXX: chain->buf->last_buf || chain->buf->last_in_chain */
151+
is_request_processed = chain->buf->last_buf;
150152

151-
for (chain = in; chain != NULL; chain = chain->next)
152-
{
153-
u_char *data = chain->buf->pos;
153+
if (is_request_processed) {
154+
ngx_pool_t *old_pool;
155+
156+
old_pool = ngx_http_modsecurity_pcre_malloc_init(r->pool);
157+
msc_process_response_body(ctx->modsec_transaction);
158+
ngx_http_modsecurity_pcre_malloc_done(old_pool);
154159

155-
msc_append_response_body(ctx->modsec_transaction, data, chain->buf->last - data);
160+
/* XXX: I don't get how body from modsec being transferred to nginx's buffer. If so - after adjusting of nginx's
161+
XXX: body we can proceed to adjust body size (content-length). see xslt_body_filter() for example */
156162
ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r);
157163
if (ret > 0) {
158-
return ngx_http_filter_finalize_request(r,
159-
&ngx_http_modsecurity_module, ret);
164+
return ret;
160165
}
161-
}
162-
163-
old_pool = ngx_http_modsecurity_pcre_malloc_init(r->pool);
164-
msc_process_response_body(ctx->modsec_transaction);
165-
ngx_http_modsecurity_pcre_malloc_done(old_pool);
166+
else if (ret < 0) {
167+
return ngx_http_filter_finalize_request(r,
168+
&ngx_http_modsecurity_module, NGX_HTTP_INTERNAL_SERVER_ERROR);
166169

167-
/* XXX: I don't get how body from modsec being transferred to nginx's buffer. If so - after adjusting of nginx's
168-
XXX: body we can proceed to adjust body size (content-length). see xslt_body_filter() for example */
169-
ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r);
170-
if (ret > 0) {
171-
return ret;
172-
}
173-
else if (ret < 0) {
174-
return ngx_http_filter_finalize_request(r,
175-
&ngx_http_modsecurity_module, NGX_HTTP_INTERNAL_SERVER_ERROR);
170+
}
176171
}
177172
}
178-
else
173+
if (!is_request_processed)
179174
{
180175
dd("buffer was not fully loaded! ctx: %p", ctx);
181176
}

0 commit comments

Comments
 (0)