Skip to content

Commit 921d73d

Browse files
airweenairween
committed
Re-add patch from owasp-modsecurity#326 
1 parent 6ee9481 commit 921d73d

File tree

8 files changed

+133
-79
lines changed

8 files changed

+133
-79
lines changed

‎.github/workflows/test.yml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ jobs:
112112
fi
113113
- name: Run file consistency check 1
114114
run: |
115-
curl -sS "http://localhost/data50k.json" --output data50k.json
115+
curl -m 5 -sS "http://localhost/data50k.json" --output data50k.json
116116
if [ -f data50k.json ]; then
117117
diff data50k.json /usr/local/nginx/html/data50k.json > /dev/null
118118
if [ $? -eq 0 ]; then
@@ -127,7 +127,7 @@ jobs:
127127
fi
128128
- name: Run file consistency check 2
129129
run: |
130-
curl -sS "http://localhost/plugged.png" --output plugged.png
130+
curl -m 5 -sS "http://localhost/plugged.png" --output plugged.png
131131
if [ -f plugged.png ]; then
132132
diff plugged.png /usr/local/nginx/html/plugged.png > /dev/null
133133
if [ $? -eq 0 ]; then

‎src/ngx_http_modsecurity_body_filter.c‎

Lines changed: 79 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,11 @@ ngx_http_modsecurity_body_filter_init(void)
3737
ngx_int_t
3838
ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in)
3939
{
40-
ngx_chain_t *chain = in;
4140
ngx_http_modsecurity_ctx_t *ctx = NULL;
41+
ngx_chain_t *chain = in;
42+
ngx_int_t ret;
43+
ngx_pool_t *old_pool;
44+
ngx_int_t is_request_processed = 0;
4245
#if defined(MODSECURITY_SANITY_CHECKS) && (MODSECURITY_SANITY_CHECKS)
4346
ngx_http_modsecurity_conf_t *mcf;
4447
ngx_list_part_t *part = &r->headers_out.headers.part;
@@ -47,14 +50,19 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in)
4750
#endif
4851

4952
if (in == NULL) {
53+
ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "MDS input chain is null");
5054
return ngx_http_next_body_filter(r, in);
5155
}
5256

57+
/* get context for request */
5358
ctx = ngx_http_get_module_ctx(r, ngx_http_modsecurity_module);
54-
5559
dd("body filter, recovering ctx: %p", ctx);
5660

57-
if (ctx == NULL) {
61+
62+
if (ctx == NULL || r->filter_finalize || ctx->response_body_filtered) {
63+
if (ctx && ctx->response_body_filtered) {
64+
r->filter_finalize = 1;
65+
}
5866
return ngx_http_next_body_filter(r, in);
5967
}
6068

@@ -140,47 +148,82 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in)
140148
}
141149
#endif
142150

