[PATCH net] inet: stop leaking jiffies on the wire

[PATCH net] inet: stop leaking jiffies on the wire

[Eric Dumazet]

Historically linux tried to stick to RFC 791, 1122, 2003
for IPv4 ID field generation.

RFC 6864 made clear that no matter how hard we try,
we can not ensure unicity of IP ID within maximum
lifetime for all datagrams with a given source
address/destination address/protocol tuple.

Linux uses a per socket inet generator (inet_id), initialized
at connection startup with a XOR of 'jiffies' and other
fields that appear clear on the wire.

Thiemo Nagel pointed that this strategy is a privacy
concern as this provides 16 bits of entropy to fingerprint
devices.

Let's switch to a random starting point, this is just as
good as far as RFC 6864 is concerned and does not leak
anything critical.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Thiemo Nagel <tnagel@google.com>
---
 drivers/crypto/chelsio/chtls/chtls_cm.c | 2 +-
 net/dccp/ipv4.c                         | 2 +-
 net/ipv4/datagram.c                     | 2 +-
 net/ipv4/tcp_ipv4.c                     | 4 ++--
 net/sctp/socket.c                       | 2 +-
 5 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/crypto/chelsio/chtls/chtls_cm.c b/drivers/crypto/chelsio/chtls/chtls_cm.c
index 774d991d7cca49011016d41c00914ad84059ccb8..aca75237bbcf83eb1d440bcdf1e4d3b702cff0a1 100644
--- a/drivers/crypto/chelsio/chtls/chtls_cm.c
+++ b/drivers/crypto/chelsio/chtls/chtls_cm.c
@@ -1297,7 +1297,7 @@ static void make_established(struct sock *sk, u32 snd_isn, unsigned int opt)
 	tp->write_seq = snd_isn;
 	tp->snd_nxt = snd_isn;
 	tp->snd_una = snd_isn;
-	inet_sk(sk)->inet_id = tp->write_seq ^ jiffies;
+	inet_sk(sk)->inet_id = prandom_u32();
 	assign_rxopt(sk, opt);
 
 	if (tp->rcv_wnd > (RCV_BUFSIZ_M << 10))
diff --git a/net/dccp/ipv4.c b/net/dccp/ipv4.c
index d9b4200ed12df8ecc7ff7de26827207c5a290e37..0d8f782c25ccc031e5322beccb0242ee42b032b9 100644
--- a/net/dccp/ipv4.c
+++ b/net/dccp/ipv4.c
@@ -117,7 +117,7 @@ int dccp_v4_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
 						    inet->inet_daddr,
 						    inet->inet_sport,
 						    inet->inet_dport);
-	inet->inet_id = dp->dccps_iss ^ jiffies;
+	inet->inet_id = prandom_u32();
 
 	err = dccp_connect(sk);
 	rt = NULL;
diff --git a/net/ipv4/datagram.c b/net/ipv4/datagram.c
index 9a0fe0c2fa02c9707e6fc8c02529a48e84f7d680..4a8550c49202db13b17d5cf4ed1e44dd8852c212 100644
--- a/net/ipv4/datagram.c
+++ b/net/ipv4/datagram.c
@@ -73,7 +73,7 @@ int __ip4_datagram_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len
 	reuseport_has_conns(sk, true);
 	sk->sk_state = TCP_ESTABLISHED;
 	sk_set_txhash(sk);
-	inet->inet_id = jiffies;
+	inet->inet_id = prandom_u32();
 
 	sk_dst_set(sk, &rt->dst);
 	err = 0;
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 6be568334848c7841a4a09126937f71f60420103..7512c04f72103da9c25d25dfd91a7d39443d0f3e 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -303,7 +303,7 @@ int tcp_v4_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
 						 inet->inet_daddr);
 	}
 
-	inet->inet_id = tp->write_seq ^ jiffies;
+	inet->inet_id = prandom_u32();
 
 	if (tcp_fastopen_defer_connect(sk, &err))
 		return err;
@@ -1450,7 +1450,7 @@ struct sock *tcp_v4_syn_recv_sock(const struct sock *sk, struct sk_buff *skb,
 	inet_csk(newsk)->icsk_ext_hdr_len = 0;
 	if (inet_opt)
 		inet_csk(newsk)->icsk_ext_hdr_len = inet_opt->opt.optlen;
-	newinet->inet_id = newtp->write_seq ^ jiffies;
+	newinet->inet_id = prandom_u32();
 
 	if (!dst) {
 		dst = inet_csk_route_child_sock(sk, newsk, req);
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index ca81e06df1651f16ab332cd9fc880c21b89a5c6d..ffd3262b7a41eac2e3d825c3f0665066f376ea3c 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -9306,7 +9306,7 @@ void sctp_copy_sock(struct sock *newsk, struct sock *sk,
 	newinet->inet_rcv_saddr = inet->inet_rcv_saddr;
 	newinet->inet_dport = htons(asoc->peer.port);
 	newinet->pmtudisc = inet->pmtudisc;
-	newinet->inet_id = asoc->next_tsn ^ jiffies;
+	newinet->inet_id = prandom_u32();
 
 	newinet->uc_ttl = inet->uc_ttl;
 	newinet->mc_loop = 1;
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

Re: [PATCH net] inet: stop leaking jiffies on the wire

[David Miller]

From: Eric Dumazet <edumazet@google.com>
Date: Fri,  1 Nov 2019 10:32:19 -0700

> Historically linux tried to stick to RFC 791, 1122, 2003
> for IPv4 ID field generation.
> 
> RFC 6864 made clear that no matter how hard we try,
> we can not ensure unicity of IP ID within maximum
> lifetime for all datagrams with a given source
> address/destination address/protocol tuple.
> 
> Linux uses a per socket inet generator (inet_id), initialized
> at connection startup with a XOR of 'jiffies' and other
> fields that appear clear on the wire.
> 
> Thiemo Nagel pointed that this strategy is a privacy
> concern as this provides 16 bits of entropy to fingerprint
> devices.
> 
> Let's switch to a random starting point, this is just as
> good as far as RFC 6864 is concerned and does not leak
> anything critical.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Thiemo Nagel <tnagel@google.com>

Applied and queued up for -stable, thanks.

Re: [PATCH net] inet: stop leaking jiffies on the wire

[Thiemo Nagel]

Thanks a lot, Eric!

Grepping through the source, it seems to me there are two more
occurrences of jiffies in inet_id:

https://github.com/torvalds/linux/blob/v5.3/net/dccp/ipv4.c#L120
https://github.com/torvalds/linux/blob/v5.3/net/dccp/ipv4.c#L419

Kind regards,
Thiemo

Re: [PATCH net] inet: stop leaking jiffies on the wire

[Eric Dumazet]

On Mon, Nov 4, 2019 at 7:24 AM Thiemo Nagel <tnagel@google.com> wrote:
>
> Thanks a lot, Eric!
>
> Grepping through the source, it seems to me there are two more
> occurrences of jiffies in inet_id:
>
> https://github.com/torvalds/linux/blob/v5.3/net/dccp/ipv4.c#L120
> https://github.com/torvalds/linux/blob/v5.3/net/dccp/ipv4.c#L419
>

Indeed.

The one in dccp_v4_connect() has been handled in my patch.
I missed it in dccp_v4_request_recv_sock()

Thanks.

> Kind regards,
> Thiemo

Re: [PATCH net] inet: stop leaking jiffies on the wire

[Thiemo Nagel]

Hey Eric,

I've been thinking about this some more. The prandom_u32() description
says: "This algorithm is NOT considered safe for cryptographic use."
-- Afaiu this implies that an attacker could deduce internal state by
looking at a sequence of random numbers. Consequently, I believe that
we shouldn't use prandom_* for data that gets sent over the wire.
Instead get_random_* should be used which is described as
cryptographically secure.

Kind regards,
Thiemo

From /drivers/char/random.c:

[About get_random_*:]
 * Besides the obvious cryptographic uses, these numbers are also good
 * for seeding TCP sequence numbers, and other places where it is
 * desirable to have numbers which are not only random, but hard to
 * predict by an attacker.
[...]
 * It *is* safe to expose get_random_int() output to attackers (e.g. as
 * network cookies); given outputs 1..n, it's not feasible to predict
 * outputs 0 or n+1.
[...]

 * prandom_u32()
 * -------------
 *
 * For even weaker applications, see the pseudorandom generator
 * prandom_u32(), prandom_max(), and prandom_bytes().  If the random
 * numbers aren't security-critical at all, these are *far* cheaper.
 * Useful for self-tests, random error simulation, randomized backoffs,
 * and any other application where you trust that nobody is trying to
 * maliciously mess with you by guessing the "random" numbers.


On Mon, Nov 4, 2019 at 4:50 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Mon, Nov 4, 2019 at 7:24 AM Thiemo Nagel <tnagel@google.com> wrote:
> >
> > Thanks a lot, Eric!
> >
> > Grepping through the source, it seems to me there are two more
> > occurrences of jiffies in inet_id:
> >
> > https://github.com/torvalds/linux/blob/v5.3/net/dccp/ipv4.c#L120
> > https://github.com/torvalds/linux/blob/v5.3/net/dccp/ipv4.c#L419
> >
>
> Indeed.
>
> The one in dccp_v4_connect() has been handled in my patch.
> I missed it in dccp_v4_request_recv_sock()
>
> Thanks.
>
> > Kind regards,
> > Thiemo



-- 

Thiemo Nagel

Software Engineer


Google Germany GmbH, Erika-Mann-Straße 33, 80686 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Re: [PATCH net] inet: stop leaking jiffies on the wire

[Eric Dumazet]

On Mon, Nov 11, 2019 at 12:53 AM Thiemo Nagel <tnagel@google.com> wrote:
>
> Hey Eric,
>
> I've been thinking about this some more. The prandom_u32() description
> says: "This algorithm is NOT considered safe for cryptographic use."
> -- Afaiu this implies that an attacker could deduce internal state by
> looking at a sequence of random numbers.

Keep in mind that we need some entropy, and some device can not provide any.

prandom_u32() gets some reasonable amount of entropy. And it seems
better than ' jiffies' .

If you want strong (hardware) generators, I guess next Android/Chrome
devices will cost a bit more.


 Consequently, I believe that
> we shouldn't use prandom_* for data that gets sent over the wire.
> Instead get_random_* should be used which is described as
> cryptographically secure.
>

If IP ID had to be cryptographically secure, you can be sure we would
have addressed the problem 20 years ago.

Please discuss this matter with random maintainers, not networking ones ;)

> Kind regards,
> Thiemo
>
> From /drivers/char/random.c:
>
> [About get_random_*:]
>  * Besides the obvious cryptographic uses, these numbers are also good
>  * for seeding TCP sequence numbers, and other places where it is
>  * desirable to have numbers which are not only random, but hard to
>  * predict by an attacker.
> [...]
>  * It *is* safe to expose get_random_int() output to attackers (e.g. as
>  * network cookies); given outputs 1..n, it's not feasible to predict
>  * outputs 0 or n+1.
> [...]
>
>  * prandom_u32()
>  * -------------
>  *
>  * For even weaker applications, see the pseudorandom generator
>  * prandom_u32(), prandom_max(), and prandom_bytes().  If the random
>  * numbers aren't security-critical at all, these are *far* cheaper.
>  * Useful for self-tests, random error simulation, randomized backoffs,
>  * and any other application where you trust that nobody is trying to
>  * maliciously mess with you by guessing the "random" numbers.
>
>
> On Mon, Nov 4, 2019 at 4:50 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Mon, Nov 4, 2019 at 7:24 AM Thiemo Nagel <tnagel@google.com> wrote:
> > >
> > > Thanks a lot, Eric!
> > >
> > > Grepping through the source, it seems to me there are two more
> > > occurrences of jiffies in inet_id:
> > >
> > > https://github.com/torvalds/linux/blob/v5.3/net/dccp/ipv4.c#L120
> > > https://github.com/torvalds/linux/blob/v5.3/net/dccp/ipv4.c#L419
> > >
> >
> > Indeed.
> >
> > The one in dccp_v4_connect() has been handled in my patch.
> > I missed it in dccp_v4_request_recv_sock()
> >
> > Thanks.
> >
> > > Kind regards,
> > > Thiemo
>
>
>
> --
>
> Thiemo Nagel
>
> Software Engineer
>
>
> Google Germany GmbH, Erika-Mann-Straße 33, 80686 München
>
> Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
>
> Registergericht und -nummer: Hamburg, HRB 86891
>
> Sitz der Gesellschaft: Hamburg

Re: [PATCH net] inet: stop leaking jiffies on the wire

[Thiemo Nagel]

Hey Eric,

I think we should distinguish two different problems here:
a) There's insufficient entropy to seed the RNG. As a consequence,
observing a (partial) sequence of numbers may allow an attacker to
determine the internal state, independently of whether the RNG is
cryptographically secure or not.
b) The RNG is not cryptographically secure. In that case, observing a
(partial) sequence of numbers may allow an attacker to determine the
internal state, independent of the amount of entropy that was used to
seed it.

