Clears GHSA-f6x5-jh6r-wrfv (ssh/agent OOB panic) and GHSA-j5w8-q4qc-rx2x
(ssh unbounded memory). We only import x/crypto/md4 and x/crypto/pbkdf2,
so neither vuln is reachable from this repo, but bumping silences the
Dependabot alerts. x/net and x/text pulled forward by go mod tidy.
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 #32. Request-side change with no
parser-side regression detection, so it was held out of #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#32.
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.
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.
Two compounding bugs in wmiexec / atexec / smbexec / dcomexec made
the output-retrieval loop return early on any command that didn't
finish within the first 100ms poll, leaking the temp file in ADMIN$
/ C$. Issue #22 reports the symptom for wmiexec (systeminfo, ping);
the same broken pattern existed in all four tools.
1. Wrong call site. The polling loop treated a sharing-violation
on smbClient.Cat as the signal "command still running." Cat
opens with FILE_SHARE_READ-compatible access — it succeeds
against the writer on the first poll and returns the file as
it currently exists (typically empty). The conflict actually
lives on Rm: deleting the file requires DELETE access, which
conflicts with the writer's share mode. Moved the sharing-
violation check from Cat to Rm. Matches Impacket's wmiexec.py.
2. Dead string match. The check was
strings.Contains(err.Error(), "STATUS_SHARING_VIOLATION"). The
underlying smb2 library's NtStatus.Error() returns only the
human-readable description ("A file cannot be opened because
the share access flags are incompatible.") — the literal
constant name never appears in err.Error(), so the check could
never fire. Same issue for STATUS_OBJECT_NAME_NOT_FOUND in the
not-found branch (further muddled by smb2/conn.go translating
that NTSTATUS to os.ErrNotExist before wrapping).
Added typed helpers in pkg/smb (IsSharingViolation, IsNotFound)
that unwrap through *os.PathError and match on
*smb2.ResponseError.Code plus the os.ErrNotExist sentinel.
NTSTATUS values are hardcoded since the smb2/internal/erref
package can't be imported from outside the smb2 tree.
The dcomexec "broken / connection reset / use of closed" branch
stays string-matched — those errors come from net, not smb2.
Thanks to @aimogging in #22 for the diagnosis and proof-of-concept
in #29; this change applies the same Cat-vs-Rm insight across all
four exec tools and replaces the err.Error() substring matching
with typed-error helpers so future smb2 library changes don't
silently break the check again.
Verified against GOAD winterfell: wmiexec whoami / systeminfo /
ping -n 4 1.1.1.1 all return full output, exit 0, no ADMIN$
residue, both directly from the operator box and over SOCKS5H
proxy.
Cover the GetValueData guard added in the previous commit with two
table-driven tests: the rejection path (dataLen ∈ {5, 8, 0xFF,
0x7FFFFFFF} with resident bit set) confirms the guard fires without
panicking and surfaces a helpful error; the happy path (dataLen ∈
{1, 2, 3, 4}) confirms the inline DataOffset bytes are returned
correctly so the guard didn't regress valid hives.
The matching guards in SetValueData (resident + non-resident write
paths) and enumSubKeys (riSig branch) require full NK/VK/sub-list
hive synthesis to exercise end-to-end; they're structurally identical
to the read-path guard and are validated by build/vet plus the lab
regression check on secretsdump. Worth a follow-up to extend the
synth helpers and cover them directly.
Four parser-panic / silent-corruption bugs in pkg/registry/hive.go,
all reachable from attacker-controlled hive bytes:
1. GetValueData resident branch: VKRecord.DataLen lower 31 bits are
read verbatim and used to slice a 4-byte buffer at [:dataLen]. A
DataLen of 0x80000005 panics with "slice bounds out of range".
Found by kajaaz using Zorya (issue #25); fix matches her suggested
one-line bounds check.
2. SetValueData resident branch (structurally identical to #1): the
existing len(newData) == dataLen check doesn't enforce the 4-byte
cap, so a hostile dataLen=5 with a matching 5-byte newData slices
one byte past the DataOffset field into the adjacent cell. Same
guard.
3. SetValueData non-resident branch: vk.DataOffset is attacker-
controlled and the code does copy(h.data[dataPos:dataPos+dataLen],
newData) without ever validating the destination. Hostile offsets
either panic on out-of-bounds or silently scribble over arbitrary
hive bytes (a value of 0xFFFFF000 lands near the regf header). Route
through readCell (which validates the cell header and bounds) and
verify dataLen fits before mutating.
4. enumSubKeys riSig branch: readCell can return a slice shorter than
the 4-byte cell header (minimum-size cell with no usable bytes), so
the immediate subCell[0:2] / subCell[2:4] reads can panic on a
malformed sub-list. The sibling code at line 397/437 already guards
the analogous index; mirror it here.
Fixes#25 plus three structurally similar bugs surfaced while patching.
The embedded gokrb5/v8 library hard-coded net.DialTimeout for AS/TGS
exchanges, bypassing -proxy and leaking the operator's source IP to the
KDC (UDP/88 first, TCP/88 fallback). The DCERPC Kerberos auth path used
a separate library (oiweiwei/gokrb5.fork/v9 via go-msrpc) that leaked the
same way.
Vendor jcmturner/gokrb5/v8 in-tree at pkg/third_party/gokrb5 with a
required KDCDialer first argument on every client constructor, so
proxy-bypass becomes a compile error. Wire kerberos.TransportKDCDialer
everywhere a gokrb5 client is built. Stamp udp_preference_limit=1 and
dns_lookup_kdc/realm=false unconditionally so KRB5 is TCP-only and the
OS resolver is never consulted; /etc/krb5.conf and $KRB5_CONFIG are
deliberately not read.
For DCERPC: set krbConfig.KDCDialer on every krb5.Config, pass
dcerpc.WithDialer(transport.ContextDialer{}) on every dcerpc.Dial, and
use the "ncacn_ip_tcp:" StringBinding prefix on the OXID-pivot dial so
go-msrpc's hard-coded pre-dial net.LookupIP is skipped (defers FQDN
resolution to the SOCKS5 proxy).
Verified against a live GOAD lab: 8 Kerberos-touching tools plus 5
NTLM/password/PtH regressions all operate through SOCKS5 with zero
direct packets to the AD subnet. Negative control (no -proxy)
immediately emits direct SYNs to the KDC, confirming both the leak
class and the fix.
Jacob Paullus