From 030d38251ec5d2cb680cfffd38752700b06c733b Mon Sep 17 00:00:00 2001 From: Tom Sommer Date: Sat, 19 Sep 2026 16:13:47 +0200 Subject: [PATCH] Fix leak of the ARGS XML parser context on malformed documents With SecParseXmlIntoArgs set to On, XML::processChunk() creates a second libxml2 push parser context, xml_data::parsing_ctx_arg, in addition to the regular parsing_ctx. That context is only released in XML::complete(), in the block that terminates the ARGS parsing. XML::complete() finishes the main context first and returns early with "XML: Failed to parse document." when the document is not well formed, before reaching the block that frees parsing_ctx_arg. ~XML() released parsing_ctx and doc but not parsing_ctx_arg, so nothing ever freed it. A remote client could therefore leak the ARGS parser context, including its body-sized input buffer, on every request carrying a malformed XML body. parsing_ctx_arg is now freed in ~XML() and, through the same small helper (XML::freeArgsParserCtx()), also freed and cleared before the early return in XML::complete() so that the memory is released as soon as the parsing is known to have failed rather than at the end of the transaction. The copy of the pointer kept in xml_data::xml_parser_state is only used from the SAX callbacks while parsing is in progress, and both changes stay inside the existing WITH_LIBXML2 guard. A regression test covering a mismatched end tag and a truncated document is added; under valgrind the unfixed code reports the context allocated in xmlCreatePushParserCtxt() as definitely lost for each request. --- src/request_body_processor/xml.cc | 10 ++ src/request_body_processor/xml.h | 1 + .../request-body-parser-xml-into-args.json | 96 +++++++++++++++++++ test/test-suite.in | 1 + 4 files changed, 108 insertions(+) create mode 100644 test/test-cases/regression/request-body-parser-xml-into-args.json diff --git a/src/request_body_processor/xml.cc b/src/request_body_processor/xml.cc index cbb7894c9b..4fe67a7eb3 100644 --- a/src/request_body_processor/xml.cc +++ b/src/request_body_processor/xml.cc @@ -165,12 +165,21 @@ XML::~XML() { xmlFreeParserCtxt(m_data.parsing_ctx); m_data.parsing_ctx = NULL; } + freeArgsParserCtx(); if (m_data.doc != NULL) { xmlFreeDoc(m_data.doc); m_data.doc = NULL; } } + +void XML::freeArgsParserCtx() { + if (m_data.parsing_ctx_arg != nullptr) { + xmlFreeParserCtxt(m_data.parsing_ctx_arg); + m_data.parsing_ctx_arg = nullptr; + } +} + bool XML::init() { //xmlParserInputBufferCreateFilenameFunc entity; if (m_transaction->m_rules->m_secXMLExternalEntity @@ -329,6 +338,7 @@ bool XML::complete(std::string *error) { if (m_data.well_formed != 1) { error->assign("XML: Failed to parse document."); ms_dbg_a(m_transaction, 4, "XML: Failed to parse document."); + freeArgsParserCtx(); return false; } } diff --git a/src/request_body_processor/xml.h b/src/request_body_processor/xml.h index aac1299de1..9a7359525a 100644 --- a/src/request_body_processor/xml.h +++ b/src/request_body_processor/xml.h @@ -96,6 +96,7 @@ class XML { xml_data m_data; private: + void freeArgsParserCtx(); Transaction *m_transaction; std::string m_header; }; diff --git a/test/test-cases/regression/request-body-parser-xml-into-args.json b/test/test-cases/regression/request-body-parser-xml-into-args.json new file mode 100644 index 0000000000..ea542b2ebe --- /dev/null +++ b/test/test-cases/regression/request-body-parser-xml-into-args.json @@ -0,0 +1,96 @@ +[ + { + "enabled": 1, + "version_min": 300000, + "title": "XML request body parser with SecParseXmlIntoArgs (mismatched end tag)", + "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": "*/*", + "Content-Type": "text/xml", + "Content-Length": "10" + }, + "uri": "/", + "method": "POST", + "body": [ + "" + ] + }, + "response": { + "headers": { + "Content-Length": "0" + }, + "body": [ + "" + ] + }, + "expected": { + "debug_log": "XML: Failed to parse document", + "http_code": 200 + }, + "rules": [ + "SecRuleEngine On", + "SecRequestBodyAccess On", + "SecParseXmlIntoArgs On", + "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500080,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"", + "SecRule REQBODY_ERROR \"!@eq 0\" \"id:500081,phase:2,pass,log\"" + ] + }, + { + "enabled": 1, + "version_min": 300000, + "title": "XML request body parser with SecParseXmlIntoArgs (truncated document)", + "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": "*/*", + "Content-Type": "text/xml", + "Content-Length": "6" + }, + "uri": "/", + "method": "POST", + "body": [ + "" + ] + }, + "response": { + "headers": { + "Content-Length": "0" + }, + "body": [ + "" + ] + }, + "expected": { + "debug_log": "XML: Failed to parse document", + "http_code": 200 + }, + "rules": [ + "SecRuleEngine On", + "SecRequestBodyAccess On", + "SecParseXmlIntoArgs On", + "SecRule REQUEST_HEADERS:Content-Type \"^text/xml$\" \"id:500080,phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML\"", + "SecRule REQBODY_ERROR \"!@eq 0\" \"id:500081,phase:2,pass,log\"" + ] + } +] diff --git a/test/test-suite.in b/test/test-suite.in index ebda49fb81..1ee362ab9e 100644 --- a/test/test-suite.in +++ b/test/test-suite.in @@ -99,6 +99,7 @@ TESTS+=test/test-cases/regression/request-body-parser-json.json TESTS+=test/test-cases/regression/request-body-parser-multipart-crlf.json TESTS+=test/test-cases/regression/request-body-parser-multipart.json TESTS+=test/test-cases/regression/request-body-parser-xml.json +TESTS+=test/test-cases/regression/request-body-parser-xml-into-args.json TESTS+=test/test-cases/regression/request-body-parser-xml-validade-dtd.json TESTS+=test/test-cases/regression/rule-920120.json TESTS+=test/test-cases/regression/rule-920200.json