diff --git a/headers/modsecurity/rules_properties.h b/headers/modsecurity/rules_properties.h index a62d6742..eb9f70ec 100644 --- a/headers/modsecurity/rules_properties.h +++ b/headers/modsecurity/rules_properties.h @@ -56,6 +56,7 @@ class RulesProperties { requestBodyInMemoryLimit(0), secRequestBodyAccess(false), secResponseBodyAccess(false), + secXMLExternalEntity(false), requestBodyLimitAction(ProcessPartialBodyLimitAction), responseBodyLimit(0), responseBodyLimitAction(ProcessPartialBodyLimitAction), @@ -71,6 +72,7 @@ class RulesProperties { requestBodyInMemoryLimit(0), secRequestBodyAccess(false), secResponseBodyAccess(false), + secXMLExternalEntity(false), requestBodyLimitAction(ProcessPartialBodyLimitAction), responseBodyLimit(0), responseBodyLimitAction(ProcessPartialBodyLimitAction), @@ -202,6 +204,7 @@ class RulesProperties { bool secRequestBodyAccess; bool secResponseBodyAccess; + bool secXMLExternalEntity; std::string audit_log_path; std::string audit_log_parts; std::list components; diff --git a/src/actions/ctl_request_body_processor_xml.cc b/src/actions/ctl_request_body_processor_xml.cc index 66b4a8ae..ea681b90 100644 --- a/src/actions/ctl_request_body_processor_xml.cc +++ b/src/actions/ctl_request_body_processor_xml.cc @@ -24,8 +24,9 @@ namespace modsecurity { namespace actions { -bool CtlRequestBodyProcessorXML::evaluate(Rule *rule, Transaction *transaction) { - transaction->m_requestBodyProcessor = modsecurity::Transaction::XMLRequestBody; +bool CtlRequestBodyProcessorXML::evaluate(Rule *rule, + Transaction *transaction) { + transaction->m_requestBodyProcessor = Transaction::XMLRequestBody; return true; } diff --git a/src/operators/validate_dtd.cc b/src/operators/validate_dtd.cc index 5b2f892d..c857eebd 100644 --- a/src/operators/validate_dtd.cc +++ b/src/operators/validate_dtd.cc @@ -39,14 +39,6 @@ bool ValidateDTD::init(const std::string &file, const char **error) { xmlSetGenericErrorFunc(NULL, null_error); - m_dtd = xmlParseDTD(NULL, (const xmlChar *)m_resource.c_str()); - if (m_dtd == NULL) { - std::string err = std::string("XML: Failed to load DTD: ") \ - + m_resource; - *error = strdup(err.c_str()); - return false; - } - return true; } @@ -54,6 +46,14 @@ bool ValidateDTD::init(const std::string &file, const char **error) { bool ValidateDTD::evaluate(Transaction *t, const std::string &str) { xmlValidCtxtPtr cvp; + m_dtd = xmlParseDTD(NULL, (const xmlChar *)m_resource.c_str()); + if (m_dtd == NULL) { + std::string err = std::string("XML: Failed to load DTD: ") \ + + m_resource; + t->debug(4, err); + return true; + } + if (t->m_xml->m_data.doc == NULL) { t->debug(4, "XML document tree could not "\ "be found for DTD validation."); diff --git a/src/operators/validate_schema.cc b/src/operators/validate_schema.cc index 106c18ce..815edd2d 100644 --- a/src/operators/validate_schema.cc +++ b/src/operators/validate_schema.cc @@ -33,6 +33,14 @@ bool ValidateSchema::init(const std::string &file, const char **error) { return false; } + return true; +} + + +bool ValidateSchema::evaluate(Transaction *t, + const std::string &str) { + int rc; + m_parserCtx = xmlSchemaNewParserCtxt(m_resource.c_str()); if (m_parserCtx == NULL) { std::stringstream err; @@ -42,8 +50,8 @@ bool ValidateSchema::init(const std::string &file, const char **error) { if (m_err.empty() == false) { err << m_err; } - *error = strdup(err.str().c_str()); - return false; + t->debug(4, err.str()); + return true; } xmlSchemaSetParserErrors(m_parserCtx, @@ -65,9 +73,9 @@ bool ValidateSchema::init(const std::string &file, const char **error) { if (m_err.empty() == false) { err << " " << m_err; } - *error = strdup(err.str().c_str()); + t->debug(4, err.str()); xmlSchemaFreeParserCtxt(m_parserCtx); - return false; + return true; } m_validCtx = xmlSchemaNewValidCtxt(m_schema); @@ -76,18 +84,10 @@ bool ValidateSchema::init(const std::string &file, const char **error) { if (m_err.empty() == false) { err << " " << m_err; } - *error = strdup(err.str().c_str()); - return false; + t->debug(4, err.str()); + return true; } - return true; -} - - -bool ValidateSchema::evaluate(Transaction *t, - const std::string &str) { - int rc; - /* Send validator errors/warnings to msr_log */ xmlSchemaSetValidErrors(m_validCtx, (xmlSchemaValidityErrorFunc)error_runtime, diff --git a/src/parser/seclang-parser.yy b/src/parser/seclang-parser.yy index 5d65b10c..2a48e773 100644 --- a/src/parser/seclang-parser.yy +++ b/src/parser/seclang-parser.yy @@ -213,6 +213,8 @@ using modsecurity::Variables::XML; %token CONFIG_DIR_DEBUG_LOG %token CONFIG_DIR_DEBUG_LVL +%token CONFIG_XML_EXTERNAL_ENTITY + %token CONFIG_DIR_SEC_ACTION %token CONFIG_DIR_SEC_DEFAULT_ACTION %token CONFIG_DIR_SEC_MARKER @@ -652,6 +654,14 @@ expression: driver.m_responseBodyTypeToBeInspected.insert(*it); } } + | CONFIG_XML_EXTERNAL_ENTITY CONFIG_VALUE_OFF + { + driver.secXMLExternalEntity = false; + } + | CONFIG_XML_EXTERNAL_ENTITY CONFIG_VALUE_ON + { + driver.secXMLExternalEntity = true; + } | CONGIG_DIR_SEC_TMP_DIR | CONGIG_DIR_SEC_DATA_DIR | CONGIG_DIR_SEC_ARG_SEP diff --git a/src/parser/seclang-scanner.ll b/src/parser/seclang-scanner.ll index d4fe383c..8040dde6 100755 --- a/src/parser/seclang-scanner.ll +++ b/src/parser/seclang-scanner.ll @@ -83,6 +83,7 @@ CONFIG_DIR_RULE_ENG (?i:SecRuleEngine) CONFIG_DIR_REQ_BODY (?i:SecRequestBodyAccess) CONFIG_DIR_RES_BODY (?i:SecResponseBodyAccess) +CONFIG_XML_EXTERNAL_ENTITY (?i:SecXmlExternalEntity) CONFIG_DIR_AUDIT_DIR_MOD (?i:SecAuditLogDirMode) CONFIG_DIR_AUDIT_DIR (?i:SecAuditLogStorageDir) @@ -285,6 +286,7 @@ CONFIG_DIR_UNICODE_MAP_FILE (?i:SecUnicodeMapFile) {CONFIG_COMPONENT_SIG}[ ]["]{FREE_TEXT}["] { return yy::seclang_parser::make_CONFIG_COMPONENT_SIG(strchr(yytext, ' ') + 2, *driver.loc.back()); } %{ /* Other configurations */ %} +{CONFIG_XML_EXTERNAL_ENTITY} { return yy::seclang_parser::make_CONFIG_XML_EXTERNAL_ENTITY(yytext, *driver.loc.back()); } {CONFIG_DIR_PCRE_MATCH_LIMIT_RECURSION}[ ]{CONFIG_VALUE_NUMBER} { return yy::seclang_parser::make_CONFIG_DIR_PCRE_MATCH_LIMIT_RECURSION(strchr(yytext, ' ') + 1, *driver.loc.back()); } {CONFIG_DIR_PCRE_MATCH_LIMIT}[ ]{CONFIG_VALUE_NUMBER} { return yy::seclang_parser::make_CONFIG_DIR_PCRE_MATCH_LIMIT(strchr(yytext, ' ') + 1, *driver.loc.back()); } {CONGIG_DIR_RESPONSE_BODY_MP}[ ]{FREE_TEXT_NEW_LINE} { return yy::seclang_parser::make_CONGIG_DIR_RESPONSE_BODY_MP(strchr(yytext, ' ') + 1, *driver.loc.back()); } diff --git a/src/request_body_processor/xml.cc b/src/request_body_processor/xml.cc index d2d405d7..865ac304 100644 --- a/src/request_body_processor/xml.cc +++ b/src/request_body_processor/xml.cc @@ -41,13 +41,25 @@ XML::~XML() { bool XML::init() { - // xmlParserInputBufferCreateFilenameFunc entity; - // entity = xmlParserInputBufferCreateFilenameDefault( - // this->unloadExternalEntity); + xmlParserInputBufferCreateFilenameFunc entity; + if (m_transaction->m_rules->secXMLExternalEntity == true) { + entity = xmlParserInputBufferCreateFilenameDefault( + __xmlParserInputBufferCreateFilename); + } else { + entity = xmlParserInputBufferCreateFilenameDefault( + this->unloadExternalEntity); + } + return true; } +xmlParserInputBufferPtr XML::unloadExternalEntity(const char *URI, + xmlCharEncoding enc) { + return NULL; +} + + bool XML::processChunk(const char *buf, unsigned int size) { /* We want to initialise our parsing context here, to * enable us to pass it the first chunk of data so that diff --git a/src/request_body_processor/xml.h b/src/request_body_processor/xml.h index 3136966a..fbcfceb3 100644 --- a/src/request_body_processor/xml.h +++ b/src/request_body_processor/xml.h @@ -21,6 +21,7 @@ #include #include "modsecurity/transaction.h" +#include "modsecurity/rules.h" #ifndef SRC_REQUEST_BODY_PROCESSOR_XML_H_ #define SRC_REQUEST_BODY_PROCESSOR_XML_H_ @@ -48,7 +49,7 @@ class XML { bool processChunk(const char *buf, unsigned int size); bool complete(); static xmlParserInputBufferPtr unloadExternalEntity(const char *URI, - xmlCharEncoding enc) { return NULL; } + xmlCharEncoding enc); #ifndef NO_LOGS void debug(int a, std::string str) { diff --git a/src/rules.cc b/src/rules.cc index 5bd75aea..ff10054d 100644 --- a/src/rules.cc +++ b/src/rules.cc @@ -206,6 +206,7 @@ int Rules::merge(Driver *from) { this->secRuleEngine = from->secRuleEngine; this->secRequestBodyAccess = from->secRequestBodyAccess; this->secResponseBodyAccess = from->secResponseBodyAccess; + this->secXMLExternalEntity = from->secXMLExternalEntity; if (from->m_debugLog && this->m_debugLog && from->m_debugLog->isLogFileSet()) { this->m_debugLog->setDebugLogFile(from->m_debugLog->getDebugLogFile()); diff --git a/src/variables/xml.cc b/src/variables/xml.cc index d2b7fb9c..dafd89b9 100644 --- a/src/variables/xml.cc +++ b/src/variables/xml.cc @@ -34,6 +34,8 @@ #include #include "modsecurity/transaction.h" +#include "modsecurity/rules_properties.h" +#include "modsecurity/rules.h" #include "src/request_body_processor/xml.h" #include "src/actions/action.h" diff --git a/test/test-cases/regression/config-xml_external_entity.json b/test/test-cases/regression/config-xml_external_entity.json new file mode 100644 index 00000000..fa0b405f --- /dev/null +++ b/test/test-cases/regression/config-xml_external_entity.json @@ -0,0 +1,142 @@ +[ + { + "enabled":1, + "version_min":300000, + "title":"Testing SecXMLExternalEntity/XXE 1", + "expected":{ + "debug_log": "Target value: \" jo smith\"" + }, + "client":{ + "ip":"200.249.12.31", + "port":123 + }, + "request":{ + "headers":{ + "Host":"localhost", + "User-Agent":"curl/7.38.0", + "Accept":"*/*", + "Cookie": "PHPSESSID=rAAAAAAA2t5uvjq435r4q7ib3vtdjq120", + "Content-Type": "text/xml" + }, + "uri":"/?key=value&key=other_value", + "method":"POST", + "body": [ + "", + "", + "", + "]>", + "", + " &js;", + "" + ] + + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "rules":[ + "SecRuleEngine On", + "SecRequestBodyAccess On", + "SecXMLExternalEntity Off", + "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500005,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"", + "SecRule XML:/bookstore/book[text()] \".*\" \"id:500006,phase:3,t:none,t:lowercase,nolog,pass\"" + ] + }, + { + "enabled":1, + "version_min":300000, + "title":"Testing SecXMLExternalEntity/XXE 2", + "expected":{ + "debug_log": "XML: Failed to load DTD: test-cases/data/SoapEnvelope.dtd", + "http_code": 403 + }, + "client":{ + "ip":"200.249.12.31", + "port":123 + }, + "request":{ + "headers":{ + "Host":"localhost", + "User-Agent":"curl/7.38.0", + "Accept":"*/*", + "Cookie": "PHPSESSID=rAAAAAAA2t5uvjq435r4q7ib3vtdjq120", + "Content-Type": "text/xml" + }, + "uri":"/?key=value&key=other_value", + "method":"POST", + "body": [ + "", + "", + "", + "]>", + "", + " &js;", + "" + ] + + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "rules":[ + "SecRuleEngine On", + "SecRequestBodyAccess On", + "SecXMLExternalEntity Off", + "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500005,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"", + "SecRule XML:/bookstore/book \".*\" \"id:500006,phase:3,t:none,t:lowercase,nolog,pass,xmlns:soap='http://schemas.xmlsoap.org/soap/envelope/'\"", + "SecRule XML \"@validateDTD test-cases/data/SoapEnvelope.dtd\" \"id:500007,phase:3,deny\"" + ] + }, + { + "enabled":1, + "version_min":300000, + "title":"Testing SecXMLExternalEntity/XXE 3", + "expected":{ + "debug_log": "XML Error: No declaration for element bookstore", + "http_code": 403 + }, + "client":{ + "ip":"200.249.12.31", + "port":123 + }, + "request":{ + "headers":{ + "Host":"localhost", + "User-Agent":"curl/7.38.0", + "Accept":"*/*", + "Cookie": "PHPSESSID=rAAAAAAA2t5uvjq435r4q7ib3vtdjq120", + "Content-Type": "text/xml" + }, + "uri":"/?key=value&key=other_value", + "method":"POST", + "body": [ + "", + "", + "", + "]>", + "", + " &js;", + "" + ] + + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "rules":[ + "SecRuleEngine On", + "SecRequestBodyAccess On", + "SecXMLExternalEntity On", + "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500005,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"", + "SecRule XML:/bookstore/book \".*\" \"id:500006,phase:3,t:none,t:lowercase,nolog,pass,xmlns:soap='http://schemas.xmlsoap.org/soap/envelope/'\"", + "SecRule XML \"@validateDTD test-cases/data/SoapEnvelope.dtd\" \"id:500007,phase:3,deny\"" + ] + } +] + diff --git a/test/test-cases/regression/request-body-parser-xml-validade-dtd.json b/test/test-cases/regression/request-body-parser-xml-validade-dtd.json index 028987d4..c01c8c75 100644 --- a/test/test-cases/regression/request-body-parser-xml-validade-dtd.json +++ b/test/test-cases/regression/request-body-parser-xml-validade-dtd.json @@ -39,6 +39,7 @@ "rules":[ "SecRuleEngine On", "SecRequestBodyAccess On", + "SecXMLExternalEntity On", "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500008,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"", "SecRule XML \"@validateDTD test-cases/data/SoapEnvelope.dtd\" \"id:500007,phase:3,deny\"" ] @@ -84,6 +85,7 @@ "rules":[ "SecRuleEngine On", "SecRequestBodyAccess On", + "SecXMLExternalEntity On", "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500008,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"", "SecRule XML \"@validateDTD test-cases/data/SoapEnvelope.dtd\" \"id:500007,phase:3,deny\"" ] @@ -129,6 +131,7 @@ "rules":[ "SecRuleEngine On", "SecRequestBodyAccess On", + "SecXMLExternalEntity On", "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500008,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"", "SecRule XML \"@validateDTD test-cases/data/SoapEnvelope.dtd\" \"id:500007,phase:3,deny\"" ] @@ -138,7 +141,8 @@ "version_min":300000, "title":"Testing XML request body parser - validateDTD (bad DTD)", "expected":{ - "parser_error": "Line: 4. Column: 12. XML: Failed to load DTD: test-cases/data/SoapEnvelope-bad.dtd" + "debug_log": "Failed to load DTD: test-cases/data/SoapEnvelope-bad.dtd", + "http_code": 403 }, "client":{ "ip":"200.249.12.31", @@ -173,6 +177,7 @@ "rules":[ "SecRuleEngine On", "SecRequestBodyAccess On", + "SecXMLExternalEntity On", "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500008,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"", "SecRule XML \"@validateDTD test-cases/data/SoapEnvelope-bad.dtd\" \"id:500007,phase:3,deny\"" ] diff --git a/test/test-cases/regression/request-body-parser-xml.json b/test/test-cases/regression/request-body-parser-xml.json index 824fa546..072912d4 100644 --- a/test/test-cases/regression/request-body-parser-xml.json +++ b/test/test-cases/regression/request-body-parser-xml.json @@ -43,6 +43,7 @@ "rules":[ "SecRuleEngine On", "SecRequestBodyAccess On", + "SecXMLExternalEntity On", "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500005,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"", "SecRule XML \"@validateSchema test-cases/data/SoapEnvelope.xsd\" \"id:500007,phase:3,deny\"" ] @@ -92,6 +93,7 @@ "rules":[ "SecRuleEngine On", "SecRequestBodyAccess On", + "SecXMLExternalEntity On", "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500008,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"", "SecRule XML \"@validateSchema test-cases/data/SoapEnvelope.xsd\" \"id:500007,phase:3,deny\"" ] @@ -141,6 +143,7 @@ "rules":[ "SecRuleEngine On", "SecRequestBodyAccess On", + "SecXMLExternalEntity On", "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500008,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"", "SecRule XML \"@validateSchema test-cases/data/SoapEnvelope.xsd\" \"id:500007,phase:3,deny\"" ] @@ -190,6 +193,7 @@ "rules":[ "SecRuleEngine On", "SecRequestBodyAccess On", + "SecXMLExternalEntity On", "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500008,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"", "SecRule XML \"@validateSchema test-cases/data/SoapEnvelope.xsd\" \"id:500007,phase:3,deny\"" ] @@ -199,7 +203,8 @@ "version_min":300000, "title":"Testing XML request body parser (bad schema)", "expected":{ - "parser_error": " XML: Failed to load Schema: test-cases/data/SoapEnvelope-bad.xsd. XML Error: Failed to parse the XML resource 'test-cases/data/SoapEnvelope-bad.xsd" + "debug_log": "XML: Failed to load Schema: test-cases/data/SoapEnvelope-bad.xsd. XML Error: Failed to parse the XML resource 'test-cases/data/SoapEnvelope-bad.xsd", + "http_code": 403 }, "client":{ "ip":"200.249.12.31", @@ -238,6 +243,7 @@ "rules":[ "SecRuleEngine On", "SecRequestBodyAccess On", + "SecXMLExternalEntity On", "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500008,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"", "SecRule XML \"@validateSchema test-cases/data/SoapEnvelope-bad.xsd\" \"id:500007,phase:3,deny\"" ]