* nf_conntrack: Infoleak via CTA_ID and CTA_EXPECT_ID @ 2017-06-30 19:25 Richard Weinberger 2017-06-30 19:35 ` Florian Westphal 0 siblings, 1 reply; 9+ messages in thread From: Richard Weinberger @ 2017-06-30 19:25 UTC (permalink / raw) To: linux-kernel Cc: Pablo Neira Ayuso, fw, David Miller, netfilter-devel, coreteam, netdev, David Gstir, kaber, keescook Hi! I noticed that nf_conntrack leaks kernel addresses, it uses the memory address as identifier used for generating conntrack and expect ids.. Since these ids are also visible to unprivileged users via network namespaces I suggest reverting these commits: commit 7f85f914721ffcef382a57995182916bd43d8a65 Author: Patrick McHardy <kaber@trash.net> Date: Fri Sep 28 14:41:27 2007 -0700 [NETFILTER]: nf_conntrack: kill unique ID Remove the per-conntrack ID, its not necessary anymore for dumping. For compatiblity reasons we send the address of the conntrack to userspace as ID. Signed-off-by: Patrick McHardy <kaber@trash.net> Signed-off-by: David S. Miller <davem@davemloft.net> commit 3583240249ef354760e04ae49bd7b462a638f40c Author: Patrick McHardy <kaber@trash.net> Date: Fri Sep 28 14:41:50 2007 -0700 [NETFILTER]: nf_conntrack_expect: kill unique ID Similar to the conntrack ID, the per-expectation ID is not needed anymore, kill it. Signed-off-by: Patrick McHardy <kaber@trash.net> Signed-off-by: David S. Miller <davem@davemloft.net> Thanks, //richard ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: nf_conntrack: Infoleak via CTA_ID and CTA_EXPECT_ID 2017-06-30 19:25 nf_conntrack: Infoleak via CTA_ID and CTA_EXPECT_ID Richard Weinberger @ 2017-06-30 19:35 ` Florian Westphal 2017-06-30 19:45 ` Richard Weinberger 0 siblings, 1 reply; 9+ messages in thread From: Florian Westphal @ 2017-06-30 19:35 UTC (permalink / raw) To: Richard Weinberger Cc: linux-kernel, Pablo Neira Ayuso, fw, David Miller, netfilter-devel, coreteam, netdev, David Gstir, kaber, keescook Richard Weinberger <richard@nod.at> wrote: > Hi! > > I noticed that nf_conntrack leaks kernel addresses, it uses the memory address > as identifier used for generating conntrack and expect ids.. > Since these ids are also visible to unprivileged users via network namespaces > I suggest reverting these commits: Why not use a hash of the address? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: nf_conntrack: Infoleak via CTA_ID and CTA_EXPECT_ID 2017-06-30 19:35 ` Florian Westphal @ 2017-06-30 19:45 ` Richard Weinberger 2017-06-30 19:55 ` Florian Westphal 0 siblings, 1 reply; 9+ messages in thread From: Richard Weinberger @ 2017-06-30 19:45 UTC (permalink / raw) To: Florian Westphal Cc: linux-kernel, Pablo Neira Ayuso, David Miller, netfilter-devel, coreteam, netdev, David Gstir, kaber, keescook Florian, Am 30.06.2017 um 21:35 schrieb Florian Westphal: > Richard Weinberger <richard@nod.at> wrote: >> Hi! >> >> I noticed that nf_conntrack leaks kernel addresses, it uses the memory address >> as identifier used for generating conntrack and expect ids.. >> Since these ids are also visible to unprivileged users via network namespaces >> I suggest reverting these commits: > > Why not use a hash of the address? Would also work. Or xor it with a random number. On the other hand, for user space it would be more useful when the conntrack id does not repeat that often. That's why I favor the good old counter method. Currently the conntrack id is reused very fast. e.g. in one of our applications we use the conntack id via NFQUEUE and watch the destroy events via conntrack. It happens regularly that a new connection has the same id than a different connection we saw some moments before, before we receive the destroy event from the conntrack socket. Thanks, //richard ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: nf_conntrack: Infoleak via CTA_ID and CTA_EXPECT_ID 2017-06-30 19:45 ` Richard Weinberger @ 2017-06-30 19:55 ` Florian Westphal 2017-06-30 20:23 ` Richard Weinberger 0 siblings, 1 reply; 9+ messages in thread From: Florian Westphal @ 2017-06-30 19:55 UTC (permalink / raw) To: Richard Weinberger Cc: Florian Westphal, linux-kernel, Pablo Neira Ayuso, David Miller, netfilter-devel, coreteam, netdev, David Gstir, kaber, keescook Richard Weinberger <richard@nod.at> wrote: > Florian, > > Am 30.06.2017 um 21:35 schrieb Florian Westphal: > > Richard Weinberger <richard@nod.at> wrote: > >> Hi! > >> > >> I noticed that nf_conntrack leaks kernel addresses, it uses the memory address > >> as identifier used for generating conntrack and expect ids.. > >> Since these ids are also visible to unprivileged users via network namespaces > >> I suggest reverting these commits: > > > > Why not use a hash of the address? > > Would also work. Or xor it with a random number. > > On the other hand, for user space it would be more useful when the conntrack id > does not repeat that often. That's why I favor the good old counter method. > Currently the conntrack id is reused very fast. > e.g. in one of our applications we use the conntack id via NFQUEUE and watch the > destroy events via conntrack. It happens regularly that a new connection has the > same id than a different connection we saw some moments before, before we receive > the destroy event from the conntrack socket. Perhaps we can place that in a new extension (its not needed in any fastpath ops)? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: nf_conntrack: Infoleak via CTA_ID and CTA_EXPECT_ID 2017-06-30 19:55 ` Florian Westphal @ 2017-06-30 20:23 ` Richard Weinberger 2017-07-01 9:44 ` Pablo Neira Ayuso 2017-07-01 10:35 ` Florian Westphal 0 siblings, 2 replies; 9+ messages in thread From: Richard Weinberger @ 2017-06-30 20:23 UTC (permalink / raw) To: Florian Westphal Cc: linux-kernel, Pablo Neira Ayuso, David Miller, netfilter-devel, coreteam, netdev, David Gstir, kaber, keescook Florian, Am 30.06.2017 um 21:55 schrieb Florian Westphal: >>> Why not use a hash of the address? >> >> Would also work. Or xor it with a random number. >> >> On the other hand, for user space it would be more useful when the conntrack id >> does not repeat that often. That's why I favor the good old counter method. >> Currently the conntrack id is reused very fast. >> e.g. in one of our applications we use the conntack id via NFQUEUE and watch the >> destroy events via conntrack. It happens regularly that a new connection has the >> same id than a different connection we saw some moments before, before we receive >> the destroy event from the conntrack socket. > > Perhaps we can place that in a new extension (its not needed in any > fastpath ops)? To get rid of the infoleak we have to re-introduce the id field in struct nf_conn and struct nf_conntrack_expect. Otherwise have nothing to compare against in the conntrack/expect remove case. So the only question is what to store, IMHO a counter that can wrap around is the cheapest method and would also not harm the fast-path. Thanks, //richard ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: nf_conntrack: Infoleak via CTA_ID and CTA_EXPECT_ID 2017-06-30 20:23 ` Richard Weinberger @ 2017-07-01 9:44 ` Pablo Neira Ayuso 2017-07-01 10:35 ` Florian Westphal 1 sibling, 0 replies; 9+ messages in thread From: Pablo Neira Ayuso @ 2017-07-01 9:44 UTC (permalink / raw) To: Richard Weinberger Cc: Florian Westphal, linux-kernel, David Miller, netfilter-devel, coreteam, netdev, David Gstir, kaber, keescook On Fri, Jun 30, 2017 at 10:23:24PM +0200, Richard Weinberger wrote: > Florian, > > Am 30.06.2017 um 21:55 schrieb Florian Westphal: > >>> Why not use a hash of the address? > >> > >> Would also work. Or xor it with a random number. > >> > >> On the other hand, for user space it would be more useful when the conntrack id > >> does not repeat that often. That's why I favor the good old counter method. > >> Currently the conntrack id is reused very fast. > >> e.g. in one of our applications we use the conntack id via NFQUEUE and watch the > >> destroy events via conntrack. It happens regularly that a new connection has the > >> same id than a different connection we saw some moments before, before we receive > >> the destroy event from the conntrack socket. > > > > Perhaps we can place that in a new extension (its not needed in any > > fastpath ops)? > > To get rid of the infoleak we have to re-introduce the id field in struct nf_conn > and struct nf_conntrack_expect. > Otherwise have nothing to compare against in the conntrack/expect remove case. > > So the only question is what to store, IMHO a counter that can wrap around is the > cheapest method and would also not harm the fast-path. I have a patch to assign ids through percpu approach that I can recover. It's dividing the u64 id space between the existing num_cpus. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: nf_conntrack: Infoleak via CTA_ID and CTA_EXPECT_ID 2017-06-30 20:23 ` Richard Weinberger 2017-07-01 9:44 ` Pablo Neira Ayuso @ 2017-07-01 10:35 ` Florian Westphal 2017-07-12 21:26 ` Richard Weinberger 1 sibling, 1 reply; 9+ messages in thread From: Florian Westphal @ 2017-07-01 10:35 UTC (permalink / raw) To: Richard Weinberger Cc: Florian Westphal, linux-kernel, Pablo Neira Ayuso, David Miller, netfilter-devel, coreteam, netdev, David Gstir, kaber, keescook Richard Weinberger <richard@nod.at> wrote: > Florian, > > Am 30.06.2017 um 21:55 schrieb Florian Westphal: > >>> Why not use a hash of the address? > >> > >> Would also work. Or xor it with a random number. > >> > >> On the other hand, for user space it would be more useful when the conntrack id > >> does not repeat that often. That's why I favor the good old counter method. > >> Currently the conntrack id is reused very fast. > >> e.g. in one of our applications we use the conntack id via NFQUEUE and watch the > >> destroy events via conntrack. It happens regularly that a new connection has the > >> same id than a different connection we saw some moments before, before we receive > >> the destroy event from the conntrack socket. > > > > Perhaps we can place that in a new extension (its not needed in any > > fastpath ops)? > > To get rid of the infoleak we have to re-introduce the id field in struct nf_conn > and struct nf_conntrack_expect. Why will this not work? > Otherwise have nothing to compare against in the conntrack/expect remove case. Not following, sorry. The id is not used anywhere except when we send info to userspace. The compare on removal is not needed afaics, and its also not used when doing lookup to begin with, so we can just recompute it? diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c --- a/net/netfilter/nf_conntrack_netlink.c +++ b/net/netfilter/nf_conntrack_netlink.c @@ -25,6 +25,7 @@ #include <linux/security.h> #include <linux/skbuff.h> #include <linux/errno.h> +#include <linux/hash.h> #include <linux/netlink.h> #include <linux/spinlock.h> #include <linux/interrupt.h> @@ -443,9 +444,44 @@ static int ctnetlink_dump_ct_seq_adj(struct sk_buff *skb, struct nf_conn *ct) return -1; } +static __be32 ct_to_id(const struct nf_conn *ct) +{ + static u32 seed __read_mostly; + u32 a, b, c; + + if (!ct) + return 0; + + if (!seed) + seed = get_random_u32(); + + a = jhash2((u32 *)ct->tuplehash, sizeof(ct->tuplehash) / sizeof(u32), + hash32_ptr(ct)); + b = ct_to_id(ct->master) ^ net_hash_mix(nf_ct_net(ct)); + c = hash32_ptr(ct->ext); + + return (__force __be32)jhash_3words(a, b, c, seed); +} + +static __be32 ctexp_to_id(const struct nf_conntrack_expect *exp) +{ + static u32 seed __read_mostly; + u32 a, b, c; + + if (!seed) + seed = get_random_u32(); + + a = ct_to_id(exp->master); + b = hash32_ptr(exp->helper); + c = jhash2((u32 *)&exp->tuple, sizeof(exp->tuple) / sizeof(u32), + hash32_ptr(exp)); + + return (__force __be32)jhash_3words(a, b, c, seed); +} + static int ctnetlink_dump_id(struct sk_buff *skb, const struct nf_conn *ct) { - if (nla_put_be32(skb, CTA_ID, htonl((unsigned long)ct))) + if (nla_put_be32(skb, CTA_ID, ct_to_id(ct))) goto nla_put_failure; return 0; @@ -1164,8 +1200,8 @@ static int ctnetlink_del_conntrack(struct net *net, struct sock *ctnl, ct = nf_ct_tuplehash_to_ctrack(h); if (cda[CTA_ID]) { - u_int32_t id = ntohl(nla_get_be32(cda[CTA_ID])); - if (id != (u32)(unsigned long)ct) { + __be32 id = nla_get_be32(cda[CTA_ID]); + if (id != ct_to_id(ct)) { nf_ct_put(ct); return -ENOENT; } @@ -2563,7 +2599,7 @@ ctnetlink_exp_dump_expect(struct sk_buff *skb, } #endif if (nla_put_be32(skb, CTA_EXPECT_TIMEOUT, htonl(timeout)) || - nla_put_be32(skb, CTA_EXPECT_ID, htonl((unsigned long)exp)) || + nla_put_be32(skb, CTA_EXPECT_ID, ctexp_to_id(exp)) || nla_put_be32(skb, CTA_EXPECT_FLAGS, htonl(exp->flags)) || nla_put_be32(skb, CTA_EXPECT_CLASS, htonl(exp->class))) goto nla_put_failure; @@ -2871,7 +2907,7 @@ static int ctnetlink_get_expect(struct net *net, struct sock *ctnl, if (cda[CTA_EXPECT_ID]) { __be32 id = nla_get_be32(cda[CTA_EXPECT_ID]); - if (ntohl(id) != (u32)(unsigned long)exp) { + if (id != ctexp_to_id(exp)) { nf_ct_expect_put(exp); return -ENOENT; } ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: nf_conntrack: Infoleak via CTA_ID and CTA_EXPECT_ID 2017-07-01 10:35 ` Florian Westphal @ 2017-07-12 21:26 ` Richard Weinberger 2017-07-12 22:19 ` Florian Westphal 0 siblings, 1 reply; 9+ messages in thread From: Richard Weinberger @ 2017-07-12 21:26 UTC (permalink / raw) To: Florian Westphal Cc: linux-kernel, Pablo Neira Ayuso, David Miller, netfilter-devel, coreteam, netdev, David Gstir, kaber, keescook Florian, Am 01.07.2017 um 12:35 schrieb Florian Westphal: >>> Perhaps we can place that in a new extension (its not needed in any >>> fastpath ops)? >> >> To get rid of the infoleak we have to re-introduce the id field in struct nf_conn >> and struct nf_conntrack_expect. > > Why will this not work? You are right, when we compute the ID from the whole object, it should be fine. >> Otherwise have nothing to compare against in the conntrack/expect remove case. > > Not following, sorry. The id is not used anywhere except when we send > info to userspace. > > The compare on removal is not needed afaics, and its also not used when > doing lookup to begin with, so we can just recompute it? Isn't this a way too much overhead? I personally favor Pablo's per-cpu counter approach. That way the IDs are unique again and we get rid of the info leak without much effort. Thanks, //richard ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: nf_conntrack: Infoleak via CTA_ID and CTA_EXPECT_ID 2017-07-12 21:26 ` Richard Weinberger @ 2017-07-12 22:19 ` Florian Westphal 0 siblings, 0 replies; 9+ messages in thread From: Florian Westphal @ 2017-07-12 22:19 UTC (permalink / raw) To: Richard Weinberger Cc: Florian Westphal, linux-kernel, Pablo Neira Ayuso, David Miller, netfilter-devel, coreteam, netdev, David Gstir, kaber, keescook Richard Weinberger <richard@nod.at> wrote: > Am 01.07.2017 um 12:35 schrieb Florian Westphal: > > The compare on removal is not needed afaics, and its also not used when > > doing lookup to begin with, so we can just recompute it? > > Isn't this a way too much overhead? I don't think so. This computation only occurs when we dump events to userspace. > I personally favor Pablo's per-cpu counter approach. > That way the IDs are unique again and we get rid of the info leak without > much effort. I have not seen these patches so can't really comment. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-07-12 22:21 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-30 19:25 nf_conntrack: Infoleak via CTA_ID and CTA_EXPECT_ID Richard Weinberger 2017-06-30 19:35 ` Florian Westphal 2017-06-30 19:45 ` Richard Weinberger 2017-06-30 19:55 ` Florian Westphal 2017-06-30 20:23 ` Richard Weinberger 2017-07-01 9:44 ` Pablo Neira Ayuso 2017-07-01 10:35 ` Florian Westphal 2017-07-12 21:26 ` Richard Weinberger 2017-07-12 22:19 ` Florian Westphal
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).