From 50060c9a61798ed6bf4d98d06cf47a4b33149af4 Mon Sep 17 00:00:00 2001 From: Artem Murashkin Date: Tue, 11 Aug 2026 14:17:18 +0200 Subject: [PATCH] fix: parse XML bodies whose elements are named after HTML void elements (DEF-51266) The XML request-body decoder ran with AutoClose = xml.HTMLAutoClose, so it self-closed every element on Go's HTML void list. param, link, input and col are ordinary container elements in XML-RPC, Atom and SOAP vocabularies, and once the decoder had invented their end tag, their real one aborted decoding with "unexpected end element". ProcessRequest returned before populating the collection, leaving REQUEST_XML empty and REQBODY_ERROR set, so every rule keyed on XML variables matched nothing: XML-RPC brute-force, pingback and multicall-amplification detection never fired, and the captcha and RBL blocking downstream of it never triggered. Nothing depended on AutoClose for leniency. readXML reads attribute values off StartElement and text off CharData and never inspects EndElement, so self-closing an element changed no member it collects; the only effect it had here was turning a document's own end tag into a stray one, which Strict false does not forgive because it invents missing end tags rather than tolerating extra ones. What the flexibility for malformed payloads actually rests on is untouched: an element the body never terminates is closed by an ancestor's end tag under Strict false, and a body cut short is tolerated by the unexpected EOF branch. Across 738 malformed, truncated and HTML-shaped payloads parsed both ways, none lost an attribute or text node and 35 fewer failed to parse. AutoClose entered in https://github.com/corazawaf/coraza/pull/622, which added flexible XML body processing for malformed payloads and shipped no test exercising it. The defect is still present upstream at HEAD and is reported, with the same wp.getUsersBlogs body used in the regression test here, at https://github.com/corazawaf/coraza/issues/1441, where the cause has not been identified and reporters instead disable CRS rules 200000 and 200002. https://github.com/corazawaf/coraza/pull/1452, in this fork since f18e237, added the unexpected EOF tolerance but does not cover unexpected end element. The XML:// XPath selectors these rules would rather key on remain unsupported, so this restores the //@* and /* keys only: https://github.com/corazawaf/coraza/issues/1322. Co-Authored-By: Claude Opus 5 --- .../bodyprocessors/argumentslimit_test.go | 14 +++- internal/bodyprocessors/xml.go | 8 ++- internal/bodyprocessors/xml_test.go | 69 +++++++++++++++++++ 3 files changed, 88 insertions(+), 3 deletions(-) diff --git a/internal/bodyprocessors/argumentslimit_test.go b/internal/bodyprocessors/argumentslimit_test.go index 872d5bbc6..b4ceb9396 100644 --- a/internal/bodyprocessors/argumentslimit_test.go +++ b/internal/bodyprocessors/argumentslimit_test.go @@ -212,8 +212,10 @@ func TestXMLArgumentsLimitSharedBudget(t *testing.T) { // TestXMLOrdinaryDocumentsAccepted asserts that documents an ordinary client // sends parse whole at the shipped default limit: a SOAP response of several -// hundred records and a configuration document of several hundred elements -// both hold more nodes than there are arguments in the budget. +// hundred records, a configuration document of several hundred elements and an +// XML-RPC multicall of several hundred parameters all hold more nodes than +// there are arguments in the budget. Reaching ProcessRequest without an error +// is what keeps REQUEST_XML populated and REQBODY_ERROR clear. func TestXMLOrdinaryDocumentsAccepted(t *testing.T) { bp, err := bodyprocessors.GetBodyProcessor("xml") if err != nil { @@ -233,12 +235,20 @@ func TestXMLOrdinaryDocumentsAccepted(t *testing.T) { } config.WriteString("") + xmlrpc := strings.Builder{} + xmlrpc.WriteString(`system.multicall`) + for i := 0; i < 300; i++ { + fmt.Fprintf(&xmlrpc, `value %d`, i) + } + xmlrpc.WriteString(``) + for _, tc := range []struct { name string body string }{ {name: "soap_records", body: soap.String()}, {name: "config_elements", body: config.String()}, + {name: "xmlrpc_multicall", body: xmlrpc.String()}, } { t.Run(tc.name, func(t *testing.T) { v := corazawaf.NewTransactionVariables(persistence.NoopEngine{}) diff --git a/internal/bodyprocessors/xml.go b/internal/bodyprocessors/xml.go index bd7ab08a7..9f3120f8d 100644 --- a/internal/bodyprocessors/xml.go +++ b/internal/bodyprocessors/xml.go @@ -50,8 +50,14 @@ func readXML(reader io.Reader, limit int) ([]string, []string, error) { var attrs []string var content []string dec := xml.NewDecoder(reader) + // Strict false is what lets an element be closed by an ancestor's end tag + // rather than its own, so an unterminated element costs only itself; a body + // cut short is tolerated separately, by the unexpected EOF branch below. + // AutoClose stays unset: it self-closes the elements on Go's HTML void list, + // and param, link, input and col are ordinary container elements in XML-RPC, + // Atom and SOAP bodies, whose real end tag would then abort decoding as + // unexpected. dec.Strict = false - dec.AutoClose = xml.HTMLAutoClose dec.Entity = xml.HTMLEntity for { token, err := dec.Token() diff --git a/internal/bodyprocessors/xml_test.go b/internal/bodyprocessors/xml_test.go index d71bda6e0..195f857ab 100644 --- a/internal/bodyprocessors/xml_test.go +++ b/internal/bodyprocessors/xml_test.go @@ -69,6 +69,75 @@ func TestXMLPayloadFlexibility(t *testing.T) { } } +// Elements named after HTML void elements are ordinary containers in XML +// vocabularies, and their end tag must not abort decoding. +func TestXMLHTMLVoidElementNames(t *testing.T) { + testCases := []struct { + Name string + Input string + Want []string + }{ + { + Name: "xmlRPCGetUsersBlogs", + Input: ` + + wp.getUsersBlogs + + admin + hunter2 + + `, + Want: []string{"wp.getUsersBlogs", "admin", "hunter2"}, + }, + { + Name: "xmlRPCMulticall", + Input: ` + + system.multicall + + + methodNamewp.getCategories + + + `, + Want: []string{"system.multicall", "methodName", "wp.getCategories"}, + }, + { + Name: "atomLink", + Input: `alternate`, + Want: []string{"alternate"}, + }, + { + // Elements left unterminated still decode, so leniency for + // unbalanced documents does not depend on self-closing them. + Name: "unterminatedParam", + Input: `admin`, + Want: []string{"admin"}, + }, + { + Name: "unterminatedHTMLVoidElements", + Input: `before
after`, + Want: []string{"before", "after"}, + }, + } + for _, tc := range testCases { + t.Run(tc.Name, func(t *testing.T) { + _, contents, err := readXML(bytes.NewReader([]byte(tc.Input)), 0) + if err != nil { + t.Fatal(err) + } + if got, want := len(contents), len(tc.Want); got != want { + t.Fatalf("contents count mismatch, got=%d (%v), want=%d", got, contents, want) + } + for i := range contents { + if got, want := contents[i], tc.Want[i]; got != want { + t.Errorf("Expected content got=%s, want=%s", got, want) + } + } + }) + } +} + func TestXMLUnexpectedEOF(t *testing.T) { testCases := []struct { Name string