From patchwork Sat Jul 1 10:35:04 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Westphal X-Patchwork-Id: 783057 X-Patchwork-Delegate: pablo@netfilter.org Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3x08vl75DVz9t2J for ; Sat, 1 Jul 2017 20:36:35 +1000 (AEST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751853AbdGAKgU (ORCPT ); Sat, 1 Jul 2017 06:36:20 -0400 Received: from Chamillionaire.breakpoint.cc ([146.0.238.67]:49154 "EHLO Chamillionaire.breakpoint.cc" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751753AbdGAKgT (ORCPT ); Sat, 1 Jul 2017 06:36:19 -0400 Received: from fw by Chamillionaire.breakpoint.cc with local (Exim 4.84_2) (envelope-from ) id 1dRFk0-0002uJ-SZ; Sat, 01 Jul 2017 12:35:04 +0200 Date: Sat, 1 Jul 2017 12:35:04 +0200 From: Florian Westphal To: Richard Weinberger Cc: Florian Westphal , "linux-kernel@vger.kernel.org" , Pablo Neira Ayuso , David Miller , netfilter-devel@vger.kernel.org, coreteam@netfilter.org, "netdev@vger.kernel.org" , David Gstir , kaber@trash.net, "keescook@chromium.org" Subject: Re: nf_conntrack: Infoleak via CTA_ID and CTA_EXPECT_ID Message-ID: <20170701103504.GO9307@breakpoint.cc> References: <20170630193544.GM9307@breakpoint.cc> <20170630195547.GN9307@breakpoint.cc> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: netfilter-devel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netfilter-devel@vger.kernel.org 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. 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? --- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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 #include #include +#include #include #include #include @@ -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; }