143-
int is_request_processed = 0;
144-
for (; chain != NULL; chain = chain->next)
145-
{
146-
u_char *data = chain->buf->pos;
147-
int ret;
151+
for (chain = in; chain != NULL; chain = chain->next) {
148152

149-
msc_append_response_body(ctx->modsec_transaction, data, chain->buf->last - data);
150-
ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r, 0);
153+
154+
ngx_buf_t *copy_buf;
155+
ngx_chain_t* copy_chain;
156+
is_request_processed = chain->buf->last_buf;
157+
u_char *data = chain->buf->pos;
158+
msc_append_response_body(ctx->modsec_transaction, data,
159+
chain->buf->last - data);
160+
ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction,
161+
r, 0);
151162
if (ret > 0) {
152163
return ngx_http_filter_finalize_request(r,
153164
&ngx_http_modsecurity_module, ret);
154165
}
155-
156-
/* XXX: chain->buf->last_buf || chain->buf->last_in_chain */
157-
is_request_processed = chain->buf->last_buf;
158-
159-
if (is_request_processed) {
160-
ngx_pool_t *old_pool;
161-
162-
old_pool = ngx_http_modsecurity_pcre_malloc_init(r->pool);
163-
msc_process_response_body(ctx->modsec_transaction);
164-
ngx_http_modsecurity_pcre_malloc_done(old_pool);
165-
166-
/* XXX: I don't get how body from modsec being transferred to nginx's buffer. If so - after adjusting of nginx's
167-
XXX: body we can proceed to adjust body size (content-length). see xslt_body_filter() for example */
168-
ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r, 0);
169-
if (ret > 0) {
170-
return ret;
166+
if (!chain->buf->last_buf){
167+
copy_chain = ngx_alloc_chain_link(r->pool);
168+
if (copy_chain == NULL) {
169+
return NGX_ERROR;
171170
}
172-
else if (ret < 0) {
173-
return ngx_http_filter_finalize_request(r,
174-
&ngx_http_modsecurity_module, NGX_HTTP_INTERNAL_SERVER_ERROR);
175-
171+
copy_buf = ngx_calloc_buf(r->pool);
172+
if (copy_buf == NULL) {
173+
return NGX_ERROR;
176174
}
175+
copy_buf->pos = chain->buf->pos ;
176+
copy_buf->end = chain->buf->end;
177+
copy_buf->last = chain->buf->last;
178+
copy_buf->temporary = (chain->buf->temporary == 1) ? 1 : 0;
179+
copy_buf->memory = (chain->buf->memory == 1) ? 1 : 0;
180+
copy_chain->buf = copy_buf;
181+
copy_chain->buf->last_buf = chain->buf->last_buf;
182+
copy_chain->next = NULL;
183+
//chain->buf->pos = chain->buf->last;
184+
}
185+
else {
186+
copy_chain = chain;
187+
}
188+
if (ctx->temp_chain == NULL) {
189+
ctx->temp_chain = copy_chain;
190+
} else {
191+
if (ctx->current_chain == NULL) {
192+
ctx->temp_chain->next = copy_chain;
193+
ctx->temp_chain->buf->last_buf = 0;
194+
} else {
195+
ctx->current_chain->next = copy_chain;
196+
ctx->current_chain->buf->last_buf = 0;
197+
}
198+
ctx->current_chain = copy_chain;
177199
}
178-
}
179-
if (!is_request_processed)
180-
{
181-
dd("buffer was not fully loaded! ctx: %p", ctx);
182200
}
183201

184-
/* XXX: xflt_filter() -- return NGX_OK here */
185-
return ngx_http_next_body_filter(r, in);
202+
if (is_request_processed) {
203+
old_pool = ngx_http_modsecurity_pcre_malloc_init(r->pool);
204+
msc_process_response_body(ctx->modsec_transaction);
205+
ngx_http_modsecurity_pcre_malloc_done(old_pool);
206+
ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r, 0);
207+
if (ret > 0) {
208+
if (ret < NGX_HTTP_BAD_REQUEST && ctx->header_pt != NULL){
209+
ctx->header_pt(r);
210+
}
211+
else {
212+
ctx->response_body_filtered = 1;
213+
return ngx_http_filter_finalize_request(r,
214+
&ngx_http_modsecurity_module
215+
, ret);
216+
}
217+
} else if (ret < 0) {
218+
ctx->response_body_filtered = 1;
219+
return ngx_http_filter_finalize_request(r,
220+
&ngx_http_modsecurity_module, NGX_HTTP_INTERNAL_SERVER_ERROR);
221+
}
222+
ctx->response_body_filtered = 1;
223+
if (ctx->header_pt != NULL)
224+
ctx->header_pt(r);
225+
return ngx_http_next_body_filter(r, ctx->temp_chain);
226+
} else {
227+
return NGX_AGAIN;
228+
}
186229
}

‎src/ngx_http_modsecurity_common.h‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,6 @@ typedef struct {
7676
ngx_str_t value;
7777
} ngx_http_modsecurity_header_t;
7878

79-
8079
typedef struct {
8180
ngx_http_request_t *r;
8281
Transaction *modsec_transaction;
@@ -97,6 +96,10 @@ typedef struct {
9796
unsigned waiting_more_body:1;
9897
unsigned body_requested:1;
9998
unsigned processed:1;
99+
ngx_http_output_header_filter_pt header_pt;
100+
ngx_chain_t* temp_chain;
101+
ngx_chain_t* current_chain;
102+
unsigned response_body_filtered:1;
100103
unsigned logged:1;
101104
unsigned intervention_triggered:1;
102105
} ngx_http_modsecurity_ctx_t;

‎src/ngx_http_modsecurity_header_filter.c‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -551,10 +551,17 @@ ngx_http_modsecurity_header_filter(ngx_http_request_t *r)
551551
*
552552
*/
553553

554-
/*
555-
* The line below is commented to make the spdy test to work
556-
*/
557-
//r->headers_out.content_length_n = -1;
558-
559-
return ngx_http_next_header_filter(r);
554+
/*
555+
* The line below is commented to make the spdy test to work
556+
*/
557+
//r->headers_out.content_length_n = -1;
558+
559+
if (ctx->response_body_filtered){
560+
return ngx_http_next_header_filter(r);
561+
}
562+
else {
563+
ctx->header_pt = ngx_http_next_header_filter;
564+
r->headers_out.content_length_n = -1;
565+
return NGX_AGAIN;
566+
}
560567
}

‎src/ngx_http_modsecurity_module.c‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -292,6 +292,7 @@ ngx_http_modsecurity_create_ctx(ngx_http_request_t *r)
292292

293293
dd("transaction created");
294294

295+
ctx->response_body_filtered = 0;
295296
ngx_http_set_ctx(r, ctx, ngx_http_modsecurity_module);
296297

297298
cln = ngx_pool_cleanup_add(r->pool, sizeof(ngx_http_modsecurity_ctx_t));

‎tests/modsecurity-proxy.t‎

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -114,28 +114,28 @@ unlike(http_head('/'), qr/SEE-THIS/, 'proxy head request');
114114

115115

116116
# Redirect (302)
117-
like(http_get('/phase1?what=redirect302'), qr/^HTTP.*302/, 'redirect 302 - phase 1');
118-
like(http_get('/phase2?what=redirect302'), qr/^HTTP.*302/, 'redirect 302 - phase 2');
119-
like(http_get('/phase3?what=redirect302'), qr/^HTTP.*302/, 'redirect 302 - phase 3');
120-
is(http_get('/phase4?what=redirect302'), '', 'redirect 302 - phase 4');
117+
like(http_get('/phase1?what=redirect302'), qr/302 Moved Temporarily/, 'redirect 302 - phase 1');
118+
like(http_get('/phase2?what=redirect302'), qr/302 Moved Temporarily/, 'redirect 302 - phase 2');
119+
like(http_get('/phase3?what=redirect302'), qr/302 Moved Temporarily/, 'redirect 302 - phase 3');
120+
like(http_get('/phase4?what=redirect302'), qr/404/, 'redirect 302 - phase 4');
121121

122122
# Redirect (301)
123-
like(http_get('/phase1?what=redirect301'), qr/^HTTP.*301/, 'redirect 301 - phase 1');
124-
like(http_get('/phase2?what=redirect301'), qr/^HTTP.*301/, 'redirect 301 - phase 2');
125-
like(http_get('/phase3?what=redirect301'), qr/^HTTP.*301/, 'redirect 301 - phase 3');
126-
is(http_get('/phase4?what=redirect301'), '', 'redirect 301 - phase 4');
123+
like(http_get('/phase1?what=redirect301'), qr/301 Moved Permanently/, 'redirect 301 - phase 1');
124+
like(http_get('/phase2?what=redirect301'), qr/301 Moved Permanently/, 'redirect 301 - phase 2');
125+
like(http_get('/phase3?what=redirect301'), qr/301 Moved Permanently/, 'redirect 301 - phase 3');
126+
like(http_get('/phase4?what=redirect301'), qr/404/, 'redirect 301 - phase 4');
127127

128128
# Block (401)
129-
like(http_get('/phase1?what=block401'), qr/^HTTP.*401/, 'block 401 - phase 1');
130-
like(http_get('/phase2?what=block401'), qr/^HTTP.*401/, 'block 401 - phase 2');
131-
like(http_get('/phase3?what=block401'), qr/^HTTP.*401/, 'block 401 - phase 3');
132-
is(http_get('/phase4?what=block401'), '', 'block 401 - phase 4');
129+
like(http_get('/phase1?what=block401'), qr/401 Unauthorized/, 'block 401 - phase 1');
130+
like(http_get('/phase2?what=block401'), qr/401 Unauthorized/, 'block 401 - phase 2');
131+
like(http_get('/phase3?what=block401'), qr/401 Unauthorized/, 'block 401 - phase 3');
132+
like(http_get('/phase4?what=block401'), qr/401 Unauthorized/, 'block 401 - phase 4');
133133

134134
# Block (403)
135-
like(http_get('/phase1?what=block403'), qr/^HTTP.*403/, 'block 403 - phase 1');
136-
like(http_get('/phase2?what=block403'), qr/^HTTP.*403/, 'block 403 - phase 2');
137-
like(http_get('/phase3?what=block403'), qr/^HTTP.*403/, 'block 403 - phase 3');
138-
is(http_get('/phase4?what=block403'), '', 'block 403 - phase 4');
135+
like(http_get('/phase1?what=block403'), qr/403 Forbidden/, 'block 403 - phase 1');
136+
like(http_get('/phase2?what=block403'), qr/403 Forbidden/, 'block 403 - phase 2');
137+
like(http_get('/phase3?what=block403'), qr/403 Forbidden/, 'block 403 - phase 3');
138+
like(http_get('/phase4?what=block403'), qr/403 Forbidden/, 'block 403 - phase 4');
139139

140140
# Nothing to detect
141141
like(http_get('/phase1?what=nothing'), qr/phase1\?what=nothing\' not found/, 'nothing phase 1');

‎tests/modsecurity-scoring.t‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ http {
4646
SecRuleEngine On
4747
SecRule ARGS "@streq badarg1" "id:11,phase:2,setvar:tx.score=1"
4848
SecRule ARGS "@streq badarg2" "id:12,phase:2,setvar:tx.score=2"
49-
SecRule TX:SCORE "@ge 2" "id:199,phase:request,deny,log,status:403"
49+
SecRule tx:score "@ge 2" "id:199,phase:request,deny,log,status:403"
5050
';
5151
}
5252
@@ -56,7 +56,7 @@ http {
5656
SecRule ARGS "@streq badarg1" "id:21,phase:2,setvar:tx.score=+1"
5757
SecRule ARGS "@streq badarg2" "id:22,phase:2,setvar:tx.score=+1"
5858
SecRule ARGS "@streq badarg3" "id:23,phase:2,setvar:tx.score=+1"
59-
SecRule TX:SCORE "@ge 3" "id:299,phase:request,deny,log,status:403"
59+
SecRule tx:score "@ge 3" "id:299,phase:request,deny,log,status:403"
6060
';
6161
}
6262
}

