Skip to content

drsuapi: fix V6 parser drift on child-domain UPTODATE_VECTOR#31

Merged
psycep merged 1 commit into
mainfrom
drsuapi-uptodate-vec-align
May 12, 2026
Merged

drsuapi: fix V6 parser drift on child-domain UPTODATE_VECTOR#31
psycep merged 1 commit into
mainfrom
drsuapi-uptodate-vec-align

Conversation

@psycep
Copy link
Copy Markdown
Collaborator

@psycep psycep commented May 12, 2026

Summary

Fixes #28: secretsdump DCSync silently returning zero accounts against child domains. Root cause is a missing Align(8) in skipUpToDateVectorV2. UPTODATE_VECTOR_V{1,2}_EXT cursors contain LONGLONG (USN, DSTIME), giving the struct 8-byte alignment. NDR hoists the array's MaxCount to the front using its primitive 4-byte alignment; the struct's 8-byte alignment then applies AFTER MaxCount before the first field. This is the exact same correctness pattern PR #16 (8127aec) enumerated for PROPERTY_META_DATA_EXT_VECTOR, just missed for UPTODATE_VECTOR.

PR #16's verification ran against sevenkingdoms.local (forest root), where the response's DSNAME ends at pos 260, so post-MaxCount lands at 264 (already 8-aligned). Align(8) was a no-op and the miss was invisible. north.sevenkingdoms.local's longer DN puts post-MaxCount at 284 (%8 == 4); the missing pad drops 4 bytes, the parser misreads cNumCursors as 0, skips zero cursor bytes instead of the real 32-byte cursor, and walks into garbage in the prefix-table deferred data. Zero accounts come back, no error surfaces (V6 parse errors are swallowed unless -debug is on).

Verified against the live GOAD lab:

Hardening sweep

While in the parser, fixed a set of related correctness and DoS-hardening issues surfaced by a focused audit:

  • MaxCount wire authority. skipUpToDateVectorV2 and readPrefixTableV2 now drive their loops from the wire MaxCount (the authoritative NDR field) rather than the sibling in-struct cNumCursors / cNumPrefixes. They agree on well-formed replies; on malformed ones, trusting the wire keeps the cursor aligned.

  • Linked-list early termination. readREPLENTINFLISTArrayV2 now breaks on pNextEntInf == 0. REPLENTINFLIST is structurally a linked list per MS-DRSR; the chain terminator is the structural truth, not cNumObjects.

  • Pointer-only deferred gating. Three call sites previously gated deferred reads on pointer != 0 && count > 0. NDR serializes a 4-byte MaxCount==0 even for a non-null pointer to an empty array; the old gating dropped that read and drifted. Gate solely on the pointer.

  • Decoder.Fail() helper. New method that sets d.err only on first failure, so descriptive failure context survives downstream Skip/Align calls. Every maxReasonable bail-out now uses it. Previously the silent return left the cursor unadvanced and the next helper read into the wrong data.

  • SeekTo first-error-wins. SeekTo (and by extension Skip and Align) now short-circuit when d.err is already set, so a generic seek-range error in a later Skip doesn't clobber a more descriptive Fail() error.

  • 32-bit overflow caps. DSNAME and PROPERTY_META_DATA_EXT_VECTOR helpers now apply the maxReasonable cap on count * size products that could wrap int on 32-bit Go builds. The cap is promoted to a package-level constant so all helpers share it.

  • SID minimum length. RID extraction in readDSNAMEv2 and processAttribute (ATTID_objectSid) now requires sidLen >= 12 (rev + subAuthCount + idAuth + at least one SubAuthority). At length 8 the "last 4 bytes" is the tail of IdentifierAuthority, which would have produced bogus DES keys for password decryption on a malformed SID.

  • DN parsing. Case-insensitive CN= prefix check and backslash-aware RDN split, so an account with an escaped comma in its CN (e.g. CN=Smith\, John,OU=...) extracts the right SAMAccountName.

