Change handling of invalid request body

creepyghost · creepyghost · commit 980561c40a77 · 2022-01-25T16:40:21.000+05:30
### modsecurity.h
* Standardize body parser return codes

### msc_json.c, msc_xml.c
* Return special error code on body parsing failure

### msc_reqbody.c, apache2_io.c, mod_security2.c
* Change body parsing error codes to refer to constants
  defined in header.
diff --git a/apache2/apache2_io.c b/apache2/apache2_io.c
@@ -193,7 +193,7 @@ apr_status_t read_request_body(modsec_rec *msr, char **error_msg) {
         msr_log(msr, 4, "Input filter: Reading request body.");
     }
     if (modsecurity_request_body_start(msr, error_msg) < 0) {
-        return -1;
+        return BODY_PARSER_ERR_GENERIC;
     }
 
     finished_reading = 0;
@@ -226,7 +226,7 @@ apr_status_t read_request_body(modsec_rec *msr, char **error_msg) {
                     return -2;
                 default :
                     *error_msg = apr_psprintf(msr->mp, "Error reading request body: %s", get_apr_error(msr->mp, rc));
-                    return -1;
+                    return BODY_PARSER_ERR_GENERIC;
             }
         }
 
@@ -321,7 +321,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))
-                        return -1;
+                        return BODY_PARSER_ERR_GENERIC;
                 }
 
             }
diff --git a/apache2/mod_security2.c b/apache2/mod_security2.c
@@ -1071,19 +1071,14 @@ static int hook_request_late(request_rec *r) {
                 }
                 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;
+            case BODY_PARSER_ERR_INVALID_BODY : /* Error Parsing Body. To be handled by modsec rules */
             default :
                 /* allow through */
                 break;
diff --git a/apache2/modsecurity.h b/apache2/modsecurity.h
@@ -273,6 +273,11 @@ extern DSOLOCAL char *msc_waf_lock_group;
 
 #define NBSP                            160
 
+
+#define BODY_PARSER_ERR_GENERIC         -1
+#define BODY_PARSER_ERR_INVALID_BODY    -8
+#define BODY_PARSER_OK_SUCCESS          1
+
 struct rule_exception {
     int                  type;
     const char          *param;
diff --git a/apache2/msc_json.c b/apache2/msc_json.c
@@ -292,7 +292,7 @@ int json_process_chunk(modsec_rec *msr, const char *buf, unsigned int size, char
 int json_complete(modsec_rec *msr, char **error_msg) {
     char *json_data = (char *) NULL;
 
-    if (error_msg == NULL) return -1;
+    if (error_msg == NULL) return BODY_PARSER_ERR_GENERIC;
     *error_msg = NULL;
 
     /* Wrap up the parsing process */
@@ -301,10 +301,10 @@ int json_complete(modsec_rec *msr, char **error_msg) {
         char *yajl_err = yajl_get_error(msr->json->handle, 0, NULL, 0);
         *error_msg = apr_pstrdup(msr->mp, yajl_err);
         yajl_free_error(msr->json->handle, yajl_err);
-        return -1;
+        return BODY_PARSER_ERR_INVALID_BODY;
     }
 
-    return 1;
+    return BODY_PARSER_OK_SUCCESS;
 }
 
 /**
diff --git a/apache2/msc_reqbody.c b/apache2/msc_reqbody.c
@@ -697,45 +697,47 @@ apr_status_t modsecurity_request_body_end(modsec_rec *msr, char **error_msg) {
                 if (msr->txcfg->debuglog_level >= 4) {
                     msr_log(msr, 4, "%s", *error_msg);
                 }
-                return -1;
+                return BODY_PARSER_ERR_INVALID_BODY;
             }
 
             if (multipart_get_arguments(msr, "BODY", msr->arguments) < 0) {
                 *error_msg = "Multipart parsing error: Failed to retrieve arguments.";
                 msr->msc_reqbody_error = 1;
                 msr->msc_reqbody_error_msg = *error_msg;
                 msr_log(msr, 2, "%s", *error_msg);
-                return -1;
+                return BODY_PARSER_ERR_INVALID_BODY;
             }
         }
         else if (strcmp(msr->msc_reqbody_processor, "JSON") == 0) {
 #ifdef WITH_YAJL
-            if (json_complete(msr, &my_error_msg) < 0 && msr->msc_reqbody_length > 0) {
+            int json_parser_response = json_complete(msr, &my_error_msg);
+            if (json_parser_response < 0 && msr->msc_reqbody_length > 0) {
                 *error_msg = apr_psprintf(msr->mp, "JSON parser error: %s", my_error_msg);
                 msr->msc_reqbody_error = 1;
                 msr->msc_reqbody_error_msg = *error_msg;
                 msr_log(msr, 2, "%s", *error_msg);
-                 return -1;
+                 return json_parser_response;
              }
 #else
             *error_msg = apr_psprintf(msr->mp, "JSON support was not enabled");
             msr->msc_reqbody_error = 1;
             msr->msc_reqbody_error_msg = *error_msg;
             msr_log(msr, 2, "%s", *error_msg);
-            return -1;
+            return BODY_PARSER_ERR_GENERIC;
 #endif
 
         }
         else if (strcmp(msr->msc_reqbody_processor, "URLENCODED") == 0) {
             return modsecurity_request_body_end_urlencoded(msr, error_msg);
         }
         else if (strcmp(msr->msc_reqbody_processor, "XML") == 0) {
-            if (xml_complete(msr, &my_error_msg) < 0) {
+            int xml_parser_response = xml_complete(msr, &my_error_msg);
+            if (xml_parser_response < 0) {
                 *error_msg = apr_psprintf(msr->mp, "XML parser error: %s", my_error_msg);
                 msr->msc_reqbody_error = 1;
                 msr->msc_reqbody_error_msg = *error_msg;
                 msr_log(msr, 2, "%s", *error_msg);
-                return -1;
+                return xml_parser_response;
             }
         }
     } else if (msr->txcfg->reqbody_buffering != REQUEST_BODY_FORCEBUF_OFF) {
@@ -746,7 +748,7 @@ apr_status_t modsecurity_request_body_end(modsec_rec *msr, char **error_msg) {
     /* Note the request body no files length. */
     msr_log(msr, 4, "Request body no files length: %" APR_SIZE_T_FMT, msr->msc_reqbody_no_files_length);
 
-    return 1;
+    return BODY_PARSER_OK_SUCCESS;
 }
 
 /**
diff --git a/apache2/msc_xml.c b/apache2/msc_xml.c
@@ -36,7 +36,7 @@ int xml_init(modsec_rec *msr, char **error_msg) {
         entity = xmlParserInputBufferCreateFilenameDefault(xml_unload_external_entity);
     }
 
-    return 1;
+    return BODY_PARSER_OK_SUCCESS;
 }
 
 #if 0
@@ -59,7 +59,7 @@ static void xml_receive_sax_error(void *data, const char *msg, ...) {
  * Feed one chunk of data to the XML parser.
  */
 int xml_process_chunk(modsec_rec *msr, const char *buf, unsigned int size, char **error_msg) {
-    if (error_msg == NULL) return -1;
+    if (error_msg == NULL) return BODY_PARSER_ERR_GENERIC;
     *error_msg = NULL;
 
     /* We want to initialise our parsing context here, to
@@ -87,7 +87,7 @@ int xml_process_chunk(modsec_rec *msr, const char *buf, unsigned int size, char
         msr->xml->parsing_ctx = xmlCreatePushParserCtxt(NULL, NULL, buf, size, "body.xml");
         if (msr->xml->parsing_ctx == NULL) {
             *error_msg = apr_psprintf(msr->mp, "XML: Failed to create parsing context.");
-            return -1;
+            return BODY_PARSER_ERR_GENERIC;
         }
     } else {
 
@@ -96,18 +96,18 @@ int xml_process_chunk(modsec_rec *msr, const char *buf, unsigned int size, char
         xmlParseChunk(msr->xml->parsing_ctx, buf, size, 0);
         if (msr->xml->parsing_ctx->wellFormed != 1) {
             *error_msg = apr_psprintf(msr->mp, "XML: Failed parsing document.");
-            return -1;
+            return BODY_PARSER_ERR_INVALID_BODY;
         }
     }
 
-    return 1;
+    return BODY_PARSER_OK_SUCCESS;
 }
 
 /**
  * Finalise XML parsing.
  */
 int xml_complete(modsec_rec *msr, char **error_msg) {
-    if (error_msg == NULL) return -1;
+    if (error_msg == NULL) return BODY_PARSER_ERR_GENERIC;
     *error_msg = NULL;
 
     /* Only if we have a context, meaning we've done some work. */
@@ -126,11 +126,11 @@ int xml_complete(modsec_rec *msr, char **error_msg) {
 
         if (msr->xml->well_formed != 1) {
             *error_msg = apr_psprintf(msr->mp, "XML: Failed parsing document.");
-            return -1;
+            return BODY_PARSER_ERR_INVALID_BODY;
         }
     }
 
-    return 1;
+    return BODY_PARSER_OK_SUCCESS;
 }
 
 /**
@@ -149,5 +149,5 @@ apr_status_t xml_cleanup(modsec_rec *msr) {
         msr->xml->doc = NULL;
     }
 
-    return 1;
+    return BODY_PARSER_OK_SUCCESS;
 }

Original file line number	Diff line number	Diff line change
`@@ -193,7 +193,7 @@ apr_status_t read_request_body(modsec_rec msr, char *error_msg) {`
`193`	`193`	`msr_log(msr, 4, "Input filter: Reading request body.");`
`194`	`194`	`}`
`195`	`195`	`if (modsecurity_request_body_start(msr, error_msg) < 0) {`
`196`		`- return -1;`
	`196`	`+ return BODY_PARSER_ERR_GENERIC;`
`197`	`197`	`}`
`198`	`198`
`199`	`199`	`finished_reading = 0;`
`@@ -226,7 +226,7 @@ apr_status_t read_request_body(modsec_rec msr, char *error_msg) {`
`226`	`226`	`return -2;`
`227`	`227`	`default :`
`228`	`228`	`*error_msg = apr_psprintf(msr->mp, "Error reading request body: %s", get_apr_error(msr->mp, rc));`
`229`		`- return -1;`
	`229`	`+ return BODY_PARSER_ERR_GENERIC;`
`230`	`230`	`}`
`231`	`231`	`}`
`232`	`232`
`@@ -321,7 +321,7 @@ apr_status_t read_request_body(modsec_rec msr, char *error_msg) {`
`321`	`321`	`}`
`322`	`322`
`323`	`323`	`if((msr->txcfg->is_enabled == MODSEC_ENABLED) && (msr->txcfg->if_limit_action == REQUEST_BODY_LIMIT_ACTION_REJECT))`
`324`		`- return -1;`
	`324`	`+ return BODY_PARSER_ERR_GENERIC;`
`325`	`325`	`}`
`326`	`326`
`327`	`327`	`}`
Original file line number	Diff line number	Diff line change
`@@ -36,7 +36,7 @@ int xml_init(modsec_rec msr, char *error_msg) {`
`36`	`36`	`entity = xmlParserInputBufferCreateFilenameDefault(xml_unload_external_entity);`
`37`	`37`	`}`
`38`	`38`
`39`		`- return 1;`
	`39`	`+ return BODY_PARSER_OK_SUCCESS;`
`40`	`40`	`}`
`41`	`41`
`42`	`42`	`#if 0`
`@@ -59,7 +59,7 @@ static void xml_receive_sax_error(void data, const char msg, ...) {`
`59`	`59`	`* Feed one chunk of data to the XML parser.`
`60`	`60`	`*/`
`61`	`61`	`int xml_process_chunk(modsec_rec msr, const char buf, unsigned int size, char **error_msg) {`
`62`		`- if (error_msg == NULL) return -1;`
	`62`	`+ if (error_msg == NULL) return BODY_PARSER_ERR_GENERIC;`
`63`	`63`	`*error_msg = NULL;`
`64`	`64`
`65`	`65`	`/* We want to initialise our parsing context here, to`
`@@ -87,7 +87,7 @@ int xml_process_chunk(modsec_rec msr, const char buf, unsigned int size, char`
`87`	`87`	`msr->xml->parsing_ctx = xmlCreatePushParserCtxt(NULL, NULL, buf, size, "body.xml");`
`88`	`88`	`if (msr->xml->parsing_ctx == NULL) {`
`89`	`89`	`*error_msg = apr_psprintf(msr->mp, "XML: Failed to create parsing context.");`
`90`		`- return -1;`
	`90`	`+ return BODY_PARSER_ERR_GENERIC;`
`91`	`91`	`}`
`92`	`92`	`} else {`
`93`	`93`
`@@ -96,18 +96,18 @@ int xml_process_chunk(modsec_rec msr, const char buf, unsigned int size, char`
`96`	`96`	`xmlParseChunk(msr->xml->parsing_ctx, buf, size, 0);`
`97`	`97`	`if (msr->xml->parsing_ctx->wellFormed != 1) {`
`98`	`98`	`*error_msg = apr_psprintf(msr->mp, "XML: Failed parsing document.");`
`99`		`- return -1;`
	`99`	`+ return BODY_PARSER_ERR_INVALID_BODY;`
`100`	`100`	`}`
`101`	`101`	`}`
`102`	`102`
`103`		`- return 1;`
	`103`	`+ return BODY_PARSER_OK_SUCCESS;`
`104`	`104`	`}`
`105`	`105`
`106`	`106`	`/**`
`107`	`107`	`* Finalise XML parsing.`
`108`	`108`	`*/`
`109`	`109`	`int xml_complete(modsec_rec msr, char *error_msg) {`
`110`		`- if (error_msg == NULL) return -1;`
	`110`	`+ if (error_msg == NULL) return BODY_PARSER_ERR_GENERIC;`
`111`	`111`	`*error_msg = NULL;`
`112`	`112`
`113`	`113`	`/* Only if we have a context, meaning we've done some work. */`
`@@ -126,11 +126,11 @@ int xml_complete(modsec_rec msr, char *error_msg) {`
`126`	`126`
`127`	`127`	`if (msr->xml->well_formed != 1) {`
`128`	`128`	`*error_msg = apr_psprintf(msr->mp, "XML: Failed parsing document.");`
`129`		`- return -1;`
	`129`	`+ return BODY_PARSER_ERR_INVALID_BODY;`
`130`	`130`	`}`
`131`	`131`	`}`
`132`	`132`
`133`		`- return 1;`
	`133`	`+ return BODY_PARSER_OK_SUCCESS;`
`134`	`134`	`}`
`135`	`135`
`136`	`136`	`/**`
`@@ -149,5 +149,5 @@ apr_status_t xml_cleanup(modsec_rec *msr) {`
`149`	`149`	`msr->xml->doc = NULL;`
`150`	`150`	`}`
`151`	`151`
`152`		`- return 1;`
	`152`	`+ return BODY_PARSER_OK_SUCCESS;`
`153`	`153`	`}`