| Message ID | 25a0abdf61ab4698773d417af9e49fcd6e6559a4.1500393028.git.shli@fb.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Shaohua Li <shli@kernel.org> Date: Tue, 18 Jul 2017 12:03:37 -0700 > + /* Since this is being sent on the wire obfuscate hash a bit > + * to minimize possbility that any useful information to an > + * attacker is leaked. Only lower 20 bits are relevant. > + */ > + rol32(hash, 16); This doesn't help anything at all. I don't like things that try to give a sense of security (however little) but actually don't give any at all. I'd rather, therefore, that you remove this altogether. And note that maybe not using the same flow label prevents attacks of that nature, ever consider that?
On Mon, Jul 24, 2017 at 1:34 PM, David Miller <davem@davemloft.net> wrote: > From: Shaohua Li <shli@kernel.org> > Date: Tue, 18 Jul 2017 12:03:37 -0700 > >> + /* Since this is being sent on the wire obfuscate hash a bit >> + * to minimize possbility that any useful information to an >> + * attacker is leaked. Only lower 20 bits are relevant. >> + */ >> + rol32(hash, 16); > > This doesn't help anything at all. I believe the above code is copy-n-pasted from ip6_make_flowlabel() (with few adjustments). Don't know why we can't refactor that function for reuse.
On Mon, Jul 24, 2017 at 04:12:53PM -0700, Cong Wang wrote: > On Mon, Jul 24, 2017 at 1:34 PM, David Miller <davem@davemloft.net> wrote: > > From: Shaohua Li <shli@kernel.org> > > Date: Tue, 18 Jul 2017 12:03:37 -0700 > > > >> + /* Since this is being sent on the wire obfuscate hash a bit > >> + * to minimize possbility that any useful information to an > >> + * attacker is leaked. Only lower 20 bits are relevant. > >> + */ > >> + rol32(hash, 16); > > > > This doesn't help anything at all. > > I believe the above code is copy-n-pasted from ip6_make_flowlabel() > (with few adjustments). Don't know why we can't refactor that function > for reuse. There are just several lines of code, I really don't want to add adhoc if-else for fast path. Thanks, Shaohua
On Mon, Jul 24, 2017 at 01:34:55PM -0700, David Miller wrote: > From: Shaohua Li <shli@kernel.org> > Date: Tue, 18 Jul 2017 12:03:37 -0700 > > > + /* Since this is being sent on the wire obfuscate hash a bit > > + * to minimize possbility that any useful information to an > > + * attacker is leaked. Only lower 20 bits are relevant. > > + */ > > + rol32(hash, 16); > > This doesn't help anything at all. > > I don't like things that try to give a sense of security (however > little) but actually don't give any at all. > > I'd rather, therefore, that you remove this altogether. ok, so you suggest removing the rol32() for ip6_make_flowlabel too, right? just want to make sure. Thanks, Shaohua
diff --git a/include/net/ipv6.h b/include/net/ipv6.h index f2a1ddb..0a21c17 100644 --- a/include/net/ipv6.h +++ b/include/net/ipv6.h @@ -804,6 +804,31 @@ static inline __be32 ip6_make_flowlabel(struct net *net, struct sk_buff *skb, return flowlabel; } +/* Like ip6_make_flowlabel, but already has hash */ +static inline __be32 ip6_make_flowlabel_from_hash(struct net *net, + bool autolabel, u32 hash) +{ + __be32 flowlabel; + + if (net->ipv6.sysctl.auto_flowlabels == IP6_AUTO_FLOW_LABEL_OFF || + (!autolabel && + net->ipv6.sysctl.auto_flowlabels != IP6_AUTO_FLOW_LABEL_FORCED)) + return 0; + + /* Since this is being sent on the wire obfuscate hash a bit + * to minimize possbility that any useful information to an + * attacker is leaked. Only lower 20 bits are relevant. + */ + rol32(hash, 16); + + flowlabel = (__force __be32)hash & IPV6_FLOWLABEL_MASK; + + if (net->ipv6.sysctl.flowlabel_state_ranges) + flowlabel |= IPV6_FLOWLABEL_STATELESS_FLAG; + + return flowlabel; +} + static inline int ip6_default_np_autolabel(struct net *net) { switch (net->ipv6.sysctl.auto_flowlabels) { diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c index 0ff83c1..8e17058 100644 --- a/net/ipv4/tcp_minisocks.c +++ b/net/ipv4/tcp_minisocks.c @@ -276,11 +276,17 @@ void tcp_time_wait(struct sock *sk, int state, int timeo) #if IS_ENABLED(CONFIG_IPV6) if (tw->tw_family == PF_INET6) { struct ipv6_pinfo *np = inet6_sk(sk); + __be32 flowlabel; tw->tw_v6_daddr = sk->sk_v6_daddr; tw->tw_v6_rcv_saddr = sk->sk_v6_rcv_saddr; tw->tw_tclass = np->tclass; - tw->tw_flowlabel = be32_to_cpu(np->flow_label & IPV6_FLOWLABEL_MASK); + flowlabel = np->flow_label & IPV6_FLOWLABEL_MASK; + if (flowlabel == 0) + flowlabel = ip6_make_flowlabel_from_hash( + sock_net(sk), np->autoflowlabel, + sk->sk_txhash); + tw->tw_flowlabel = be32_to_cpu(flowlabel); tw->tw_ipv6only = sk->sk_ipv6only; } #endif diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c index 2521690..bb47b6c 100644 --- a/net/ipv6/tcp_ipv6.c +++ b/net/ipv6/tcp_ipv6.c @@ -891,6 +891,8 @@ static void tcp_v6_send_reset(const struct sock *sk, struct sk_buff *skb) struct sock *sk1 = NULL; #endif int oif; + u8 tclass = 0; + __be32 flowlabel = 0; if (th->rst) return; @@ -939,7 +941,21 @@ static void tcp_v6_send_reset(const struct sock *sk, struct sk_buff *skb) (th->doff << 2); oif = sk ? sk->sk_bound_dev_if : 0; - tcp_v6_send_response(sk, skb, seq, ack_seq, 0, 0, 0, oif, key, 1, 0, 0); + if (sk) { + if (sk_fullsock(sk)) { + struct ipv6_pinfo *np = inet6_sk(sk); + + tclass = np->tclass; + flowlabel = np->flow_label & IPV6_FLOWLABEL_MASK; + } else { + struct inet_timewait_sock *tw = inet_twsk(sk); + + tclass = tw->tw_tclass; + flowlabel = cpu_to_be32(tw->tw_flowlabel); + } + } + tcp_v6_send_response(sk, skb, seq, ack_seq, 0, 0, 0, oif, key, 1, + tclass, flowlabel); #ifdef CONFIG_TCP_MD5SIG out: