Conversation
WalkthroughThe PR introduces comprehensive Active Directory attack primitives to Nuclei's JavaScript runtime by wrapping pure-Go Kerberos and DCE/RPC implementations. It enables ticket forging, AS-REP/Kerberoasting, DCSync, SMB operations, and WMI execution, all with integrated per-execution network policy enforcement via execution-aware dialers. Changes
Sequence Diagram(s)sequenceDiagram
participant JS as JavaScript<br/>Template
participant KRB as krbroast<br/>Module
participant KDC as Kerberos KDC
participant SEC as Host Policy<br/>Enforcer
JS->>KRB: ASRepRoast(username, domain, kdcHost)
KRB->>SEC: IsHostAllowed(execId, kdcHost)
alt Host Denied
SEC-->>KRB: ErrHostDenied
KRB-->>JS: throw error
else Host Allowed
SEC-->>KRB: proceed
KRB->>KDC: SendASRequest(username, domain)
KDC-->>KRB: ASREPResponse
KRB->>KRB: Extract & format hash
KRB-->>JS: cracking hash string
end
sequenceDiagram
participant JS as JavaScript<br/>Template
participant SD as secretsdump<br/>Module
participant SEC as Host Policy<br/>Enforcer
participant SMB as SMB Client
participant DC as Domain Controller<br/>DRSUAPI
JS->>SD: DCSync(target, dcHost)
SD->>SEC: IsHostAllowed(execId, dcHost)
alt Host Denied
SEC-->>SD: ErrHostDenied
SD-->>JS: throw error
else Host Allowed
SEC-->>SD: proceed
SD->>SMB: connect(dcHost:445)
SMB->>DC: authenticate
DC-->>SMB: authenticated session
SD->>DC: openRpcPipe(lsass or PIPE\lsass)
SD->>DC: bindDRSUAPI()
SD->>DC: DsGetNCChanges(targetDN)
DC-->>SD: encrypted attributes
SD->>SD: decode hashes & history
SD-->>JS: Secret{SAMName, hashes, history...}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
pkg/js/libs/dcerpc/dcerpc.go (2)
434-440:FileEntrytype declaration is oddly placed inside theSmbLsdocumentation block.The
FileEntrystruct is inserted betweenSmbLs's godoc comment (lines 426-433) and its function signature (line 440).go docwill not associate the comment withSmbLscorrectly and the type appears as if documented by theSmbLsexample. Move theFileEntrydeclaration above the doc comment (or to the top of the file with the other type aliases) so godoc renders as intended.🔧 Suggested change
+type FileEntry struct { + Name string `json:"name"` + Size int64 `json:"size"` + IsDir bool `json:"is_dir"` +} + // SmbLs lists files under dir on the given share. dir = "" lists the root. // // `@example` // ```javascript // const c = new dcerpc.Client('dc01', 'acme.local', 'admin', 'P@ss'); // const entries = c.SmbLs('backup', ''); // for (const e of entries) { log(e.Name + (e.IsDir ? '/' : '')); } // ``` -type FileEntry struct { - Name string `json:"name"` - Size int64 `json:"size"` - IsDir bool `json:"is_dir"` -} - func (c *Client) SmbLs(share, dir string) ([]FileEntry, error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/js/libs/dcerpc/dcerpc.go` around lines 434 - 440, The FileEntry struct is placed between the SmbLs godoc and its function signature which breaks godoc association; move the FileEntry type declaration (type FileEntry struct { Name string; Size int64; IsDir bool }) out of the SmbLs comment block — either place it immediately above the SmbLs godoc or with the file's other type declarations/aliases — so the existing godoc comment attaches to func (c *Client) SmbLs(share, dir string) ([]FileEntry, error) correctly.
321-325: 10sSmbExectimeout is likely to truncate real-world output.
gpsmbexec.Execis used to create a service, start it, and poll back stdout/stderr via SMB. Commands likewhoami /all, PowerShell one-liners, or anything that touches WMI routinely take longer than 10s end-to-end on busy DCs. This short timeout will frequently return a partialOutputor a timeout error for otherwise-successful executions, which templates will report as scan failures.AtExecuses 15s on line 382 which has the same concern but is less aggressive.Consider either raising the default (30–60s) or exposing an options argument so callers can size the timeout for their workload.
🔧 Suggested change
res, err := gpsmbexec.Exec(sc, c.smb, command, gpsmbexec.Options{ Share: share, Mode: gpsmbexec.ModeShare, - Timeout: 10 * time.Second, + Timeout: 30 * time.Second, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/js/libs/dcerpc/dcerpc.go` around lines 321 - 325, The current gpsmbexec.Exec call in dcerpc.go uses a 10s Timeout which often truncates real-world command output; update the call that constructs gpsmbexec.Options (used in the Exec invocation) to use a larger default (e.g., 30–60s, recommend 60s) or refactor to accept a configurable timeout parameter from the caller path so callers can size it; also align the AtExec usage (which currently uses 15s) to the same approach so both Exec and AtExec use a consistent, higher timeout or accept the same configurable option.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/js/libs/dcerpc/dcerpc.go`:
- Around line 414-424: SmbCat currently reads and returns an entire file into
memory via c.smb.Cat without size or time limits; update Client.SmbCat to
enforce a maximum byte cap (e.g., 10 MiB default) and a timeout/ctx-aware
operation: replace the direct call to c.smb.Cat with a streamed read using an
io.LimitedReader (or by adding a size limit parameter to the underlying smb read
method), stop reading and return a clear "size limit exceeded" error if the
limit is hit, and ensure the operation respects a context or connection deadline
(pass a context.Context or set a read timeout on the connection) so untrusted
templates cannot OOM or hang; optionally expose a size-limit argument on SmbCat
to allow callers to override the default.
- Around line 128-131: SetPort currently accepts any int and can allow
out-of-range values; change its signature from func (c *Client) SetPort(port
int) to return an error (func (c *Client) SetPort(port int) error), validate
that port is within 1..65535 (or 0..65535 if 0 is intentionally allowed —
confirm policy) before assigning, and return a descriptive error if invalid; on
success set c.Port and c.target.Port (matching gpsession.Target.Port type) and
return nil. Update any callers to handle the returned error.
- Around line 153-158: The Close method on Client currently calls c.smb.Close()
but leaves c.smb non-nil, risking double-close and leaks; update Client.Close
(and ensure consistency with connect) to set c.smb = nil after successfully
closing the underlying *gpsmb.Client and keep c.started = false so repeated
Close() calls become no-ops and subsequent connect() won't leak the old
reference.
In `@pkg/js/libs/dcerpc/transport_init.go`:
- Around line 35-38: NewExecDialer currently returns a zero-value
gptransport.Dialer when execID == "", which relies on zero-value semantics;
change it to return a failing/closed dialer by setting DialFn to a function that
always returns an error (similar to the global init hook). Update NewExecDialer
to detect execID == "" and return &gptransport.Dialer{DialFn: func(ctx
context.Context, addr string) (net.Conn, error) { return nil, fmt.Errorf("no
execID provided") }} (use appropriate context/net/error packages already used in
the file) so callers like dcerpc.go using ExecutionId() get an explicit failure
instead of ambiguous zero-value behavior.
In `@pkg/js/libs/dcerpc/wmiexec.go`:
- Around line 59-60: Replace the hardcoded context.Background() passed to
gpwmiexec.Exec with the incoming execution context so cancellations/deadlines
propagate; wrap the provided ctx in a 15s timeout (using context.WithTimeout and
deferring cancel()) when you need the 15s limit, and pass that ctxWithTimeout to
gpwmiexec.Exec (update the call site in wmiexec.go where gpwmiexec.Exec is
invoked).
In `@pkg/js/libs/krbroast/krbroast.go`:
- Around line 106-109: Kerberoast currently calls GetTGSWithOptions without a
dialer so its TCP connection bypasses the fastdialer used by
ASRepRoast/GetASREPWithDialer; add a dialer-aware TGS API and wire it through
the JS binding: update the Go API by adding GetTGSWithDialer (or extend
GetTGSWithOptions to accept a net.Dialer-like interface) and implement the
dialer usage inside the function that currently performs the TGS request, then
change Kerberoast to call the new GetTGSWithDialer (and update any JS binding
wrappers) so the KDC connection is created via the provided execution-bound
dialer used for proxy/resolver/timeout/policy handling.
In `@pkg/js/libs/secretsdump/secretsdump.go`:
- Around line 108-110: The error uses protocolstate.ErrHostDenied.Msgf with
c.Host as the format string which can mis-handle '%' in host values; update the
call in secretsdump.go (and the similar sites in dcerpc.go) to pass a format
string and the host as an argument (e.g. use Msgf("%s", c.Host)) so the host is
treated as data, not a format specifier; search for ErrHostDenied.Msgf(...) and
replace direct host-as-format usages with a "%s" format and c.Host as the
parameter.
- Around line 118-132: The OpenPipe fallback comment is misleading and there is
a resource leak if gprpc.NewClient(pipe).BindAuth(...) fails because
rpc.Transport.Close() is deferred only after the bind succeeds; ensure the pipe
is closed on bind failure and fix the comment: after calling
smb.OpenPipe("\\PIPE\\lsass") and falling back to smb.OpenPipe("lsass") update
the comment to reflect that the fallback uses the alternate lsass pipe name (not
"drsuapi"), then either register the defer rpc.Transport.Close() immediately
after creating rpc (so it always runs) or explicitly close the
pipe/rpc.Transport when rpc.BindAuth returns an error (refer to OpenPipe,
gprpc.NewClient, rpc.BindAuth, rpc.Transport.Close and follow the
rpcOverNamedPipe pattern to close the pipe on bind failure).
- Line 112: The call to gpsmb.NewClient(c.target, c.creds) in secretsdump
bypasses nuclei’s fastdialer and per-execution network policy; replace it with
the dialer-aware constructor used by dcerpc.Client.connect() (e.g.,
gpsmb.NewClientWithDialer(c.target, c.creds, NewExecDialer(c.nj.ExecutionId())))
so DNS, proxy, resolved-IP allow/deny and timeouts are enforced at dial time,
and ensure IsHostAllowed still runs against c.target before dialing; import the
dcerpc package or move NewExecDialer to a shared helper to avoid an import
cycle.
- Around line 149-160: The DN-detection currently checks target[:3] for "CN="
which misclassifies valid DNs that start with other RDNs; update the heuristic
in the block that assigns userDN (and the conditional before calling
gpdrs.DsCrackNames) to use strings.ContainsRune(target, '=') (i.e., treat as DN
if it contains '=') instead of the prefix CN check, and add "strings" to the
import list so strings.ContainsRune can be used; keep the rest of the
DsCrackNames calls (including DS_NT4_ACCOUNT_NAME/DS_UNIQUE_ID_NAME) unchanged.
---
Nitpick comments:
In `@pkg/js/libs/dcerpc/dcerpc.go`:
- Around line 434-440: The FileEntry struct is placed between the SmbLs godoc
and its function signature which breaks godoc association; move the FileEntry
type declaration (type FileEntry struct { Name string; Size int64; IsDir bool })
out of the SmbLs comment block — either place it immediately above the SmbLs
godoc or with the file's other type declarations/aliases — so the existing godoc
comment attaches to func (c *Client) SmbLs(share, dir string) ([]FileEntry,
error) correctly.
- Around line 321-325: The current gpsmbexec.Exec call in dcerpc.go uses a 10s
Timeout which often truncates real-world command output; update the call that
constructs gpsmbexec.Options (used in the Exec invocation) to use a larger
default (e.g., 30–60s, recommend 60s) or refactor to accept a configurable
timeout parameter from the caller path so callers can size it; also align the
AtExec usage (which currently uses 15s) to the same approach so both Exec and
AtExec use a consistent, higher timeout or accept the same configurable option.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 414976f4-cb52-4f14-b483-3e52f7db98b6
⛔ Files ignored due to path filters (14)
go.sumis excluded by!**/*.suminternal/tests/integration/testdata/protocols/javascript/asrep-roast.yamlis excluded by!**/*.yamlpkg/js/generated/go/libdcerpc/dcerpc.gois excluded by!**/generated/**pkg/js/generated/go/libkrbforge/krbforge.gois excluded by!**/generated/**pkg/js/generated/go/libkrbroast/krbroast.gois excluded by!**/generated/**pkg/js/generated/go/librdp/rdp.gois excluded by!**/generated/**pkg/js/generated/go/librsync/rsync.gois excluded by!**/generated/**pkg/js/generated/go/libsecretsdump/secretsdump.gois excluded by!**/generated/**pkg/js/generated/go/libtelnet/telnet.gois excluded by!**/generated/**pkg/js/generated/ts/dcerpc.tsis excluded by!**/generated/**pkg/js/generated/ts/index.tsis excluded by!**/generated/**pkg/js/generated/ts/krbforge.tsis excluded by!**/generated/**pkg/js/generated/ts/krbroast.tsis excluded by!**/generated/**pkg/js/generated/ts/secretsdump.tsis excluded by!**/generated/**
📒 Files selected for processing (10)
go.modinternal/tests/integration/javascript_krbroast_test.gointernal/tests/integration/javascript_test.gopkg/js/compiler/pool.gopkg/js/libs/dcerpc/dcerpc.gopkg/js/libs/dcerpc/transport_init.gopkg/js/libs/dcerpc/wmiexec.gopkg/js/libs/krbforge/krbforge.gopkg/js/libs/krbroast/krbroast.gopkg/js/libs/secretsdump/secretsdump.go
| func (c *Client) SetPort(port int) { | ||
| c.Port = port | ||
| c.target.Port = port | ||
| } |
There was a problem hiding this comment.
SetPort accepts any int (including 0, negative, or >65535).
gpsession.Target.Port is presumably uint16 or validated downstream, but with an int parameter this can silently accept out-of-range ports that then either panic, wrap, or fail obscurely at connect time. Reject invalid ports up front to surface a clear JS-side error.
🔧 Suggested fix
func (c *Client) SetPort(port int) {
+ c.nj.Require(port > 0 && port <= 65535, "port must be in range 1-65535")
c.Port = port
c.target.Port = port
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/js/libs/dcerpc/dcerpc.go` around lines 128 - 131, SetPort currently
accepts any int and can allow out-of-range values; change its signature from
func (c *Client) SetPort(port int) to return an error (func (c *Client)
SetPort(port int) error), validate that port is within 1..65535 (or 0..65535 if
0 is intentionally allowed — confirm policy) before assigning, and return a
descriptive error if invalid; on success set c.Port and c.target.Port (matching
gpsession.Target.Port type) and return nil. Update any callers to handle the
returned error.
| func (c *Client) Close() { | ||
| if c.smb != nil { | ||
| c.smb.Close() | ||
| } | ||
| c.started = false | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether gpsmb.Client.Close is safe to call multiple times.
rg -nP -C3 'func \(.*\*Client\) Close\(\)' --type=goRepository: projectdiscovery/nuclei
Length of output: 1627
🏁 Script executed:
# Get full context around the Close() method
cat -n pkg/js/libs/dcerpc/dcerpc.go | sed -n '140,165p'Repository: projectdiscovery/nuclei
Length of output: 996
🏁 Script executed:
# Search for usage of dcerpc.Client.Close()
rg -n 'Close\(\)' pkg/js/libs/dcerpc/ -A 2 -B 2Repository: projectdiscovery/nuclei
Length of output: 3390
🏁 Script executed:
# Find how gpsmb is imported and used
rg -n 'gpsmb' pkg/js/libs/dcerpc/dcerpc.go -B 2 -A 2Repository: projectdiscovery/nuclei
Length of output: 905
🏁 Script executed:
# Look for connect() method to understand the started flag behavior
rg -n 'func.*\(c \*Client\) connect' pkg/js/libs/dcerpc/dcerpc.go -A 10Repository: projectdiscovery/nuclei
Length of output: 470
🏁 Script executed:
# Search for calls to dcerpc.Client Close method
rg -n '\.Close\(\)' pkg/js/libs/dcerpc/dcerpc.go | head -20Repository: projectdiscovery/nuclei
Length of output: 397
🏁 Script executed:
# Check if there are JS bindings/exports and how Close is used from JS
rg -n 'NewClient\|Close' pkg/js/libs/dcerpc/dcerpc.go | grep -E '(func New|Close)'Repository: projectdiscovery/nuclei
Length of output: 49
🏁 Script executed:
# Look for any template or deferred cleanup patterns mentioned in the comment
rg -n 'defer.*Close\|finally' pkg/js/libs/dcerpc/ --type=goRepository: projectdiscovery/nuclei
Length of output: 49
🏁 Script executed:
# Check if this Client and Close are exported/exposed to JS
rg -n 'Register\|Export\|Runtime' pkg/js/libs/dcerpc/dcerpc.go | head -20Repository: projectdiscovery/nuclei
Length of output: 49
🏁 Script executed:
# Look at the top of the file for JS runtime bindings
head -80 pkg/js/libs/dcerpc/dcerpc.goRepository: projectdiscovery/nuclei
Length of output: 2980
🏁 Script executed:
# Search for any JS template examples or usage patterns of dcerpc Client
find . -name '*.yaml' -o -name '*.yml' | xargs grep -l 'dcerpc' 2>/dev/null | head -5Repository: projectdiscovery/nuclei
Length of output: 49
🏁 Script executed:
# Check if there are tests that show how Close() should be used
rg -n 'dcerpc.*Close\|Close.*dcerpc' . --type=go | head -10Repository: projectdiscovery/nuclei
Length of output: 49
🏁 Script executed:
# Look for any documentation or comments about idempotence or double-close
rg -n 'idempotent\|double.*close\|close.*twice' pkg/js/libs/dcerpc/ --type=goRepository: projectdiscovery/nuclei
Length of output: 49
🏁 Script executed:
# Look at the entire Client struct and any cleanup patterns
sed -n '48,80p' pkg/js/libs/dcerpc/dcerpc.goRepository: projectdiscovery/nuclei
Length of output: 1068
🏁 Script executed:
# Check if there are any defer statements that use this client or call Close
rg -n 'defer' pkg/js/libs/dcerpc/dcerpc.goRepository: projectdiscovery/nuclei
Length of output: 319
🏁 Script executed:
# Verify the suggested fix would compile and make sense
cat -n pkg/js/libs/dcerpc/dcerpc.go | sed -n '150,165p'Repository: projectdiscovery/nuclei
Length of output: 600
🏁 Script executed:
# Check if gpsmb.Client is from an external package and verify it's not safe for double-close
rg -n 'github.com/Mzack9999/goimpacket' pkg/js/libs/dcerpc/dcerpc.go | head -5Repository: projectdiscovery/nuclei
Length of output: 366
🏁 Script executed:
# Look for any linter/fmt/vet issues in the file
go fmt -d pkg/js/libs/dcerpc/dcerpc.go 2>&1 || echo "No fmt issues"Repository: projectdiscovery/nuclei
Length of output: 180
🏁 Script executed:
# Run go vet on this file to check for any obvious issues
go vet ./pkg/js/libs/dcerpc/ 2>&1 | head -20Repository: projectdiscovery/nuclei
Length of output: 49
🏁 Script executed:
# Verify the fix compiles by checking syntax
cat > /tmp/test_close.go << 'EOF'
package dcerpc
type smb struct{}
func (s *smb) Close() {}
type Client struct {
smb *smb
started bool
}
func (c *Client) Close() {
if c.smb != nil {
c.smb.Close()
c.smb = nil
}
c.started = false
}
EOF
go fmt /tmp/test_close.go && echo "Fix syntax is valid"Repository: projectdiscovery/nuclei
Length of output: 105
Clear the SMB client pointer after closing to ensure idempotence.
The Close() method does not set c.smb = nil after calling c.smb.Close(). This creates two problems:
- Calling
Close()a second time will invokeClose()on an already-closed*gpsmb.Clientfrom the external goimpacket package, which may panic or error. - If
connect()is called afterClose()withc.smbstill non-nil, it will overwrite the pointer without cleaning up the old reference, creating a resource leak.
Since this is a JS binding exposed to templates, user code may call Close() from both finally blocks and deferred cleanup paths, making defensive programming essential.
🔧 Suggested fix
func (c *Client) Close() {
if c.smb != nil {
c.smb.Close()
+ c.smb = nil
}
c.started = false
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/js/libs/dcerpc/dcerpc.go` around lines 153 - 158, The Close method on
Client currently calls c.smb.Close() but leaves c.smb non-nil, risking
double-close and leaks; update Client.Close (and ensure consistency with
connect) to set c.smb = nil after successfully closing the underlying
*gpsmb.Client and keep c.started = false so repeated Close() calls become no-ops
and subsequent connect() won't leak the old reference.
| func (c *Client) SmbCat(share, file string) (string, error) { | ||
| c.nj.Require(share != "", "share cannot be empty") | ||
| c.nj.Require(file != "", "file cannot be empty") | ||
| if err := c.connect(); err != nil { | ||
| return "", err | ||
| } | ||
| if err := c.smb.UseShare(share); err != nil { | ||
| return "", fmt.Errorf("use share %s: %w", share, err) | ||
| } | ||
| return c.smb.Cat(file) | ||
| } |
There was a problem hiding this comment.
SmbCat has no size cap and returns the whole file as a Go string.
A malicious or misconfigured target can trivially OOM the scanner by pointing SmbCat at a multi-GB file on a share — there is no max-size limit, no streaming, and no context/timeout. Since this is invoked from untrusted template logic running against arbitrary hosts, consider capping the maximum bytes read (e.g. 10 MiB by default) and/or exposing a size-limit argument, and returning a clear error when exceeded.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/js/libs/dcerpc/dcerpc.go` around lines 414 - 424, SmbCat currently reads
and returns an entire file into memory via c.smb.Cat without size or time
limits; update Client.SmbCat to enforce a maximum byte cap (e.g., 10 MiB
default) and a timeout/ctx-aware operation: replace the direct call to c.smb.Cat
with a streamed read using an io.LimitedReader (or by adding a size limit
parameter to the underlying smb read method), stop reading and return a clear
"size limit exceeded" error if the limit is hit, and ensure the operation
respects a context or connection deadline (pass a context.Context or set a read
timeout on the connection) so untrusted templates cannot OOM or hang; optionally
expose a size-limit argument on SmbCat to allow callers to override the default.
| func NewExecDialer(execID string) *gptransport.Dialer { | ||
| if execID == "" { | ||
| return &gptransport.Dialer{} | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
github.com/Mzack9999/goimpacket pkg/transport Dialer zero value DialFn behavior
💡 Result:
No public repository found at github.com/Mzack9999/goimpacket. Searches across GitHub, pkg.go.dev, and general web returned no matching results for the exact path or package with a pkg/transport Dialer DialFn. Mzack9999 has contributions to projectdiscovery projects like goflags and fastdialer (which depends on Mzack9999/gcache), but no goimpacket repo. Similar projects exist like amzza0x00/go-impacket or Enelg52/go-impacket (SMB/DCERPC implementations), but not matching the path. The repository may be private, deleted, or the path is incorrect. Without source code, cannot determine zero-value DialFn behavior for Dialer in pkg/transport.
Citations:
- 1: https://pkg.go.dev/github.com/mzack9999/gopacket
- 2: https://github.com/projectdiscovery/fastdialer/blob/master/go.mod
- 3: https://pkg.go.dev/github.com/Enelg52/go-impacket/pkg
- 4: https://github.com/amzza0x00/go-impacket/issues
- 5: Ignoring all non goflags keys goflags#13
🏁 Script executed:
# Locate the file mentioned in the review
find . -name "transport_init.go" -type fRepository: projectdiscovery/nuclei
Length of output: 105
🏁 Script executed:
# Check if goimpacket is imported and understand the dependency
git ls-files | grep -E "(go\.mod|go\.sum)" | head -5Repository: projectdiscovery/nuclei
Length of output: 80
🏁 Script executed:
# Get the go.mod content to understand dependencies
cat go.mod | head -50Repository: projectdiscovery/nuclei
Length of output: 1829
🏁 Script executed:
# Check for goimpacket in go.mod
cat go.mod | grep -i "goimpacket\|gptransport"Repository: projectdiscovery/nuclei
Length of output: 134
🏁 Script executed:
# Read the transport_init.go file to see the actual implementation
cat -n ./pkg/js/libs/dcerpc/transport_init.goRepository: projectdiscovery/nuclei
Length of output: 3335
🏁 Script executed:
# Search for gptransport package definition in the codebase
find . -type f -name "*.go" | xargs grep -l "gptransport" | head -10Repository: projectdiscovery/nuclei
Length of output: 105
🏁 Script executed:
# Search for usages of NewExecDialer
rg "NewExecDialer" --type goRepository: projectdiscovery/nuclei
Length of output: 885
🏁 Script executed:
# Check if there are any other transport initializations or gptransport.Dialer uses
rg "gptransport\.Dialer" --type goRepository: projectdiscovery/nuclei
Length of output: 748
🏁 Script executed:
# Look for patterns where execID might be empty when NewExecDialer is called
rg -A 3 -B 3 "NewExecDialer\(\)" --type goRepository: projectdiscovery/nuclei
Length of output: 49
🏁 Script executed:
# Check dcerpc.go to see the context of NewExecDialer usage
cat -n ./pkg/js/libs/dcerpc/dcerpc.go | grep -A 5 -B 5 "NewExecDialer"Repository: projectdiscovery/nuclei
Length of output: 482
🏁 Script executed:
# Check krbroast.go to see the context of NewExecDialer usage
cat -n ./pkg/js/libs/krbroast/krbroast.go | grep -A 5 -B 5 "NewExecDialer"Repository: projectdiscovery/nuclei
Length of output: 457
🏁 Script executed:
# Check what ExecutionId() returns - look for its definition
rg "ExecutionId\(" --type go -A 2 | head -40Repository: projectdiscovery/nuclei
Length of output: 2175
🏁 Script executed:
# Check the ExecutionId() implementation to understand if it can legitimately return ""
cat -n ./pkg/js/utils/nucleijs.go | grep -A 10 "func (j \*NucleiJS) ExecutionId"Repository: projectdiscovery/nuclei
Length of output: 452
🏁 Script executed:
# Check if there are defensive guards in dcerpc.go before calling NewExecDialer
cat -n ./pkg/js/libs/dcerpc/dcerpc.go | head -150 | tail -40Repository: projectdiscovery/nuclei
Length of output: 1533
🏁 Script executed:
# Go fmt and go vet checks for the file
go fmt ./pkg/js/libs/dcerpc/transport_init.go && echo "Format OK" || echo "Format issues"Repository: projectdiscovery/nuclei
Length of output: 76
🏁 Script executed:
# Run go vet on the transport_init.go file
go vet ./pkg/js/libs/dcerpc/transport_init.goRepository: projectdiscovery/nuclei
Length of output: 49
Return an explicitly failing dialer when execID is empty.
NewExecDialer("") currently returns a zero-value gptransport.Dialer{}, leaving behavior dependent on goimpacket's internal zero-value semantics. ExecutionId() can legitimately return "" when not set in context, and dcerpc.go calls NewExecDialer(c.nj.ExecutionId()) without guarding against this case. Make the returned dialer fail closed by setting DialFn to error explicitly when execID is empty, consistent with the defensive pattern in the global init hook.
Proposed fail-closed change
func NewExecDialer(execID string) *gptransport.Dialer {
if execID == "" {
- return &gptransport.Dialer{}
+ return &gptransport.Dialer{
+ DialFn: func(_ context.Context, network, address string) (net.Conn, error) {
+ return nil, fmt.Errorf("goimpacket: refusing to dial %s/%s without an executionId", network, address)
+ },
+ }
}
return &gptransport.Dialer{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/js/libs/dcerpc/transport_init.go` around lines 35 - 38, NewExecDialer
currently returns a zero-value gptransport.Dialer when execID == "", which
relies on zero-value semantics; change it to return a failing/closed dialer by
setting DialFn to a function that always returns an error (similar to the global
init hook). Update NewExecDialer to detect execID == "" and return
&gptransport.Dialer{DialFn: func(ctx context.Context, addr string) (net.Conn,
error) { return nil, fmt.Errorf("no execID provided") }} (use appropriate
context/net/error packages already used in the file) so callers like dcerpc.go
using ExecutionId() get an explicit failure instead of ambiguous zero-value
behavior.
| res, err := gpwmiexec.Exec( | ||
| context.Background(), |
There was a problem hiding this comment.
Propagate the execution context instead of using context.Background().
This drops scan cancellation/deadlines, so WMI/DCOM/SMB work can continue after the nuclei execution is cancelled. Use the JS/execution context here, wrapping it with the 15s timeout if needed.
🛡️ Suggested shape
+ ctx, cancel := context.WithTimeout(executionCtx, 15*time.Second)
+ defer cancel()
+
res, err := gpwmiexec.Exec(
- context.Background(),
+ ctx,
target,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/js/libs/dcerpc/wmiexec.go` around lines 59 - 60, Replace the hardcoded
context.Background() passed to gpwmiexec.Exec with the incoming execution
context so cancellations/deadlines propagate; wrap the provided ctx in a 15s
timeout (using context.WithTimeout and deferring cancel()) when you need the 15s
limit, and pass that ctxWithTimeout to gpwmiexec.Exec (update the call site in
wmiexec.go where gpwmiexec.Exec is invoked).
| // Note: the underlying jcmturner/gokrb5 client used by GetTGSWithOptions | ||
| // performs its own net.Dial that is not routed through nuclei's fastdialer. | ||
| // The host is still pre-validated against the per-execution network policy so | ||
| // allowlist / denylist constraints are enforced before any traffic is sent. |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
github.com/Mzack9999/goimpacket kerberos GetTGSWithOptions custom dialer fastdialer
💡 Result:
No public GitHub repository exists at github.com/Mzack9999/goimpacket. Mzack9999 (likely mzack from projectdiscovery) maintains a fork of gopacket at github.com/mzack9999/gopacket, a packet decoding library unrelated to Impacket or Kerberos. Existing Go Impacket implementations like github.com/amzza0x00/go-impacket (SMB2/DCE/RPC, no Kerberos mentioned) and github.com/Enelg52/go-impacket (SMB2/DCE/RPC) do not reference GetTGSWithOptions, custom dialer, or fastdialer. Mandiant's gopacket has Kerberos tools (getTGT, getST) but no GetTGSWithOptions or dialer options. Fastdialer (github.com/projectdiscovery/fastdialer, contributed by Mzack9999) is a separate optimized net.Dialer with DNS cache, potentially usable for custom connections in network tools. The query likely refers to a private/unreleased project or internal feature for Kerberos TGS requests with customizable dialers (e.g., fastdialer for performance) in a Go Impacket-like library. No official docs or code found for the exact API.
Citations:
- 1: https://pkg.go.dev/github.com/mzack9999/gopacket
- 2: https://pkg.go.dev/github.com/Enelg52/go-impacket/pkg
- 3: https://github.com/amzza0x00/go-impacket/projects?query=is%3Aopen
- 4: https://github.com/amzza0x00/go-impacket/issues
- 5: https://github.com/mandiant/gopacket
- 6: https://github.com/projectdiscovery/fastdialer
- 7: https://github.com/amzza0x00/go-impacket/forks
- 8: https://github.com/fortra/impacket/blob/5026d261d21b01f56ed4c8b39abac0028637a7f6/impacket/krb5/kerberosv5.py
- 9: https://github.com/CoreSecurity/impacket/blob/master/impacket/krb5/kerberosv5.py
- 10: https://github.com/fortra/impacket/blob/impacket_0_12_0/examples/getST.py
- 11: fix missing browser init #5896
- 12: https://github.com/Amzza0x00
🏁 Script executed:
cat -n pkg/js/libs/krbroast/krbroast.go | head -180Repository: projectdiscovery/nuclei
Length of output: 6908
Add dialer-aware TGS API to route Kerberoast through fastdialer.
ASRepRoast routes through GetASREPWithDialer (line 72) while Kerberoast calls GetTGSWithOptions (lines 153–161) with no dialer parameter. This asymmetry bypasses the fastdialer infrastructure that ASRepRoast uses for proxy/resolver/timeout/policy handling. Pre-validating the KDC host does not substitute for routing the actual connection through the execution-bound dialer.
Update GetTGSWithOptions (or add a GetTGSWithDialer variant) to accept a custom dialer parameter before wiring the JS binding.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/js/libs/krbroast/krbroast.go` around lines 106 - 109, Kerberoast
currently calls GetTGSWithOptions without a dialer so its TCP connection
bypasses the fastdialer used by ASRepRoast/GetASREPWithDialer; add a
dialer-aware TGS API and wire it through the JS binding: update the Go API by
adding GetTGSWithDialer (or extend GetTGSWithOptions to accept a net.Dialer-like
interface) and implement the dialer usage inside the function that currently
performs the TGS request, then change Kerberoast to call the new
GetTGSWithDialer (and update any JS binding wrappers) so the KDC connection is
created via the provided execution-bound dialer used for
proxy/resolver/timeout/policy handling.
| if !protocolstate.IsHostAllowed(c.nj.ExecutionId(), c.Host) { | ||
| return nil, protocolstate.ErrHostDenied.Msgf(c.Host) | ||
| } |
There was a problem hiding this comment.
ErrHostDenied.Msgf(c.Host) passes host as a format string.
Msgf is a printf-style formatter. If c.Host ever contains a literal % (rare but valid in hostnames-as-arbitrary-strings, e.g. when users paste IPv6 zone identifiers like fe80::1%eth0), this will either double-format or emit %! verbs. Pass the host as an argument instead. Same pattern appears in dcerpc.go (lines 142, 190, 297, 362).
🔧 Suggested fix
- return nil, protocolstate.ErrHostDenied.Msgf(c.Host)
+ return nil, protocolstate.ErrHostDenied.Msgf("%s", c.Host)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !protocolstate.IsHostAllowed(c.nj.ExecutionId(), c.Host) { | |
| return nil, protocolstate.ErrHostDenied.Msgf(c.Host) | |
| } | |
| if !protocolstate.IsHostAllowed(c.nj.ExecutionId(), c.Host) { | |
| return nil, protocolstate.ErrHostDenied.Msgf("%s", c.Host) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/js/libs/secretsdump/secretsdump.go` around lines 108 - 110, The error
uses protocolstate.ErrHostDenied.Msgf with c.Host as the format string which can
mis-handle '%' in host values; update the call in secretsdump.go (and the
similar sites in dcerpc.go) to pass a format string and the host as an argument
(e.g. use Msgf("%s", c.Host)) so the host is treated as data, not a format
specifier; search for ErrHostDenied.Msgf(...) and replace direct host-as-format
usages with a "%s" format and c.Host as the parameter.
| return nil, protocolstate.ErrHostDenied.Msgf(c.Host) | ||
| } | ||
|
|
||
| smb := gpsmb.NewClient(c.target, c.creds) |
There was a problem hiding this comment.
Bypasses nuclei's fastdialer / per-execution network policy.
Unlike dcerpc.Client.connect() which uses gpsmb.NewClientWithDialer(c.target, c.creds, NewExecDialer(c.nj.ExecutionId())), this helper calls gpsmb.NewClient(c.target, c.creds) directly. The pre-flight IsHostAllowed check on line 108 only validates the originally-supplied host string; the actual TCP dial inside goimpacket will go straight through the default transport, so proxy settings, DNS cache, resolved-IP allow/deny, and timeouts enforced by nuclei's fastdialer are not applied. This contradicts the PR objective that all network connections go through the existing fastdialer infrastructure, and also means the executionId-scoped policy is not enforced at dial-time.
🔧 Suggested fix
- smb := gpsmb.NewClient(c.target, c.creds)
+ smb := gpsmb.NewClientWithDialer(c.target, c.creds, dcerpc.NewExecDialer(c.nj.ExecutionId()))
if err := smb.Connect(); err != nil {
return nil, fmt.Errorf("smb connect: %w", err)
}(and add an import for the dcerpc package, or move NewExecDialer to a shared helper so both secretsdump and dcerpc can use it without introducing a cycle.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/js/libs/secretsdump/secretsdump.go` at line 112, The call to
gpsmb.NewClient(c.target, c.creds) in secretsdump bypasses nuclei’s fastdialer
and per-execution network policy; replace it with the dialer-aware constructor
used by dcerpc.Client.connect() (e.g., gpsmb.NewClientWithDialer(c.target,
c.creds, NewExecDialer(c.nj.ExecutionId()))) so DNS, proxy, resolved-IP
allow/deny and timeouts are enforced at dial time, and ensure IsHostAllowed
still runs against c.target before dialing; import the dcerpc package or move
NewExecDialer to a shared helper to avoid an import cycle.
| pipe, err := smb.OpenPipe("\\PIPE\\lsass") | ||
| if err != nil { | ||
| // Fall back to drsuapi-named pipe; both are accepted by the DC. | ||
| pipe, err = smb.OpenPipe("lsass") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("open lsass pipe: %w", err) | ||
| } | ||
| } | ||
| rpc := gprpc.NewClient(pipe) | ||
| if err := rpc.BindAuth(gpdrs.UUID, gpdrs.MajorVersion, gpdrs.MinorVersion, c.creds); err != nil { | ||
| return nil, fmt.Errorf("drsuapi bind: %w", err) | ||
| } | ||
| defer func() { | ||
| _ = rpc.Transport.Close() | ||
| }() |
There was a problem hiding this comment.
Pipe leak on BindAuth failure; misleading fallback comment.
Two issues in this block:
- If
rpc.BindAuthon line 127 fails, the function returns at line 128 without closingpipe. Thedefer rpc.Transport.Close()on line 130-132 is registered after the error return, so the pipe file handle (and its SMB tree/session resource) is leaked. Compare withrpcOverNamedPipeindcerpc.go(lines 170-175) which explicitly closes the pipe on bind failure. - The comment on line 120 says "Fall back to drsuapi-named pipe", but the fallback value is
"lsass"(same endpoint, different naming form), notdrsuapi. Please correct the comment to avoid confusion for future readers.
🔧 Suggested fix
pipe, err := smb.OpenPipe("\\PIPE\\lsass")
if err != nil {
- // Fall back to drsuapi-named pipe; both are accepted by the DC.
+ // Fall back to the unprefixed pipe name; both forms are accepted by the DC.
pipe, err = smb.OpenPipe("lsass")
if err != nil {
return nil, fmt.Errorf("open lsass pipe: %w", err)
}
}
rpc := gprpc.NewClient(pipe)
if err := rpc.BindAuth(gpdrs.UUID, gpdrs.MajorVersion, gpdrs.MinorVersion, c.creds); err != nil {
+ _ = pipe.Close()
return nil, fmt.Errorf("drsuapi bind: %w", err)
}
defer func() {
_ = rpc.Transport.Close()
}()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pipe, err := smb.OpenPipe("\\PIPE\\lsass") | |
| if err != nil { | |
| // Fall back to drsuapi-named pipe; both are accepted by the DC. | |
| pipe, err = smb.OpenPipe("lsass") | |
| if err != nil { | |
| return nil, fmt.Errorf("open lsass pipe: %w", err) | |
| } | |
| } | |
| rpc := gprpc.NewClient(pipe) | |
| if err := rpc.BindAuth(gpdrs.UUID, gpdrs.MajorVersion, gpdrs.MinorVersion, c.creds); err != nil { | |
| return nil, fmt.Errorf("drsuapi bind: %w", err) | |
| } | |
| defer func() { | |
| _ = rpc.Transport.Close() | |
| }() | |
| pipe, err := smb.OpenPipe("\\PIPE\\lsass") | |
| if err != nil { | |
| // Fall back to the unprefixed pipe name; both forms are accepted by the DC. | |
| pipe, err = smb.OpenPipe("lsass") | |
| if err != nil { | |
| return nil, fmt.Errorf("open lsass pipe: %w", err) | |
| } | |
| } | |
| rpc := gprpc.NewClient(pipe) | |
| if err := rpc.BindAuth(gpdrs.UUID, gpdrs.MajorVersion, gpdrs.MinorVersion, c.creds); err != nil { | |
| _ = pipe.Close() | |
| return nil, fmt.Errorf("drsuapi bind: %w", err) | |
| } | |
| defer func() { | |
| _ = rpc.Transport.Close() | |
| }() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/js/libs/secretsdump/secretsdump.go` around lines 118 - 132, The OpenPipe
fallback comment is misleading and there is a resource leak if
gprpc.NewClient(pipe).BindAuth(...) fails because rpc.Transport.Close() is
deferred only after the bind succeeds; ensure the pipe is closed on bind failure
and fix the comment: after calling smb.OpenPipe("\\PIPE\\lsass") and falling
back to smb.OpenPipe("lsass") update the comment to reflect that the fallback
uses the alternate lsass pipe name (not "drsuapi"), then either register the
defer rpc.Transport.Close() immediately after creating rpc (so it always runs)
or explicitly close the pipe/rpc.Transport when rpc.BindAuth returns an error
(refer to OpenPipe, gprpc.NewClient, rpc.BindAuth, rpc.Transport.Close and
follow the rpcOverNamedPipe pattern to close the pipe on bind failure).
| // Resolve target -> DN if it doesn't already look like one. | ||
| userDN := target | ||
| if len(target) < 3 || (target[:3] != "CN=" && target[:3] != "cn=") { | ||
| cracked, err := gpdrs.DsCrackNames(rpc, bind.Handle, 7 /* DS_NT4_ACCOUNT_NAME */, 1 /* DS_FQDN_1779_NAME */, []string{c.Domain + "\\" + target}) | ||
| if err != nil || len(cracked) == 0 || cracked[0].Name == "" { | ||
| cracked, err = gpdrs.DsCrackNames(rpc, bind.Handle, 11 /* DS_UNIQUE_ID_NAME (SID) */, 1, []string{target}) | ||
| if err != nil || len(cracked) == 0 || cracked[0].Name == "" { | ||
| return nil, fmt.Errorf("could not resolve %q to a DN", target) | ||
| } | ||
| } | ||
| userDN = cracked[0].Name | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the DN-detection call sites and whether goimpacket exposes a canonical helper.
rg -nP -C2 'DsCrackNames|GetDomainDN' --type=goRepository: projectdiscovery/nuclei
Length of output: 1261
🏁 Script executed:
# Check the imports and full context of the secretsdump.go file
head -30 pkg/js/libs/secretsdump/secretsdump.go | cat -nRepository: projectdiscovery/nuclei
Length of output: 1495
🏁 Script executed:
# Read the complete context around lines 149-160
sed -n '140,170p' pkg/js/libs/secretsdump/secretsdump.go | cat -nRepository: projectdiscovery/nuclei
Length of output: 1451
🏁 Script executed:
# Search for other DN-related checks or similar heuristics in the file
rg -n "target\[:3\]|ContainsRune|DN|Distinguished" pkg/js/libs/secretsdump/secretsdump.goRepository: projectdiscovery/nuclei
Length of output: 841
🏁 Script executed:
# Check if there are other DN validation patterns elsewhere in the codebase
rg -n "CN=.*OU=|RDN|Distinguished.*Name" --type=goRepository: projectdiscovery/nuclei
Length of output: 689
Resolve DN heuristic to handle any RDN attribute type, not just CN=.
The check target[:3] != "CN=" && target[:3] != "cn=" incorrectly treats legitimate DNs starting with other RDN attributes (OU=, DC=, UID=, O=, etc.) as non-DNs, routing them through DsCrackNames with DS_NT4_ACCOUNT_NAME. Well-formed DNs that don't start with CN= will fail the first crack, fall through to the SID cracker, and then error out with "could not resolve".
Replace with !strings.ContainsRune(target, '=') since all valid DNs contain at least one = character. Add "strings" to the import list.
🔧 Suggested fix
- userDN := target
- if len(target) < 3 || (target[:3] != "CN=" && target[:3] != "cn=") {
+ userDN := target
+ if !strings.ContainsRune(target, '=') {
cracked, err := gpdrs.DsCrackNames(rpc, bind.Handle, 7 /* DS_NT4_ACCOUNT_NAME */, 1 /* DS_FQDN_1779_NAME */, []string{c.Domain + "\\" + target})(Add "strings" to the import list.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/js/libs/secretsdump/secretsdump.go` around lines 149 - 160, The
DN-detection currently checks target[:3] for "CN=" which misclassifies valid DNs
that start with other RDNs; update the heuristic in the block that assigns
userDN (and the conditional before calling gpdrs.DsCrackNames) to use
strings.ContainsRune(target, '=') (i.e., treat as DN if it contains '=') instead
of the prefix CN check, and add "strings" to the import list so
strings.ContainsRune can be used; keep the rest of the DsCrackNames calls
(including DS_NT4_ACCOUNT_NAME/DS_UNIQUE_ID_NAME) unchanged.
1 participant
Proposed changes
Closes #7355
Checklist
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores