Skip to content

Commit 9804b96

Browse files
vortexopenclawvortexopenclaw
committed
Return HTTP 400 for JSON completion errors without relaxing rejection
1 parent 42b02c6 commit 9804b96

3 files changed

Lines changed: 364 additions & 3 deletions

File tree

‎apache2/apache2_io.c‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -354,6 +354,9 @@ apr_status_t read_request_body(modsec_rec *msr, char **error_msg) {
354354
if (rcbe == -5) {
355355
return HTTP_REQUEST_ENTITY_TOO_LARGE;
356356
}
357+
if (rcbe == -2) {
358+
return HTTP_BAD_REQUEST;
359+
}
357360
if (rcbe < 0) {
358361
return HTTP_INTERNAL_SERVER_ERROR;
359362
}

‎apache2/msc_reqbody.c‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -651,7 +651,8 @@ static apr_status_t modsecurity_request_body_end_urlencoded(modsec_rec *msr, cha
651651
}
652652

653653
/**
654-
* Stops receiving the request body.
654+
* Stops receiving the request body. Returns -2 for a JSON completion error,
655+
* -5 for the no-files limit, and -1 for other failures.
655656
*/
656657
apr_status_t modsecurity_request_body_end(modsec_rec *msr, char **error_msg) {
657658
assert(msr != NULL);
@@ -728,8 +729,8 @@ apr_status_t modsecurity_request_body_end(modsec_rec *msr, char **error_msg) {
728729
msr->msc_reqbody_error = 1;
729730
msr->msc_reqbody_error_msg = *error_msg;
730731
msr_log(msr, 2, "%s", *error_msg);
731-
return -1;
732-
}
732+
return -2;
733+
}
733734
#else
734735
*error_msg = apr_psprintf(msr->mp, "JSON support was not enabled");
735736
msr->msc_reqbody_error = 1;

‎tests/regression/rule/15-json-eof.t‎

Lines changed: 357 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,357 @@
1+
### JSON completion errors return 400 without changing rejection policy.
2+
### These declarative cases require YAJL, like the parser assertions in 15-json.t.
3+
### DetectionOnly rejection is known-wrong behavior tracked by #2807; it is
4+
### deliberately not asserted here. Phase-2-controlled handling also remains open.
5+
{
6+
type => "rule",
7+
comment => "json parser - issue #2807 - premature EOF is rejected before phase 2",
8+
conf => qq(
9+
SecRuleEngine On
10+
SecRequestBodyAccess On
11+
SecDebugLog $ENV{DEBUG_LOG}
12+
SecDebugLogLevel 9
13+
SecRule REQUEST_HEADERS:Content-Type "^application/json" \\
14+
"id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON"
15+
SecRule REQBODY_ERROR "!\@eq 0" \\
16+
"id:200002,phase:2,t:none,log,deny,status:400,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'"
17+
SecAction "id:200005,phase:2,pass,log,msg:'JSON phase 2 reached'"
18+
),
19+
match_log => {
20+
error => [ qr/JSON parser error:.*premature EOF/s, 1 ],
21+
-error => [ qr/\[id "20000[25]"\]/, 1 ],
22+
},
23+
match_response => {
24+
status => qr/^400$/,
25+
},
26+
request => new HTTP::Request(
27+
POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt",
28+
[ "Content-Type" => "application/json" ],
29+
'{',
30+
),
31+
},
32+
33+
{
34+
type => "rule",
35+
comment => "json parser - issue #2807 - incomplete object retains rejection before a custom error rule",
36+
conf => qq(
37+
SecRuleEngine On
38+
SecRequestBodyAccess On
39+
SecDebugLog $ENV{DEBUG_LOG}
40+
SecDebugLogLevel 9
41+
SecRule REQUEST_HEADERS:Content-Type "^application/json" \\
42+
"id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON"
43+
SecRule REQBODY_ERROR "!\@eq 0" \\
44+
"id:200002,phase:2,t:none,log,deny,status:422,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'"
45+
SecAction "id:200005,phase:2,pass,log,msg:'JSON phase 2 reached'"
46+
),
47+
match_log => {
48+
error => [ qr/JSON parser error:.*premature EOF/s, 1 ],
49+
-error => [ qr/\[id "20000[25]"\]/, 1 ],
50+
},
51+
match_response => {
52+
status => qr/^400$/,
53+
},
54+
request => new HTTP::Request(
55+
POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt",
56+
[ "Content-Type" => "application/json" ],
57+
'{ "id" : "123"',
58+
),
59+
},
60+
61+
{
62+
type => "rule",
63+
comment => "json parser - issue #2807 - premature EOF retains rejection with a non-disruptive error rule",
64+
conf => qq(
65+
SecRuleEngine On
66+
SecRequestBodyAccess On
67+
SecDebugLog $ENV{DEBUG_LOG}
68+
SecDebugLogLevel 9
69+
SecRule REQUEST_HEADERS:Content-Type "^application/json" \\
70+
"id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON"
71+
SecRule REQBODY_ERROR "!\@eq 0" \\
72+
"id:200002,phase:2,t:none,log,pass,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'"
73+
SecAction "id:200005,phase:2,pass,log,msg:'JSON phase 2 reached'"
74+
),
75+
match_log => {
76+
error => [ qr/JSON parser error:.*premature EOF/s, 1 ],
77+
-error => [ qr/\[id "20000[25]"\]/, 1 ],
78+
},
79+
match_response => {
80+
status => qr/^400$/,
81+
},
82+
request => new HTTP::Request(
83+
POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt",
84+
[ "Content-Type" => "application/json" ],
85+
'{',
86+
),
87+
},
88+
89+
{
90+
type => "rule",
91+
comment => "json parser - issue #2807 - valid empty object does not set REQBODY_ERROR",
92+
conf => qq(
93+
SecRuleEngine On
94+
SecRequestBodyAccess On
95+
SecDebugLog $ENV{DEBUG_LOG}
96+
SecDebugLogLevel 9
97+
SecRule REQUEST_HEADERS:Content-Type "^application/json" \\
98+
"id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON"
99+
SecRule REQBODY_ERROR "!\@eq 0" \\
100+
"id:200002,phase:2,t:none,log,deny,status:400,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'"
101+
),
102+
match_log => {
103+
-error => [ qr/\[id "200002"\]/, 1 ],
104+
},
105+
match_response => {
106+
status => qr/^200$/,
107+
},
108+
request => new HTTP::Request(
109+
POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt",
110+
[ "Content-Type" => "application/json" ],
111+
'{}',
112+
),
113+
},
114+
115+
{
116+
type => "rule",
117+
comment => "json parser - issue #2807 - empty body retains existing behavior",
118+
conf => qq(
119+
SecRuleEngine On
120+
SecRequestBodyAccess On
121+
SecDebugLog $ENV{DEBUG_LOG}
122+
SecDebugLogLevel 9
123+
SecRule REQUEST_HEADERS:Content-Type "^application/json" \\
124+
"id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON"
125+
SecRule REQBODY_ERROR "!\@eq 0" \\
126+
"id:200002,phase:2,t:none,log,deny,status:400,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'"
127+
),
128+
match_log => {
129+
-error => [ qr/\[id "200002"\]/, 1 ],
130+
},
131+
match_response => {
132+
status => qr/^200$/,
133+
},
134+
request => new HTTP::Request(
135+
POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt",
136+
[ "Content-Type" => "application/json" ],
137+
'',
138+
),
139+
},
140+
141+
{
142+
type => "rule",
143+
comment => "json parser - issue #2807 - in-stream syntax errors still use the error rule",
144+
conf => qq(
145+
SecRuleEngine On
146+
SecRequestBodyAccess On
147+
SecDebugLog $ENV{DEBUG_LOG}
148+
SecDebugLogLevel 9
149+
SecRule REQUEST_HEADERS:Content-Type "^application/json" \\
150+
"id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON"
151+
SecRule REQBODY_ERROR "!\@eq 0" \\
152+
"id:200002,phase:2,t:none,log,deny,status:400,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'"
153+
),
154+
match_log => {
155+
error => [ qr/Access denied with code 400.*\[id "200002"\]/s, 1 ],
156+
},
157+
match_response => {
158+
status => qr/^400$/,
159+
},
160+
request => new HTTP::Request(
161+
POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt",
162+
[ "Content-Type" => "application/json" ],
163+
'{ id : "123" }',
164+
),
165+
},
166+
167+
{
168+
type => "rule",
169+
comment => "json parser - issue #2807 - valid JSON arguments are still inspected",
170+
conf => qq(
171+
SecRuleEngine On
172+
SecRequestBodyAccess On
173+
SecDebugLog $ENV{DEBUG_LOG}
174+
SecDebugLogLevel 9
175+
SecRule REQUEST_HEADERS:Content-Type "^application/json" \\
176+
"id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON"
177+
SecRule REQBODY_ERROR "!\@eq 0" \\
178+
"id:200002,phase:2,t:none,log,deny,status:400,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'"
179+
SecRule ARGS:foo "\@streq bar" "id:200003,phase:2,t:none,log,deny,status:403"
180+
),
181+
match_log => {
182+
error => [ qr/Access denied with code 403.*\[id "200003"\]/s, 1 ],
183+
},
184+
match_response => {
185+
status => qr/^403$/,
186+
},
187+
request => new HTTP::Request(
188+
POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt",
189+
[ "Content-Type" => "application/json" ],
190+
'{"foo":"bar"}',
191+
),
192+
},
193+
194+
{
195+
type => "rule",
196+
comment => "json parser - issue #2807 - chunked premature EOF is rejected before phase 2",
197+
conf => qq(
198+
SecRuleEngine On
199+
SecRequestBodyAccess On
200+
SecDebugLog $ENV{DEBUG_LOG}
201+
SecDebugLogLevel 9
202+
SecRule REQUEST_HEADERS:Content-Type "^application/json" \\
203+
"id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON"
204+
SecRule REQBODY_ERROR "!\@eq 0" \\
205+
"id:200002,phase:2,t:none,log,deny,status:400,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'"
206+
SecAction "id:200005,phase:2,pass,log,msg:'JSON phase 2 reached'"
207+
),
208+
match_log => {
209+
error => [ qr/JSON parser error:.*premature EOF/s, 1 ],
210+
-error => [ qr/\[id "20000[25]"\]/, 1 ],
211+
},
212+
match_response => {
213+
status => qr/^400$/,
214+
},
215+
request => normalize_raw_request_data(
216+
qq(
217+
POST /test.txt HTTP/1.1
218+
Host: $ENV{SERVER_NAME}:$ENV{SERVER_PORT}
219+
User-Agent: $ENV{USER_AGENT}
220+
Content-Type: application/json
221+
Transfer-Encoding: chunked
222+
223+
),
224+
) . encode_chunked('{ "id" : "123"', 3),
225+
},
226+
227+
{
228+
type => "rule",
229+
comment => "json parser - issue #2807 - no error rule still rejects incomplete JSON",
230+
conf => qq(
231+
SecRuleEngine On
232+
SecRequestBodyAccess On
233+
SecRule REQUEST_HEADERS:Content-Type "^application/json" \\
234+
"id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON"
235+
SecAction "id:200005,phase:2,pass,log,msg:'JSON phase 2 reached'"
236+
),
237+
match_log => {
238+
error => [ qr/JSON parser error:.*premature EOF/s, 1 ],
239+
-error => [ qr/\[id "20000[25]"\]/, 1 ],
240+
},
241+
match_response => {
242+
status => qr/^400$/,
243+
},
244+
request => new HTTP::Request(
245+
POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt",
246+
[ "Content-Type" => "application/json" ],
247+
'{',
248+
),
249+
},
250+
251+
{
252+
type => "rule",
253+
comment => "json parser - issue #2807 - excluded error rule still rejects incomplete JSON",
254+
conf => qq(
255+
SecRuleEngine On
256+
SecRequestBodyAccess On
257+
SecRule REQUEST_HEADERS:Content-Type "^application/json" \\
258+
"id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON"
259+
SecRule REQUEST_URI "^/test[.]txt\$" \\
260+
"id:200004,phase:1,pass,nolog,ctl:ruleRemoveById=200002"
261+
SecRule REQBODY_ERROR "!\@eq 0" \\
262+
"id:200002,phase:2,t:none,log,deny,status:400,msg:'JSON body error'"
263+
SecAction "id:200005,phase:2,pass,log,msg:'JSON phase 2 reached'"
264+
),
265+
match_log => {
266+
error => [ qr/JSON parser error:.*premature EOF/s, 1 ],
267+
-error => [ qr/\[id "20000[25]"\]/, 1 ],
268+
},
269+
match_response => {
270+
status => qr/^400$/,
271+
},
272+
request => new HTTP::Request(
273+
POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt",
274+
[ "Content-Type" => "application/json" ],
275+
'{',
276+
),
277+
},
278+
279+
{
280+
type => "rule",
281+
comment => "json parser - issue #2807 - disabled engine retains existing behavior",
282+
conf => qq(
283+
SecRuleEngine Off
284+
SecRequestBodyAccess On
285+
SecDebugLog $ENV{DEBUG_LOG}
286+
SecDebugLogLevel 9
287+
SecRule REQUEST_HEADERS:Content-Type "^application/json" \\
288+
"id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON"
289+
SecRule REQBODY_ERROR "!\@eq 0" \\
290+
"id:200002,phase:2,t:none,log,deny,status:400,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'"
291+
SecAction "id:200005,phase:2,pass,log,msg:'JSON phase 2 reached'"
292+
),
293+
match_log => {
294+
-error => [ qr/JSON parser error:/, 1 ],
295+
},
296+
match_response => {
297+
status => qr/^200$/,
298+
},
299+
request => new HTTP::Request(
300+
POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt",
301+
[ "Content-Type" => "application/json" ],
302+
'{',
303+
),
304+
},
305+
306+
{
307+
type => "rule",
308+
comment => "json parser - issue #2807 - disabled request body access retains existing behavior",
309+
conf => qq(
310+
SecRuleEngine On
311+
SecRequestBodyAccess Off
312+
SecDebugLog $ENV{DEBUG_LOG}
313+
SecDebugLogLevel 9
314+
SecRule REQUEST_HEADERS:Content-Type "^application/json" \\
315+
"id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON"
316+
SecRule REQBODY_ERROR "!\@eq 0" \\
317+
"id:200002,phase:2,t:none,log,deny,status:400,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'"
318+
SecAction "id:200005,phase:2,pass,log,msg:'JSON phase 2 reached'"
319+
),
320+
match_log => {
321+
-error => [ qr/JSON parser error:/, 1 ],
322+
},
323+
match_response => {
324+
status => qr/^200$/,
325+
},
326+
request => new HTTP::Request(
327+
POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt",
328+
[ "Content-Type" => "application/json" ],
329+
'{',
330+
),
331+
},
332+
333+
{
334+
type => "rule",
335+
comment => "json parser - issue #2807 - in-stream syntax errors retain custom error status",
336+
conf => qq(
337+
SecRuleEngine On
338+
SecRequestBodyAccess On
339+
SecDebugLog $ENV{DEBUG_LOG}
340+
SecDebugLogLevel 9
341+
SecRule REQUEST_HEADERS:Content-Type "^application/json" \\
342+
"id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON"
343+
SecRule REQBODY_ERROR "!\@eq 0" \\
344+
"id:200002,phase:2,t:none,log,deny,status:422,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'"
345+
),
346+
match_log => {
347+
error => [ qr/Access denied with code 422.*\[id "200002"\]/s, 1 ],
348+
},
349+
match_response => {
350+
status => qr/^422$/,
351+
},
352+
request => new HTTP::Request(
353+
POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt",
354+
[ "Content-Type" => "application/json" ],
355+
'{ id : "123" }',
356+
),
357+
},

0 commit comments

Comments
 (0)