-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add --defeat-icmp-ratelimit option for UDP scanning. [Issue #216] #353
Conversation
Hello @sergeykhegay, I have successfully reviewed, built and tested your modifications and everything worked. I will speak to my mentor about it and we'll keep you updated, but I am positive that everything is ready to commit. |
adjust timing as we could have done retransmissions due to conjested network */ | ||
if (rcvdtime != NULL | ||
&& o.defeat_icmp_ratelimit | ||
&& (newstate == PORT_CLOSED || newstate == PORT_FILTERED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to @dmiller-nmap in issue #216, we should:
treat open|filtered UDP ports as closed, and only report the definitely-open ones as up
Thus I would suggest to replace this line (2123) by:
&& (newstate == PORT_CLOSED || (newstate == PORT_OPEN && newstate == PORT_FILTERED))
That way, if the port is closed or if the port is in state open|filtered, we do not want to slow down and thus don't adjust timing and ping.
Waiting for your feedback,
Vincent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello Vincent,
This change would not make any sense because (newstate == PORT_OPEN && newstate == PORT_FILTERED)
will always be false, as new state cannot be both in PORT_OPEN
and PORT_FILTERED
states.
There is another option to change the condition as following:
&& (newstate == PORT_CLOSED || (newstate == PORT_OPENFILTERED))
but, as I have already pointed out, this will not work. By default if we use UDP scan, all ports are initialized with PORT_OPENFILTERED
state. This makes sense because we do not know yet if the port is open/closed/filtered, we can draw our conclusion only when we get an ICMP message (probably meaning that the port is closed or rate limited) or a UDP response (port open) or a timeout (contested network, firewall, rogue server).
Check this code out to understand how a port's state is changed. Basically, when we get an ICMP response during a UDP scan, the port's state can be changed to either PORT_CLOSED
or PORT_FILTERED
. But those ICMP responses might be rate limited, thus do not represent adequate information about probe timings, hence we do not adjust timings in the --defeat-icmp-ratelimit
mode.
Makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @sergeykhegay,
Thanks for your reply with explanation. I understand why it cannot work like that. The thing is that with your code, it will show some ports as open|filtered
which should be closed
if we took the time to slow down and wait for an ICMP response.
After more investigation, it seems very hard to differentiate them, especially because of firewalls and ACL filtering the ICMP responses of the target, leading to even more inaccuracy... I will have to talk to my mentor about it tomorrow.
Talk to you soon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we exactly sacrifice accuracy to get a speed up.
Thanks for your work, and I'm waiting for updates.
@sergeykhegay I've reviewed this and I think it is nearly ready to commit. My only remaining concern is that the default state for timed-out ports is "open|filtered", which means that service scan will try to probe them and NSE will run against them, but they are more likely closed than open. The easiest fix to this seems to be changing --- a/scan_engine.cc
+++ b/scan_engine.cc
@@ -851,7 +851,7 @@ static void set_default_port_state(std::vector<Target *> &targets, stype scantyp
(*target)->ports.setDefaultPortState(IPPROTO_TCP, PORT_OPENFILTERED);
break;
case UDP_SCAN:
- (*target)->ports.setDefaultPortState(IPPROTO_UDP, PORT_OPENFILTERED);
+ (*target)->ports.setDefaultPortState(IPPROTO_UDP, o.defeat_icmp_ratelimit ? PORT_CLOSEDFILTERED : PORT_OPENFILTERED);
break;
case IPPROT_SCAN:
(*target)->ports.setDefaultPortState(IPPROTO_IP, PORT_OPENFILTERED); I think that maybe adding a warning at the end of the scan would be helpful if we did this. Something like "WARNING: Some ports marked closed|filtered may actually be open. For more accurate results, do not use --defeat-icmp-ratelimit" |
@@ -206,7 +206,7 @@ int determineScanGroupSize(int hosts_scanned_so_far, | |||
|
|||
class UltraScanInfo; | |||
|
|||
struct ppkt { /* Beginning of ICMP Echo/Timestamp header */ | |||
struct ppkt { /* Beginning of ICMP Echo/Timestamp header */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change isn't necessary for this patch. If you want to make a separate whitespace-only commit to clean up stuff like this, feel free to do so elsewhere.
PORT_OPENFILTERED _if_ o.defeat_icmp_ratelimit is set. This will prevent service scan probing and NSE running against supposedly closed ports.
possible inaccuracy of the results at the end of the scan. Some ports marked closed|filtered may actually be open.
@@ -2526,6 +2526,9 @@ void printfinaloutput() { | |||
log_write(LOG_PLAIN, "OS detection performed. Please report any incorrect results at https://nmap.org/submit/ .\n"); | |||
else if (o.servicescan) | |||
log_write(LOG_PLAIN, "Service detection performed. Please report any incorrect results at https://nmap.org/submit/ .\n"); | |||
else if (o.udpscan && o.defeat_icmp_ratelimit) | |||
log_write(LOG_PLAIN, "WARNING: Some ports marked closed|filtered may actually be open. For more accurate results, do not use --defeat-icmp-ratelimit .\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to use "Note: Some ports marked ..."
. It did not catch attention as it should have done, so I changed to the initial Daniel's suggestion, warning.
Should I have used error("WARNING: Some ports marked...")
or current version is ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmiller-nmap, @W0naN0w I have made changes as was suggested. Whitespace commit was removed.
This PR patches Issue 216.
As was discussed:
I figured out that Nmap initializes all ports as open|filtered. A port's state is changed only when
some kind of response is received, this also triggers timing adjustment.
One might skim through this email I sent to dev@nmap.org earlier:
I realize that discarding timeout'ed probes is a bad idea. Since UDP does not guarantee
delivery packets might be lost because of congestion.
If we want to scan faster, what we should not do is to adjust timing when Nmap changes
port's state because of ICMP response. Otherwise Nmap will behave as described:
We do not want such behavior. On the other hand we still want Nmap to address
network congestion problems, so we adjust timing when Nmap changes a port's
state to open (a UDP response was received).