ylavic
commented
3 years ago I can send a PR, but I am not the author
Please do if needed ;)
Open pgajdos opened 3 years ago
ylavic
commented
3 years ago I can send a PR, but I am not the author
Please do if needed ;)
marcstern
commented
3 months ago Let's come back on this problem. Questions about the patch:
ylavic
commented
3 months ago I don't know the mod_security code well so please take my below answers with a grain of salt..
1. Shouldn't we have exactly the same treatment for all errors (except the message and retrun code of course)?Why don't we always set r->connection->keepalive to AP_CONN_CLOSE (for instance for -1)?
I think any failure the read the request body entirely should indeed cause the connection to be closed. Some return values already imply that in httpd though (see https://github.com/apache/httpd/blob/trunk/include/httpd.h#L572)
2. Is the logic around (msr->txcfg->is_enabled == MODSEC_ENABLED) correct? Shouldn't we: * skip this processing if (msr->txcfg->is_enabled == MODSEC_DISABLED)
I think this is already the case for MODSEC_DISABLED here: https://github.com/owasp-modsecurity/ModSecurity/blob/v2/master/apache2/mod_security2.c#L984 ?
It's been 3 years now but I think the patch removes the test && msr->txcfg->is_enabled == MODSEC_ENABLED because of MODSEC_DETECTION_ONLY. With MODSEC_DETECTION_ONLY I don't think mod_sec should ignore the error to read the body either and fall through https://github.com/owasp-modsecurity/ModSecurity/blob/v2/master/apache2/mod_security2.c#L1099
* perform all checks * log message
Not sure what you mean for those two.
* set r->connection->keepalive = AP_CONN_CLOSE & return an error if (msr->txcfg->is_enabled == MODSEC_ENABLED)
Same response as above?
3. If it's a security issue, we log at level 1; if it's an error, we log at level 4. Is that the best option? An error is an invalid request, right? So, shouldn't it be logged at level 1 as well? At least by default, and use level 4 only for specific non-attack cases (incomplete request)?
No opinion here sorry.
ylavic
commented
3 months ago Also note that httpd provides the ap_map_http_request_error() helper (https://github.com/apache/httpd/blob/trunk/modules/http/http_filters.c#L1400) to map an input filter error to an HTTP status code, though the hook_request_late()::read_request_body()::ap_get_brigade() => -1/-2/... error danse does not really help here.
marcstern
commented
3 months ago ap_map_http_request_error() seems the way to go, indeed.
So, in apache2_io.c, we would return ap_map_http_request_error(rc) and use a unique code for all cases in mod_security2.c. Correct?
ylavic
commented
3 months ago I would say something like this (not even compile tested, just to get the idea..):
diff --git a/apache2/apache2.h b/apache2/apache2.h
index 87a17ed2..40586a1e 100644
--- a/apache2/apache2.h
+++ b/apache2/apache2.h
@@ -67,7 +67,7 @@ apr_status_t DSOLOCAL input_filter(ap_filter_t *f, apr_bucket_brigade *bb_out,
apr_status_t DSOLOCAL output_filter(ap_filter_t *f, apr_bucket_brigade *bb_in);
-apr_status_t DSOLOCAL read_request_body(modsec_rec *msr, char **error_msg);
+int DSOLOCAL read_request_body(modsec_rec *msr, char **error_msg);
/* Utility functions */
diff --git a/apache2/apache2_io.c b/apache2/apache2_io.c
index 5d2ef85b..b9cc8113 100644
--- a/apache2/apache2_io.c
+++ b/apache2/apache2_io.c
@@ -178,7 +178,7 @@ apr_status_t input_filter(ap_filter_t *f, apr_bucket_brigade *bb_out,
/**
* Reads request body from a client.
*/
-apr_status_t read_request_body(modsec_rec *msr, char **error_msg) {
+int read_request_body(modsec_rec *msr, char **error_msg) {
assert(msr != NULL);
assert(error_msg!= NULL);
request_rec *r = msr->r;
@@ -192,55 +192,45 @@ apr_status_t read_request_body(modsec_rec *msr, char **error_msg) {
if (msr->txcfg->debuglog_level >= 4) {
msr_log(msr, 4, "Input filter: This request does not have a body.");
}
- return 0;
+ return OK;
}
if (msr->txcfg->reqbody_access != 1) {
if (msr->txcfg->debuglog_level >= 4) {
msr_log(msr, 4, "Input filter: Request body access not enabled.");
}
- return 0;
+ return OK;
}
if (msr->txcfg->debuglog_level >= 4) {
msr_log(msr, 4, "Input filter: Reading request body.");
}
if (modsecurity_request_body_start(msr, error_msg) < 0) {
- return -1;
+ return HTTP_INTERNAL_SERVER_ERROR;
}
finished_reading = 0;
msr->if_seen_eos = 0;
bb_in = apr_brigade_create(msr->mp, r->connection->bucket_alloc);
- if (bb_in == NULL) return -1;
+ if (bb_in == NULL)
+ return HTTP_INTERNAL_SERVER_ERROR;
do {
apr_status_t rc;
rc = ap_get_brigade(r->input_filters, bb_in, AP_MODE_READBYTES, APR_BLOCK_READ, HUGE_STRING_LEN);
if (rc != APR_SUCCESS) {
- /* NOTE Apache returns AP_FILTER_ERROR here when the request is
- * too large and APR_EGENERAL when the client disconnects.
- */
switch(rc) {
- case APR_INCOMPLETE :
- *error_msg = apr_psprintf(msr->mp, "Error reading request body: %s", get_apr_error(msr->mp, rc));
- return -7;
- case APR_EOF :
- *error_msg = apr_psprintf(msr->mp, "Error reading request body: %s", get_apr_error(msr->mp, rc));
- return -6;
- case APR_TIMEUP :
- *error_msg = apr_psprintf(msr->mp, "Error reading request body: %s", get_apr_error(msr->mp, rc));
- return -4;
case AP_FILTER_ERROR :
*error_msg = apr_psprintf(msr->mp, "Error reading request body: HTTP Error 413 - Request entity too large. (Most likely.)");
- return -3;
+ break;
case APR_EGENERAL :
*error_msg = apr_psprintf(msr->mp, "Error reading request body: Client went away.");
- return -2;
+ break;
default :
*error_msg = apr_psprintf(msr->mp, "Error reading request body: %s", get_apr_error(msr->mp, rc));
- return -1;
+ break;
}
+ return ap_map_http_request_error(rc, HTTP_BAD_REQUEST);
}
/* Loop through the buckets in the brigade in order
@@ -256,7 +246,7 @@ apr_status_t read_request_body(modsec_rec *msr, char **error_msg) {
rc = apr_bucket_read(bucket, &buf, &buflen, APR_BLOCK_READ);
if (rc != APR_SUCCESS) {
*error_msg = apr_psprintf(msr->mp, "Failed reading input / bucket (%d): %s", rc, get_apr_error(msr->mp, rc));
- return -1;
+ return HTTP_INTERNAL_SERVER_ERROR;
}
if (msr->txcfg->debuglog_level >= 9) {
@@ -269,7 +259,7 @@ apr_status_t read_request_body(modsec_rec *msr, char **error_msg) {
if((msr->txcfg->is_enabled == MODSEC_ENABLED) && (msr->txcfg->if_limit_action == REQUEST_BODY_LIMIT_ACTION_REJECT)) {
*error_msg = apr_psprintf(msr->mp, "Request body is larger than the "
"configured limit (%ld).", msr->txcfg->reqbody_limit);
- return -5;
+ return HTTP_REQUEST_ENTITY_TOO_LARGE;
} else if((msr->txcfg->is_enabled == MODSEC_ENABLED) && (msr->txcfg->if_limit_action == REQUEST_BODY_LIMIT_ACTION_PARTIAL)) {
*error_msg = apr_psprintf(msr->mp, "Request body is larger than the "
@@ -290,7 +280,7 @@ apr_status_t read_request_body(modsec_rec *msr, char **error_msg) {
*error_msg = apr_psprintf(msr->mp, "Request body is larger than the "
"configured limit (%ld).", msr->txcfg->reqbody_limit);
- return -5;
+ return HTTP_REQUEST_ENTITY_TOO_LARGE;
}
}
@@ -300,7 +290,7 @@ apr_status_t read_request_body(modsec_rec *msr, char **error_msg) {
modsecurity_request_body_to_stream(msr, buf, buflen, error_msg);
#else
if (modsecurity_request_body_to_stream(msr, buf, buflen, error_msg) < 0) {
- return -1;
+ return HTTP_INTERNAL_SERVER_ERROR;
}
#endif
}
@@ -319,7 +309,7 @@ apr_status_t read_request_body(modsec_rec *msr, char **error_msg) {
if((msr->txcfg->is_enabled == MODSEC_ENABLED) && (msr->txcfg->if_limit_action == REQUEST_BODY_LIMIT_ACTION_REJECT)) {
*error_msg = apr_psprintf(msr->mp, "Request body no files data length is larger than the "
"configured limit (%ld).", msr->txcfg->reqbody_no_files_limit);
- return -5;
+ return HTTP_REQUEST_ENTITY_TOO_LARGE;
} else if ((msr->txcfg->is_enabled == MODSEC_ENABLED) && (msr->txcfg->if_limit_action == REQUEST_BODY_LIMIT_ACTION_PARTIAL)) {
*error_msg = apr_psprintf(msr->mp, "Request body no files data length is larger than the "
"configured limit (%ld).", msr->txcfg->reqbody_no_files_limit);
@@ -329,12 +319,12 @@ apr_status_t read_request_body(modsec_rec *msr, char **error_msg) {
} else {
*error_msg = apr_psprintf(msr->mp, "Request body no files data length is larger than the "
"configured limit (%ld).", msr->txcfg->reqbody_no_files_limit);
- return -5;
+ return HTTP_REQUEST_ENTITY_TOO_LARGE;
}
}
if((msr->txcfg->is_enabled == MODSEC_ENABLED) && (msr->txcfg->if_limit_action == REQUEST_BODY_LIMIT_ACTION_REJECT))
- return -1;
+ return HTTP_INTERNAL_SERVER_ERROR;
}
}
@@ -357,7 +347,13 @@ apr_status_t read_request_body(modsec_rec *msr, char **error_msg) {
msr->if_status = IF_STATUS_WANTS_TO_RUN;
- return rcbe;
+ if (rcbe == -5) {
+ return HTTP_REQUEST_ENTITY_TOO_LARGE;
+ }
+ if (rcbe < 0) {
+ return HTTP_INTERNAL_SERVER_ERROR;
+ }
+ return OK;
}
diff --git a/apache2/mod_security2.c b/apache2/mod_security2.c
index 0ee72865..c8a3a669 100644
--- a/apache2/mod_security2.c
+++ b/apache2/mod_security2.c
@@ -1032,56 +1032,15 @@ static int hook_request_late(request_rec *r) {
}
rc = read_request_body(msr, &my_error_msg);
- if (rc < 0 && msr->txcfg->is_enabled == MODSEC_ENABLED) {
- switch(rc) {
- case -1 :
- if (my_error_msg != NULL) {
- msr_log(msr, 1, "%s", my_error_msg);
- }
- return HTTP_INTERNAL_SERVER_ERROR;
- break;
- case -4 : /* Timeout. */
- if (my_error_msg != NULL) {
- msr_log(msr, 4, "%s", my_error_msg);
- }
- r->connection->keepalive = AP_CONN_CLOSE;
- return HTTP_REQUEST_TIME_OUT;
- break;
- case -5 : /* Request body limit reached. */
- msr->inbound_error = 1;
- if((msr->txcfg->is_enabled == MODSEC_ENABLED) && (msr->txcfg->if_limit_action == REQUEST_BODY_LIMIT_ACTION_REJECT)) {
- r->connection->keepalive = AP_CONN_CLOSE;
- if (my_error_msg != NULL) {
- msr_log(msr, 1, "%s. Deny with code (%d)", my_error_msg, HTTP_REQUEST_ENTITY_TOO_LARGE);
- }
- return HTTP_REQUEST_ENTITY_TOO_LARGE;
- } else {
- if (my_error_msg != NULL) {
- msr_log(msr, 1, "%s", my_error_msg);
- }
- }
- break;
- case -6 : /* EOF when reading request body. */
- if (my_error_msg != NULL) {
- msr_log(msr, 4, "%s", my_error_msg);
- }
- r->connection->keepalive = AP_CONN_CLOSE;
- return HTTP_BAD_REQUEST;
- break;
- case -7 : /* Partial recieved */
- if (my_error_msg != NULL) {
- msr_log(msr, 4, "%s", my_error_msg);
- }
- r->connection->keepalive = AP_CONN_CLOSE;
- return HTTP_BAD_REQUEST;
- break;
- default :
- /* allow through */
- break;
+ if (rc != OK) {
+ if (my_error_msg != NULL) {
+ msr_log(msr, 1, "%s", my_error_msg);
}
-
- msr->msc_reqbody_error = 1;
- msr->msc_reqbody_error_msg = my_error_msg;
+ if (rc == HTTP_REQUEST_ENTITY_TOO_LARGE) {
+ msr->inbound_error = 1;
+ }
+ r->connection->keepalive = AP_CONN_CLOSE;
+ return rc;
}
/* Update the request headers. They might have changed afterLooks much cleaner. We should test this against all 8 identified errors (at least) and check the behavior, before and after the change.
orangetw
commented
2 months ago Hi, the maintainer asked me to include current tests here that might cause a double response to help the fix.
GET /test.php HTTP/1.1
Host: server
Content-Length: x
2. [http_filters.c#402](https://github.com/apache/httpd/blob/2.4.58/modules/http/http_filters.c#L402) - triggering by malformed chunked size, which is the issue owner encountered.GET /test.php HTTP/1.1 Host: server Transfer-Encoding: chunked
-2 AA 0
3. [http_filters.c#429](https://github.com/apache/httpd/blob/2.4.58/modules/http/http_filters.c#L429) - triggering by sending large POST data (default value is 1073741824, you can lower the size via `LimitRequestBody 1024` to reproduce the behavior)POST /test.php HTTP/1.1 Host: server Content-Length: 1073741825
AAAAA...[1073741825]...AAA
Thanks a lot, we'll try to integrate this
Bug Description
After specific wrong request I get second response from Apache httpd server:
Where
is content of index.html in docroot.
Logs and dumps
Not necessary.
To Reproduce
Minimal httpd config:
Expected behavior
Server (please complete the following information):
OS (and distro): openSUSE Tumbleweed
Tested also with Ubuntu 20.04/http 2.4.41/mod_security 2.9.3 and fedora33/httpd 2.4.46/mod_security 2.9.3.
Patch
Yann Ylavic (@ylavic) wrote following patch:
I can send a PR, but I am not the author.