Skip to content

Commit 6d7e8eb

Browse files
airweenairween
authored
Merge commit from fork
fix: prevent two responses in case of invalid request
2 parents e1ea8e5 + dfbde55 commit 6d7e8eb

File tree

2 files changed

+32
-73
lines changed

2 files changed

+32
-73
lines changed

‎apache2/apache2_io.c

Lines changed: 24 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -192,27 +192,29 @@ apr_status_t read_request_body(modsec_rec *msr, char **error_msg) {
192192
if (msr->txcfg->debuglog_level >= 4) {
193193
msr_log(msr, 4, "Input filter: This request does not have a body.");
194194
}
195-
return 0;
195+
return APR_SUCCESS;
196196
}
197197

198198
if (msr->txcfg->reqbody_access != 1) {
199199
if (msr->txcfg->debuglog_level >= 4) {
200200
msr_log(msr, 4, "Input filter: Request body access not enabled.");
201201
}
202-
return 0;
202+
return APR_SUCCESS;
203203
}
204204

205205
if (msr->txcfg->debuglog_level >= 4) {
206206
msr_log(msr, 4, "Input filter: Reading request body.");
207207
}
208208
if (modsecurity_request_body_start(msr, error_msg) < 0) {
209-
return -1;
209+
return HTTP_INTERNAL_SERVER_ERROR;
210210
}
211211

