Proposed patch for Port Randomization modifications according to RFC6056

[Bjoern A. Zeeb]

On Sat, 5 Mar 2011, Doug Barton wrote:

Hi,

as you may have noticed, I had committed logical upfront changes to
the current code this weekend, to make it easier for anyone to later
understand what happened, when looking at revision history.

I have updated the patch for HEAD and it can be found here:
http://people.freebsd.org/~bz/20110313-01-rfc6056.diff

Looking through the functional changes and the RFC while working on
the ip(4) man page update I though noticed that several alogrithms
could need implementation improvements.  Given the replies I thought
the functional review had long happened.

Could someone please go and verify:
Algo 2:
 	- could we miss a port due to the % missing the +1?
Algo 3:
 	- we do not increment *lastport (next_ephemeral) as suggested
 	  in the RFC.
Algo 3+4+5:
 	- check the %s for +1 needed as well
 	- factor out the system boot time changes (as suggested in
 	  the RFC) to a VNET_SYSINIT?
Algo 3+4:
 	- doing a secret key per request in my view changes the
 	  suggested per (laddr, faddr, fport, secret) from a 3-tuple
 	  to a 4-tuple.  What implications does that have aoart from
           cost?  Could we per section 3.4 of the RFC use ipport_tick()
 	  to change it, getting one/two arc4random() calls out of the
 	  function as well?
Algo 5:
 	- when calculation *lastport = shouldn't first be *lastport
 	  instead?  Re-randomizing that value every time instead of
 	  once on boottime, does change the algorithm, doesn't it?

We might need to re-set some of the then "global" values when we
switch algos from the sysctl handler.  We should take care of that.

In various places the first things we do, due to the classic way we did
it, are:
 	a) check count, which for the first run must never
 	   return the error in our implementation disabling the
 	   dorandom for only one port left.
 	b) increment *lastport, which is not always suggested
 	c) as a result of b) have to check the range which otherwise
 	   would have to be fine because of the % operation.

Would it make sense to shuffle the code around a bit to be closer to
the RFC, use a break for the result on tmpinp to escape the loop, etc?


Also I'd like to point out that we still do not always use the random
port for TCP per:
  441         /*
  442          * For UDP, use random port allocation as long as the user
  443          * allows it.  For TCP (and as of yet unknown) connections,
  444          * use random port allocation only if the user allows it AND
  445          * ipport_tick() allows it.
  446          */
  447         if (V_ipport_randomized &&
  448             (!V_ipport_stoprandom || pcbinfo == &V_udbinfo))
  449                 dorandom = 1;
  450         else
  451                 dorandom = 0;

Do we want to change that decision, or do we want to defer the change
in a similar way to defer the change of the default algo -- understand
performance (vs. security) impact first?


/bz

-- 
Bjoern A. Zeeb                                 You have to have visions!
          Stop bit received. Insert coin for new address family.