Skip to content

Fix destination port changes unexpectedly during scan#34

Open
f10d0 wants to merge 1 commit into
trailofbits:masterfrom
f10d0:master
Open

Fix destination port changes unexpectedly during scan#34
f10d0 wants to merge 1 commit into
trailofbits:masterfrom
f10d0:master

Conversation

@f10d0
Copy link
Copy Markdown

@f10d0 f10d0 commented Mar 12, 2025

There is a bug where whenever the scanner receives a reply from another port than the set destination SNMP port (161) all subsequently send probe packets will be send to this port.

This is because send and receive use the same sockaddr_in struct (remote_addr). Incoming replies from a rogue target device can alter the destination port in the next iteration and therefore make the scanner stop working correctly and send unsolicited packets.

I fixed this by setting the destination port before each send iteration together with the next destination address.

@dguido
Copy link
Copy Markdown
Member

dguido commented Aug 30, 2025

PR Review: ✅ MERGE RECOMMENDED

This PR fixes a bug that can cause scanner failure. Excellent catch, @f10d0!

The Bug

The scanner reuses the same remote_addr struct for both sending (sendto) and receiving (recvfrom). When recvfrom() is called at line 875, it overwrites the entire struct with the source address of the response - including the source port. If any device responds from a non-standard port (not 161), all subsequent probes get sent to the wrong port, breaking the scan for all remaining hosts.

The Fix

Moving remote_addr.sin_port = htons(o.port); inside the loop ensures the correct destination port is always set before each sendto(). This is the correct fix.

Why This Should Be Merged

  1. Real Bug: This isn't theoretical - any device responding from a non-standard source port will corrupt the scanner's destination port for all subsequent hosts
  2. Correct Fix: The solution properly addresses the root cause without side effects
  3. Minimal Change: Just 1 line moved - perfectly matches onesixtyone's tight coding style
  4. Zero Performance Impact: htons() is typically a macro doing bit-shifting; adds nanoseconds to operations taking milliseconds
  5. No Breaking Changes: The change is isolated and maintains all existing behavior

Code Quality

The fix exemplifies onesixtyone's philosophy:

  • Extremely minimal (1 line moved)
  • No new variables or complexity
  • Clear and obvious what it does
  • Follows existing patterns

This is a textbook example of a good bug fix - minimal, correct, and essential.

cc @alexsotirov for merge approval

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants