From 25017dce267b2d3baab9488fbf93f99a8e49bc28 Mon Sep 17 00:00:00 2001 From: Felipe Zipitria Date: Tue, 13 Oct 2026 12:10:02 -0300 Subject: [PATCH 1/2] feat: surface libxml2's parse error in REQBODY_ERROR_MSG When the XML body processor fails, REQBODY_ERROR_MSG and the logs only said "XML: Failed to parse document." libxml2 knows the actual reason (tag mismatch, extra content after the root element, empty document, ...) with line and column, but it was discarded. The dead xml_receive_sax_error() under #if 0 was the original design for this. Read xmlCtxtGetLastError() after a failed xmlParseChunk() and build the message from it: XML: Failed to parse document: Opening and ending tag mismatch: book line 3 and bookasdf (line 6, column 12) The fixed text stays as the fallback when libxml2 has no message. The same applies to the ARGS parsing context. In xml_complete() the error is read before xmlFreeParserCtxt(). xmlCtxtGetLastError() is available since libxml2 2.6 and is not deprecated in 2.15. Adds tests/regression/target/10-variable-reqbody_error_msg-xml.t with a tag-mismatch body and the "extra content" body from v3 #3563, both asserting the libxml2 text in the error log. Part of #3680. v3 counterpart: #3679. Co-Authored-By: Claude Fable 5.1 --- apache2/msc_xml.c | 53 ++++++++++---- .../10-variable-reqbody_error_msg-xml.t | 73 +++++++++++++++++++ 2 files changed, 111 insertions(+), 15 deletions(-) create mode 100644 tests/regression/target/10-variable-reqbody_error_msg-xml.t diff --git a/apache2/msc_xml.c b/apache2/msc_xml.c index 7b0f592cf..db9097850 100644 --- a/apache2/msc_xml.c +++ b/apache2/msc_xml.c @@ -14,6 +14,9 @@ #include "msc_xml.h" +#include +#include + static void msc_xml_on_start_elementns( void *ctx, const xmlChar *localname, @@ -200,21 +203,35 @@ int xml_init(modsec_rec *msr, char **error_msg) { return 1; } -#if 0 -static void xml_receive_sax_error(void *data, const char *msg, ...) { - modsec_rec *msr = (modsec_rec *)data; - char message[256]; +/** + * Build the error message from libxml2's last error on the context, so the + * actual reason ("Extra content at the end of the document", a tag mismatch, + * ...) reaches REQBODY_ERROR_MSG and the logs instead of a fixed string. + * Falls back to "XML: ." when libxml2 has no message. + * xmlCtxtGetLastError() keeps working under XML_PARSE_NOERROR. + */ +static char *xml_parse_error(modsec_rec *msr, xmlParserCtxtPtr ctx, const char *what) { + const xmlError *e = (ctx != NULL) ? xmlCtxtGetLastError(ctx) : NULL; + char *detail; + size_t len; + + assert(msr != NULL); + assert(what != NULL); - if (msr == NULL) return; + if (e == NULL || e->message == NULL || e->message[0] == '\0') { + return apr_psprintf(msr->mp, "XML: %s.", what); + } - apr_snprintf(message, sizeof(message), "%s (line %d offset %d)", - log_escape_nq(msr->mp, msr->xml->parsing_ctx->lastError.message), - msr->xml->parsing_ctx->lastError.line, - msr->xml->parsing_ctx->lastError.int2); + detail = apr_pstrdup(msr->mp, e->message); + len = strlen(detail); + while (len > 0 && isspace((unsigned char)detail[len - 1])) { + detail[--len] = '\0'; + } - msr_log(msr, 5, "XML: Parsing error: %s", message); + return apr_psprintf(msr->mp, "XML: %s: %s (line %d, column %d)", + what, log_escape_nq(msr->mp, detail), e->line, e->int2); } -#endif + /** * Feed one chunk of data to the XML parser. @@ -273,7 +290,8 @@ int xml_process_chunk(modsec_rec *msr, const char *buf, unsigned int size, char msr->txcfg->parse_xml_into_args != MSC_XML_ARGS_ONLYARGS) { xmlParseChunk(msr->xml->parsing_ctx, buf, size, 0); if (msr->xml->parsing_ctx->wellFormed != 1) { - *error_msg = apr_psprintf(msr->mp, "XML: Failed to parse document."); + *error_msg = xml_parse_error(msr, msr->xml->parsing_ctx, + "Failed to parse document"); return -1; } } @@ -285,7 +303,8 @@ int xml_process_chunk(modsec_rec *msr, const char *buf, unsigned int size, char *error_msg = msr->xml->xml_error; } else { - *error_msg = apr_psprintf(msr->mp, "XML: Failed to parse document for ARGS."); + *error_msg = xml_parse_error(msr, msr->xml->parsing_ctx_arg, + "Failed to parse document for ARGS"); } return -1; } @@ -317,6 +336,10 @@ int xml_complete(modsec_rec *msr, char **error_msg) { /* Preserve the results for our reference. */ msr->xml->well_formed = msr->xml->parsing_ctx->wellFormed; msr->xml->doc = msr->xml->parsing_ctx->myDoc; + if (msr->xml->well_formed != 1) { + *error_msg = xml_parse_error(msr, msr->xml->parsing_ctx, + "Failed to parse document"); + } /* Clean up everything else. */ xmlFreeParserCtxt(msr->xml->parsing_ctx); @@ -324,7 +347,6 @@ int xml_complete(modsec_rec *msr, char **error_msg) { msr_log(msr, 4, "XML: Parsing complete (well_formed %u).", msr->xml->well_formed); if (msr->xml->well_formed != 1) { - *error_msg = apr_psprintf(msr->mp, "XML: Failed to parse document."); return -1; } } @@ -336,7 +358,8 @@ int xml_complete(modsec_rec *msr, char **error_msg) { *error_msg = msr->xml->xml_error; } else { - *error_msg = apr_psprintf(msr->mp, "XML: Failed to parse document for ARGS."); + *error_msg = xml_parse_error(msr, msr->xml->parsing_ctx_arg, + "Failed to parse document for ARGS"); } xmlFreeParserCtxt(msr->xml->parsing_ctx_arg); msr->xml->parsing_ctx_arg = NULL; diff --git a/tests/regression/target/10-variable-reqbody_error_msg-xml.t b/tests/regression/target/10-variable-reqbody_error_msg-xml.t new file mode 100644 index 000000000..6557164b5 --- /dev/null +++ b/tests/regression/target/10-variable-reqbody_error_msg-xml.t @@ -0,0 +1,73 @@ +### REQBODY_ERROR_MSG for XML parse failures (#3680) +# +# libxml2's own reason (with line and column) must reach the error message. +# The parser contexts also carry XML_PARSE_NOERROR, so libxml2's own +# "body.xml:N: parser error" lines must never show up in the error log +# (Apache's stderr); the -error checks below guard that. + +# Tag mismatch +{ + type => "target", + comment => "REQBODY_ERROR_MSG carries the libxml2 reason (tag mismatch)", + conf => qq( + SecRuleEngine On + SecRequestBodyAccess On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRule REQUEST_HEADERS:Content-Type "^text/xml\$" "id:500005, \\ + phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML" + ), + match_log => { + error => [ qr/XML parser error: XML: Failed to parse document: Opening and ending tag mismatch: book line \d+ and bookasdf \(line \d+, column \d+\)/, 1 ], + -error => [ qr/body\.xml:\d+: parser error/, 1 ], + debug => [ qr/XML: Parsing complete \(well_formed 0\)/, 1 ], + }, + match_response => { + status => qr/^500$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", + [ + "Content-Type" => "text/xml", + ], + normalize_raw_request_data( + q( + + + + Learning XML + Erik T. Ray + + + ), + ), + ), +}, + +# Extra content after the root element (the #3563 reproducer) +{ + type => "target", + comment => "REQBODY_ERROR_MSG carries the libxml2 reason (extra content)", + conf => qq( + SecRuleEngine On + SecRequestBodyAccess On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRule REQUEST_HEADERS:Content-Type "^text/xml\$" "id:500005, \\ + phase:1,t:none,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML" + ), + match_log => { + error => [ qr/XML parser error: XML: Failed to parse document: Extra content at the end of the document \(line \d+, column \d+\)/, 1 ], + -error => [ qr/body\.xml:\d+: parser error/, 1 ], + }, + match_response => { + status => qr/^500$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", + [ + "Content-Type" => "text/xml", + ], + " Testing modsec", + ), +}, From f6d6d5e9cf1d5d8648a90e9f90e8be0873f0e0d2 Mon Sep 17 00:00:00 2001 From: Felipe Zipitria Date: Tue, 13 Oct 2026 12:10:05 -0300 Subject: [PATCH 2/2] fix: set XML_PARSE_NOERROR on the XML body parser contexts Neither push parser context had XML_PARSE_NOERROR | XML_PARSE_NOWARNING, so libxml2 was free to print its own "body.xml:N: parser error" lines to stderr. Set them through xmlCtxtUseOptions(), which applies on every libxml2 version; OR-ing ctx->options directly is a no-op before 2.13 (see v3 #3563 / #3677). The reason is still reported through xmlCtxtGetLastError(). Note: under Apache 2.4 (worker MPM) with libxml2 2.9.14 no such stderr lines were observed even before this change, so for the Apache module this is parity with v3 rather than a measured fix. Part of #3680. v3 counterpart: #3677. Co-Authored-By: Claude Fable 5.1 --- apache2/msc_xml.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/apache2/msc_xml.c b/apache2/msc_xml.c index db9097850..75896572b 100644 --- a/apache2/msc_xml.c +++ b/apache2/msc_xml.c @@ -232,6 +232,15 @@ static char *xml_parse_error(modsec_rec *msr, xmlParserCtxtPtr ctx, const char * what, log_escape_nq(msr->mp, detail), e->line, e->int2); } +/** + * Keep libxml2 from writing parser errors to stderr on its own; the reason + * is reported through xml_parse_error() instead. Must go through + * xmlCtxtUseOptions(): OR-ing ctx->options directly is a no-op before + * libxml2 2.13 (see v3 #3563). + */ +static void xml_silence_parser(xmlParserCtxtPtr ctx) { + xmlCtxtUseOptions(ctx, XML_PARSE_NOWARNING | XML_PARSE_NOERROR); +} /** * Feed one chunk of data to the XML parser. @@ -269,6 +278,7 @@ int xml_process_chunk(modsec_rec *msr, const char *buf, unsigned int size, char *error_msg = apr_psprintf(msr->mp, "XML: Failed to create parsing context."); return -1; } + xml_silence_parser(msr->xml->parsing_ctx); } if (msr->txcfg->parse_xml_into_args != MSC_XML_ARGS_OFF) { msr->xml->parsing_ctx_arg = xmlCreatePushParserCtxt( @@ -281,6 +291,7 @@ int xml_process_chunk(modsec_rec *msr, const char *buf, unsigned int size, char *error_msg = apr_psprintf(msr->mp, "XML: Failed to create parsing context for ARGS."); return -1; } + xml_silence_parser(msr->xml->parsing_ctx_arg); } } else {