Test plan

  • secretsdump -k -no-pass -just-dc against winterfell.north.sevenkingdoms.local returns 18 NTDS hashes matching impacket byte-for-byte (was 0).
  • secretsdump -k -no-pass -just-dc against kingslanding.sevenkingdoms.local still returns the original PR drsuapi: fix DsGetNCChanges V6 parser to actually extract NTDS hashes #16 baseline 17 hashes.
  • go build ./... clean.
  • go vet ./... clean.

skipUpToDateVectorV2 was missing the Align(8) pad between the hoisted
MaxCount conformance and the struct's fixed fields. UPTODATE_VECTOR_V1/V2
cursors contain LONGLONG fields (USN, DSTIME), so the struct's alignment
is 8; NDR demands the struct alignment is applied AFTER MaxCount (which
uses its own primitive 4-byte alignment), before the first struct field.
This is the same correctness pattern PR #16 (commit 8127aec) enumerated
for PROPERTY_META_DATA_EXT_VECTOR -- it was missed for UPTODATE_VECTOR.

PR #16's verification ran against sevenkingdoms.local (forest root)
where the response's DSNAME ends at pos 260 (%8 == 4), so the post-
MaxCount position landed at 264 -- already 8-aligned. Align(8) was a
no-op and the miss was invisible. north.sevenkingdoms.local's longer
DN puts post-MaxCount at 284 (%8 == 4), where the missing pad drops 4
bytes; the parser then misreads cNumCursors as 0, skips zero cursor
bytes instead of the real 32-byte cursor, and walks into garbage in the
prefix-table deferred data. Result: zero accounts emitted, no error
surfaced (V6 parse errors are swallowed unless -debug is on).

Verified against a live GOAD lab: 18/18 NTDS hashes on
north.sevenkingdoms.local now match impacket-secretsdump byte-for-byte,
17/17 on sevenkingdoms.local still match PR #16's original verification.

Bundles a sweep of related correctness and hardening issues surfaced
during the fix:

- skipUpToDateVectorV2 and readPrefixTableV2 now use the wire MaxCount
  (the authoritative NDR field for array layout) instead of the in-
  struct cNumCursors/cNumPrefixes. The two agree on any well-formed
  reply, but the wire value dictates stream position; trusting it
  keeps the cursor aligned on malformed input.

- readREPLENTINFLISTArrayV2 now early-terminates when pNextEntInf is
  0. REPLENTINFLIST is structurally a linked list per MS-DRSR; the
  pointer chain terminator is the structural truth, not cNumObjects.

- Deferred-data reads are now gated only on the pointer referent
  being non-zero, never on a sibling inline count. NDR serializes a
  4-byte MaxCount==0 for a non-null pointer to an empty array; the
  old "pointer AND count > 0" gating dropped that read and drifted.

- Every helper that bails on a maxReasonable cap now calls a new
  Decoder.Fail() helper so d.err is set and downstream Read*/Skip
  calls become no-ops. The previous silent return left the cursor
  unadvanced and downstream helpers read into the wrong data.

- SeekTo now respects the first-error-wins invariant by short-
  circuiting when d.err is already set, so descriptive Fail() errors
  aren't clobbered by a generic seek-range error in a later Skip.

- DSNAME and PROPERTY_META_DATA_EXT_VECTOR helpers now also enforce
  the maxReasonable cap on count*size products that could overflow
  int on 32-bit Go builds.

- readDSNAMEv2's RID extraction and processAttribute's RID extraction
  for ATTID_objectSid now require sidLen >= 12 (rev + subAuthCount +
  idAuth + at least 1 SubAuthority). At sidLen == 8 the "last 4 bytes"
  is the tail of IdentifierAuthority, not a RID, and would produce
  bogus DES keys for password decryption.

- DN-to-SAMAccountName fallback now uses a backslash-aware RDN split
  and a case-insensitive "CN=" prefix check.

Closes #28.
@psycep psycep merged commit 604deaa into main May 12, 2026
2 checks passed
@psycep psycep deleted the drsuapi-uptodate-vec-align branch May 12, 2026 20:28
pull Bot pushed a commit to rvrsh3ll/gopacket that referenced this pull request May 13, 2026
…elper

Three of the four follow-ups in mandiant#32 from the PR mandiant#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

secretsdump DCSync silently returns zero accounts (DRSUAPI step bails without error)

1 participant