drsuapi: tighten V6 parser allocations, fix RDN unescape, drop dead helper#33
Merged
Merged
Conversation
…elper Three of the four follow-ups in #32 from the PR #31 review pass: 1. Replace the loose maxReasonable (10M element) ceiling with a precise per-call-site bounds check: every wire-driven make/Skip in the V6 parser now validates count * elemSize against d.Remaining() before allocating, so a malformed reply with a tiny payload and a huge embedded count can no longer trigger a multi-MB speculative make for bytes that don't actually exist on the wire. Product math is uint64 so it stays safe on 32-bit builds. CheckBounds lives on Decoder so other parsers can reach for the same primitive. 2. firstRDNValue now actually unescapes RFC 4514 backslash sequences via strings.Builder, so a DN like "CN=Smith\, John,OU=Foo" extracts to "Smith, John" rather than "Smith\, John". Trailing-backslash (malformed) drops silently. Table-driven tests cover the common cases. 3. Remove the dead writePartialAttrSet helper. writeGetNCChangesRequestV8 has always written a NULL referent for pPartialAttrSet, so the function was never reached. Item #3 of the issue (writeDSNAME structLen += 2 cargo-cult) is deferred: it's request-side, so there's no parser-side regression detection, and needs positive lab confirmation across DsBind/DsCrackNames/DsGetNCChanges/ DsGetDomainControllerInfo on both forest root and child domains before flipping. Filed for a separate PR. Verified end-to-end against the GOAD lab (sevenkingdoms.local forest root + north.sevenkingdoms.local child domain): full --just-dc-ntlm and --just-dc --history runs both return all accounts with correct NTLM hashes, hash histories, and parsed Kerberos keys.
This was referenced May 12, 2026
pull Bot
pushed a commit
to rvrsh3ll/gopacket
that referenced
this pull request
May 13, 2026
MS-DRSR 4.1.4.1.1 specifies structLen as the exact byte size, rounded to a 4-byte boundary. Impacket carries a "+2" on top of the round-up that no part of the spec or wire trace justifies; AD is forgiving in practice but a stricter implementation could reject. Keep just the alignment round, drop the unexplained 2 extra bytes. This is the last open item from mandiant#32. Request-side change with no parser-side regression detection, so it was held out of mandiant#33 pending lab verification. Verified end-to-end against the GOAD lab (sevenkingdoms.local forest root + north.sevenkingdoms.local child domain): secretsdump --just-dc-ntlm exercises DsBind, DsDomainControllerInfo, DsCrackNames, and DsGetNCChanges in sequence and returns success on both domains. Output is byte-for-byte identical to the pre-fix dumps captured before removing the +2. Closes mandiant#32.
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.
Summary
Three of the four follow-ups from #32 (PR #31 review punch list). Item #3 (writeDSNAME structLen +2 cargo-cult) is intentionally deferred to a separate PR with lab verification, per the issue's own recommendation.
1. Pre-allocation bounds check
The previous
maxReasonable(10M) ceiling rejected pathological values but still allowed a malformed reply with a tiny payload and a huge embedded count to trigger a multi-MB speculativemakefor bytes that don't exist on the wire. Replaced with a precise per-call-site check:count * elemSize ≤ d.Remaining()before every wire-driven allocation orSkip. Wire element sizes are fixed and documented per call site:Product math is
uint64so it's safe on 32-bit builds whereint(count) * elemSizecould otherwise overflow.CheckBoundslives onDecoderso other parsers in the package can reach for the same primitive.2.
firstRDNValueactually unescapes nowPrevious behavior correctly walked past
\<char>when splitting on,but returned the raw slice with the backslash retained, soCN=Smith\, John,OU=FooyieldedSmith\, Johninstead ofSmith, John. Switched to building the result viastrings.Builder. Trailing-backslash (malformed per RFC 4514) drops silently. Table-driven tests cover plain, escaped comma, escaped equals, escaped backslash, multiple escapes, trailing backslash, and edge cases.3. Drop dead
writePartialAttrSetwriteGetNCChangesRequestV8has always written a NULL referent forpPartialAttrSet, so the helper was never reached.Verification
Unit tests:
TestFirstRDNValue(10 subcases) — happy path through malformed escapes.TestCheckBounds(6 subcases) — exact-fit, over-by-one, pathological uint64 product, zero-count, sticky prior error, post-failure read no-ops.End-to-end against GOAD (
sevenkingdoms.localforest root +north.sevenkingdoms.localchild domain):secretsdump --just-dc-ntlmon both DCs returns every expected account with the right SIDs/RIDs/NTLM hashes.secretsdump --just-dc --historyreturns full NT/LM history chains, Kerberos key sets (aes256-cts, des-cbc-md5), and supplemental credentials.Closes part of #32 (items 1, 2, 4). Item 3 will follow in a separate PR.
Test plan
go build ./...go vet ./pkg/dcerpc/drsuapi/...go test ./pkg/dcerpc/drsuapi/...secretsdump --just-dc-ntlmagainst forest root and child domainsecretsdump --just-dc --historyagainst forest root (exercises hash history, Kerberos keys, supplemental credentials)