Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix race condition when scanning short ranges #16617

Merged
merged 3 commits into from
Jul 8, 2022

Conversation

NikitaKovaljov
Copy link
Contributor

@NikitaKovaljov NikitaKovaljov commented May 24, 2022

  • This change fix race condition in ARP while scanning hosts

Reference to Issue

Verification without changes

  • run ./msfconsole

  • use ipv6_neighbor.rb module without changes.

  • set rhosts 10.0.0.1-6

  • run module run or exploit

Current behavior

[*] Discovering IPv4 nodes via ARP...
[+]           10.0.0.2 ALIVE
[*] Discovering IPv6 addresses for IPv4 nodes...
[*] 
[*] Scanned 6 of 6 hosts (100% complete)
[*] Auxiliary module execution completed

Verification with changes

  • run ./msfconsole

  • use ipv6_neighbor.rb module with changes.

  • set rhosts 10.0.0.1-6

  • run module run or exploit

Fixed behavior

[*] Discovering IPv4 nodes via ARP...
[+]           10.0.0.2 ALIVE
[+]           10.0.0.6 ALIVE
[*] Discovering IPv6 addresses for IPv4 nodes...
[*] 
[*] Scanned 6 of 6 hosts (100% complete)
[*] Auxiliary module execution completed

Co-authored-by: Spencer McIntyre <58950994+smcintyre-r7@users.noreply.github.com>
@smcintyre-r7
Copy link
Contributor

Yeah that makes sense. A configurable sleep probably would be the best option. I wasn't entirely sure that the time should scale with the hosts either, in which case I was thinking a minimum was probably the safest option.

Right now it looks like the TIMEOUT datastore option is defaulting to 500, probably from the Capture mixin that's included. If you want to use that datastore option, which again makes sense, you probably just want to update that default value.

@smcintyre-r7
Copy link
Contributor

@NikitaKovaljov just wondering if you have any thoughts regarding the above comments or if we want to keep things as is?

Just want to make sure this isn't forgotten about.

@NikitaKovaljov
Copy link
Contributor Author

NikitaKovaljov commented Jun 29, 2022

@NikitaKovaljov just wondering if you have any thoughts regarding the above comments or if we want to keep things as is?

Just want to make sure this isn't forgotten about.

Hi @smcintyre-r7 ,

Just now added our idea with datastore['TIMEOUT'] - tested locally seems legit.

Now waiting for github check to complete.

Copy link
Contributor

@smcintyre-r7 smcintyre-r7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good now and I confirmed this fixed the original issue. In the screenshot below you can see the options are the same and when scanning 2 hosts, the race condition would cause the unpatched version to miss the hosts that were online. I ran this a few times and observed that the patched module consistently reported the results while the unpatched was hit or miss.

image

I'll get this landed in a moment, thanks for the bug fix @NikitaKovaljov !

@smcintyre-r7 smcintyre-r7 merged commit 781597b into rapid7:master Jul 8, 2022
@smcintyre-r7
Copy link
Contributor

Release Notes

This fixes a race condition that was present in the ipv6_neighbor module that would cause hosts to be missed when the scanned range was very short due to an adaptive timeout with an insufficient floor value.

@smcintyre-r7 smcintyre-r7 added the rn-fix release notes fix label Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug module rn-fix release notes fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants