Re: CVS commit: src

[jnemeth%victoria.tc.ca@localhost (John Nemeth)]

On Dec 15,  1:36pm, David Laight wrote:
} On Fri, Jul 25, 2008 at 09:49:11AM +0200, Wolfgang Solfrank wrote:
} > 
} > Darren Reed wrote:
} > > cvs rdiff -r1.36 -r1.37 src/sys/dist/ipf/netinet/ip_nat.c
} > 
} > Hmm, this has an interesting number of bugs in just one statement ;-)
} > 
} >                                 port = ipf_random() % (ntohs(np->in_pmax) -
} >                                                        ntohs(np->in_pmin));
} > 
} > I think what you meant here is something like:
} > 
} >                                 port = ntohs(ipf_random() % (np->in_pmax - 
} >                                 np->in_pmin)
} >                                              + np->in_pmin);
} > 
} > i.e., the placement of ntohs is wrong, plus the offset of the port number 
} > is missing.
} 
} I suspect you don't want to rely on ntohs() only evaluating its arg once.
} so:
}               port = ipf_random() % (np->in_pmax - np->in_pmin);
}               port = ntohs(port);

     If np->in_pmax and np->in_pmin are in network order, then you
definitely want them in host order in order to do math on them.
However, it seems weird to me that they would be in network order since
that wouldn't be suitable for comparison purposes.  But, pretty much
every other use of them runs them through ntohs() first, so I believe
this use is correct.  ipf_random is simply arc4random() which returns a
32 bit random number.  Since in_pmax - in_pmin simply gives the range
of ports, I believe Wolfgang is correct that in_pmin needs to be added
to result.  Thus the code should be:

                                port = ntohs(np->in_pmin) + ipf_random() %
                                  (ntohs(np->in_pmax) - ntohs(np->in_pmin));

Of much greater concern is that I'm not seeing a test for collision.  I
haven't done an indepth analysis of the code, but given that we're
looking at a space of <= 16 bits, a collision is a very real
possibility on a busy server so it is something that definitely needs
to be handled.

}-- End of excerpt from David Laight