Re: Spurious port closed bug fix

[sean rivera <sean.au.rivera () gmail com>]

Here is an updated version of the patch

--- scan_engine.cc      (revision 29646)
+++ scan_engine.cc      (working copy)
@@ -117,7 +117,10 @@
 using namespace std;
 extern NmapOps o;
 class UltraScanInfo;
+static u16 base_port;

+static bool firstTime=true;
+
 /* A few extra performance tuning parameters specific to ultra_scan. */
 struct ultra_scan_performance_vars : public scan_performance_vars {
   /* When a successful ping response comes back, it counts as this many
@@ -2476,7 +2479,7 @@
               || seq32_decode(USI, ntohl(tcp->th_seq), &tryno, &pingseq);
   } else {
     /* Get the values from the destination port (our source port). */
-    sport_decode(USI, o.magic_port, ntohs(tcp->th_dport), &tryno, &pingseq);
+    sport_decode(USI, base_port, ntohs(tcp->th_dport), &tryno, &pingseq);
     goodseq = true;
   }

@@ -3356,7 +3359,7 @@
   if (o.magic_port_set)
     sport = o.magic_port;
   else
-    sport = sport_encode(USI, o.magic_port, tryno, pingseq);
+    sport = sport_encode(USI, base_port, tryno, pingseq);

   probe->tryno = tryno;
   probe->pingseq = pingseq;
@@ -5261,7 +5264,7 @@
         if (o.magic_port_set) {
           trynum = probe->tryno;
         } else {
-          sport_decode(USI, o.magic_port, ntohs(udp->uh_dport), &trynum, NULL);
+          sport_decode(USI, base_port, ntohs(udp->uh_dport), &trynum, NULL);
         }

         /* Sometimes we get false results when scanning localhost with
@@ -5676,6 +5679,14 @@
   UltraScanInfo *USI = NULL;
   o.current_scantype = scantype;

+  if (firstTime) {
+         base_port= 33000 + (get_random_uint() % 31000);
+  }
+
+  if(base_port >= (65536 - 256)){
+         base_port = 33000; //Because we start out at 33000 everytime
+  }
+         base_port +=256
   init_payloads(); /* Load up _all_ payloads into a mapped table */

   if (Targets.size() == 0) {


On Sun, Aug 19, 2012 at 3:14 PM, David Fifield <david () bamsoftware com> wrote:
> On Fri, Aug 17, 2012 at 09:21:40PM -0600, sean rivera wrote:
> > I've written a very simple patch for the spurious port closed bug
> > (below). All it does is create a local version of the o.magic_port
> > variable and increments it by 256 every time we reach a new phase in
> > the scan.
>
> Thanks for this, Sean. Here are some comments inline.
>
> > +/*I chose to declare this here in the same place as o in order to keep the code base as similar as possible*/
> > +static u16 magic_port = o.magic_port;
> > +
>
> I think this initialization might have a problem known as the "static
> initialization order fiasco." The problem is that the global o object
> may be initialized before or after this assignment.
>
> http://www.yosefk.com/c++fqa/ctors.html#fqa-10.12
>
> I think what we should rather do is initialize this variable to a static
> value not depending on another global object.
>
> >    init_payloads(); /* Load up _all_ payloads into a mapped table */
> > -
> > +  if (magic_port > 65278) { //Catch overflow
> > +    magic_port=33000; //Because we start out at 33000 everytime;
> > +  }
> > +  magic_port+=256;
>
> The magic numnber 65278 needs some explanation. I see it is 2^16 - 258;
> I guess that's because you want to leave at least tryno_cap + 256
> headroom? How about just capping it at 65536 - 256? It should be written
> that way in the source code.
>
> Here's what I suggest for implementing this fix. Let's rename the
> file-scope variable magic_port to base_port so there's no ore confusion
> with the global o.magic_port. Remove this initialization from NmapOps.cc:
>   magic_port = 33000 + (get_random_uint() % 31000);
> and instead do this to base_port on the first call to ultra_scan. You
> can use a boolean variable or a distinguished value in the static
> initialization. The rest of the patch should work as you wrote it.
>
> David Fifield



-- 

~Sean Rivera
_______________________________________________
Sent through the nmap-dev mailing list
http://cgi.insecure.org/mailman/listinfo/nmap-dev
Archived at http://seclists.org/nmap-dev/