refactor(http-honeypot): one file per CVE, and pin the classifier order (#3464) - #3470
Merged
Merged
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
…er (#3464) main.go's classifyPayload switch and the helper block below it are the two regions every CVE classifier edits, so any two CVE branches in flight collide in both at once -- #3423/#3444, #3425/#3449 and #3442 have all hit it, and each resolution had to be re-derived by hand. The cases move out into classify_roundcube.go, classify_wordpress.go, classify_ollure.go, classify_odata.go and classify_generic.go, each holding its own case and its own helpers. What is left in classify.go is the ordered slice and a first-match-wins loop over it, so a new CVE is a new file and one line of a list rather than two regions of a 700-line switch. The order is semantic, so it is now asserted rather than trusted. TestPayloadClassOrderIsPinned compares the whole dispatch to a pinned list position by position, and a precedence table gives one payload per deliberate overlap -- a base64 dropper that is also php-code, a pagename traversal that is also a pearcmd chain, version.bind that is also a bare hostname, a new class that would land below serialized-object and never fire. Swapping any two entries fails a row. The same table adds the one payload per class, which is also the first coverage jndi-lookup, xxe, template-injection and info-disclosure have had through classifyPayload. TestClassifyPayloadCorpusCoversEveryDispatchClass is what keeps the file from going stale again. The pinned order and the two corpora are three separate lists and nothing forced a rebase that adds a class to update all three; this does. It is the failure the issue exists to prevent, in the shape it takes after a merge rather than before one. Rebased onto main, which grew the classifier while this branch was open, so what the rebase reconciled is part of the change rather than a footnote to it: - #3449 added wordpress-template-inclusion, CVE-2026-87902's second reading, and main puts it at position 4: after wordpress-pagename-traversal, which shares the CVE and whose labels must not move, and before pearcmd-rce and the generic cases below. It is a class, so it is a case in classify_wordpress.go next to the one it divides with, one line in the dispatch, one payload of its own and four overlaps in the precedence table -- the split-request PEAR argv it claims from pearcmd-rce, the credential read it claims from secret-read, the pagename traversal it must lose to, and the generic LFI it must not take at all. - #3449 also refactored the pagename case onto shared formValues and decodeUpTo helpers, because url.ParseQuery drops a pair containing a semicolon and `data://text/plain;base64,...` is one value at WordPress. The rewrite moved with the case into classify_wordpress.go. Left behind it would have been a stale copy of a helper main had already changed, which is a silent detection gap rather than a compile error. - The refactor had put teamcity-agent-deserialization first and roundcube-virtuser-query-sqli second; main has them the other way round. No payload currently matches both, so the transposition changed no label -- but it is still a change to a first-match-wins list, and the pinned order is main's order rather than the refactor's. No behaviour change. Every label is distinct, so the label is the identity of the case. The 34 cases, their order and every helper body are main's; the one rewrite a case body needs is a comma becoming an ||. Verified rather than asserted: a corpus of 167 payloads -- every (query, body) pair the package's own test tables contain, harvested with go/ast from all seven of them -- was replayed through the pre-split classifier at b15213e and through this tree, and the two label streams are identical row for row, all 34 classes included and none of them reached by a payload the other tree labelled differently.
Xore
force-pushed
the
oc/3464-classifier-split
branch
from
September 28, 2026 12:57
4dc2bbf to
c48fef9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
classifyPayload's switch and the helper block below it move out ofmain.gointo one file per CVE, and the order they run in becomes a pinned, asserted list instead of something a rebase can silently transpose.classify.goclassifyPayloadclassify_teamcity.goclassify_roundcube.goclassify_wordpress.goclassify_ollure.goclassify_odata.goclassify_generic.gomain.go: 2386 → 1265 lines, and the six lines it gains are comment fixes for references the move made stale.Adding a CVE is now a new file plus one line of a list, rather than two regions of a 700-line switch.
The order is the invariant, so the order is a test
The dispatch is first-match-wins and several classes overlap deliberately: a base64 dropper is also
php-code, a pagename traversal into pearcmd is also a pearcmd chain, a Roundcube login carrying SQL is also the genericsqliclass, a TeamCity agent poll carrying a Java stream is also a serialized object. Transposing two entries does not shuffle labels around — it relabels every payload both classes match, and the request says nothing about it.classify_order_3464_test.gohas three parts:TestPayloadClassOrderIsPinned— the whole 33-entry order, compared position by position against a pinned list. Reorder, insert or drop anything and this fails, whatever the payloads do.TestClassifyPayloadReachesOneClassPerPayload— one representative payload per class, plus the unlabelled tail. This is also the first coveragejndi-lookup,xxe,template-injectionandinfo-disclosurehave had throughclassifyPayload.TestClassifyPayloadFirstMatchWins— 20 payloads that each match at least two classes, asserted against the specific one. The table that fails if the order is wrong.The existing suite already covers the per-CVE boundaries and I did not restate it:
payload_test.go,roundcube_sqli_test.go,roundcube_coverage_3364_test.go,ollure_coverage_3394_test.go,odata_double_encode_test.goandscanner_laundering_3430_test.goall still run unmodified, andteamcity_deser_test.goalready pins the TeamCity-vs-serialized-objectboundary from both sides — every positive case in it is itself an ordering test. What was missing was the order itself, and the four classes with no coverage at all.No behaviour change, and how that was checked
Every label is distinct, so a label is the identity of a case, and "which case won" and "which label came back" are the same question.
scanner_laundering_3430_test.go, and three unlabelled shapes — was replayed through the unmodified classifier and recorded. After the split, the identical corpus: identical, row for row.var/constblock, normalised only for the parameter rename. All identical. The one rewrite a case body needs is a comma becoming an||, sincecase a, b:isa || boutside a switch.serialized-object— the exact trap feat(http-honeypot): classify CVE-2026-63077 TeamCity agent deserialization #3444 had to reason about by hand — kills the new test andteamcity_deser_test.go. So do swaps ofphp-code/php-base64-shell,command-injection/secret-read,sqli/odata-double-encode-probeanddns-version-probe/open-resolver-probe, each of which relabels real traffic and each of which fails a row of the precedence table.gofmt,go vetandgo test ./...are clean; 380 tests pass.Rebased over #3444
mainmoved to #3444 (TeamCity) while this was being written, so this is rebased onto it. That is the collision the issue describes, and the resolution is what the split is for:classify_teamcity.goholds #3444's case and all ten of its helpers, one line was added at the top of the dispatch, and the order list grew an entry. The TeamCity case's own 330 lines of rationale and 332 lines of tests came across untouched.One deviation from the issue
The issue proposed "a small dispatcher in
main.go". It is inclassify.goinstead, so that the region every CVE branch touches is a small file whose whole content is an ordered list, and somain.goshrinks rather than keeping a 33-line slice. The twoclassifyInput-shaped adapters (ollamaModelTargetCase,odataDoubleEncodeCase) exist only because those two helpers keep their plain signatures for existing tests to call directly.Refs #3464