Fix UDP payload decoding: probes with literal text go out malformed - #2
Merged
Merged
Conversation
parser() kept every ASCII hex digit in a payload literal and dropped everything else, with no notion of what was an escape and what was text. `\x06` survived by luck; `public` in the SNMPv1 GetRequest kept only its `b` and `c` and became the single byte 0xbc. The resulting probe is malformed -- its BER header declares an octet string of length 6 and a message length of 31, but 26 bytes go out -- so agents discard it without replying and the port reads as closed. Every payload written as literal text is affected: udp/137, 389, 427, 1900, 3283, 11211 and 626 can never elicit a response. Track quoted regions, decode the escapes, and take every other character as the byte it denotes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
The bug
build.rsturnsnmap-payloadsinto the payload table compiled into the binary. Itsparser()keeps every ASCII hex digit and discards everything else:It has no notion of what is an escape and what is literal text.
\x06survives by luck — the backslash is skipped,xis not a hex digit,06parses. But payloads are not all hex. The SNMPv1 GetRequest spells its community string out:publickeeps only itsbandcand becomes the single byte0xbc:The BER header still declares an octet string of length 6 and a message length of 31, but 26 bytes go out. tcpdump labels our own packet
[len26<asnlen31]. Agents discard it without replying, so the port reads as closed.Impact
Every payload written as literal text is affected. Ports where all registered variants are corrupt, i.e. never detectable over UDP:
CKAAAAAAAA…objectClassservice:service-agentM-SEARCH * HTTP/1.1version161has two variants — the SNMPv3 one is pure hex and fine, but both share the key[161,260,3401]in aBTreeMap, so the later (corrupt) entry overwrites the good one.53also has a corrupt variant but a working one survives.Ports whose payloads are pure hex —
67,111,123,500,623,3478— were never affected.This is not specific to our fork: upstream
masterhas a byte-identicalbuild.rs. Introduced in45fa80ac"Statically generated payloads (bee-san#643)", 2024-09-13.The fix
Track quoted regions, decode
\xNNand the usual C escapes, and take every other character as the byte it denotes. Whitespace and the quotes separating concatenated strings are structure, not payload, so they are skipped.Verification
Lab: net-snmp 5.9.4 (v1/v2c, community
public) and a scanner image built from this branch, on one Docker network. Identical flags to production.After the fix the agent answers, carrying the sysDescr our fingerprinter reads:
cargo test— 70 passed, 0 failed, 8 ignored.rustfmt --checkclean on both changed files.Tests
tests/udp_payloads.rsasserts againstgenerated::get_parsed_data(), so it exercises the real file → parser → table path rather than a copy of the logic:public\xNNto single bytes, and no payload retained a literal\x— the opposite regressionWithout these, this regresses silently: a mis-decoded payload looks exactly like a closed port, which is how it went unnoticed for two years.
Not fixed here
The duplicate-key collapse.
port_payload_mapkeys on the port vector, so ports with several payload variants keep only the last — nmap tries each in turn. Worth a follow-up; upstream PR bee-san#886 is rewriting that lookup.🤖 Generated with Claude Code