Message-ID: <1273170813.2222.10.camel@edumazet-laptop>
Date:	Thu, 06 May 2010 20:33:33 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Neil Horman <nhorman@...driver.com>
Cc:	netdev@...r.kernel.org, davem@...emloft.net, kuznet@....inr.ac.ru,
	jmorris@...ei.org, yoshfuji@...ux-ipv6.org, kaber@...sh.net
Subject: Re: [PATCH] ipv4: remove ip_rt_secret timer

Le jeudi 06 mai 2010 à 14:02 -0400, Neil Horman a écrit :
> On Thu, May 06, 2010 at 07:32:35PM +0200, Eric Dumazet wrote:
> > Le jeudi 06 mai 2010 à 13:16 -0400, Neil Horman a écrit :
> > > A while back there was a discussion regarding the rt_secret_interval timer.
> > > Given that we've had the ability to do emergency route cache rebuilds for awhile
> > > now, based on a statistical analysis of the various hash chain lengths in the
> > > cache, the use of the flush timer is somewhat redundant.  This patch removes the
> > > rt_secret_interval sysctl, allowing us to rely solely on the statistical
> > > analysis mechanism to determine the need for route cache flushes.
> > > 
> > > Signed-off-by: Neil Horman <nhorman@...driver.com>
> > > 
> > > 
> > 
> > Nice cleanup try Neil, but this gives to attackers more time to hit the
> > cache (infinite time should be enough as a matter of fact ;) )
> > 
> Not sure I follow what your complaint is.  I get that this gives attackers
> plenty of time to try to attack the cache, but thats rather the point of the
> statistics gathering for the cache, and why I don't think we need the secret
> timer any more.   With the statistical analysis we do on the route cache every
> gc cycle, we can tell if an attacker has guessed our rt_genid value, and is
> making any chains in the cache abnormally long.  Thats when we do the rebuild,
> modifying the rt_genid, forcing the attacker to re-discover it (which should be
> difficult).  Theres no need to change this periodically if you're not being
> attacked.
>  
> > Hints : 
> > 
> > - What is the initial value of rt_genid ?
> > 
> > - How/When is it changed (full 32 bits are changed or small
> > perturbations ? check rt_cache_invalidate())
> > 
> /*
>  * Pertubation of rt_genid by a small quantity [1..256]
>  * Using 8 bits of shuffling ensure we can call rt_cache_invalidate()
>  * many times (2^24) without giving recent rt_genid.
>  * Jenkins hash is strong enough that litle changes of rt_genid are OK.
>  */
> static void rt_cache_invalidate(struct net *net)
> {
>         unsigned char shuffle;
> 
>         get_random_bytes(&shuffle, sizeof(shuffle));
>         atomic_add(shuffle + 1U, &net->ipv4.rt_genid);
> }
> 
> Clearly, its small changes.  To paraphrase the comment, Changes to rt_genid are
> small enough to be confident that we don't repetatively use a gen_id often, but
> sufficiently random that attackers cannot easily guess the next gen_id based on
> the current value.  Both the timer and the statistics code use this invalidation
> technique previously, and the latter continues to do so.
> 
> I've not changed anything regarding how we
> invalidate, only when we choose to invalidate.  Invalidation can lead to
> slowdowns during routing, since it send frames through the slow path after an
> invalidation.  It behooves us to avoid preforming this invalidation without
> need, and since we have a mechanism in place to do that invalidation specfically
> when we need to, lets get rid of the code that handles that, and make it a bit
> cleaner.  If there are users that feel strongly that they need to defend against
> potential attacks by periodically changing their rt_genid, its still possible.
> Its as simple as putting:
> echo -1 > /proc/sys/net/ipv4/route/flush
> in a cron job.
> 

I have some customers that will simply kill me if their routing cache is
disabled by a smart attack, slowing down their server by a 4x factor.

I know its possible, it has been done.

For a quiet machine possible rt_genid values range are known from
attacker, and hash size is also known. Thats really too easy for the bad
guys...

Neil, I think your cleanup should stay a cleanup for the moment, or you
must make sure rt_genid initial value is not 0 (read your patch
again...)

I agree we dont need anymore the complex timer logic. We could keep the
secret_interval (default to 0 if you really want) and force a
rt_cache_invalidate() call once in a while from the periodic
rt_check_expire() for example.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists