From 9e48194d2a9ed2a7c682de3f50673eee8824a4f6 Mon Sep 17 00:00:00 2001 From: Tom Sommer Date: Sat, 19 Sep 2026 16:19:04 +0200 Subject: [PATCH] Do not keep the @validateSchema parser errors in the operator ValidateSchema::evaluate() installs ValidateSchema::error_load() and ValidateSchema::warn_load() as the libxml2 schema parser callbacks and passes the address of the ValidateSchema member m_err as their user context. Those callbacks append to that string and nothing ever clears it. An operator instance is created while the rules are parsed and lives for as long as the rule set does, which for a web server means the lifetime of the process. Therefore: * every evaluation whose schema parse emits a warning or an error makes m_err grow by the size of the new message, without any bound, and * the message logged for one transaction contains the messages of all the previous transactions that used the same rule, and * concurrent transactions sharing the rule set append to the same std::string at the same time, which is a data race. The accumulated text is per evaluation state, so it is moved to a local std::string in evaluate() and the member is removed from the header. The member was declared inside the WITH_LIBXML2 guard and is only used by the guarded code, so builds configured with --without-libxml are unaffected. The check of the accumulated text in the branch where the parser context itself cannot be created is dropped: at that point no callback has run yet, so the local is always empty. The unbounded part of the growth is cross request and cannot be observed by the single transaction regression harness, but the accumulation is visible within one transaction as soon as the same operator instance is evaluated twice. The regression test added to request-body-parser-xml.json therefore reuses the existing SoapEnvelope-bad.xsd case, which fails to parse, and adds multiMatch so that the operator runs once per transformation. Before this change the debug log of the second evaluation reads XML: Failed to load Schema: test-cases/data/SoapEnvelope-bad.xsd. XML Error: Failed to parse the XML resource '...'. XML Error: Failed to parse the XML resource '...'. with the message repeated once per previous evaluation; after it every evaluation logs the message exactly once. The expected debug_log of the new test asserts that no log line starts with a repeated "XML Error:", so it fails before the change and passes after it. --- src/operators/validate_schema.cc | 15 +++-- src/operators/validate_schema.h | 1 - .../regression/request-body-parser-xml.json | 60 +++++++++++++++++++ 3 files changed, 67 insertions(+), 9 deletions(-) diff --git a/src/operators/validate_schema.cc b/src/operators/validate_schema.cc index d45ef5750c..112696952f 100644 --- a/src/operators/validate_schema.cc +++ b/src/operators/validate_schema.cc @@ -54,22 +54,21 @@ bool ValidateSchema::evaluate(Transaction *transaction, return true; } + std::string schemaErr; + xmlSchemaParserCtxtPtr parserCtx = xmlSchemaNewParserCtxt(m_resource.c_str()); if (parserCtx == NULL) { std::stringstream err; err << "XML: Failed to load Schema from file: "; err << m_resource; err << ". "; - if (m_err.empty() == false) { - err << m_err; - } ms_dbg_a(transaction, 4, err.str()); return true; } xmlSchemaSetParserErrors(parserCtx, (xmlSchemaValidityErrorFunc)error_load, - (xmlSchemaValidityWarningFunc)warn_load, &m_err); + (xmlSchemaValidityWarningFunc)warn_load, &schemaErr); xmlSchemaPtr schema = xmlSchemaParse(parserCtx); if (schema == NULL) { @@ -77,8 +76,8 @@ bool ValidateSchema::evaluate(Transaction *transaction, err << "XML: Failed to load Schema: "; err << m_resource; err << "."; - if (m_err.empty() == false) { - err << " " << m_err; + if (schemaErr.empty() == false) { + err << " " << schemaErr; } ms_dbg_a(transaction, 4, err.str()); xmlSchemaFreeParserCtxt(parserCtx); @@ -88,8 +87,8 @@ bool ValidateSchema::evaluate(Transaction *transaction, xmlSchemaValidCtxtPtr validCtx = xmlSchemaNewValidCtxt(schema); if (validCtx == NULL) { std::stringstream err("XML: Failed to create validation context."); - if (m_err.empty() == false) { - err << " " << m_err; + if (schemaErr.empty() == false) { + err << " " << schemaErr; } ms_dbg_a(transaction, 4, err.str()); xmlSchemaFree(schema); diff --git a/src/operators/validate_schema.h b/src/operators/validate_schema.h index 96cca8f25f..65e450de5f 100644 --- a/src/operators/validate_schema.h +++ b/src/operators/validate_schema.h @@ -100,7 +100,6 @@ class ValidateSchema : public Operator { private: std::string m_resource; - std::string m_err; #endif }; diff --git a/test/test-cases/regression/request-body-parser-xml.json b/test/test-cases/regression/request-body-parser-xml.json index cd4426c7fa..45a31ac285 100644 --- a/test/test-cases/regression/request-body-parser-xml.json +++ b/test/test-cases/regression/request-body-parser-xml.json @@ -298,5 +298,65 @@ "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\"" ] + }, + { + "enabled": 1, + "version_min": 300000, + "title": "Testing XML request body parser (bad schema, errors are not accumulated between evaluations)", + "resource": "libxml2", + "client": { + "ip": "200.249.12.31", + "port": 123 + }, + "server": { + "ip": "200.249.12.31", + "port": 80 + }, + "request": { + "headers": { + "Host": "localhost", + "User-Agent": "curl/7.38.0", + "Accept": "*/*", + "Cookie": "PHPSESSID=rAAAAAAA2t5uvjq435r4q7ib3vtdjq120", + "Content-Type": "text/xml", + "Content-Length": "683" + }, + "uri": "/?key=value&key=other_value", + "method": "POST", + "body": [ + "", + " ", + " ", + " ", + " 12123", + " ", + " ", + "" + ] + }, + "response": { + "headers": { + "Content-Length": "0" + }, + "body": [ + "" + ] + }, + "expected": { + "debug_log": "XML: Failed to load Schema: test-cases/data/SoapEnvelope-bad\\.xsd\\. XML Error: Failed to parse(?!.*^XML Error:)", + "http_code": 403 + }, + "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,t:none,t:lowercase,multiMatch\"" + ] } ]