From 1c47d70247d36746dc61a8f65fd0ed938f73e356 Mon Sep 17 00:00:00 2001 From: Xore Date: Tue, 29 Sep 2026 09:19:17 +0200 Subject: [PATCH 1/2] docs(#3364): record CVE-2026-48842 coverage, and fix the semicolon evasion under it The research document the issue asked for, and a root-cause parser fix that came out of writing it. The detector itself is not new: #3420 added the class, #3441 closed its case-folding gap, #3464 gave it a file. The issue's STATUS block ("implementation PRs are OPEN") is stale. docs/research/3364-roundcube-sqli.md documents the CVE as implemented, citing file:line, and carries forward the honest caveat from the source: coverage is NOT measured against the fleet corpus, because the honeypot-v2-* indices are not reachable from a branch. What is measured is #1888's pinned 30-day fixture, mirrored in roundcube_coverage_3364_test.go: 0 claims, 9/9 published shapes. No CVSS, build, or date is asserted beyond what the issue body and the class's own comment carry; no Kibana field names are invented. THE FIX. Four cases parsed parameters with url.ParseQuery behind if err != nil && len(values) == 0 { continue } Since Go 1.17 that parser rejects and drops any pair containing a semicolon, returning a map of the pairs that had none -- so the guard does not fire, parsing continues on a truncated map, and the attacker's parameter is simply absent: q="_user=x&;_action=login%27+OR+1%3D1--" err=invalid semicolon separator in query url.Values{"_user":[]string{"x"}} The targets are PHP, whose only separator is "&", so the payload reaches Roundcube in full while the sensor cannot see it. The guard's other silent case is the same bug: a value that will not decode makes ParseQuery return an empty map, the guard fires, and a deliberately broken escape (%zz) deletes the sensor's own evidence. The parser this needs already exists. classify_wordpress.go's formValues (#3449) splits on &, then the first =, unescapes each side, and keeps an undecodable side as-is rather than dropping it. All four call sites now use it -- classify_roundcube.go, classify_odata.go, classify_teamcity.go, laundering.go -- so this is one root cause retired rather than four bugs left. No new parser, no new abstraction, no new file; no deps, env vars, routes or ports. Surrounding logic and bounds are unchanged, and the 64 KiB body cap upstream of ServeHTTP still holds. BEHAVIOUR CHANGE, stated: a `;` in a value no longer makes the parameter invisible, and an undecodable value is no longer discarded. No false-positive surface is widened where it matters -- `;_action` is still not `_action`, `;$filter` is still not an OData system option, a semicolon with no payload behind it stays unlabelled, and qualifyingODataRequest's value-consistency gate is untouched and still runs on the value it is shown. One existing expectation changed: `$filter=Year%zz%2520eq` was unlabelled and is now odata-double-encode-probe. The old answer was url.ParseQuery deleting the pair on the %zz, not the gate declining -- the value carries a real %25, which is a % that decodes twice, and the target reads `Year%zz%20eq`. The rule is unchanged (a malformed escape is still not a residual escape, now pinned separately on a value whose only escape is broken); which requests reach the rule did change. Justified at the pin in odata_double_encode_test.go and in scanner_laundering_3430_test.go. classify_order_3464_test.go, the deliberate dispatch-order gate, passes unmodified and is not in this diff. PROOF. form_values_semicolon_3364_test.go fails on unmodified main (7 failing subtests, recorded before any source change) and passes after. It drives classifyPayload, qualifyingODataRequest and the real two-request laundering state, plus one ServeHTTP end-to-end for the event. Mutation-checked by watching each go red: dropping formUnescape's undecodable-side case (2 tests), re-introducing ';' as a separator in formValues (5 tests, including a pre-existing #3447 one), and reverting each of the three productive call sites individually. The teamcity site is pinned as gaining nothing, measured rather than assumed: its call name is a whole-string test, so the target's parser reaches the same verdict. Baseline -> final, same package: 129 top-level / 293 subtests -> 134 / 311, 0 fail, 0 skip, no new xfail, no weakened assertion. gofmt -l clean, go vet clean. tests/docs: 659 passed, 1 xfailed, 17 subtests, before and after. --- .../http-honeypot/classify_odata.go | 21 +- .../http-honeypot/classify_roundcube.go | 20 +- .../http-honeypot/classify_teamcity.go | 29 +- .../form_values_semicolon_3364_test.go | 320 ++++++++++++++++++ .../honeypot-http/http-honeypot/laundering.go | 15 +- .../http-honeypot/odata_double_encode_test.go | 20 +- .../scanner_laundering_3430_test.go | 6 +- docs/research/3364-roundcube-sqli.md | 248 ++++++++++++++ 8 files changed, 640 insertions(+), 39 deletions(-) create mode 100644 arcane/home/honeypot-http/http-honeypot/form_values_semicolon_3364_test.go create mode 100644 docs/research/3364-roundcube-sqli.md diff --git a/arcane/home/honeypot-http/http-honeypot/classify_odata.go b/arcane/home/honeypot-http/http-honeypot/classify_odata.go index b2561f87..7d476cfb 100644 --- a/arcane/home/honeypot-http/http-honeypot/classify_odata.go +++ b/arcane/home/honeypot-http/http-honeypot/classify_odata.go @@ -6,10 +6,7 @@ package main // state exists, which is why it is a case in the dispatch and not a hook // behind it. -import ( - "net/url" - "strings" -) +import "strings" // odataDoubleEncode reports an OData system query option -- $select, $filter, // $top and the rest -- whose key or value still carries a percent-escape @@ -47,10 +44,14 @@ func odataDoubleEncode(query, body string) bool { "$format": true, "$search": true, "$skiptoken": true, "$index": true, } for _, raw := range []string{query, body} { - values, err := url.ParseQuery(raw) - if err != nil && len(values) == 0 { - continue - } + // formValues, not url.ParseQuery, for the reason + // roundcubeVirtuserSQLi gives at its own call site: a `;` inside + // a pair makes url.ParseQuery drop that pair, so an option whose + // own value carries one was invisible to this gate while the + // target parsed it. `;` is also what a Java/ASP.NET-style + // client sends when it is being careless, and a request nobody + // sends is not evidence. + values := formValues(raw) for key, vals := range values { // OData allows a namespace alias prefix, so compare the last // dotted part: `northwind.$filter` is the same option. @@ -75,8 +76,8 @@ func odataDoubleEncode(query, body string) bool { } // odataDoubleEncodeCase is the dispatch's entry for odataDoubleEncode. The -// raw query and body go in, not the lowercased pair: url.ParseQuery does its -// own decoding, and the residue this class is looking for is destroyed by a +// raw query and body go in, not the lowercased pair: formValues does its own +// decoding, and the residue this class is looking for is destroyed by a // decode that happens before it. func odataDoubleEncodeCase(c classifyInput) bool { return odataDoubleEncode(c.Query, c.Body) diff --git a/arcane/home/honeypot-http/http-honeypot/classify_roundcube.go b/arcane/home/honeypot-http/http-honeypot/classify_roundcube.go index 02ee1186..f3d359c5 100644 --- a/arcane/home/honeypot-http/http-honeypot/classify_roundcube.go +++ b/arcane/home/honeypot-http/http-honeypot/classify_roundcube.go @@ -4,10 +4,7 @@ package main // change to it touches one new-ish file and one line of the dispatch in // classify.go, and nothing else in this package. -import ( - "net/url" - "strings" -) +import "strings" // roundcubeVirtuserSQLi reports a pre-authentication SQL injection aimed at // Roundcube Webmail's virtuser_query plugin (CVE-2026-48842). CVSS 8.1; the @@ -43,8 +40,8 @@ import ( // wordpressPagenameTraversal gives: the text "virtuser_query" inside some // other value must not trigger this, and neither must a payload that happens // to contain "_task". Nothing is deserialized and nothing is evaluated -- -// url.ParseQuery splits a string into key/value pairs, and every value is -// then matched as bytes. +// formValues splits a string into key/value pairs the way PHP will split it, +// and every value is then matched as bytes. // // Its place in the dispatch is first, ahead of the generic sqli class, // because a Roundcube probe is often both at once -- the same value is caught @@ -65,10 +62,13 @@ import ( // a fixed number of linear passes over bytes already in memory. func roundcubeVirtuserSQLi(c classifyInput) bool { for _, raw := range []string{c.Query, c.Body} { - values, err := url.ParseQuery(raw) - if err != nil && len(values) == 0 { - continue - } + // formValues, not url.ParseQuery: the target is PHP, whose only + // separator is "&", so a `;` inside a value is data that reaches + // the plugin intact -- while url.ParseQuery rejects the pair + // carrying it and hands back a map with the parameter missing. + // The payload would arrive at Roundcube and never reach this + // case. See formValues, and form_values_semicolon_3364_test.go. + values := formValues(raw) roundcube := false for key, vals := range values { // The dispatch parameters, and the plugin named as a key. diff --git a/arcane/home/honeypot-http/http-honeypot/classify_teamcity.go b/arcane/home/honeypot-http/http-honeypot/classify_teamcity.go index 4ab0a9e1..240be0ca 100644 --- a/arcane/home/honeypot-http/http-honeypot/classify_teamcity.go +++ b/arcane/home/honeypot-http/http-honeypot/classify_teamcity.go @@ -5,10 +5,7 @@ package main // ordering decision #3444 had to make by hand and the one this file's own // tests now pin from both sides. -import ( - "net/url" - "strings" -) +import "strings" // teamcityAgentDeserialization reports a request aimed at a Java // deserialization sink through TeamCity's build-agent polling protocol @@ -236,7 +233,7 @@ var teamcityProtocolMarkers = []string{ // xmlrpc/allowRegistrationAndPing, and a call name sitting inside somebody // else's parameter value, both keep their own answers. // -// Nothing is parsed in order to answer this. url.ParseQuery splits a string +// Nothing is parsed in order to answer this. formValues splits a string // into key/value pairs and the element extractor is two index searches; // neither is told what to do with what it found, and neither can fail in a // way that changes the answer. Parsing a request to learn what it asked for @@ -250,12 +247,22 @@ func teamcityAgentProtocol(lowerQuery, lowerBody string) bool { for _, channel := range []string{lowerQuery, lowerBody} { // A body that is not a parameter list at all parses into junk keys // and values, which is harmless: the key test below rejects them. - // An error alongside real values is tolerated for the same reason - // wordpressPagenameTraversal tolerates it. - values, err := url.ParseQuery(channel) - if err != nil && len(values) == 0 { - continue - } + // formValues, for the reason the other three call sites give: a + // `;` inside a pair must not delete the pair. + // + // This site is the exception on the evidence, and the exception is + // in the call name, not the parser. teamcityAgentCall compares a + // value to a fixed list as a whole string, so a `;` in the value + // makes it a different value rather than the same one the parser + // dropped, and a `;` in front of the key makes it a different key + // rather than the parameter it resembles. There is no semicolon + // payload this class can be shown to gain here: the target's parser + // reaches the same verdict. The swap is for consistency with the + // other three sites and to retire one root cause rather than four, + // and it is pinned as gaining nothing in + // form_values_semicolon_3364_test.go so that "nothing to gain" is + // a measured claim rather than an assumption. + values := formValues(channel) for key, vals := range values { if key != "methodname" && key != "method" { continue diff --git a/arcane/home/honeypot-http/http-honeypot/form_values_semicolon_3364_test.go b/arcane/home/honeypot-http/http-honeypot/form_values_semicolon_3364_test.go new file mode 100644 index 00000000..e5524bbc --- /dev/null +++ b/arcane/home/honeypot-http/http-honeypot/form_values_semicolon_3364_test.go @@ -0,0 +1,320 @@ +package main + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// The semicolon root cause, and the four sites that carried it. +// +// #3364's research asked what the fleet can see of CVE-2026-48842, and this +// file is the answer to the part of that question which was still open when the +// answer was written: whether the class sees the payload when the attacker +// puts a `;` in it. +// +// Four cases parse their parameters with url.ParseQuery behind this guard: +// +// if err != nil && len(values) == 0 { continue } +// +// Since Go 1.17 url.ParseQuery rejects every pair containing a semicolon and +// drops it, and returns a non-nil map of the pairs that had none. So the guard +// does not fire -- there are values left -- and the case keeps reading the +// truncated map, in which the attacker's own pair is simply absent. The one +// thing the guard was written to tolerate, a body that is not a parameter list +// at all, is what makes the miss invisible: there is no error to notice. +// +// That matters because the targets are PHP applications, and PHP's only query +// separator is "&". A `;` in the value is ordinary data that arrives at the +// target intact, while the sensor drops the parameter that carried it. The +// bytes the target will parse are not the bytes this sensor parsed, and the +// CVE works by making the target decode a value: so the classification has to +// be done on the bytes the target will see. classify_wordpress.go's formValues +// is that parser, added by #3449 for the same reason in both WordPress cases; +// this file is the proof the other three sites needed and did not have. +// +// Two shapes hide a payload, and both are ordinary to send: +// +// - a literal `;` inside a value. It is a sub-delim, legal in a query +// unencoded, and it costs the attacker nothing. +// - a deliberately broken escape earlier in the same value +// (`%zzadmin' or 1=1--`), which makes the whole side undecodable. A +// parser that discards an undecodable value has just been handed a way to +// delete its own evidence; the target keeps the raw bytes. +// +// Every positive below fails on main and is asserted through the real +// entry point (classifyPayload, or the laundering state), not through a copy of +// the logic. The negatives are the other half: the same semicolon and the same +// broken escape, with no payload behind them, must stay unlabelled -- a +// parser that sees more is only useful if it still requires the payload. + +// TestSemicolonAndBrokenEscapeDoNotHideAPayload is the table, per class, and +// the whole argument in one place. +func TestSemicolonAndBrokenEscapeDoNotHideAPayload(t *testing.T) { + cases := []struct { + name, query, body, want string + }{ + // --- #3364, roundcube-virtuser-query-sqli. The CVE's root cause + // is a `preg_replace()` escape defeated from inside the value, so + // the payload is a backslash-quote plus SQL on one of Roundcube's + // own dispatch parameters. The `;` goes inside the injected value: + // the target's parser keeps it, and a pair-wise parser that + // rejects semicolons cannot see the parameter at all. + { + name: "roundcube: semicolon inside the injected value, form body", + body: "_task=login&_action=login&_timezone=Europe%2FBerlin&_user=admin%5C%27;or+1%3D1--&_pass=Summer2026", + want: "roundcube-virtuser-query-sqli", + }, + { + // The same request as a query string, which is the GET half. + // A pre-auth SQLi is not a POST-only event. + name: "roundcube: semicolon inside the injected value, query string", + query: "_task=login&_action=login&_user=admin%5C%27;or+1%3D1--", + want: "roundcube-virtuser-query-sqli", + }, + { + // The broken-escape half. The value as a whole will not + // decode, so a parser that drops undecodable values drops + // the payload with them -- while `or 1=1--` sits in it in + // cleartext, which is what the target reads. + name: "roundcube: broken escape ahead of a cleartext payload", + body: "_task=login&_action=login&_user=%zzadmin' or 1=1--", + want: "roundcube-virtuser-query-sqli", + }, + { + // Not the CVE's root cause, but the same class and the same + // transport: no backslash at all, quote closed and the rest + // appended. A deployment that does not escape the value is + // still pre-auth SQLi against the same plugin. + name: "roundcube: unescaped quote, semicolon in the value", + body: "_task=login&_action=login&_user=admin';or+1%3D1--", + want: "roundcube-virtuser-query-sqli", + }, + + // --- #3430, odata-double-encode-probe. The gate is a real OData + // system option whose key or value still holds a %XX escape after + // one decode, so the `;` rides on the option's own pair. + { + name: "odata: semicolon on the option's own pair", + query: `$filter=Year%2520eq%25202026;$top=10`, + want: "odata-double-encode-probe", + }, + { + name: "odata: semicolon on an aliased option's own pair", + query: `northwind.$select=Year%252cValue;$top=1`, + want: "odata-double-encode-probe", + }, + { + name: "odata: semicolon on the option pair in a form body", + body: `$top=10&$filter=Year%2520eq%25202026;$format=json`, + want: "odata-double-encode-probe", + }, + + // --- negatives. Each is the same transport trick with no payload + // behind it, and each is traffic the fleet sees: a semicolon in a + // value is a `Content-Type` parameter, a matrix parameter and a + // very common typo. Requiring the payload is what keeps the wider + // parser from being a wider false-positive machine. + { + name: "roundcube login, semicolon in a value, nothing injected", + body: "_task=login&_action=login&_user=alice.smith%40example.com;x=1&_pass=Summer2026", + want: "", + }, + { + name: "roundcube login, broken escape, nothing injected", + body: "_task=login&_action=login&_user=%zzalice.smith%40example.com", + want: "", + }, + { + // A semicolon in front of a dispatch parameter is not a + // dispatch parameter. The target's parser does not strip it + // either, so the pair is not the parameter it looks like -- + // and the gate must not open on the resemblance. + name: "semicolon-prefixed roundcube key is not a dispatch parameter", + query: ";_action=login&_user=admin%5C%27+or+1%3D1--", + want: "", + }, + { + // The other half of that: the payload is visible and the gate + // is not open, so it stays somebody else's class or nobody's. + // This is the #3364 boundary the class's own table pins, and a + // semicolon must not quietly become a way around it. + name: "payload behind a semicolon with no roundcube shape", + query: ";_user=admin%5C%27+or+1%3D1--", + want: "", + }, + { + name: "odata option with a semicolon but no residual escape", + query: `$filter=Year%20eq%202026;$top=10`, + want: "", + }, + { + name: "semicolon-prefixed odata key is not a system option", + query: `;$filter=Year%2520eq%25202026`, + want: "", + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := classifyPayload(c.query, c.body); got != c.want { + t.Errorf("classifyPayload(%q, %q) = %q, want %q", c.query, c.body, got, c.want) + } + }) + } +} + +// TestTeamcityCallNameSurvivesTheParserSwap is the honest result for +// teamcity-agent-deserialization, and it is not a positive. +// +// The class asks whether a parameter value is a whole, named TeamCity call +// (`teamcityAgentCall`, an equality test against a fixed list). A `;` inside +// the value breaks the equality, and a `;` in front of the key breaks the +// key, so there is no semicolon payload this class can be shown to gain: for +// this site the parser swap is consistency, not detection. +// +// It is pinned anyway, in both directions, because "gains nothing" is only +// worth believing while the case still works and still refuses the shape that +// looks like it. If a future widening of the call-name test made a semicolon +// meaningful here, this table is where that has to show up. +func TestTeamcityCallNameSurvivesTheParserSwap(t *testing.T) { + const want = "teamcity-agent-deserialization" + // Both halves a real probe carries: a serialization container, and the + // agent protocol. The container is a gadget class name, so every row + // below is parsed rather than refused for want of the second half -- + // an unclaimed call name has to be unclaimed because the value is not + // one, not because the request was missing something else. + const container = "com.ysoserial" + cases := []struct { + name, query, body, want string + }{ + { + name: "the plain form still claims it", + query: "methodName=xmlrpc/allowRegistration&x=" + container, + want: want, + }, + { + // A wider parser cannot rescue this one either, and the + // reason is the target's: the value at the far end is + // `xmlrpc/allowRegistration;x=1` whichever parser read it, + // and that is not a call name. Refusing it is the correct + // answer, not a gap. + name: "semicolon inside the value is not that call name", + query: "methodName=xmlrpc/allowRegistration;x=1&y=" + container, + want: "", + }, + { + name: "semicolon-prefixed key is not the method parameter", + query: ";methodName=xmlrpc/allowRegistration&x=" + container, + want: "", + }, + { + name: "broken escape makes the value something else", + query: "methodName=%zzxmlrpc/allowRegistration&x=" + container, + want: "", + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := classifyPayload(c.query, c.body); got != c.want { + t.Errorf("classifyPayload(%q, %q) = %q, want %q", c.query, c.body, got, c.want) + } + }) + } +} + +// TestQualifyingODataRequestReadsSemicolonSeparatedPairs is the fourth site, +// which is not a dispatch class: #3447's layer C asks whether a request is a +// well-formed OData system query, and uses that as one half of a pair. +// +// The `;` is on the option's own pair, so the value the consistency gate sees +// carries a trailing `;$top=1` that a pair-wise parser would have thrown away +// with the option. The gate still has to do its job on what it is shown, which +// is what the negatives below check. +func TestQualifyingODataRequestReadsSemicolonSeparatedPairs(t *testing.T) { + const path = "/odata/Products" + yes := []string{ + `$filter=Year%20eq%202026;$top=1`, + `$filter=Year%20eq%20'Widget';$skip=10`, + `northwind.$filter=Year%20eq%202026;$top=1`, + `$filter=startswith(Name,'a.b.c');$top=1`, + } + for _, q := range yes { + if !qualifyingODataRequest(path, q, "") { + t.Errorf("qualifyingODataRequest(%q, %q) = false, want true", path, q) + } + } + no := []string{ + // The value-consistency gate, still enforced on the value it is + // shown. `$top` wants a bounded integer and gets a fragment. + `$top=10;$filter=Year%20eq%202026`, + `$format=yaml;$top=1`, + // The same gate on an identifier list: the fragment is not an + // identifier, so `$select` does not get to count a semicolon as + // part of the field name. A wider parser must not widen this. + `$select=Year,Value;$format=json`, + // A semicolon in front of the option is not an option. + `;$filter=Year%20eq%202026`, + `;$top=10`, + } + for _, q := range no { + if qualifyingODataRequest(path, q, "") { + t.Errorf("qualifyingODataRequest(%q, %q) = true, want false", path, q) + } + } +} + +// TestLayerCSeesTheODataHalfThroughASemicolon is that half reaching the class +// it exists for, through the real two-request path. The first request is +// unlabelled whatever happens -- the pair is what fires -- so this asserts the +// seed as well as the completion: if the semicolon hides the option, the entry +// is never created and the bypass that follows is unlabelled forever. +func TestLayerCSeesTheODataHalfThroughASemicolon(t *testing.T) { + const fp = "scanner-fingerprint" + d := newLaunderingState(launderMaxEntries, launderPerFingerprint, launderTTL) + + if got := d.observe(fp, "/odata/Products", `$filter=Year%20eq%202026;$top=1`, ""); got != "" { + t.Fatalf("the OData probe alone was labelled %q; the pair is what fires", got) + } + if got := d.observe(fp, "/odata/%2561", "", ""); got != launderingClass { + t.Errorf("observe(path-borne bypass after a semicolon-separated option) = %q, want %q", got, launderingClass) + } +} + +// TestRoundcubeSemicolonPayloadReachesTheEvent is the half a table of +// classifyPayload results cannot cover: that the class lands on the emitted +// event, with the injected value still readable in it. +// +// Reuse of a wider parser must not cost the fleet the payload bytes. #3213's +// rule is that redaction may not remove a signature, and the form redaction +// pass runs over this body because it is a form -- so the value beside +// `_pass` has to survive into the stored event or the analyst has a class with +// no evidence in it. +func TestRoundcubeSemicolonPayloadReachesTheEvent(t *testing.T) { + s, output := newTestServer() + + const body = "_task=login&_action=login&_timezone=Europe%2FBerlin&_user=admin%5C%27;or+1%3D1--&_pass=Summer2026" + + r := httptest.NewRequest(http.MethodPost, + "http://example/?_task=login&_action=login", strings.NewReader(body)) + r.Header.Set("Content-Type", "application/x-www-form-urlencoded") + r.RemoteAddr = "203.0.113.7:54321" + w := httptest.NewRecorder() + s.ServeHTTP(w, r) + + line := output.String() + if !strings.Contains(line, `"payload_class":"roundcube-virtuser-query-sqli"`) { + t.Fatalf("the event did not carry the payload class: %s", line) + } + if !strings.Contains(line, "_user=admin%5C%27;or+1%3D1--") { + t.Fatalf("the injected value was scrubbed out of the stored body: %s", line) + } + if strings.Contains(line, "Summer2026") { + t.Fatalf("the password was stored in the event: %s", line) + } + // Still the pre-auth shape: nothing in serve() decided an identity. + if !strings.Contains(line, `"auth_outcome":"unknown"`) { + t.Fatalf("expected no authentication decision for this request: %s", line) + } +} diff --git a/arcane/home/honeypot-http/http-honeypot/laundering.go b/arcane/home/honeypot-http/http-honeypot/laundering.go index 4d283144..659e1605 100644 --- a/arcane/home/honeypot-http/http-honeypot/laundering.go +++ b/arcane/home/honeypot-http/http-honeypot/laundering.go @@ -82,7 +82,6 @@ import ( "encoding/hex" "fmt" "net/http" - "net/url" "os" "sort" "strings" @@ -742,10 +741,16 @@ func qualifyingODataRequest(decodedPath, query, body string) bool { return false } for _, raw := range []string{query, body} { - values, err := url.ParseQuery(raw) - if err != nil && len(values) == 0 { - continue - } + // formValues, not url.ParseQuery, and the reason is the same one + // at the other three call sites: since Go 1.17 url.ParseQuery + // rejects and drops any pair containing a ";", so an option + // carrying one was not a half this layer could see and the bypass + // that followed it could not complete a pair. The + // value-consistency gate below is unchanged and still runs on the + // value it is shown, which is what keeps a wider parser from + // being a wider over-match -- see + // form_values_semicolon_3364_test.go. + values := formValues(raw) for key, vals := range values { name := strings.ToLower(key) if i := strings.LastIndexByte(name, '.'); i >= 0 { diff --git a/arcane/home/honeypot-http/http-honeypot/odata_double_encode_test.go b/arcane/home/honeypot-http/http-honeypot/odata_double_encode_test.go index 3f664804..5d8c5a5a 100644 --- a/arcane/home/honeypot-http/http-honeypot/odata_double_encode_test.go +++ b/arcane/home/honeypot-http/http-honeypot/odata_double_encode_test.go @@ -190,12 +190,28 @@ func TestODataDoubleEncodeProbe(t *testing.T) { // A malformed escape decodes to nothing, so it is not a residual // escape. A deliberately broken % sequence must not become a way // to hide the payload, but it is not evidence of a second decode - // either. + // either. Stated on a value whose only escape is broken, because + // that is what the rule is about -- see the next row for the + // distinction this used to blur. { name: "malformed escape is not a residual escape", - query: `$filter=Year%zz%2520eq`, + query: `$filter=Year%zz%2Gz`, want: "", }, + // The distinction, and it used to be blurred by which parser read + // the request. This value carries a broken escape AND a genuine + // `%25`, which is a `%` that decodes once to `%20` and once more + // to a space -- the double-decode signature, and the reason the + // option is this class in the first place. url.ParseQuery threw + // the whole pair away on the `%zz` and this was unlabelled; that + // was a parser deleting its own evidence, not the gate declining. + // The target reads `Year%zz%20eq` and decodes that to `Year eq`, + // so the class now says what happens to those bytes. #3364. + { + name: "a broken escape does not hide a real residual escape in the same value", + query: `$filter=Year%zz%2520eq`, + want: want, + }, // Ordinary traffic that happens to carry a percent sign. { name: "percent in an ordinary value", diff --git a/arcane/home/honeypot-http/http-honeypot/scanner_laundering_3430_test.go b/arcane/home/honeypot-http/http-honeypot/scanner_laundering_3430_test.go index 63eae4ea..85ad6f19 100644 --- a/arcane/home/honeypot-http/http-honeypot/scanner_laundering_3430_test.go +++ b/arcane/home/honeypot-http/http-honeypot/scanner_laundering_3430_test.go @@ -88,7 +88,11 @@ var publishedShapes3430 = []struct{ name, query, body, want string }{ {"residual escape with no OData option", `q=%2561%2562`, "", ""}, {"the relay-laundered request itself", `page=2&sort=name`, "", ""}, {"a self-submitting form body", "", `url=https%3A%2F%2F203.0.113.7%2Fdump&submit=go`, ""}, - {"malformed escape is not an escape", `$filter=Year%zz%2520eq`, "", ""}, + // A value whose only escape is broken. A deliberately broken % sequence + // is not evidence of a second decode, and this stays true after #3364's + // parser swap: formValues keeps an undecodable side rather than dropping + // it, and residualEscape still demands two hex digits behind the %. + {"malformed escape is not an escape", `$filter=Year%zz%2Gz`, "", ""}, } // ungatedOData3430 is #3430's layer-A signature with no gate: any query key diff --git a/docs/research/3364-roundcube-sqli.md b/docs/research/3364-roundcube-sqli.md new file mode 100644 index 00000000..9e97c30b --- /dev/null +++ b/docs/research/3364-roundcube-sqli.md @@ -0,0 +1,248 @@ +# #3364 — CVE-2026-48842: Roundcube pre-auth SQLi in `virtuser_query`, and what the http-honeypot classifier actually sees + +Scope: the APIARY `http-honeypot` payload classifier's coverage of +CVE-2026-48842, as implemented, plus the one evasion path that survived +implementation and the parser fix that closed it. Written 2026-09-29 against +`origin/main` at `43061f27`. + +The issue's STATUS block is stale. The detector is **shipped** — #3420 added +the class, #3441 closed a case-folding gap in it, #3464 moved it into its own +file. This document is the research deliverable #3364 asked for, not a new +detector. + +## Finding + +**CVE-2026-48842** is a pre-authentication SQL injection in Roundcube Webmail's +`virtuser_query` plugin. The value the plugin is handed is escaped with +`preg_replace()`, and a backslash inside the value defeats that escape: a +backslash immediately before the quote turns the escape into a second +backslash and the quote closes the string literal anyway. CVSS **8.1**. + +**Exploitation is confirmed in the wild, and the technique is not published.** +The Canadian Centre for Cyber Security's advisory **AV26-503 (2026-09-21)** +reports the CVE as exploited and has published no exploitation detail. The +shape below therefore comes from the advisory's description of the root cause +and from Roundcube's own dispatch, **not from a capture of this fleet being +attacked**. That distinction is load-bearing and is carried through every claim +below. + +What follows is the part of the CVE that reaches the sensor as bytes: Roundcube +addresses its own requests with a small set of dispatch parameters, and +`?_task=login` *is* the login form, so both the form and a plugin endpoint +(`?_action=plugin.`) are reachable with no session and no credential. +That is the pre-auth half of the CVE. + +## Why this matters for APIARY + +The fleet serves no webmail UI, and this is deliberate. A bait path for this +CVE would never match, because in a real attack the path is whatever endpoint +the *target's* dispatch resolves — the same reasoning #2919 and #3309 each +recorded for their own cases. So the class matches on the request's own +structure, and a genuine probe against a real Roundcube instance is a request +this sensor would see arriving on port 80/443 with no preceding session. + +The cost of matching on bytes is that the bytes have to be parsed the way the +*target* parses them, not the way Go does. That is where the evasion was, and +it is in the code because the class is in the code. + +## Coverage as implemented + +Class `roundcube-virtuser-query-sqli`, in +`arcane/home/honeypot-http/http-honeypot/classify_roundcube.go`, dispatched +first in `classify.go:73` — ahead of the generic `sqli` case, because a +Roundcube probe is often both at once and this class is the one that can say +which CVE it was and that no session was needed to reach it. + +It is a two-part test, and both parts are required because each alone is +ordinary traffic: + +1. **The gate** (`classify_roundcube.go:79`–`92`): one of Roundcube's dispatch + parameters present as a *key* (`_task`, `_action`, or the plugin named + outright as `virtuser_query`/`virtuser`), or the plugin named as a *value* + (`virtuser_query`, `plugin.virtuser_query`). Compared whole and + case-insensitively, never by substring. +2. **The payload** (`roundcubeSQLPayload`, `classify_roundcube.go:126`): a + value carrying a SQL metacharacter sequence — tautologies, `union select`, + `information_schema`, `sleep(`/`benchmark(`/`waitfor delay`, or + `extractvalue(`/`updatexml(` — **or** the CVE's own root-cause pair, + backslash-quote plus a metacharacter in the same value + (`pregReplaceEscapeBypass`, `classify_roundcube.go:162`). + +Both halves are matched case-insensitively; the generic `sqli` case is not, +which is exactly the gap #3441 closed after a measured uppercase `UNION SELECT` +went unlabelled on both GET and POST. + +**What the tests pin, and what they deliberately refuse.** The boundary cases +in `roundcube_sqli_test.go` are the ones that decide whether the class is +usable rather than noisy, because mail addresses contain apostrophes and this +classifier is the thing reading them: a real Roundcube login, an apostrophe in +a mail address (`o'brien@example.com`), a backslash with no SQL metacharacter +(a Windows path, a JSON escape, a regex), a bare quote with no metacharacter, +and plugin enumeration with no payload. SQLi with no Roundcube shape keeps +`sqli`, and a payload in a query string with no Roundcube shape stays +unlabelled. `classify_order_3464_test.go` additionally pins that this class +still beats `sqli` on the same bytes — a precedence the dispatch order owns. + +**The parser, and the fix this document records.** Until this branch, the class +parsed its parameters with `url.ParseQuery` behind a +`if err != nil && len(values) == 0 { continue }` guard. That guard is the +whole bug, and it is the *other* three classes' bug too. + +Since Go 1.17, `url.ParseQuery` **rejects and drops any pair containing a +semicolon**, and returns a non-nil map of the pairs that had none. So the +guard does not fire — there *are* values left — and the case keeps reading a +truncated map in which the attacker's own parameter is simply absent: + +``` +q="_user=x&;_action=login%27+OR+1%3D1--" + err=invalid semicolon separator in query + url.Values{"_user":[]string{"x"}} +``` + +`err != nil`, `len(values) == 1`. Parsing continues, and `_action=login' OR +1=1--` is invisible to the sensor. + +This is a real evasion path rather than a theoretical one, because **the +targeted application is PHP, whose only query separator is `&`**. The payload +reaches Roundcube in full while the sensor cannot see it. The same applies to +the guard's other silent case: a value that will not decode +(`%zzadmin' or 1=1--`) makes `url.ParseQuery` return an *empty* map, the guard +fires, and a deliberately broken escape deletes the sensor's own evidence. + +**The fix: reuse the parser this codebase already has.** `formValues` +(`classify_wordpress.go:50`), added by #3449 for exactly this reason in both +WordPress cases, splits on `&`, then on the first `=`, unescapes each side, and +**keeps an undecodable side as-is rather than dropping it**. No new parser, no +new abstraction, no new file. All four call sites now use it, so this is one +root cause retired rather than four bugs left. + +**New behaviour, stated plainly.** After the fix: + +- A Roundcube dispatch parameter whose value contains a literal `;` is now + read. `_task=login&_user=admin%5C%27;or+1%3D1--` is classified; before, the + `_user` pair was dropped and the request was unlabelled. +- A value with a deliberately broken escape is now read rather than discarded. +- `odata-double-encode-probe` (`classify_odata.go`) likewise now sees an OData + option whose own value carries a `;`, and now sees a genuine residual escape + that a broken escape ahead of it was hiding. This changed one existing + expectation, `$filter=Year%zz%2520eq`, from unlabelled to + `odata-double-encode-probe`; the justification is in + `odata_double_encode_test.go` at the pin. The *rule* did not change — a + malformed escape is still not a residual escape, pinned separately on a value + whose only escape is broken — but which requests reach the rule did, and they + were previously not reaching it because the parser had deleted them. +- **No false-positive surface was widened in the ways that matter.** A `;` in + front of a key is *not* that key: `;_action` is not a Roundcube dispatch + parameter, and `;$filter` is not an OData system option. A semicolon with no + payload behind it stays unlabelled, which is ordinary traffic — `;` is a + `Content-Type` parameter, a matrix parameter, and a very common typo. Every + one of these negatives is asserted, not asserted-in-a-comment. +- `qualifyingODataRequest` (`laundering.go:740`, #3447's layer C) now sees an + OData option separated by a `;`. Its value-consistency gate is unchanged and + still runs on the value it is shown, so a wider parser did not become a wider + over-match: `$top=10;$filter=Year%20eq%202026` still does not qualify, because + `$top` wants a bounded integer and gets a fragment. + +`classify_order_3464_test.go` — the deliberate dispatch-order regression gate — +**passes unmodified.** It is not in this branch's diff. + +## Known gaps + +- **Coverage against the fleet corpus is unmeasured, and #3364 marks it + unmeasured.** The live corpus (the `honeypot-v2-*` indices) is not reachable + from a branch. What is measured is the corpus that *is* on `main`: the pinned + 30-day real-traffic fixture from #1888, mirrored entry-for-entry in + `roundcube_coverage_3364_test.go`. Over it, the class claims **0** entries + and #3364's naive pre-gate signature claims **0**, both asserted rather than + logged; over the CVE's own published shapes it catches **9/9**. This is a + fixture, not the fleet, and it says nothing about recall against real + Roundcube exploitation. +- **No capture of this fleet being attacked exists.** Every positive follows + the advisory's description of the root cause plus Roundcube's own dispatch. + AV26-503 reports exploitation but publishes no technique, so a scanner that + exploits this CVE in a shape neither the advisory nor the product's dispatch + suggests would be missed. That gap is upstream's, not this classifier's, and + it is not closable from here. +- **A value-level `;` is now read; a key-level one is deliberately still not.** + `;_action=login&_user=…` is refused, because the target's parser does not + strip the `;` either — the parameter the target sees is not `_action`. The + class reports what the target would act on, not what the attacker meant. +- **Not reachable at all:** the issue's third proposed shape — a webmail/plugin + path segment from a source with no prior session in the correlation window. + This sensor keeps no session state and consults no backend, so it is + structurally unimplementable in-request, as #3447's own file argues for the + analogous question. +- **The gate is Roundcube-shaped, so a bare SQLi against a webmail client that + is not Roundcube is somebody else's class or nobody's.** Widening to cover it + would be a second generic detection mechanism rather than this CVE. +- **The `teamcity-agent-deserialization` call site was changed for consistency + and is measured to gain nothing.** Its call-name test compares a value to a + fixed list as a whole string, so a `;` makes a different value rather than + recovering a dropped one, and the target's parser reaches the same verdict. + That is pinned as a measured negative, not assumed — the value of the swap + there is retiring one root cause, not coverage. +- **No live detection rate is claimed.** Nothing was deployed; this change is + repo-only. An Arcane build + redeploy is needed before any of it can appear in + Elasticsearch, and the first real hit must be verified on a document indexed + after that deploy. +- **No traffic was sent anywhere.** All fixtures are inert strings in unit + tests. No exploitation, no docker, no Elasticsearch, no model load. + +## Severity + +**CVE severity: CVSS 8.1** (from the advisory, as recorded in the class's own +doc comment). Known exploited in the wild per AV26-503, 2026-09-21, with no +published technique. + +**APIARY exposure: none, by design.** This fleet does not run Roundcube, so +there is nothing to exploit. The value of the class is detection quality — +telling an analyst that a probe aimed at a pre-auth webmail SQLi arrived, and +distinguishing it from the ordinary mail-shaped traffic the classifier would +otherwise have to treat as an attack. The pre-fix gap was a false negative in +a decoy's detection surface, not a vulnerability in the decoy. + +## References + +- GitHub issue **#3364** — this research request; the source of the CVE id, + the "actively exploited" claim, and the honest note that the coverage gap is + unmeasured. Its STATUS block is stale and this document supersedes it. +- PRs **#3420** (the class), **#3441** (case-folding gap), **#3449** + (`formValues`, for the same root cause in the WordPress cases), **#3464** + (one file per CVE, dispatch order pinned). +- Canadian Centre for Cyber Security advisory **AV26-503, 2026-09-21** — cited + as the source for "exploited in the wild" and for the absence of published + exploitation detail. Cited here as recorded in + `classify_roundcube.go:14`; **not independently re-fetched for this + document**, so treat the advisory's own text as second-hand here. +- **#2919**, **#3309** — the "match the request's bytes, not a bait path" + reasoning this class follows, and **#1888** — the pinned 30-day real-traffic + corpus the measurement runs against. +- Code: `classify_roundcube.go`, `classify.go:73`, `classify_wordpress.go:50` + (`formValues`), `classify_odata.go`, `classify_teamcity.go`, + `laundering.go:740`; tests `roundcube_sqli_test.go`, + `roundcube_coverage_3364_test.go`, `classify_order_3464_test.go`, + `form_values_semicolon_3364_test.go`. + +No Kibana field names, dashboards, or index patterns are asserted anywhere in +this document: this change adds none, and the only field the classes write is +the existing `payload_class`. + +## Veracity note + +- **CVSS 8.1, the CVSS identifier, the exploitability claim and the advisory + id/date are transcribed from the issue body and from the class's own doc + comment, not from memory and not from a fresh fetch.** No build number, + fixed version, or affected-version range is claimed, because no source + consulted for this document states one. +- **No capture exists.** Every payload shape is derived from the advisory's + description of the root cause plus Roundcube's dispatch parameters. Nothing + here is presented as observed traffic. +- **The corpus numbers (0 claims, 9/9 published shapes) are measured against a + pinned fixture, re-runnable via `roundcube_coverage_3364_test.go`.** They are + not a fleet measurement and must not be quoted as one. #3364's own + "unmeasured" marking is carried forward deliberately. +- **The parser behaviour quoted in the Finding** was reproduced directly against + this toolchain (`go1.26.7`) before and after the fix, and the failing-test + run against unmodified `main` is recorded in the branch's commit history. +- **`teamcity-agent-deserialization` gaining nothing is a measured claim** from + `form_values_semicolon_3364_test.go`, not an inference from reading the code. From 70f926f2e6d2a75a9f8729f5c48a9b821329649d Mon Sep 17 00:00:00 2001 From: Xore Date: Tue, 29 Sep 2026 11:40:02 +0200 Subject: [PATCH 2/2] ci: retrigger after runner network failure