212212
finished_reading = 0;
213213
msr->if_seen_eos = 0;
214214
bb_in = apr_brigade_create(msr->mp, r->connection->bucket_alloc);
215-
if (bb_in == NULL) return -1;
215+
if (bb_in == NULL) {
216+
return HTTP_INTERNAL_SERVER_ERROR;
217+
}
216218
do {
217219
apr_status_t rc;
218220

@@ -222,25 +224,17 @@ apr_status_t read_request_body(modsec_rec *msr, char **error_msg) {
222224
* too large and APR_EGENERAL when the client disconnects.
223225
*/
224226
switch(rc) {
225-
case APR_INCOMPLETE :
226-
*error_msg = apr_psprintf(msr->mp, "Error reading request body: %s", get_apr_error(msr->mp, rc));
227-
return -7;
228-
case APR_EOF :
229-
*error_msg = apr_psprintf(msr->mp, "Error reading request body: %s", get_apr_error(msr->mp, rc));
230-
return -6;
231-
case APR_TIMEUP :
232-
*error_msg = apr_psprintf(msr->mp, "Error reading request body: %s", get_apr_error(msr->mp, rc));
233-
return -4;
234227
case AP_FILTER_ERROR :
235228
*error_msg = apr_psprintf(msr->mp, "Error reading request body: HTTP Error 413 - Request entity too large. (Most likely.)");
236-
return -3;
229+
break;
237230
case APR_EGENERAL :
238231
*error_msg = apr_psprintf(msr->mp, "Error reading request body: Client went away.");
239-
return -2;
232+
break;
240233
default :
241234
*error_msg = apr_psprintf(msr->mp, "Error reading request body: %s", get_apr_error(msr->mp, rc));
242-
return -1;
235+
break;
243236
}
237+
return ap_map_http_request_error(rc, HTTP_BAD_REQUEST);
244238
}
245239

246240
/* Loop through the buckets in the brigade in order
@@ -256,7 +250,7 @@ apr_status_t read_request_body(modsec_rec *msr, char **error_msg) {
256250
rc = apr_bucket_read(bucket, &buf, &buflen, APR_BLOCK_READ);
257251
if (rc != APR_SUCCESS) {
258252
*error_msg = apr_psprintf(msr->mp, "Failed reading input / bucket (%d): %s", rc, get_apr_error(msr->mp, rc));
259-
return -1;
253+
return HTTP_INTERNAL_SERVER_ERROR;
260254
}
261255

262256
if (msr->txcfg->debuglog_level >= 9) {
@@ -269,7 +263,7 @@ apr_status_t read_request_body(modsec_rec *msr, char **error_msg) {
269263
if((msr->txcfg->is_enabled == MODSEC_ENABLED) && (msr->txcfg->if_limit_action == REQUEST_BODY_LIMIT_ACTION_REJECT)) {
270264
*error_msg = apr_psprintf(msr->mp, "Request body is larger than the "
271265
"configured limit (%ld).", msr->txcfg->reqbody_limit);
272-
return -5;
266+
return HTTP_REQUEST_ENTITY_TOO_LARGE;
273267
} else if((msr->txcfg->is_enabled == MODSEC_ENABLED) && (msr->txcfg->if_limit_action == REQUEST_BODY_LIMIT_ACTION_PARTIAL)) {
274268

275269
*error_msg = apr_psprintf(msr->mp, "Request body is larger than the "
@@ -290,7 +284,7 @@ apr_status_t read_request_body(modsec_rec *msr, char **error_msg) {
290284
*error_msg = apr_psprintf(msr->mp, "Request body is larger than the "
291285
"configured limit (%ld).", msr->txcfg->reqbody_limit);
292286

293-
return -5;
287+
return HTTP_REQUEST_ENTITY_TOO_LARGE;
294288
}
295289
}
296290

@@ -300,7 +294,7 @@ apr_status_t read_request_body(modsec_rec *msr, char **error_msg) {
300294
modsecurity_request_body_to_stream(msr, buf, buflen, error_msg);
301295
#else
302296
if (modsecurity_request_body_to_stream(msr, buf, buflen, error_msg) < 0) {
303-
return -1;
297+
return HTTP_INTERNAL_SERVER_ERROR;
304298
}
305299
#endif
306300
}
@@ -319,7 +313,7 @@ apr_status_t read_request_body(modsec_rec *msr, char **error_msg) {
319313
if((msr->txcfg->is_enabled == MODSEC_ENABLED) && (msr->txcfg->if_limit_action == REQUEST_BODY_LIMIT_ACTION_REJECT)) {
320314
*error_msg = apr_psprintf(msr->mp, "Request body no files data length is larger than the "
321315
"configured limit (%ld).", msr->txcfg->reqbody_no_files_limit);
322-
return -5;
316+
return HTTP_REQUEST_ENTITY_TOO_LARGE;
323317
} else if ((msr->txcfg->is_enabled == MODSEC_ENABLED) && (msr->txcfg->if_limit_action == REQUEST_BODY_LIMIT_ACTION_PARTIAL)) {
324318
*error_msg = apr_psprintf(msr->mp, "Request body no files data length is larger than the "
325319
"configured limit (%ld).", msr->txcfg->reqbody_no_files_limit);
@@ -329,12 +323,12 @@ apr_status_t read_request_body(modsec_rec *msr, char **error_msg) {
329323
} else {
330324
*error_msg = apr_psprintf(msr->mp, "Request body no files data length is larger than the "
331325
"configured limit (%ld).", msr->txcfg->reqbody_no_files_limit);
332-
return -5;
326+
return HTTP_REQUEST_ENTITY_TOO_LARGE;
333327
}
334328
}
335329

336330
if((msr->txcfg->is_enabled == MODSEC_ENABLED) && (msr->txcfg->if_limit_action == REQUEST_BODY_LIMIT_ACTION_REJECT))
337-
return -1;
331+
return HTTP_INTERNAL_SERVER_ERROR;
338332
}
339333

340334
}
@@ -357,7 +351,13 @@ apr_status_t read_request_body(modsec_rec *msr, char **error_msg) {
357351

358352
msr->if_status = IF_STATUS_WANTS_TO_RUN;
359353

360-
return rcbe;
354+
if (rcbe == -5) {
355+
return HTTP_REQUEST_ENTITY_TOO_LARGE;
356+
}
357+
if (rcbe < 0) {
358+
return HTTP_INTERNAL_SERVER_ERROR;
359+
}
360+
return APR_SUCCESS;
361361
}
362362

363363

‎apache2/mod_security2.c

Lines changed: 8 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -1032,56 +1032,15 @@ static int hook_request_late(request_rec *r) {
10321032
}
10331033

10341034
rc = read_request_body(msr, &my_error_msg);
1035-
if (rc < 0 && msr->txcfg->is_enabled == MODSEC_ENABLED) {
1036-
switch(rc) {
1037-
case -1 :
1038-
if (my_error_msg != NULL) {
1039-
msr_log(msr, 1, "%s", my_error_msg);
1040-
}
1041-
return HTTP_INTERNAL_SERVER_ERROR;
1042-
break;
1043-
case -4 : /* Timeout. */
1044-
if (my_error_msg != NULL) {
1045-
msr_log(msr, 4, "%s", my_error_msg);
1046-
}
1047-
r->connection->keepalive = AP_CONN_CLOSE;
1048-
return HTTP_REQUEST_TIME_OUT;
1049-
break;
1050-
case -5 : /* Request body limit reached. */
1051-
msr->inbound_error = 1;
1052-
if((msr->txcfg->is_enabled == MODSEC_ENABLED) && (msr->txcfg->if_limit_action == REQUEST_BODY_LIMIT_ACTION_REJECT)) {
1053-
r->connection->keepalive = AP_CONN_CLOSE;
1054-
if (my_error_msg != NULL) {
1055-
msr_log(msr, 1, "%s. Deny with code (%d)", my_error_msg, HTTP_REQUEST_ENTITY_TOO_LARGE);
1056-
}
1057-
return HTTP_REQUEST_ENTITY_TOO_LARGE;
1058-
} else {
1059-
if (my_error_msg != NULL) {
1060-
msr_log(msr, 1, "%s", my_error_msg);
1061-
}
1062-
}
1063-
break;
1064-
case -6 : /* EOF when reading request body. */
1065-
if (my_error_msg != NULL) {
1066-
msr_log(msr, 4, "%s", my_error_msg);
1067-
}
1068-
r->connection->keepalive = AP_CONN_CLOSE;
1069-
return HTTP_BAD_REQUEST;
1070-
break;
1071-
case -7 : /* Partial recieved */
1072-
if (my_error_msg != NULL) {
1073-
msr_log(msr, 4, "%s", my_error_msg);
1074-
}
1075-
r->connection->keepalive = AP_CONN_CLOSE;
1076-
return HTTP_BAD_REQUEST;
1077-
break;
1078-
default :
1079-
/* allow through */
1080-
break;
1035+
if (rc != OK) {
1036+
if (my_error_msg != NULL) {
1037+
msr_log(msr, 1, "%s", my_error_msg);
10811038
}
1082-
1083-
msr->msc_reqbody_error = 1;
1084-
msr->msc_reqbody_error_msg = my_error_msg;
1039+
if (rc == HTTP_REQUEST_ENTITY_TOO_LARGE) {
1040+
msr->inbound_error = 1;
1041+
}
1042+
r->connection->keepalive = AP_CONN_CLOSE;
1043+
return rc;
10851044
}
10861045

10871046
/* Update the request headers. They might have changed after

0 commit comments

Comments
 (0)