Problem a) is hard -- as you mention, it may require hardware support
to solve it fully. However the problem that I'm suggesting to address
is b), and that likely can be solved by swapping out prandom_u32() for
get_random_u32().

> If IP ID had to be cryptographically secure, you can be sure we would
> have addressed the problem 20 years ago.

I don't think this is a valid point at all. There are countless
examples of things that weren't known 20 years ago but that are better
understood today. One relevant example is RFC7258 which only came out
in 2014.

Kind regards,
Thiemo

Re: [PATCH net] inet: stop leaking jiffies on the wire

[Eric Dumazet]

On Mon, Nov 11, 2019 at 9:49 AM Thiemo Nagel <tnagel@google.com> wrote:

> Problem a) is hard -- as you mention, it may require hardware support
> to solve it fully. However the problem that I'm suggesting to address
> is b), and that likely can be solved by swapping out prandom_u32() for
> get_random_u32().
>

Make sure a server can still generates 10 millions packets per second
using this thing :)

I believe get_random_u32() is at least 5 times more expensive than
prandom_u32(),
and that is on x86 where arch_has_random() is true and rdrand
instruction can be used.

On other arches this would probably kill performance, and eat entropy
way too much.

Really this is a discussion that should be taken to random subsystem
maintainers.

We can not simply use get_random_u32() here.