‎tests/modsecurity.t‎

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -139,28 +139,28 @@ $t->plan(21);
139139

140140

141141
# Redirect (302)
142-
like(http_get('/phase1?what=redirect302'), qr/^HTTP.*302/, 'redirect 302 - phase 1');
143-
like(http_get('/phase2?what=redirect302'), qr/^HTTP.*302/, 'redirect 302 - phase 2');
144-
like(http_get('/phase3?what=redirect302'), qr/^HTTP.*302/, 'redirect 302 - phase 3');
145-
is(http_get('/phase4?what=redirect302'), '', 'redirect 302 - phase 4');
142+
like(http_get('/phase1?what=redirect302'), qr/302 Moved Temporarily/, 'redirect 302 - phase 1');
143+
like(http_get('/phase2?what=redirect302'), qr/302 Moved Temporarily/, 'redirect 302 - phase 2');
144+
like(http_get('/phase3?what=redirect302'), qr/302 Moved Temporarily/, 'redirect 302 - phase 3');
145+
like(http_get('/phase4?what=redirect302'), qr/200/, 'redirect 302 - phase 4');
146146

147147
# Redirect (301)
148-
like(http_get('/phase1?what=redirect301'), qr/^HTTP.*301/, 'redirect 301 - phase 1');
149-
like(http_get('/phase2?what=redirect301'), qr/^HTTP.*301/, 'redirect 301 - phase 2');
150-
like(http_get('/phase3?what=redirect301'), qr/^HTTP.*301/, 'redirect 301 - phase 3');
151-
is(http_get('/phase4?what=redirect301'), '', 'redirect 301 - phase 4');
148+
like(http_get('/phase1?what=redirect301'), qr/301 Moved Permanently/, 'redirect 301 - phase 1');
149+
like(http_get('/phase2?what=redirect301'), qr/301 Moved Permanently/, 'redirect 301 - phase 2');
150+
like(http_get('/phase3?what=redirect301'), qr/301 Moved Permanently/, 'redirect 301 - phase 3');
151+
like(http_get('/phase4?what=redirect301'), qr/200/, 'redirect 301 - phase 4');
152152

153153
# Block (401)
154-
like(http_get('/phase1?what=block401'), qr/^HTTP.*401/, 'block 401 - phase 1');
155-
like(http_get('/phase2?what=block401'), qr/^HTTP.*401/, 'block 401 - phase 2');
156-
like(http_get('/phase3?what=block401'), qr/^HTTP.*401/, 'block 401 - phase 3');
157-
is(http_get('/phase4?what=block401'), '', 'block 401 - phase 4');
154+
like(http_get('/phase1?what=block401'), qr/401 Unauthorized/, 'block 401 - phase 1');
155+
like(http_get('/phase2?what=block401'), qr/401 Unauthorized/, 'block 401 - phase 2');
156+
like(http_get('/phase3?what=block401'), qr/401 Unauthorized/, 'block 401 - phase 3');
157+
like(http_get('/phase4?what=block401'), qr/401 Unauthorized/, 'block 401 - phase 4');
158158

159159
# Block (403)
160-
like(http_get('/phase1?what=block403'), qr/^HTTP.*403/, 'block 403 - phase 1');
161-
like(http_get('/phase2?what=block403'), qr/^HTTP.*403/, 'block 403 - phase 2');
162-
like(http_get('/phase3?what=block403'), qr/^HTTP.*403/, 'block 403 - phase 3');
163-
is(http_get('/phase4?what=block403'), '', 'block 403 - phase 4');
160+
like(http_get('/phase1?what=block403'), qr/403 Forbidden/, 'block 403 - phase 1');
161+
like(http_get('/phase2?what=block403'), qr/403 Forbidden/, 'block 403 - phase 2');
162+
like(http_get('/phase3?what=block403'), qr/403 Forbidden/, 'block 403 - phase 3');
163+
like(http_get('/phase4?what=block403'), qr/403 Forbidden/, 'block 403 - phase 4');
164164

165165
# Nothing to detect
166166
like(http_get('/phase1?what=nothing'), qr/should be moved\/blocked before this./, 'nothing phase 1');

0 commit comments

Comments
 (0)