[fix] Modifying how udp open port detection is made #6599
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/js/global/scripts.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
pkg/js/global/scripts.go
🔇 Additional comments (1)
pkg/js/global/scripts.go (1)
153-153: Good consistency fix!The error message now correctly references
isUDPPortOpeninstead ofisPortOpen, improving debugging clarity.
| defer conn.Close() | ||
|
|
||
| probe := []byte("ping") | ||
| if _, err := conn.Write(probe); err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| if dl, ok := ctx.Deadline(); ok { | ||
| _ = conn.SetReadDeadline(dl) | ||
| } else { | ||
| _ = conn.SetReadDeadline(time.Now().Add(5 * time.Second)) | ||
| } | ||
|
|
||
| buf := make([]byte, 1024) | ||
| _, err = conn.Read(buf) | ||
| if err != nil { | ||
| return false, nil | ||
| } | ||
| return true, nil |
There was a problem hiding this comment.
🧩 Analysis chain
The hardcoded probe will produce false negatives for most UDP services.
The implementation has several significant issues:
-
Protocol-agnostic probe: The hardcoded
"ping"probe won't work for protocol-specific UDP services (DNS, NTP, SNMP, TFTP, etc.). Most UDP services ignore or reject arbitrary data, leading to false negatives where open ports appear closed. -
Inconsistent error handling: Write errors return
(false, err)(line 170), but read errors return(false, nil)(line 182). A read timeout doesn't definitively prove the port is closed—it could mean the service doesn't respond to arbitrary data or has network issues. -
Missing ICMP error differentiation: While the PR description mentions detecting ICMP "port unreachable" responses, the code treats all read errors identically. Go's UDP Read will error on ICMP unreachable, but you should distinguish this from timeouts.
-
Limited reliability: UDP port detection is inherently challenging without protocol-specific probes. Consider documenting this limitation or accepting a service-type parameter to send appropriate protocol messages.
Consider these improvements:
- probe := []byte("ping")
- if _, err := conn.Write(probe); err != nil {
- return false, err
- }
-
- if dl, ok := ctx.Deadline(); ok {
- _ = conn.SetReadDeadline(dl)
- } else {
- _ = conn.SetReadDeadline(time.Now().Add(5 * time.Second))
- }
-
- buf := make([]byte, 1024)
- _, err = conn.Read(buf)
- if err != nil {
- return false, nil
- }
- return true, nil
+ // Send a minimal probe packet
+ probe := []byte("ping")
+ if _, err := conn.Write(probe); err != nil {
+ return false, nil // Write errors typically indicate network issues, not closed ports
+ }
+
+ if dl, ok := ctx.Deadline(); ok {
+ _ = conn.SetReadDeadline(dl)
+ } else {
+ _ = conn.SetReadDeadline(time.Now().Add(5 * time.Second))
+ }
+
+ buf := make([]byte, 1024)
+ _, err = conn.Read(buf)
+ if err != nil {
+ // Check if error is ICMP port unreachable (definitive "closed")
+ if opErr, ok := err.(*net.OpError); ok && opErr.Err.Error() == "connection refused" {
+ return false, nil // Port definitively closed
+ }
+ // Timeout or other error: inconclusive (may be open but unresponsive to arbitrary data)
+ return false, nil
+ }
+ return true, nilNote: Even with these changes, UDP port detection remains unreliable for services that require specific protocol handshakes. Document this limitation in the function description or consider adding a protocol-specific probe parameter.
🏁 Script executed:
#!/bin/bash
# 1. Display the file and specific lines in context
echo "=== Code at lines 166-184 with surrounding context ==="
sed -n '150,200p' pkg/js/global/scripts.go
echo ""
echo "=== Run go fmt on the file to check formatting ==="
go fmt pkg/js/global/scripts.go
echo "Exit code: $?"
echo ""
echo "=== Run go vet on the file ==="
go vet pkg/js/global/scripts.go
echo ""
echo "=== Search for the function containing these lines ==="
sed -n '1,184p' pkg/js/global/scripts.go | tail -100Length of output: 628
🏁 Script executed:
#!/bin/bash
# Display the file and specific lines in context
echo "=== Code at lines 150-200 with surrounding context ==="
sed -n '150,200p' pkg/js/global/scripts.go
echo ""
echo "=== Run go fmt to check formatting ==="
cd pkg/js/global && go fmt scripts.go 2>&1 || true
echo "---"
echo ""
echo "=== Run go vet on the file ==="
cd - > /dev/null && go vet ./pkg/js/global/scripts.go 2>&1 || true
echo ""
echo "=== Extract full function containing lines 166-184 ==="
# Search backwards from line 166 for 'func' to find function start
awk 'NR >= 140 && NR <= 190' pkg/js/global/scripts.go
echo ""
echo "=== Search for error handling patterns with net.OpError ==="
grep -n "OpError\|ICMP\|connection refused" pkg/js/global/scripts.go || echo "No matches found"Length of output: 11816
UDP port detection logic has multiple functional issues requiring fixes.
The code at lines 166-184 has three confirmed issues:
-
Hardcoded probe won't work for protocol-specific UDP services: The
"ping"probe (line 168) won't elicit responses from DNS, NTP, SNMP, TFTP, and other UDP services that require specific protocol handshakes, causing false negatives. -
Inconsistent error handling: Write errors return
(false, err)(line 170), but read errors return(false, nil)(line 182). This inconsistency masks write failures and incorrectly treats all read timeouts as open ports. -
Missing ICMP error differentiation: No code distinguishes ICMP "port unreachable" from read timeouts. Go's UDP Read will error on ICMP unreachable—this should be caught and handled separately, not treated as a timeout.
These issues conflict with the PR description's intent to detect ICMP unreachable responses. The proposed diff additions (ICMP detection and consistent error handling) are necessary improvements, though UDP port detection will remain inherently unreliable for arbitrary services without protocol-specific probes.
🤖 Prompt for AI Agents
In pkg/js/global/scripts.go around lines 166 to 184, fix UDP probe logic: stop
using a hardcoded "ping" payload — send an empty/zero-length probe or accept a
configurable/protocol-specific payload so services that require handshakes
aren't assumed closed; make error handling consistent by returning (false, err)
for any actual write failures and for non-timeout read errors (only treat read
timeouts as a nil error indicating no response); and detect ICMP "port
unreachable" separately by inspecting the read error (e.g., unwrap
net.OpError/syscall errors or use errors.Is) and treat ICMP unreachable as a
definitive closed port (return false with a specific error), while timeouts
should be treated as no response (return false, nil) as intended. Ensure the
read deadline logic remains and document that UDP checks are unreliable without
protocol-specific probes.
|
For context, I started having multiple false positive on a "snmpv3 fingerprint" template a few weeks ago (when it was created) and I believe it comes from the way UDP open port check is made. |
1 participant
Proposed changes
For UDP, an open port is not always easy to detect. Especially, using
Dialon UDP doesn’t really establish a connection, and an error won't necessary pop up if the remote port is closed. The dial may succeed even if the port isn’t open, because UDP is connectionless. I proposed in my piece of code to check if the udp port is open by sending a small packet and wait for a reply or an ICMP “port unreachable” response.I am not quite familiar with the nuclei unit test process. I only tested the change I made on udp open/closed port to check if it was more effective than the previous version of code.
Checklist
Summary by CodeRabbit