Conversation
* parse Darwin routing table with golang.org/x/net/route * add subnet mask to destination * add LinkAddr support * consolidate darwin and freebsd routing logic into one file * use windows api to get routing table instead of os.exec * fix potential nil pointer dereference * fixing minor issues --------- Co-authored-by: Mzack9999 <mzack9999@protonmail.com>
* fix: add system DNS fallback for split‑DNS/VPN resolution * make system resolver optional --------- Co-authored-by: Mzack9999 <mzack9999@protonmail.com>
* fix: scan type inconsistency after scanner initialization If SYN scan type is specified for the runner, but NewScanner failed to acquire a listen handler, NewScanner will fallback to CONNECT scan without updating the runner's scan type option. This causes the runner to later attempt to use raw packet scan with an empty listen handler when it should have used a normal connect scan. * minor fix --------- Co-authored-by: Mzack9999 <mzack9999@protonmail.com>
WalkthroughThis PR adds comprehensive service fingerprinting via Nmap probe integration, including a new fingerprinting engine supporting TCP/UDP detection, probe parsing with regex matching, DNS resolver fallback options, routing consolidation across platforms, and CPE output support. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Scanner
participant PortDiscovery as Port Discovery<br/>(TCP/UDP)
participant Fingerprint as Fingerprinting<br/>Engine
participant ProbeDB as Probe<br/>Database
participant DNS as System DNS<br/>Resolver
participant Results
User->>Scanner: Run scan with --sV
Scanner->>PortDiscovery: Discover open ports (enable TLS detection)
PortDiscovery-->>Scanner: Return discovered IP:ports
Scanner->>Fingerprint: Initialize Engine with options
Fingerprint->>ProbeDB: Load/parse nmap-service-probes
alt Probe file not found
Fingerprint->>Fingerprint: Auto-discover via locate
end
Fingerprint->>Fingerprint: Build target list from discovered ports
loop For each Target
Fingerprint->>DNS: Attempt configured DNS (if available)
alt DNS fails & SystemResolver enabled
Fingerprint->>DNS: Fallback to system resolver
end
Fingerprint->>Fingerprint: Phase 1: Parallel probes<br/>(hinted, NULL)
alt Hard match found
Fingerprint->>Fingerprint: Cancel remaining probes
else Soft match or no match
Fingerprint->>Fingerprint: Phase 2: Sequential fallback<br/>probes (non-hinted)
end
Fingerprint->>Fingerprint: Extract service name/product/<br/>version from matched pattern
Fingerprint-->>Results: Emit StreamResult (IP, Port, Service)
end
Scanner->>Results: Merge fingerprint results into<br/>discovered ports
Scanner-->>User: Output combined scan results<br/>(with CPEs)
sequenceDiagram
participant Probe as Probe Match<br/>Handler
participant TCP as TCP<br/>Connection
participant Pattern as Regex<br/>Pattern Matcher
participant Cache as Response<br/>Cache
Probe->>TCP: Dial target:port with timeout
TCP-->>Probe: Connection established
Probe->>Probe: Send probe payload<br/>(nmap encoded)
loop Read response bytes
TCP-->>Probe: Receive data chunk
Probe->>Cache: Check response cache
alt Cached result exists
Probe-->>Probe: Return cached match
else No cache hit
Probe->>Probe: Convert bytes to Latin-1
Probe->>Pattern: Evaluate regex matches<br/>(with prefix filters)
alt Hard match found
Pattern-->>Probe: Return MatchResult
Probe->>Cache: Store in cache
Probe-->>Probe: Signal match, close conn
else Soft match found
Pattern-->>Probe: Return soft MatchResult
Probe->>Cache: Store in cache
end
end
end
Probe->>Probe: Apply capture group<br/>substitutions to templates
Probe-->>Probe: Return final MatchResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes This diff introduces substantial new functionality across multiple packages with high logic density: the fingerprinting engine and parser contain intricate control flows for probe orchestration, regex matching with optimizations, and response parsing; routing consolidation requires careful validation of platform-specific behavior changes and test coverage; runner integration involves service detection streaming and DNS fallback logic; and extensive new test/benchmark infrastructure demands careful review of correctness and coverage strategy. Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (8)
pkg/port/port.go (1)
23-31: LGTM - Minor optimization suggestion.The refactor to use
fmt.Fprintfdirectly is cleaner. However, line 27 usesfmt.Fprintffor a constant string"/tls"which has unnecessary formatting overhead. Consider usingbuilder.WriteString("/tls")instead.💡 Optional micro-optimization
func (p *Port) StringWithDetails() string { var builder strings.Builder _, _ = fmt.Fprintf(&builder, "%d[%s", p.Port, p.Protocol.String()) if p.TLS { - _, _ = fmt.Fprintf(&builder, "/tls") + _, _ = builder.WriteString("/tls") } - _, _ = fmt.Fprintf(&builder, "]") + _, _ = builder.WriteString("]") return builder.String() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/port/port.go` around lines 23 - 31, In Port.StringWithDetails(), avoid using fmt.Fprintf to write the constant "/tls" (unnecessary formatting overhead); replace the fmt.Fprintf(&builder, "/tls") call with builder.WriteString("/tls") inside the TLS branch so the rest of the function (fmt.Fprintf for the formatted pieces) remains unchanged.pkg/routing/router_windows.go (1)
24-34: No fallback to outbound IP detection on Windows.Unlike the BSD implementation which falls back to
fallbackOutboundRoutes()when both native and netstat methods fail, Windows returns an error directly. This could cause initialization failures on systems where routing table access is restricted but outbound connectivity works.Consider adding a similar fallback for consistency:
💡 Optional: Add outbound IP fallback
func New() (Router, error) { routes, err := fetchRoutesNative() if err != nil { gologger.Debug().Msgf("native Windows routing API failed, falling back to netsh: %v", err) routes, err = fetchRoutesNetsh() } if err != nil { - return nil, err + gologger.Debug().Msgf("netsh fallback failed, falling back to outbound IPs: %v", err) + return fallbackOutboundRoutes() } return &baseRouter{Routes: routes}, nil }Note:
fallbackOutboundRoutes()is currently only defined inrouter_bsd.go. You'd need to extract it torouter.goor duplicate it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/routing/router_windows.go` around lines 24 - 34, The Windows New() should mirror BSD's behavior by attempting fallbackOutboundRoutes() if both fetchRoutesNative() and fetchRoutesNetsh() fail: modify New() to call fetchRoutesNative(), then on error try fetchRoutesNetsh(), and if that also errors, call fallbackOutboundRoutes() and use its returned routes; ensure the function references fetchRoutesNative, fetchRoutesNetsh, fallbackOutboundRoutes, and baseRouter; if fallbackOutboundRoutes is currently only in router_bsd.go, either move it into a shared file (router.go) or duplicate it into the Windows build so the fallback is available.pkg/fingerprint/locate.go (1)
41-88: Consider adding debug logging for probe file discovery.When
LocateNmapProbes()returns an empty string, there's no visibility into which paths were checked. This could make troubleshooting difficult for users.💡 Optional: Add debug logging
func nmapProbePaths() []string { var paths []string + // Note: Paths are checked in order by LocateNmapProbes() switch runtime.GOOS { // ... existing code ... } return paths }Or add logging in
LocateNmapProbes()when no file is found to list searched paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fingerprint/locate.go` around lines 41 - 88, LocateNmapProbes() currently returns an empty string with no visibility into what paths were searched; use nmapProbePaths() to gather the candidate list and log it when no probe file is found. Modify LocateNmapProbes() so that after iterating candidates (via nmapProbePaths()) and before returning "", it emits a debug or info log containing the slice of paths (and optionally which OS/ENV values influenced them), e.g., call the existing logger with a message like "nmap probe not found, searched paths: %v" and include nmapProbePaths() output to aid troubleshooting.pkg/routing/router_test.go (2)
345-358: Unused variablesrcIPin test.Line 357 has
_ = srcIPto silence the compiler, butsrcIP(defined at line 346) is never used meaningfully in the test. Either use it in an assertion or remove it.func TestBaseRouter_RouteWithSrc_HWAddrMatch(t *testing.T) { - srcIP := net.ParseIP("10.0.0.5") routes := []*Route{ {Type: IPv4, Destination: "10.0.0.0/8", NetworkInterface: eth0, Gateway: "10.0.0.1"}, {Type: IPv4, Destination: "192.168.0.0/16", NetworkInterface: eth1, Gateway: "192.168.0.1"}, } router := &baseRouter{Routes: routes} iface, _, _, err := router.RouteWithSrc(eth1.HardwareAddr, nil, net.ParseIP("8.8.8.8")) require.NoError(t, err) assert.Equal(t, eth1, iface) - - _ = srcIP }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/routing/router_test.go` around lines 345 - 358, The test TestBaseRouter_RouteWithSrc_HWAddrMatch defines srcIP but never uses it; remove the unused variable and the no-op `_ = srcIP` (or alternatively use srcIP in the RouteWithSrc call if the intention was to test source IP handling). Specifically, edit the test in the baseRouter tests around the RouteWithSrc invocation (function: RouteWithSrc on type baseRouter) to either pass srcIP into RouteWithSrc or delete the srcIP declaration and the `_ = srcIP` line so the test has no unused variables.
454-495: Unused variables in integration test.Lines 493-494 have
_ = routerand_ = srcIPbut neither is used in the test. The sub-tests callFindRouteForIpdirectly instead of using therouterinstance.💡 Suggested fix: Use router instance or remove
func TestBaseRouter_RealisticRouteTable(t *testing.T) { - srcIP := net.ParseIP("192.168.1.50") - routes := []*Route{ // ... } router := &baseRouter{Routes: routes} t.Run("LAN address uses /24", func(t *testing.T) { - route, err := FindRouteForIp(net.ParseIP("192.168.1.100"), routes) + _, _, _, err := router.Route(net.ParseIP("192.168.1.100")) require.NoError(t, err) - assert.Equal(t, eth0, route.NetworkInterface) - assert.Empty(t, route.Gateway) + // ... adjust assertions for Route() return values }) // ... similar changes for other sub-tests - - _ = router - _ = srcIP }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/routing/router_test.go` around lines 454 - 495, The test declares router and srcIP but never uses them; either remove the unused variables or adjust sub-tests to exercise the baseRouter instance and srcIP. Specifically, either delete the lines assigning _ = router and _ = srcIP, or replace direct calls to FindRouteForIp with calls through the router (e.g., call router.FindRouteForIp or a method on baseRouter that wraps FindRouteForIp) and use srcIP where appropriate (for example, use srcIP as the source address in any call that needs it). Ensure references to baseRouter, router, srcIP and FindRouteForIp are updated consistently so no unused variables remain.pkg/fingerprint/options.go (1)
36-41: Consider validating nil dialer to prevent potential nil pointer dereference.
WithDialeraccepts anyDialFuncincluding nil. If called with nil,e.dialerwould be set to nil, potentially causing a nil pointer dereference during TCP connections. While current callers inrunner.goconstruct valid dialers, a defensive nil check would prevent future misuse.🛡️ Optional defensive fix
func WithDialer(d DialFunc) Option { return func(e *Engine) { + if d != nil { e.dialer = d + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fingerprint/options.go` around lines 36 - 41, The WithDialer Option should defensively handle a nil DialFunc to avoid setting Engine.dialer to nil; update the WithDialer function to check if d == nil and if so return a no-op Option (leave e.dialer unchanged) otherwise set e.dialer = d. This change touches the WithDialer function and the Engine.dialer field so callers like runner.go remain unaffected but future misuse is prevented.pkg/runner/runner_test.go (1)
991-1023: Test relies on global state mutation—ensure tests remain sequential.This test mutates package-level variables (
scan.PkgRouter,privileges.IsPrivileged,scan.ListenHandlers) without synchronization. While thedeferrestores correctly and tests aren't currently parallel, this pattern will cause data races ift.Parallel()is ever added to these tests.Consider documenting this constraint or adding a comment to prevent accidental parallelization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runner/runner_test.go` around lines 991 - 1023, This test mutates package-level globals (scan.PkgRouter, privileges.IsPrivileged, scan.ListenHandlers) which can cause data races if run in parallel; update TestNewRunner_ScanTypeSyncAfterFallback by adding a clear comment above the test stating it intentionally mutates global state and must not be run with t.Parallel(), and either (preferred) protect the mutated globals with a package-level test mutex or (alternatively) document the sequential requirement in the test comment so future maintainers don't add t.Parallel(); reference the symbols scan.PkgRouter, privileges.IsPrivileged, scan.ListenHandlers, and the test name TestNewRunner_ScanTypeSyncAfterFallback when adding the comment or mutex.pkg/fingerprint/bench_test.go (1)
353-357: Avoid a hard wall-clock threshold in this test.The
<2sassertion depends on machine load and scheduler noise, so it can fail spuriously even when probe parallelism is working. This is safer as a benchmark or an opt-in integration test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/fingerprint/bench_test.go` around lines 353 - 357, The test currently fails on noisy machines because it asserts elapsed < 2*time.Second; change it to avoid a hard wall-clock check by either (a) removing the absolute threshold and instead comparing the parallel run against a measured sequential run (compute sequentialElapsed and assert elapsed < sequentialElapsed - some margin) or (b) make the timing assertion opt-in/configurable (use testing.Short or an env var like TEST_TIMING_THRESHOLD) and otherwise only t.Logf the elapsed time; locate the assertion referencing elapsed and the GetRequest/NULL probe comment in bench_test.go to implement the change.
🤖 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/fingerprint/engine_test.go`:
- Around line 197-203: The test currently only logs when an unexpected result is
present for key (fmt.Sprintf("%s:%d", host, port)) but should fail; update the
fast-mode assertion in engine_test.go so that if results[key] exists (indicating
the NULL probe or a non-fast probe ran and GetRequest executed), the test calls
t.Fatalf (or t.Errorf followed by t.FailNow) with a clear message that a result
was produced in fast mode, referencing key and that GetRequest should not have
run; modify the block checking results[key] to fail the test instead of t.Log.
In `@pkg/fingerprint/engine.go`:
- Around line 464-471: The current logic in the fingerprinting read sequence
(use of remaining, conn.SetReadDeadline and the single extra conn.Read call)
stops after at most one 200ms poll and may truncate multi-chunk responses;
change it to loop reads until the overall waitTimeout has elapsed or no more
data arrives: repeatedly set a short read deadline (e.g. 200ms), call conn.Read
and append to response while n>0, break only when a read times out/no data or
the remaining time is exhausted, and then return response and false; update the
code around the existing conn.SetReadDeadline/conn.Read usage so it continually
polls rather than returning after a single follow-up read.
- Around line 157-160: Engine.fingerprintOne currently ignores
ProbeDB.ExcludeTCP and ProbeDB.ExcludeUDP populated by ParseProbes, so excluded
ports are still dialed; update Engine.fingerprintOne to consult e.db.ExcludeTCP
and e.db.ExcludeUDP (or the corresponding exclude sets on the loaded ProbeDB)
before attempting any dial and skip fingerprinting for ports present in those
sets, and apply the same check to the other fingerprinting path referenced in
the review (the dialing loop around lines 684-688) so both code paths honor
ProbeDB.ExcludeTCP/ExcludeUDP.
- Around line 720-722: The UDP branch currently only calls
e.tryMatches(sp.Matches) and thus skips softmatch and fallback probe rules;
update the UDP matching logic so it evaluates soft matches and fallback probes
the same way TCP does: after checking sp.Matches call into the same softmatch
and fallback evaluation flow (e.g., call whatever helper(s) that handle
sp.SoftMatches and fallback probe rules or merge sp.Matches + sp.SoftMatches +
any fallback match set before invoking e.tryMatches), and ensure the final
return still uses matchResultToResult(result, false, "") so UDP results include
softmatch/fallback outcomes just like the TCP path.
- Around line 127-133: The send to results after calling e.fingerprintOne can
block indefinitely; modify the worker goroutine that calls e.fingerprintOne(ctx,
t) (and currently sends StreamResult with r.ToService()) to use a select that
attempts to send the StreamResult to the results channel or returns if
ctx.Done() is closed, and ensure wg.Done() is always executed (preferably via
defer wg.Done() at the start of the goroutine) so the closer goroutine's
wg.Wait() cannot hang; in short, guard the results <- send with a select { case
results<-sr: case <-ctx.Done(): } and guarantee wg.Done() runs even on cancel.
In `@pkg/fingerprint/fingerprint.go`:
- Around line 34-44: The ToService() method on Result is missing mapping for the
Result.DeviceType into port.Service.DeviceType; update the ToService() return
struct (in func (r *Result) ToService()) to set DeviceType: r.DeviceType so the
Result's DeviceType is propagated into the created *port.Service object.
In `@pkg/fingerprint/parser.go`:
- Around line 203-216: The code currently swallows parseMatch errors in the
parse loop (inside ParseProbes) by continuing, causing signatures to be silently
dropped; instead, have the loop propagate the error (do not continue). Replace
the `continue` in both the "match " and "softmatch " branches so that parseMatch
errors are returned (wrap with context including the offending line and whether
it was a match or softmatch) from the enclosing ParseProbes function (or
otherwise bubble the error to the caller), referencing parseMatch, ParseProbes,
current.Matches and current.SoftMatches so failures are visible to callers
rather than silently ignored.
- Around line 545-553: substituteGroups currently replaces capture placeholders
in ascending order which causes "$1" to be substituted inside "$10" etc.; change
the replacement loop in substituteGroups to iterate from the highest capture
index down to 1 (e.g., start at min(len(submatches)-1, 9) and decrement) so that
multi-digit placeholders like "$10" are replaced before "$1"; update the loop in
substituteGroups accordingly using the same strconv.Itoa and strings.ReplaceAll
calls but with descending indices.
- Around line 290-335: The decodeNmapEscapes function currently only decodes a
small whitelist and leaves unknown backslashes in the output (e.g., "\|"), so
update decodeNmapEscapes to treat any unrecognized escape as "literal next byte"
by appending s[i+1] and advancing i by 2 in the switch default branch; also
handle a trailing lone backslash at end of string by appending '\\' and
advancing to terminate. Make this change in the decodeNmapEscapes function to
ensure probes that escape delimiters are decoded correctly.
In `@pkg/routing/router_bsd.go`:
- Around line 246-260: The current IPv6 fallback creates a Route when ip6 is
nil, ignores errors from FindInterfaceByIp and assigns an IPv4-only interface
without a DefaultSourceIP which breaks baseRouter.Route()/FindSourceIpForIp; fix
by removing the blind fallback or by checking the candidate interface for IPv6
addresses before appending: call FindInterfaceByIp only when ip6 != nil and
propagate/handle its error instead of discarding it, and if you must reuse
routes[0].NetworkInterface ensure that interface actually has an IPv6 address
(or set DefaultSourceIP to a valid IPv6) before creating the Route struct
(update the code around ip6, FindInterfaceByIp, Route creation and the fallback
branch accordingly).
In `@pkg/routing/router.go`:
- Around line 63-71: baseRouter.Route currently returns (nil, nil,
route.DefaultSourceIP, nil) when route.DefaultSourceIP is set, causing callers
to get a nil *net.Interface even if the route contains NetworkInterface/Gateway
info; change the early return to return the route's NetworkInterface and Gateway
when present (i.e., return route.NetworkInterface, route.Gateway,
route.DefaultSourceIP, nil) so callers that need iface (e.g., for raw packet
sending) receive it; update the logic in Route (function baseRouter.Route) to
prefer route.NetworkInterface and route.Gateway when route.DefaultSourceIP !=
nil, falling back to nils only if those fields are actually absent.
In `@pkg/runner/healthcheck.go`:
- Line 78: The healthcheck output prints the wrong UDP endpoint (it writes
"scanme.sh:80" while the probe actually dials "scanme.sh:53"); update the
fmt.Fprintf call that writes to test (referenced by testResult and the
fmt.Fprintf call) to report the actual UDP IPv4 endpoint used by the probe (use
the same endpoint/variable used when dialing, e.g., the "scanme.sh:53" value or
the endpoint variable) so the diagnostic message matches the probe target.
In `@pkg/runner/output.go`:
- Around line 50-52: CSVFields currently uses fmt.Sprint which renders the CPEs
[]string as Go's bracketed slice (e.g. "[a b]") making CSV round-tripping
fragile; update the CSVFields implementation to explicitly serialize the CPEs
field (named CPEs on the output struct) into a single string (e.g.,
strings.Join(o.CPEs, ";") or JSON-encode) before returning it for the CSV column
so each CPE is reliably delimited and parseable; locate the CSVFields method
(around lines 161-168) and replace the fmt.Sprint usage for the CPEs case with
an explicit join/encoding step.
In `@pkg/runner/runner.go`:
- Around line 1120-1124: The log reads targetCount too early because the
goroutine that feeds targets increments targetCount concurrently, so move the
counting step before starting that goroutine: compute the total targets (e.g.,
len(targets) or pre-iterate channels/slices to get the count) and assign
targetCount prior to launching the feeding goroutine, then call
gologger.Info().Msgf using that count (or alternatively defer the log until
after the feeder has finished counting); reference variables: targetCount and
r.options.ServiceVersionWorkers and the existing feeder goroutine that populates
targets so you update where the log call and goroutine are ordered.
In `@pkg/scan/scan.go`:
- Around line 182-191: The loop presently downgrades options.ScanType from
TypeSyn to TypeConnect when Acquire fails, which silently flips SYN-only
discovery to CONNECT; instead, when Acquire returns an error and
options.ScanType == TypeSyn, stop downgrading and return or propagate the
acquireErr so the caller fails fast. Modify the loop in pkg/scan/scan.go (the
Acquire(...) call and handling around options.ScanType, TypeSyn, TypeConnect and
scanner.ListenHandler) to remove the automatic assignment options.ScanType =
TypeConnect and the continue path for the SYN case; if Acquire fails while
ScanType is TypeSyn, return acquireErr (or wrap and return) so the caller
enforces the SYN-only policy established in pkg/runner/validate.go.
In `@README.md`:
- Around line 103-104: Update the README help block to document the new native
fingerprinting CLI flags by adding entries for -sD, -sV, -sV-fast, -sV-timeout,
-sV-workers, and -sV-probes alongside the existing options; reference the same
help section that currently lists -dns-order and -sr/-system-resolver and add
concise descriptions and default values for each new flag (e.g., -sD to enable
service detection, -sV to enable version fingerprinting, -sV-fast to use a
reduced probe set, -sV-timeout to set probe timeout, -sV-workers to control
concurrency, -sV-probes to select probe profile) so the README usage output
reflects the new native fingerprinting feature instead of only showing the
deprecated -nmap option.
---
Nitpick comments:
In `@pkg/fingerprint/bench_test.go`:
- Around line 353-357: The test currently fails on noisy machines because it
asserts elapsed < 2*time.Second; change it to avoid a hard wall-clock check by
either (a) removing the absolute threshold and instead comparing the parallel
run against a measured sequential run (compute sequentialElapsed and assert
elapsed < sequentialElapsed - some margin) or (b) make the timing assertion
opt-in/configurable (use testing.Short or an env var like TEST_TIMING_THRESHOLD)
and otherwise only t.Logf the elapsed time; locate the assertion referencing
elapsed and the GetRequest/NULL probe comment in bench_test.go to implement the
change.
In `@pkg/fingerprint/locate.go`:
- Around line 41-88: LocateNmapProbes() currently returns an empty string with
no visibility into what paths were searched; use nmapProbePaths() to gather the
candidate list and log it when no probe file is found. Modify LocateNmapProbes()
so that after iterating candidates (via nmapProbePaths()) and before returning
"", it emits a debug or info log containing the slice of paths (and optionally
which OS/ENV values influenced them), e.g., call the existing logger with a
message like "nmap probe not found, searched paths: %v" and include
nmapProbePaths() output to aid troubleshooting.
In `@pkg/fingerprint/options.go`:
- Around line 36-41: The WithDialer Option should defensively handle a nil
DialFunc to avoid setting Engine.dialer to nil; update the WithDialer function
to check if d == nil and if so return a no-op Option (leave e.dialer unchanged)
otherwise set e.dialer = d. This change touches the WithDialer function and the
Engine.dialer field so callers like runner.go remain unaffected but future
misuse is prevented.
In `@pkg/port/port.go`:
- Around line 23-31: In Port.StringWithDetails(), avoid using fmt.Fprintf to
write the constant "/tls" (unnecessary formatting overhead); replace the
fmt.Fprintf(&builder, "/tls") call with builder.WriteString("/tls") inside the
TLS branch so the rest of the function (fmt.Fprintf for the formatted pieces)
remains unchanged.
In `@pkg/routing/router_test.go`:
- Around line 345-358: The test TestBaseRouter_RouteWithSrc_HWAddrMatch defines
srcIP but never uses it; remove the unused variable and the no-op `_ = srcIP`
(or alternatively use srcIP in the RouteWithSrc call if the intention was to
test source IP handling). Specifically, edit the test in the baseRouter tests
around the RouteWithSrc invocation (function: RouteWithSrc on type baseRouter)
to either pass srcIP into RouteWithSrc or delete the srcIP declaration and the
`_ = srcIP` line so the test has no unused variables.
- Around line 454-495: The test declares router and srcIP but never uses them;
either remove the unused variables or adjust sub-tests to exercise the
baseRouter instance and srcIP. Specifically, either delete the lines assigning _
= router and _ = srcIP, or replace direct calls to FindRouteForIp with calls
through the router (e.g., call router.FindRouteForIp or a method on baseRouter
that wraps FindRouteForIp) and use srcIP where appropriate (for example, use
srcIP as the source address in any call that needs it). Ensure references to
baseRouter, router, srcIP and FindRouteForIp are updated consistently so no
unused variables remain.
In `@pkg/routing/router_windows.go`:
- Around line 24-34: The Windows New() should mirror BSD's behavior by
attempting fallbackOutboundRoutes() if both fetchRoutesNative() and
fetchRoutesNetsh() fail: modify New() to call fetchRoutesNative(), then on error
try fetchRoutesNetsh(), and if that also errors, call fallbackOutboundRoutes()
and use its returned routes; ensure the function references fetchRoutesNative,
fetchRoutesNetsh, fallbackOutboundRoutes, and baseRouter; if
fallbackOutboundRoutes is currently only in router_bsd.go, either move it into a
shared file (router.go) or duplicate it into the Windows build so the fallback
is available.
In `@pkg/runner/runner_test.go`:
- Around line 991-1023: This test mutates package-level globals (scan.PkgRouter,
privileges.IsPrivileged, scan.ListenHandlers) which can cause data races if run
in parallel; update TestNewRunner_ScanTypeSyncAfterFallback by adding a clear
comment above the test stating it intentionally mutates global state and must
not be run with t.Parallel(), and either (preferred) protect the mutated globals
with a package-level test mutex or (alternatively) document the sequential
requirement in the test comment so future maintainers don't add t.Parallel();
reference the symbols scan.PkgRouter, privileges.IsPrivileged,
scan.ListenHandlers, and the test name TestNewRunner_ScanTypeSyncAfterFallback
when adding the comment or mutex.
🪄 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: 5daf89a4-5dfb-4b60-beb2-7f0db7980eb1
📒 Files selected for processing (34)
.gitignoreREADME.mdgo.modpkg/fingerprint/bench_test.gopkg/fingerprint/engine.gopkg/fingerprint/engine_test.gopkg/fingerprint/fingerprint.gopkg/fingerprint/locate.gopkg/fingerprint/nmap_comparison_test.gopkg/fingerprint/options.gopkg/fingerprint/parser.gopkg/fingerprint/parser_test.gopkg/port/port.gopkg/routing/router.gopkg/routing/router_bsd.gopkg/routing/router_darwin.gopkg/routing/router_freebsd.gopkg/routing/router_test.gopkg/routing/router_windows.gopkg/runner/default.gopkg/runner/healthcheck.gopkg/runner/options.gopkg/runner/output.gopkg/runner/output_test.gopkg/runner/runner.gopkg/runner/runner_test.gopkg/runner/util.gopkg/runner/util_test.gopkg/runner/validate.gopkg/runner/validate_test.gopkg/scan/option.gopkg/scan/scan.gopkg/scan/scan_common.gopkg/scan/scan_type_test.go
💤 Files with no reviewable changes (2)
- pkg/routing/router_darwin.go
- pkg/routing/router_freebsd.go
| key := fmt.Sprintf("%s:%d", host, port) | ||
| // NULL probe sends nothing, server sends nothing back (waiting for GET), | ||
| // so no match expected in fast mode | ||
| if _, ok := results[key]; ok { | ||
| // It's ok if NULL probe somehow matches, but GetRequest shouldn't have run | ||
| t.Log("got result in fast mode (from NULL probe)") | ||
| } |
There was a problem hiding this comment.
Make the fast-mode test fail on unexpected results.
This server only responds after it receives GET, and the target port is intentionally not hinted. A populated result here means a non-fast probe ran, but the test currently logs and still passes.
Suggested fix
- if _, ok := results[key]; ok {
- // It's ok if NULL probe somehow matches, but GetRequest shouldn't have run
- t.Log("got result in fast mode (from NULL probe)")
- }
+ if _, ok := results[key]; ok {
+ t.Fatalf("expected no result in fast mode on a non-hinted port")
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/fingerprint/engine_test.go` around lines 197 - 203, The test currently
only logs when an unexpected result is present for key (fmt.Sprintf("%s:%d",
host, port)) but should fail; update the fast-mode assertion in engine_test.go
so that if results[key] exists (indicating the NULL probe or a non-fast probe
ran and GetRequest executed), the test calls t.Fatalf (or t.Errorf followed by
t.FailNow) with a clear message that a result was produced in fast mode,
referencing key and that GetRequest should not have run; modify the block
checking results[key] to fail the test instead of t.Log.
| r := e.fingerprintOne(ctx, t) | ||
| if r != nil { | ||
| results <- StreamResult{ | ||
| IP: t.IP, | ||
| Port: t.Port, | ||
| Service: r.ToService(), | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let me check the full FingerprintStream function and surrounding context
fd -t f "engine.go" pkg/fingerprint/ --exec cat -n {}Repository: projectdiscovery/naabu
Length of output: 24596
🏁 Script executed:
# Also check the consumer loop mentioned at pkg/runner/runner.go:1085-1135
fd -t f "runner.go" pkg/runner/ --exec sed -n '1080,1140p' {}Repository: projectdiscovery/naabu
Length of output: 2023
🏁 Script executed:
# Get the full context around lines 127-133 to understand the goroutine management
fd -t f "engine.go" pkg/fingerprint/ --exec bash -c 'cat -n {} | sed -n "100,200p"'Repository: projectdiscovery/naabu
Length of output: 3335
🏁 Script executed:
rg "DefaultWorkers|const.*workers|workers.*=" pkg/fingerprint/engine.go -B2 -A2Repository: projectdiscovery/naabu
Length of output: 211
🏁 Script executed:
rg "DefaultWorkers|DefaultTimeout" pkg/fingerprint/ -r '{{}}'Repository: projectdiscovery/naabu
Length of output: 268
🏁 Script executed:
cat -n pkg/fingerprint/options.go | head -50Repository: projectdiscovery/naabu
Length of output: 1294
Guard the stream send with ctx.Done() to prevent worker goroutines from blocking indefinitely.
Worker goroutines can block on the results <- send (line 129) if the buffer fills or the consumer exits. When blocked, they never call wg.Done(), causing wg.Wait() in the closer goroutine to hang forever and preventing results from being closed. This deadlocks the consumer's range loop.
Suggested fix
r := e.fingerprintOne(ctx, t)
if r != nil {
- results <- StreamResult{
- IP: t.IP,
- Port: t.Port,
- Service: r.ToService(),
- }
+ sr := StreamResult{
+ IP: t.IP,
+ Port: t.Port,
+ Service: r.ToService(),
+ }
+ select {
+ case results <- sr:
+ case <-ctx.Done():
+ return
+ }
}📝 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.
| r := e.fingerprintOne(ctx, t) | |
| if r != nil { | |
| results <- StreamResult{ | |
| IP: t.IP, | |
| Port: t.Port, | |
| Service: r.ToService(), | |
| } | |
| r := e.fingerprintOne(ctx, t) | |
| if r != nil { | |
| sr := StreamResult{ | |
| IP: t.IP, | |
| Port: t.Port, | |
| Service: r.ToService(), | |
| } | |
| select { | |
| case results <- sr: | |
| case <-ctx.Done(): | |
| return | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/fingerprint/engine.go` around lines 127 - 133, The send to results after
calling e.fingerprintOne can block indefinitely; modify the worker goroutine
that calls e.fingerprintOne(ctx, t) (and currently sends StreamResult with
r.ToService()) to use a select that attempts to send the StreamResult to the
results channel or returns if ctx.Done() is closed, and ensure wg.Done() is
always executed (preferably via defer wg.Done() at the start of the goroutine)
so the closer goroutine's wg.Wait() cannot hang; in short, guard the results <-
send with a select { case results<-sr: case <-ctx.Done(): } and guarantee
wg.Done() runs even on cancel.
| func (e *Engine) fingerprintOne(ctx context.Context, t Target) *Result { | ||
| if e.db == nil || len(e.db.Probes) == 0 { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Honor the loaded probe file's exclude sets.
ParseProbes populates ProbeDB.ExcludeTCP and ProbeDB.ExcludeUDP, but neither fingerprint path checks them before dialing. Excluded ports from the loaded probe file will still be actively fingerprinted.
Also applies to: 684-688
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/fingerprint/engine.go` around lines 157 - 160, Engine.fingerprintOne
currently ignores ProbeDB.ExcludeTCP and ProbeDB.ExcludeUDP populated by
ParseProbes, so excluded ports are still dialed; update Engine.fingerprintOne to
consult e.db.ExcludeTCP and e.db.ExcludeUDP (or the corresponding exclude sets
on the loaded ProbeDB) before attempting any dial and skip fingerprinting for
ports present in those sets, and apply the same check to the other
fingerprinting path referenced in the review (the dialing loop around lines
684-688) so both code paths honor ProbeDB.ExcludeTCP/ExcludeUDP.
| if remaining > 200*time.Millisecond { | ||
| _ = conn.SetReadDeadline(time.Now().Add(200 * time.Millisecond)) | ||
| n2, _ := conn.Read(buf) | ||
| if n2 > 0 { | ||
| response = append(response, buf[:n2]...) | ||
| } | ||
| } | ||
| return response, false |
There was a problem hiding this comment.
Don't truncate responses after a single follow-up read.
After the first successful read this returns after at most one extra 200ms poll, even when waitTimeout is much larger. Services that split banners across more chunks or slightly slower gaps will be missed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/fingerprint/engine.go` around lines 464 - 471, The current logic in the
fingerprinting read sequence (use of remaining, conn.SetReadDeadline and the
single extra conn.Read call) stops after at most one 200ms poll and may truncate
multi-chunk responses; change it to loop reads until the overall waitTimeout has
elapsed or no more data arrives: repeatedly set a short read deadline (e.g.
200ms), call conn.Read and append to response while n>0, break only when a read
times out/no data or the remaining time is exhausted, and then return response
and false; update the code around the existing conn.SetReadDeadline/conn.Read
usage so it continually polls rather than returning after a single follow-up
read.
| if result := e.tryMatches(sp.Matches, buf[:n]); result != nil { | ||
| return matchResultToResult(result, false, "") | ||
| } |
There was a problem hiding this comment.
UDP matching ignores softmatch and fallback rules.
This branch only checks sp.Matches. Any UDP fingerprint that relies on softmatch or fallback probe rules will never surface, even though the TCP path supports both.
Suggested fix
- if result := e.tryMatches(sp.Matches, buf[:n]); result != nil {
- return matchResultToResult(result, false, "")
- }
+ hardSets := e.collectMatchSets(sp)
+ if hard, soft := e.matchResponse(sp, hardSets, buf[:n]); hard != nil {
+ return matchResultToResult(hard, false, "")
+ } else if soft != nil {
+ return matchResultToResult(soft, false, "")
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/fingerprint/engine.go` around lines 720 - 722, The UDP branch currently
only calls e.tryMatches(sp.Matches) and thus skips softmatch and fallback probe
rules; update the UDP matching logic so it evaluates soft matches and fallback
probes the same way TCP does: after checking sp.Matches call into the same
softmatch and fallback evaluation flow (e.g., call whatever helper(s) that
handle sp.SoftMatches and fallback probe rules or merge sp.Matches +
sp.SoftMatches + any fallback match set before invoking e.tryMatches), and
ensure the final return still uses matchResultToResult(result, false, "") so UDP
results include softmatch/fallback outcomes just like the TCP path.
| testResult = fmt.Sprintf("Ko (%s)", err) | ||
| } | ||
| test.WriteString(fmt.Sprintf("UDP IPv4 connectivity to scanme.sh:80 => %s\n", testResult)) | ||
| _, _ = fmt.Fprintf(&test, "UDP IPv4 connectivity to scanme.sh:80 => %s\n", testResult) |
There was a problem hiding this comment.
Report the actual UDP IPv4 endpoint being checked.
This line says scanme.sh:80, but the probe above dials scanme.sh:53. The healthcheck output will point users at the wrong endpoint when UDP diagnostics fail.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/runner/healthcheck.go` at line 78, The healthcheck output prints the
wrong UDP endpoint (it writes "scanme.sh:80" while the probe actually dials
"scanme.sh:53"); update the fmt.Fprintf call that writes to test (referenced by
testResult and the fmt.Fprintf call) to report the actual UDP IPv4 endpoint used
by the probe (use the same endpoint/variable used when dialing, e.g., the
"scanme.sh:53" value or the endpoint variable) so the diagnostic message matches
the probe target.
| Version string `json:"version,omitempty"` | ||
| Confidence int `json:"confidence,omitempty"` | ||
| CPEs []string `json:"cpes,omitempty" csv:"cpes"` |
There was a problem hiding this comment.
Serialize cpes explicitly before exporting CSV.
CSVFields on Lines 161-168 stringifies values with fmt.Sprint, so this []string will be emitted as Go's bracketed slice format ([a b]). That is hard to parse back reliably and becomes ambiguous as soon as any CPE contains spaces.
💡 Minimal fix
func (r *Result) CSVFields(excludedFields []string) ([]string, error) {
data := *r
if len(excludedFields) > 0 {
if filteredData, err := structs.FilterStruct(data, nil, excludedFields); err == nil {
data = filteredData
}
}
var fields []string
vl := reflect.ValueOf(data)
ty := reflect.TypeOf(data)
for i := 0; i < vl.NumField(); i++ {
field := vl.Field(i)
csvTag := ty.Field(i).Tag.Get("csv")
fieldValue := field.Interface()
+ if ty.Field(i).Name == "CPEs" {
+ fieldValue = strings.Join(data.CPEs, ";")
+ }
if slices.Contains(headers, csvTag) {
fields = append(fields, fmt.Sprint(fieldValue))
}
}
return fields, nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/runner/output.go` around lines 50 - 52, CSVFields currently uses
fmt.Sprint which renders the CPEs []string as Go's bracketed slice (e.g. "[a
b]") making CSV round-tripping fragile; update the CSVFields implementation to
explicitly serialize the CPEs field (named CPEs on the output struct) into a
single string (e.g., strings.Join(o.CPEs, ";") or JSON-encode) before returning
it for the CSV column so each CPE is reliably delimited and parseable; locate
the CSVFields method (around lines 161-168) and replace the fmt.Sprint usage for
the CPEs case with an explicit join/encoding step.
| if targetCount == 0 { | ||
| gologger.Info().Msgf("Fingerprinting open port(s) with %d workers", r.options.ServiceVersionWorkers) | ||
| } else { | ||
| gologger.Info().Msgf("Fingerprinting %d open port(s) with %d workers", targetCount, r.options.ServiceVersionWorkers) | ||
| } |
There was a problem hiding this comment.
Race condition: targetCount is always 0 when logged.
The goroutine at lines 1093-1118 feeds targets and increments targetCount, but this check runs immediately after the goroutine starts. Since the goroutine executes concurrently, targetCount will almost always be 0 at this point, making the log message misleading.
🐛 Suggested fix: count targets before starting goroutine or use a different logging approach
+ // Pre-count targets for logging
+ var targetCount int
+ for hostResult := range scanResults.GetIPsPorts() {
+ for range hostResult.Ports {
+ targetCount++
+ }
+ }
+
+ if targetCount == 0 {
+ gologger.Info().Msgf("No open ports to fingerprint")
+ return
+ }
+
+ gologger.Info().Msgf("Fingerprinting %d open port(s) with %d workers", targetCount, r.options.ServiceVersionWorkers)
+
targetCh := make(chan fingerprint.Target, r.options.ServiceVersionWorkers*2)
resultCh := engine.FingerprintStream(ctx, targetCh)
// Feed targets in a goroutine while collecting results concurrently.
- var targetCount int
go func() {
for hostResult := range scanResults.GetIPsPorts() {
// ... rest of goroutine
- targetCount++
}
close(targetCh)
}()
-
- if targetCount == 0 {
- gologger.Info().Msgf("Fingerprinting open port(s) with %d workers", r.options.ServiceVersionWorkers)
- } else {
- gologger.Info().Msgf("Fingerprinting %d open port(s) with %d workers", targetCount, r.options.ServiceVersionWorkers)
- }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/runner/runner.go` around lines 1120 - 1124, The log reads targetCount too
early because the goroutine that feeds targets increments targetCount
concurrently, so move the counting step before starting that goroutine: compute
the total targets (e.g., len(targets) or pre-iterate channels/slices to get the
count) and assign targetCount prior to launching the feeding goroutine, then
call gologger.Info().Msgf using that count (or alternatively defer the log until
after the feeder has finished counting); reference variables: targetCount and
r.options.ServiceVersionWorkers and the existing feeder goroutine that populates
targets so you update where the log call and goroutine are ordered.
| for { | ||
| handler, acquireErr := Acquire(options) | ||
| if acquireErr == nil { | ||
| scanner.ListenHandler = handler | ||
| break | ||
| } | ||
| if options.ScanType == TypeSyn { | ||
| gologger.Info().Msgf("syn scan is not possible, falling back to connect scan") | ||
| options.ScanType = "c" | ||
| goto acquire | ||
| options.ScanType = TypeConnect | ||
| continue |
There was a problem hiding this comment.
Don’t silently downgrade SYN-only host discovery to CONNECT mode.
pkg/runner/validate.go Lines 40-43 already enforce host discovery as SYN-only, but this fallback flips the scanner to TypeConnect whenever Acquire runs out of raw handlers. In concurrent runs, -sn / -wn can therefore lose the ARP/ICMP/raw TCP path instead of failing fast with a clear error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/scan/scan.go` around lines 182 - 191, The loop presently downgrades
options.ScanType from TypeSyn to TypeConnect when Acquire fails, which silently
flips SYN-only discovery to CONNECT; instead, when Acquire returns an error and
options.ScanType == TypeSyn, stop downgrading and return or propagate the
acquireErr so the caller fails fast. Modify the loop in pkg/scan/scan.go (the
Acquire(...) call and handling around options.ScanType, TypeSyn, TypeConnect and
scanner.ListenHandler) to remove the automatic assignment options.ScanType =
TypeConnect and the continue path for the SYN case; if Acquire fails while
ScanType is TypeSyn, return acquireErr (or wrap and return) so the caller
enforces the SYN-only policy established in pkg/runner/validate.go.
| -dns-order string dns resolution order (p/l/lp/pl) (default "l") | ||
| -sr, -system-resolver use system DNS as fallback resolver |
There was a problem hiding this comment.
Document the new native fingerprinting flags in this help block.
This PR adds -sD, -sV, -sV-fast, -sV-timeout, -sV-workers, and -sV-probes, but the README usage output still omits them and only exposes the deprecated external -nmap path. Users following the README will miss the new feature entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 103 - 104, Update the README help block to document
the new native fingerprinting CLI flags by adding entries for -sD, -sV,
-sV-fast, -sV-timeout, -sV-workers, and -sV-probes alongside the existing
options; reference the same help section that currently lists -dns-order and
-sr/-system-resolver and add concise descriptions and default values for each
new flag (e.g., -sD to enable service detection, -sV to enable version
fingerprinting, -sV-fast to use a reduced probe set, -sV-timeout to set probe
timeout, -sV-workers to control concurrency, -sV-probes to select probe profile)
so the README usage output reflects the new native fingerprinting feature
instead of only showing the deprecated -nmap option.
Neo - PR Security ReviewNo security issues found Comment |
Native Service Fingerprinting (
-sV)Adds a built-in nmap-compatible service fingerprinting engine that runs directly inside naabu, no external nmap binary required. The engine parses standard
nmap-service-probesfiles (11,951 match rules, 1,200+ identifiable services), sends protocol-specific probes to discovered ports, and matches responses against regex patterns to identify services, versions, and CPEs. Supports TLS-aware probing, probe intensity levels, fast mode, configurable concurrency, and custom probe files.Benchmarked against nmap 7.98 on 8 mock TCP services (SSH, FTP, SMTP, IMAP, POP3, MySQL, HTTP nginx, HTTP Apache):
New flags:
-sD(service discovery),-sV(service version),-sV-fast(port-hinted only),-sV-timeout,-sV-workers,-sV-probes.Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes