| Message ID | 1471008654-28755-5-git-send-email-tim.gardner@canonical.com |
|---|---|
| State | New |
| Headers | show |
On 08/12/2016 07:30 AM, Tim Gardner wrote: > From: Eric Dumazet <edumazet@google.com> > > Yue Cao claims that current host rate limiting of challenge ACKS > (RFC 5961) could leak enough information to allow a patient attacker > to hijack TCP sessions. He will soon provide details in an academic > paper. > > This patch increases the default limit from 100 to 1000, and adds > some randomization so that the attacker can no longer hijack > sessions without spending a considerable amount of probes. > > Based on initial analysis and patch from Linus. > > Note that we also have per socket rate limiting, so it is tempting > to remove the host limit in the future. > > v2: randomize the count of challenge acks per second, not the period. > > Fixes: 282f23c6ee34 ("tcp: implement RFC 5961 3.2") > Reported-by: Yue Cao <ycao009@ucr.edu> > Signed-off-by: Eric Dumazet <edumazet@google.com> > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Yuchung Cheng <ycheng@google.com> > Cc: Neal Cardwell <ncardwell@google.com> > Acked-by: Neal Cardwell <ncardwell@google.com> > Acked-by: Yuchung Cheng <ycheng@google.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > > CVE-2016-5696 > > (backported from commit 75ff39ccc1bd5d3c455b6822ab09e533c551f758 upstream) > Signed-off-by: Stefan Bader <stefan.bader@canonical.com> > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > --- > net/ipv4/tcp_input.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > index 2cc1313..1bf94fa 100644 > --- a/net/ipv4/tcp_input.c > +++ b/net/ipv4/tcp_input.c > @@ -86,7 +86,7 @@ int sysctl_tcp_adv_win_scale __read_mostly = 1; > EXPORT_SYMBOL(sysctl_tcp_adv_win_scale); > > /* rfc5961 challenge ack rate limiting */ > -int sysctl_tcp_challenge_ack_limit = 100; > +int sysctl_tcp_challenge_ack_limit = 1000; > > int sysctl_tcp_stdurg __read_mostly; > int sysctl_tcp_rfc1337 __read_mostly; > @@ -3288,17 +3288,25 @@ static void tcp_send_challenge_ack(struct sock *sk) > /* unprotected vars, we dont care of overwrites */ > static u32 challenge_timestamp; > static unsigned int challenge_count; > - u32 now = jiffies / HZ; > + u32 count, now = jiffies / HZ; > > + /* Check host-wide RFC 5961 rate limit. */ > if (now != challenge_timestamp) { > + u32 half = (sysctl_tcp_challenge_ack_limit + 1) >> 1; > + > challenge_timestamp = now; > - challenge_count = 0; > + *((volatile unsigned int *) &challenge_count) = > + WRITE_ONCE(challenge_count, half + > + prandom_u32_max(sysctl_tcp_challenge_ack_limit)); This seems kind of bogus. ^^
On 12.08.2016 15:58, Tim Gardner wrote: > On 08/12/2016 07:30 AM, Tim Gardner wrote: >> From: Eric Dumazet <edumazet@google.com> >> >> Yue Cao claims that current host rate limiting of challenge ACKS >> (RFC 5961) could leak enough information to allow a patient attacker >> to hijack TCP sessions. He will soon provide details in an academic >> paper. >> >> This patch increases the default limit from 100 to 1000, and adds >> some randomization so that the attacker can no longer hijack >> sessions without spending a considerable amount of probes. >> >> Based on initial analysis and patch from Linus. >> >> Note that we also have per socket rate limiting, so it is tempting >> to remove the host limit in the future. >> >> v2: randomize the count of challenge acks per second, not the period. >> >> Fixes: 282f23c6ee34 ("tcp: implement RFC 5961 3.2") >> Reported-by: Yue Cao <ycao009@ucr.edu> >> Signed-off-by: Eric Dumazet <edumazet@google.com> >> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> >> Cc: Yuchung Cheng <ycheng@google.com> >> Cc: Neal Cardwell <ncardwell@google.com> >> Acked-by: Neal Cardwell <ncardwell@google.com> >> Acked-by: Yuchung Cheng <ycheng@google.com> >> Signed-off-by: David S. Miller <davem@davemloft.net> >> >> CVE-2016-5696 >> >> (backported from commit 75ff39ccc1bd5d3c455b6822ab09e533c551f758 upstream) >> Signed-off-by: Stefan Bader <stefan.bader@canonical.com> >> Signed-off-by: Tim Gardner <tim.gardner@canonical.com> >> --- >> net/ipv4/tcp_input.c | 16 ++++++++++++---- >> 1 file changed, 12 insertions(+), 4 deletions(-) >> >> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c >> index 2cc1313..1bf94fa 100644 >> --- a/net/ipv4/tcp_input.c >> +++ b/net/ipv4/tcp_input.c >> @@ -86,7 +86,7 @@ int sysctl_tcp_adv_win_scale __read_mostly = 1; >> EXPORT_SYMBOL(sysctl_tcp_adv_win_scale); >> >> /* rfc5961 challenge ack rate limiting */ >> -int sysctl_tcp_challenge_ack_limit = 100; >> +int sysctl_tcp_challenge_ack_limit = 1000; >> >> int sysctl_tcp_stdurg __read_mostly; >> int sysctl_tcp_rfc1337 __read_mostly; >> @@ -3288,17 +3288,25 @@ static void tcp_send_challenge_ack(struct sock *sk) >> /* unprotected vars, we dont care of overwrites */ >> static u32 challenge_timestamp; >> static unsigned int challenge_count; >> - u32 now = jiffies / HZ; >> + u32 count, now = jiffies / HZ; >> >> + /* Check host-wide RFC 5961 rate limit. */ >> if (now != challenge_timestamp) { >> + u32 half = (sysctl_tcp_challenge_ack_limit + 1) >> 1; >> + >> challenge_timestamp = now; >> - challenge_count = 0; >> + *((volatile unsigned int *) &challenge_count) = >> + WRITE_ONCE(challenge_count, half + >> + prandom_u32_max(sysctl_tcp_challenge_ack_limit)); > > This seems kind of bogus. ^^ Yes, I had pulled back that function, too. But note that in the updated version I replaced that with its expanded form... >
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index 2cc1313..1bf94fa 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -86,7 +86,7 @@ int sysctl_tcp_adv_win_scale __read_mostly = 1; EXPORT_SYMBOL(sysctl_tcp_adv_win_scale); /* rfc5961 challenge ack rate limiting */ -int sysctl_tcp_challenge_ack_limit = 100; +int sysctl_tcp_challenge_ack_limit = 1000; int sysctl_tcp_stdurg __read_mostly; int sysctl_tcp_rfc1337 __read_mostly; @@ -3288,17 +3288,25 @@ static void tcp_send_challenge_ack(struct sock *sk) /* unprotected vars, we dont care of overwrites */ static u32 challenge_timestamp; static unsigned int challenge_count; - u32 now = jiffies / HZ; + u32 count, now = jiffies / HZ; + /* Check host-wide RFC 5961 rate limit. */ if (now != challenge_timestamp) { + u32 half = (sysctl_tcp_challenge_ack_limit + 1) >> 1; + challenge_timestamp = now; - challenge_count = 0; + *((volatile unsigned int *) &challenge_count) = + WRITE_ONCE(challenge_count, half + + prandom_u32_max(sysctl_tcp_challenge_ack_limit)); } - if (++challenge_count <= sysctl_tcp_challenge_ack_limit) { + count = ACCESS_ONCE(challenge_count); + if (count > 0) { + WRITE_ONCE(challenge_count, count - 1); NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_TCPCHALLENGEACK); tcp_send_ack(sk); } } +#undef WRITE_ONCE static void tcp_store_ts_recent(struct tcp_sock *tp) {