feat: improved XMLArgs processing

headers/modsecurity/rules_set_properties.h

@@ -225,8 +225,7 @@ class RulesSetProperties {
 
     /**
      *
-     * The ConfigBoolean enumerator consists in mapping the different
-     * states of the configuration boolean values.
+     * The ConfigBoolean enumerator the states for configuration boolean values.
      * The default value is PropertyNotSetConfigBoolean.
      */
     enum ConfigBoolean {
@@ -237,8 +236,8 @@ class RulesSetProperties {
 
     /**
      *
-     * The ConfigXMLParseXmlIntoArgs enumerator consists in mapping the
-     * different states of the configuration XMLParseXmlIntoArgs values.
+     * The ConfigXMLParseXmlIntoArgs enumerator defines the states for the configuration
+     * XMLParseXmlIntoArgs values.
      * The default value is PropertyNotSetConfigXMLParseXmlIntoArgs.
      */
     enum ConfigXMLParseXmlIntoArgs {

src/actions/ctl/parse_xml_into_args.cc

@@ -47,7 +47,7 @@ bool ParseXmlIntoArgs::init(std::string *error) {
 
 bool ParseXmlIntoArgs::evaluate(RuleWithActions *rule, Transaction *transaction) {
     std::stringstream a;
-    a << "Setting SecParseXMLIntoArgs to ";
+    a << "Setting SecParseXmlIntoArgs to ";
     a << modsecurity::RulesSetProperties::configXMLParseXmlIntoArgsString(m_secXMLParseXmlIntoArgs);
     a << " as requested by a ctl:parseXmlIntoArgs action";
 

src/parser/seclang-scanner.ll

@@ -90,7 +90,7 @@ ACTION_CTL_BDY_JSON                             (?i:ctl:requestBodyProcessor=JSO
 ACTION_CTL_BDY_XML                              (?i:ctl:requestBodyProcessor=XML)
 ACTION_CTL_BDY_URLENCODED                       (?i:ctl:requestBodyProcessor=URLENCODED)
 ACTION_CTL_FORCE_REQ_BODY_VAR                   (?i:ctl:forceRequestBodyVariable)
-ACTION_CTL_PARSE_XML_INTO_ARGS                  (?i:ctl:parseXMLintoArgs)
+ACTION_CTL_PARSE_XML_INTO_ARGS                  (?i:ctl:parseXmlIntoArgs)
 ACTION_CTL_REQUEST_BODY_ACCESS                  (?i:ctl:requestBodyAccess)
 ACTION_CTL_RULE_ENGINE                          (?i:ctl:ruleEngine)
 ACTION_CTL_RULE_REMOVE_BY_TAG                   (?i:ctl:ruleRemoveByTag)
@@ -412,7 +412,7 @@ CONFIG_VALUE_RELEVANT_ONLY              (?i:RelevantOnly)
 CONFIG_VALUE_SERIAL                     (?i:Serial)
 CONFIG_VALUE_WARN                       (?i:Warn)
 CONFIG_XML_EXTERNAL_ENTITY              (?i:SecXmlExternalEntity)
-CONFIG_XML_PARSE_XML_INTO_ARGS          (?i:SecParseXMLIntoArgs)
+CONFIG_XML_PARSE_XML_INTO_ARGS          (?i:SecParseXmlIntoArgs)
 CONGIG_DIR_RESPONSE_BODY_MP             (?i:SecResponseBodyMimeType)
 CONGIG_DIR_RESPONSE_BODY_MP_CLEAR       (?i:SecResponseBodyMimeTypesClear)
 CONGIG_DIR_SEC_ARG_SEP                  (?i:SecArgumentSeparator)

src/request_body_processor/xml.cc

@@ -65,12 +65,16 @@ class MSCSAXHandler {
                 exit(1);
             } */
             // if it's not the first (root) item, then append a '.'
-            // note, this can't occur because there is always a pseudo root element: 'xml'
+            // note, the condition should always be true because there is always a pseudo root element: 'xml'
             if (xml_data->nodes.size() > 1) {
                 xml_data->currpath.append(".");
                 xml_data->nodes[xml_data->nodes.size()-1]->has_child = true;
             }
             xml_data->currpath.append(name);
+            // set the current value empty
+            // this is necessary because if there is any text between the tags (new line, etc)
+            // it will be added to the current value
+            xml_data->currval = "";
         }
 
         void onEndElement(void * ctx, const xmlChar *localname) {
@@ -79,26 +83,30 @@ class MSCSAXHandler {
             const std::shared_ptr<NodeData>& nd = xml_data->nodes[xml_data->nodes.size()-1];
             if (nd->has_child == true) {
                 // check the return value
-                // if it false, then stop parsing
+                // if false, then stop parsing
                 // this means the number of arguments reached the limit
                 if (xml_data->m_transaction->addArgument("XML", xml_data->currpath, xml_data->currval, 0) == false) {
                     xmlStopParser(xml_data->parsing_ctx_arg);
                 }
             }
             if (xml_data->currpath.length() > 0) {
-                // set an offset to store this is the first item or not -> remove the '.' or not
+                // set an offset to store whether this is the first item, in order to know whether to remove the '.'
                 int offset = (xml_data->nodes.size() > 1) ? 1 : 0;
                 xml_data->currpath.erase(xml_data->currpath.length() - (name.length()+offset));
             }
             xml_data->nodes.pop_back();
             xml_data->node_depth--;
+            xml_data->currval = "";
         }
 
         void onCharacters(void *ctx, const xmlChar *ch, int len) {
             XMLNodes* xml_data = static_cast<XMLNodes*>(ctx);
             std::string content(reinterpret_cast<const char *>(ch), len);
 
-            xml_data->currval = content;
+            // libxml2 SAX parser will call this function multiple times
+            // during the parsing of a single node, if the value has multibyte
+            // characters, so we need to concatenate the values
+            xml_data->currval += content;
         }
 };
 
@@ -170,7 +178,7 @@ bool XML::init() {
         m_transaction->m_secXMLParseXmlIntoArgs
         == RulesSetProperties::OnlyArgsConfigXMLParseXmlIntoArgs) {
         ms_dbg_a(m_transaction, 9,
-                "XML: SecParseXMLIntoArgs is set to " \
+                "XML: SecParseXmlIntoArgs is set to " \
                 + RulesSetProperties::configXMLParseXmlIntoArgsString(static_cast<RulesSetProperties::ConfigXMLParseXmlIntoArgs>(m_transaction->m_secXMLParseXmlIntoArgs)));
         m_data.sax_handler = std::make_unique<xmlSAXHandler>();
         memset(m_data.sax_handler.get(), 0, sizeof(xmlSAXHandler));
@@ -184,6 +192,7 @@ bool XML::init() {
         m_data.xml_parser_state                  = std::make_unique<XMLNodes>(m_transaction);
         m_data.xml_parser_state->node_depth      = 0;
         m_data.xml_parser_state->currval         = "";
+        // the XML will contain at least one node, which is the pseudo root node 'xml'
         m_data.xml_parser_state->currpath        = "xml.";
     }
 
@@ -261,8 +270,8 @@ bool XML::processChunk(const char *buf, unsigned int size,
         xmlParseChunk(m_data.parsing_ctx, buf, size, 0);
         m_data.xml_parser_state->parsing_ctx_arg = m_data.parsing_ctx_arg;
         if (m_data.parsing_ctx->wellFormed != 1) {
-            error->assign("XML: Failed parsing document.");
-            ms_dbg_a(m_transaction, 4, "XML: Failed parsing document.");
+            error->assign("XML: Failed to parse document.");
+            ms_dbg_a(m_transaction, 4, "XML: Failed to parse document.");
             return false;
         }
     }
@@ -278,8 +287,8 @@ bool XML::processChunk(const char *buf, unsigned int size,
         xmlSetGenericErrorFunc(m_data.parsing_ctx_arg, null_error);
         xmlParseChunk(m_data.parsing_ctx_arg, buf, size, 0);
         if (m_data.parsing_ctx_arg->wellFormed != 1) {
-            error->assign("XML: Failed parsing document for ARGS.");
-            ms_dbg_a(m_transaction, 4, "XML: Failed parsing document for ARGS.");
+            error->assign("XML: Failed to parse document for ARGS.");
+            ms_dbg_a(m_transaction, 4, "XML: Failed to parse document for ARGS.");
             return false;
         }
     }
@@ -294,7 +303,7 @@ bool XML::complete(std::string *error) {
         if (m_data.parsing_ctx != NULL &&
             m_transaction->m_secXMLParseXmlIntoArgs
             != RulesSetProperties::OnlyArgsConfigXMLParseXmlIntoArgs) {
-            /* This is how we signalise the end of parsing to libxml. */
+            /* This is how we signal the end of parsing to libxml. */
             xmlParseChunk(m_data.parsing_ctx, NULL, 0, 1);
 
             /* Preserve the results for our reference. */
@@ -308,8 +317,8 @@ bool XML::complete(std::string *error) {
                 + std::to_string(m_data.well_formed) + ").");
 
             if (m_data.well_formed != 1) {
-                error->assign("XML: Failed parsing document.");
-                ms_dbg_a(m_transaction, 4, "XML: Failed parsing document.");
+                error->assign("XML: Failed to parse document.");
+                ms_dbg_a(m_transaction, 4, "XML: Failed to parse document.");
                 return false;
             }
         }
@@ -321,13 +330,13 @@ bool XML::complete(std::string *error) {
                 m_transaction->m_secXMLParseXmlIntoArgs
                   == RulesSetProperties::TrueConfigXMLParseXmlIntoArgs)
             ) {
-            /* This is how we signalise the end of parsing to libxml. */
+            /* This is how we signale the end of parsing to libxml. */
             if (xmlParseChunk(m_data.parsing_ctx_arg, NULL, 0, 1) != 0) {
                 if (m_data.xml_error != "") {
                     error->assign(m_data.xml_error);
                 }
                 else {
-                    error->assign("XML: Failed parsing document for ARGS.");
+                    error->assign("XML: Failed to parse document for ARGS.");
                 }
                 xmlFreeParserCtxt(m_data.parsing_ctx_arg);
                 m_data.parsing_ctx_arg = NULL;

src/request_body_processor/xml.h

@@ -55,7 +55,7 @@ class XMLNodes {
         std::string       currpath;
         std::string       currval;
         Transaction      *m_transaction;
-        // need to store context - this is the same as at the xml_data
+        // need to store context - this is the same as in xml_data
         // need to stop parsing if the number of arguments reached the limit
         xmlParserCtxtPtr  parsing_ctx_arg;
 
@@ -73,7 +73,7 @@ struct xml_data {
     /* error reporting and XML array flag */
     std::string               xml_error;
 
-    /* another parser context for arguments */
+    /* additional parser context for arguments */
     xmlParserCtxtPtr          parsing_ctx_arg;
 
     /* parser state for SAX parser */

test/test-cases/regression/action-ctl_request_body_processor.json

@@ -144,7 +144,7 @@
     "resource":"libxml2",
     "title":"Testing CtlRequestBodyProcessor=XML (3)",
     "expected":{
-      "debug_log": "XML: Failed parsing document."
+      "debug_log": "XML: Failed to parse document."
     },
     "client":{
       "ip":"200.249.12.31",

test/test-cases/regression/variable-REQBODY_PROCESSOR_ERROR.json

@@ -70,7 +70,7 @@
       ]
     },
     "expected":{  
-      "debug_log":"XML parsing error: XML: Failed parsing document"
+      "debug_log":"XML parsing error: XML: Failed to parse document"
     },
     "rules":[  
       "SecRuleEngine On",

test/test-cases/regression/variable-XML.json

@@ -81,7 +81,7 @@
     "rules":[
         "SecRuleEngine On",
         "SecRequestBodyAccess On",
-        "SecParseXMLIntoArgs On",
+        "SecParseXmlIntoArgs On",
         "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500011,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"",
         "SecRule ARGS:xml.bookstore.some-tag \"@rx aaa\" \"id:500012,phase:2,t:none,t:lowercase,log,deny,status:403\""
     ]
@@ -125,7 +125,7 @@
     "rules":[
         "SecRuleEngine On",
         "SecRequestBodyAccess On",
-        "SecParseXMLIntoArgs On",
+        "SecParseXmlIntoArgs On",
         "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500011,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"",
         "SecRule XML:/* \"@rx aaa\" \"id:500012,phase:2,t:none,t:lowercase,log,deny,status:403\""
     ]
@@ -169,7 +169,7 @@
     "rules":[
         "SecRuleEngine On",
         "SecRequestBodyAccess On",
-        "SecParseXMLIntoArgs OnlyArgs",
+        "SecParseXmlIntoArgs OnlyArgs",
         "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500011,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"",
         "SecRule ARGS:xml.bookstore.some-tag \"@rx aaa\" \"id:500012,phase:2,t:none,t:lowercase,log,deny,status:403\""
     ]
@@ -213,7 +213,7 @@
     "rules":[
         "SecRuleEngine On",
         "SecRequestBodyAccess On",
-        "SecParseXMLIntoArgs OnlyArgs",
+        "SecParseXmlIntoArgs OnlyArgs",
         "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500011,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"",
         "SecRule XML:/* \"@rx aaa\" \"id:500012,phase:2,t:none,t:lowercase,log,deny,status:403\""
     ]
@@ -257,7 +257,7 @@
     "rules":[
         "SecRuleEngine On",
         "SecRequestBodyAccess On",
-        "SecParseXMLIntoArgs Off",
+        "SecParseXmlIntoArgs Off",
         "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500011,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"",
         "SecRule ARGS \"@rx aaa\" \"id:500012,phase:2,t:none,t:lowercase,log,deny,status:403\""
     ]
@@ -301,7 +301,7 @@
     "rules":[
         "SecRuleEngine On",
         "SecRequestBodyAccess On",
-        "SecParseXMLIntoArgs Off",
+        "SecParseXmlIntoArgs Off",
         "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500011,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"",
         "SecRule XML:/* \"@rx aaa\" \"id:500012,phase:2,t:none,t:lowercase,log,deny,status:403\""
     ]
@@ -345,7 +345,7 @@
     "rules":[
         "SecRuleEngine On",
         "SecRequestBodyAccess On",
-        "SecParseXMLIntoArgs On",
+        "SecParseXmlIntoArgs On",
         "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500011,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"",
         "SecRule ARGS_GET:q \"@rx xml\" \"id:500012,phase:1,t:none,t:lowercase,ctl:parseXmlIntoArgs=Off\"",
         "SecRule ARGS:xml.bookstore.some-tag \"@rx aaa\" \"id:500013,phase:2,t:none,t:lowercase,log,deny,status:403\""
@@ -390,7 +390,7 @@
     "rules":[
         "SecRuleEngine On",
         "SecRequestBodyAccess On",
-        "SecParseXMLIntoArgs On",
+        "SecParseXmlIntoArgs On",
         "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500011,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"",
         "SecRule ARGS_GET:q \"@rx xml\" \"id:500012,phase:1,t:none,t:lowercase,ctl:parseXmlIntoArgs=Off\"",
         "SecRule XML:/* \"@rx aaa\" \"id:500013,phase:2,t:none,t:lowercase,log,deny,status:403\""
@@ -435,7 +435,7 @@
     "rules":[
         "SecRuleEngine On",
         "SecRequestBodyAccess On",
-        "SecParseXMLIntoArgs On",
+        "SecParseXmlIntoArgs On",
         "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500011,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"",
         "SecRule ARGS_GET:q \"@rx xml\" \"id:500012,phase:1,t:none,t:lowercase,ctl:parseXmlIntoArgs=OnlyArgs\"",
         "SecRule ARGS:xml.bookstore.some-tag \"@rx aaa\" \"id:500013,phase:2,t:none,t:lowercase,log,deny,status:403\""
@@ -481,14 +481,12 @@
     "rules":[
         "SecRuleEngine On",
         "SecRequestBodyAccess On",
-        "SecParseXMLIntoArgs On",
+        "SecParseXmlIntoArgs On",
         "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500011,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"",
         "SecRule ARGS_GET:q \"@rx xml\" \"id:500012,phase:1,t:none,t:lowercase,ctl:parseXmlIntoArgs=OnlyArgs\"",
         "SecRule XML:/* \"@rx aaa\" \"id:500013,phase:2,t:none,t:lowercase,log,deny,status:403\""
     ]
-  }
-
-  ,
+  },
   {
     "enabled":1,
     "version_min":300000,
@@ -528,7 +526,7 @@
     "rules":[
         "SecRuleEngine On",
         "SecRequestBodyAccess On",
-        "SecParseXMLIntoArgs Off",
+        "SecParseXmlIntoArgs Off",
         "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500011,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"",
         "SecRule ARGS_GET:q \"@rx xml\" \"id:500012,phase:1,t:none,t:lowercase,ctl:parseXmlIntoArgs=On\"",
         "SecRule ARGS:xml.bookstore.some-tag \"@rx aaa\" \"id:500013,phase:2,t:none,t:lowercase,log,deny,status:403\""
@@ -540,7 +538,7 @@
     "resource":"libxml2",
     "title":"Testing XML parsing to ARGS with Off, turn On with ctl, check XML",
     "expected":{
-      "http_code": 200
+      "http_code": 403
     },
     "client":{
       "ip":"200.249.12.31",
@@ -572,12 +570,51 @@
     },
     "rules":[
         "SecRuleEngine On",
-        "SecRequestBodyAccess Off",
-        "SecParseXMLIntoArgs On",
+        "SecRequestBodyAccess On",
+        "SecParseXmlIntoArgs Off",
         "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500011,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"",
         "SecRule ARGS_GET:q \"@rx xml\" \"id:500012,phase:1,t:none,t:lowercase,ctl:parseXmlIntoArgs=On\"",
         "SecRule XML:/* \"@rx aaa\" \"id:500013,phase:2,t:none,t:lowercase,log,deny,status:403\""
     ]
+  },
+  {
+    "enabled":1,
+    "version_min":300000,
+    "resource":"libxml2",
+    "title":"Testing XML parsing to ARGS with On, node contains utf8 character",
+    "expected":{
+      "http_code": 403
+    },
+    "client":{
+      "ip":"200.249.12.31",
+      "port":123
+    },
+    "request":{
+      "headers":{
+        "Host":"localhost",
+        "User-Agent":"curl/7.38.0",
+        "Accept":"*/*",
+        "Content-Type": "text/xml"
+      },
+      "uri":"/?q=xml",
+      "method":"POST",
+      "body": [
+        "<pizza>",
+        "<has>pineapple</has><has>🍍</has>",
+        "</pizza>"
+      ]
+    },
+    "server":{
+      "ip":"200.249.12.31",
+      "port":80
+    },
+    "rules":[
+        "SecRuleEngine On",
+        "SecRequestBodyAccess On",
+        "SecParseXmlIntoArgs On",
+        "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500011,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"",
+        "SecRule ARGS \"@rx 🍍\" \"id:500013,phase:2,t:none,t:lowercase,log,deny,status:403\""
+    ]
   }
 ]