fix secure tcp sequence number generation
Subsystems (3)
PATCH 1/1 fix secure tcp sequence number generation
Hi Dave,
Ted's recent random.c update broke the periodic rekeying:schedule_work() doesn't provide synchronization. Additionally the first syn values after boot are generated with secret 0 - not good.
Attached is a big cleanup. Linus asked me to send to to you for merging:
Description:
The tcp sequence number generator needs a random seed that is reset every few minutes. Since the sequence numbers should be constantly increasing, for each rekey 2^24 is added to the sequence number.
The actual use of the sequence number generator is lockless, synchronization is achieved by having two copies of the control structure.
The attached patch:
- fixes a race in rekey_seq_generator(): schedule_work doesn't provide synchronization.
- Uses schedule_delayed_work() for the rekey: simplifies synchronization and speeds up the hot path.
- replaces do_gettimeofday with get_seconds(): get_seconds is faster and usec resolution is not required.
- removes tmpdata - not needed with new locking.
- Adds a late_initcall for the first initialization after boot. init_call would be too early, I've checked that the late_initcall runs before net/ipv4/ipconfig.c, i.e. the BOOTP/DHCP autoconfiguration.
drivers/char/random.c +36 -35
@@ -2184,7 +2184,7 @@#undef K3/* This should not be decreased so low that ISNs wrap too fast. */ -#define REKEY_INTERVAL 300 +#define REKEY_INTERVAL (300*HZ)/* * Bit layout of the tcp sequence numbers (before adding current time): * bit 24-31: increased after every key exchange
@@ -2210,49 +2210,55 @@#define HASH_MASK ( (1<<HASH_BITS)-1 )static struct keydata { -time_t rekey_time;__u32 count;// already shifted to the final position __u32 secret[12 ]; }____cacheline_aligned ip_keydata[2 ]; -static spinlock_t ip_lock =SPIN_LOCK_UNLOCKED ;static unsigned int ip_cnt; -static void rekey_seq_generator (void *private_) -{ -struct keydata *keyptr, tmp; -struct timeval tv; +static void rekey_seq_generator (void *private_); -do_gettimeofday (&tv); -get_random_bytes (tmp.secret,sizeof (tmp.secret)); +static DECLARE_WORK(rekey_work, rekey_seq_generator, NULL); -spin_lock_bh (&ip_lock); - keyptr = &ip_keydata[ip_cnt&1 ]; +/* +* Lock avoidance: +* The ISN generation runs lockless - it's just a hash over random data. +* State changes happen every 5 minutes when the random key is replaced. +* Synchronization is performed by having two copies of the hash function +* state and rekey_seq_generator always updates the inactive copy. +* The copy is then activated by updating ip_cnt. +* The implementation breaks down if someone blocks the thread +* that processes SYN requests for more than 5 minutes. Should never +* happen, and even if that happens only a not perfectly compliant +* ISN is generated, nothing fatal. +*/ +static void rekey_seq_generator (void *private_) +{ +struct keydata *keyptr = &ip_keydata[1 ^(ip_cnt&1 )]; - keyptr = &ip_keydata[1 ^(ip_cnt&1 )]; - keyptr->rekey_time = tv.tv_sec; -memcpy (keyptr->secret, tmp.secret,sizeof (keyptr->secret)); +get_random_bytes (keyptr->secret,sizeof (keyptr->secret)); keyptr->count = (ip_cnt&COUNT_MASK )<<HASH_BITS ; -mb (); +smp_wmb (); ip_cnt++; - -spin_unlock_bh (&ip_lock); +schedule_delayed_work (&rekey_work,REKEY_INTERVAL ); } -static DECLARE_WORK(rekey_work, rekey_seq_generator, NULL); - -static inline struct keydata *check_and_rekey (time_t time) +static inline struct keydata *get_keyptr (void ) {struct keydata *keyptr = &ip_keydata[ip_cnt&1 ]; -rmb (); -if (!keyptr->rekey_time || (time - keyptr->rekey_time) >REKEY_INTERVAL ) { -schedule_work (&rekey_work); - } +smp_rmb ();return keyptr; } +static __init int seqgen_init (void ) +{ +rekey_seq_generator (NULL ); +return 0 ; +} +late_initcall (seqgen_init); +#if defined(CONFIG_IPV6 ) || defined(CONFIG_IPV6_MODULE )__u32 secure_tcpv6_sequence_number (__u32 *saddr,__u32 *daddr,__u16 sport,__u16 dport)
@@ -2260,14 +2266,12 @@struct timeval tv;__u32 seq;__u32 hash[12 ]; -struct keydata *keyptr; +struct keydata *keyptr =get_keyptr ();/* The procedure is the same as for IPv4, but addresses are longer. * Thus we must use twothirdsMD4Transform. */ -do_gettimeofday (&tv);/* We need the usecs below... */ - keyptr =check_and_rekey (tv.tv_sec);memcpy (hash, saddr,16 ); hash[4 ]=(sport <<16 ) + dport;
@@ -2275,6 +2279,8 @@ seq =twothirdsMD4Transform (daddr, hash) &HASH_MASK ; seq += keyptr->count; + +do_gettimeofday (&tv); seq += tv.tv_usec + tv.tv_sec*1000000 ;return seq;
@@ -2288,13 +2294,7 @@struct timeval tv;__u32 seq;__u32 hash[4 ]; -struct keydata *keyptr; - -/* -* Pick a random secret every REKEY_INTERVAL seconds. -*/ -do_gettimeofday (&tv);/* We need the usecs below... */ - keyptr =check_and_rekey (tv.tv_sec); +struct keydata *keyptr =get_keyptr ();/* * Pick a unique starting offset for each TCP connection endpoints
@@ -2317,6 +2317,7 @@* That's funny, Linux has one built in! Use it! * (Networks are faster now - should this be increased?) */ +do_gettimeofday (&tv); seq += tv.tv_usec + tv.tv_sec*1000000 ;#if 0 printk("init_seq(%lx, %lx, %d, %d) = %d\n",
@@ -2335,7 +2336,7 @@struct keydata *keyptr;__u32 hash[4 ]; - keyptr =check_and_rekey (get_seconds ()); + keyptr =get_keyptr ();/* * Pick a unique starting offset for each IP destination.
Quoted 3 lines · On Sat, 02 Oct 2004 20:10:22 +0200 Manfred Spraul <[email protected]>…
Ted's recent
random.cupdate broke the periodic rekeying:schedule_work()doesn't provide synchronization. Additionally the first syn values after boot are generated with secret 0 - not good.
Are you referring to his changes done back in August or some other change? August is the last time I see that Ted made a change to drivers/char/random.c
Quoted 11 lines · David S. Miller
Quoted 5 lines · On Sat, 02 Oct 2004 20:10:22 +0200 Manfred Spraul <[email protected]>…
Ted's recent
random.cupdate broke the periodic rekeying:schedule_work()doesn't provide synchronization. Additionally the first syn values after boot are generated with secret 0 - not good.
Quoted 5 lines
Are you referring to his changes done back in August or some other change? August is the last time I see that Ted made a change to
drivers/char/random.c
The change to revision 1.49 of random.c, with the comment "dev/random: Fix latency in rekeying sequence number"
http://linux.bkbits.net:8080/linux-2.5/diffs/drivers/char/[email protected]
There are two problems with that patch:
- The first call of check_and_rekey must be synchroneous, otherwise the secret stays 0 for the first ISN values after boot.
- schedule_work() doesn't provide synchronization, thus rekey_seq_generator could end up running twice.
I tried to fix it and then I noticed that it's possible to simplify the code a lot. --
Manfred
David S. Miller
maintainerQuoted 19 lines · On Sat, 02 Oct 2004 20:10:22 +0200 Manfred Spraul <[email protected]>…
Description:
The tcp sequence number generator needs a random seed that is reset every few minutes. Since the sequence numbers should be constantly increasing, for each rekey 2^24 is added to the sequence number.
The actual use of the sequence number generator is lockless, synchronization is achieved by having two copies of the control structure.The attached patch:
- fixes a race inrekey_seq_generator():schedule_workdoesn't provide synchronization.
- Usesschedule_delayed_work()for the rekey: simplifies synchronization and speeds up the hot path.
- replacesdo_gettimeofdaywithget_seconds():get_secondsis faster and usec resolution is not required.
- removes tmpdata - not needed with new locking.
- Adds alate_initcallfor the first initialization after boot.init_callwould be too early, I've checked that thelate_initcallruns beforenet/ipv4/ipconfig.c, i.e. the BOOTP/DHCP autoconfiguration.
Quoted 1 line
Signed-off-by Manfred Spraul <[email protected]>
I like this patch, but you may have sent the wrong copy. You mention in the description that you changed over to use get_seconds(), yet the only reference to get_seconds() in the patch is:
- keyptr = check_and_rekey(get_seconds());
+ keyptr = get_keyptr();
Please clarify that this is just a brain fart in the description and not the patch.
Quoted 36 lines · David S. Miller
Quoted 19 lines · On Sat, 02 Oct 2004 20:10:22 +0200 Manfred Spraul <[email protected]>…
Description:
The tcp sequence number generator needs a random seed that is reset every few minutes. Since the sequence numbers should be constantly increasing, for each rekey 2^24 is added to the sequence number.
The actual use of the sequence number generator is lockless, synchronization is achieved by having two copies of the control structure.The attached patch:
- fixes a race inrekey_seq_generator():schedule_workdoesn't provide synchronization.
- Usesschedule_delayed_work()for the rekey: simplifies synchronization and speeds up the hot path.
- replacesdo_gettimeofdaywithget_seconds():get_secondsis faster and usec resolution is not required.
- removes tmpdata - not needed with new locking.
- Adds alate_initcallfor the first initialization after boot.init_callwould be too early, I've checked that thelate_initcallruns beforenet/ipv4/ipconfig.c, i.e. the BOOTP/DHCP autoconfiguration.Quoted 3 lines
Signed-off-by Manfred Spraul <[email protected]>I like this patch, but you may have sent the wrong copy. You mention in the description that you changed over to use
get_seconds(), yet the only reference toget_seconds()in the patch is:drivers/char/random.c +1
-1- keyptr =check_and_rekey (get_seconds ()); + keyptr =get_keyptr ();
Quoted 4 lines
Please clarify that this is just a brain fart in the description and not the patch.
Right patch but wrong description, sorry:
I've recycled the description from the previous patch and that one used get_seconds() in rekey_seq_generator(). Now rekey_seq_generator() doesn't access the time at all, that's done within schedule_delayed_work().
--
Manfred
David S. Miller
maintainerQuoted 4 lines · On Tue, 05 Oct 2004 22:41:55 +0200 Manfred Spraul <[email protected]>…
Right patch but wrong description, sorry:
I've recycled the description from the previous patch and that one usedget_seconds()inrekey_seq_generator(). Nowrekey_seq_generator()doesn't access the time at all, that's done withinschedule_delayed_work().
Great, I'll apply then.
Thanks.