Align UDP #recvfrom usage with the stdlib#21562
Conversation
|
Thanks for your pull request! As part of our landing process, we manually verify that all modules work as expected. We've added the |
zeroSteiner
force-pushed
the
fix/udp/recvfrom-stdlib
branch
7 times, most recently
from
a050b7f to
e707971
Compare
| def recvfrom_nonblock(length, flags = 0) | ||
| data = super(length, flags)[0] | ||
| sockaddr = super(length, flags)[0] | ||
| sockaddr = super(MAX_SOCKADDR_LENGTH, flags)[0] |
There was a problem hiding this comment.
This fixes a different bug. What was going on here is that the socket address was being read from the peer (the other end of the socket pair), and the length argument was used for it, which caused a failure when the length wasn't sufficient for the address. The second read should always be for enough data to get the whole address.
|
Additional test pipeline started ⌛ |
|
Slice summary:
Note: build results only accessible to maintainers. |
|
Looking at the test suite in Pro, this seems to have introduced some issues with services, where the UDP ports 137 and 161 are not discovered/in the DB: ^ The |
|
Seems like the issue is the This PR: That's the only issue that I saw so far, and the only issue that Pro test suite has caught 👍 |
|
The latest commit fixed the udp_sweep issue: |
zeroSteiner
force-pushed
the
fix/udp/recvfrom-stdlib
branch
from
16aad65 to
9b587dd
Compare
|
Additional test pipeline started ⌛ |
|
Slice summary:
Note: build results only accessible to maintainers. |
|
Additional test pipeline started ⌛ |
|
Looks like something got past the changes again: In Framework (this branch): Confirmed again that it works as expected on the main branch. |
|
Slice summary:
Note: build results only accessible to maintainers. |
|
Additional test pipeline started ⌛ |
|
Slice summary:
Note: build results only accessible to maintainers. |
|
Additional test pipeline started ⌛ |
|
Slice summary:
Note: build results only accessible to maintainers. |
|
Sorry for the series of iterations! It's a series of small mistakes on my part adding up; The changes are good. The previously-failing test in Pro also works correctly after pinning Pro's Rex-Socket version to the git branch in this PR; this had to be done manually -> will have follow-up work on this internally. This was the only blocker and has now been resolved. |
zeroSteiner
force-pushed
the
fix/udp/recvfrom-stdlib
branch
2 times, most recently
from
b883f4a to
1c8c08b
Compare
This pulls in changes from rapid7/rex-socket#83 which aligns #recvfrom to match the signature and behavior of the standard library in regards to it's arguments, return type and how it handles timeouts. The historical usage of #recvfrom was a timed read and can now be replaced with #timed_recvfrom.
zeroSteiner
force-pushed
the
fix/udp/recvfrom-stdlib
branch
from
1c8c08b to
b1ced41
Compare
github-project-automation
Bot
moved this from Ready
to In Progress
in Metasploit Kanban
Release NotesUpdated the usage of rex-socket's |
4 participants
Description
This requires the changes from rapid7/rex-socket#83 and incoporates them into the Metasploit framework to align the usage of
#recvfromon UDP sockets to behave the same as the standard library. This has the strong benefit of ensuring that developers do not need to worry about their socket beingUDPSocketorRex::Socket::Udpwhich by extension makes Metasploit compatible with libraries that need UDP sockets that have no knowledge of Rex.There are three issues with Rex's
#recvfromvs the standard libraryBreaking Changes
rapid7/rex-socket#83 is a breaking change. This PR brings framework inline so that nothing breakes on this side. There are no user-facing breaking changes.
Reviewer Notes
It's not really practical to re-test every single module that uses UDP. That's the main reason that
#timed_recvfromwas added, so methods could just be changed from#recvfrom->#timed_recvfromand not worry about points 1 and 3 from the list of issues above in the description section.Verification Steps
I ran through multiple tests of the most important modules which covered the main patterns. The new
#timed_recvfromis used in place of the old#recvfromwhere there was an expected timeout but the return value is aligned with the standard libraries#recvfrom.Things I tested include
auxiliary/scanner/snmp/snmp_loginauxiliary/gather/enum_dnsauxiliary/scanner/ntp/timeroastTest Evidence
AI Usage Disclosure
I was assisted by Claude Opus during this development.
Pre-Submission Checklist
rubocopon new files with no new offenses (net new files only)msftidyon changed module files with no new offenses (modules only)msftidy_docson changed documentation files with no new offenses (documentation files only)documentation/modules(new modules only)lib/changes)