mirror of
https://github.com/mandiant/gopacket
synced 2026-06-21 13:57:02 +00:00
Jacob Paullus
23f9aae308
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.