| Message ID | 1388963586-5049-2-git-send-email-pablo@netfilter.org |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
On Mon, Jan 06, 2014 at 12:12:55AM +0100, Pablo Neira Ayuso wrote: > From: Florian Westphal <fw@strlen.de> > > All these users need an initial seed value for jhash, prandom is > perfectly fine. This avoids draining the entropy pool where > its not strictly required. Secrets protecting hash tables should be rather strong. prandom_u32() has two seeding points at boot-up. One is at late_initcall. Thanks to parallel boot-up this gets executed fairly early. The other one is when the RNG nonblocking pool is fully initialized. Only after this point we can assume prandom_u32() returns truely random values. In between, only get_random_bytes or net_get_random_once are safe for use. To get the impression when prandom_u32 gets truely seeded, watch out for the message "random: nonblocking pool is initialized" in dmesg. ;) Hmm, some of them look like good candidates for net_get_random_once. I don't see such a problem with draining entropy pool, especially as they don't run that early and they don't request so many random bits. Greetings, Hannes -- 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
Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > On Mon, Jan 06, 2014 at 12:12:55AM +0100, Pablo Neira Ayuso wrote: > > From: Florian Westphal <fw@strlen.de> > > > > All these users need an initial seed value for jhash, prandom is > > perfectly fine. This avoids draining the entropy pool where > > its not strictly required. > > Secrets protecting hash tables should be rather strong. Yes, which is why e.g. conntrack hash is not converted. > prandom_u32() has two seeding points at boot-up. One is at late_initcall. Yes. None of these locations are executed via initcalls, they are all in _checkentry (i.e., run when userspace iptables inserts a rule using the target/match), except hashlimit where its delayed until the first address is stored (so its even later). > Thanks to parallel boot-up this gets executed fairly early. The other one is > when the RNG nonblocking pool is fully initialized. Only after this point we > can assume prandom_u32() returns truely random values. In between, only > get_random_bytes or net_get_random_once are safe for use. Can you elaborate? If entropy estimate is really really low (because we're booting up), why would get_random_bytes() be a better choice [ i understand net_get_random_once() is for delaying the actual random_bytes call until a later point in time where we've hopefully collected more entropy ] > To get the impression when prandom_u32 gets truely seeded, watch out > for the message "random: nonblocking pool is initialized" in dmesg. ;) It happens very very early on my machine, even before / is remounted rw. I would be more interested in what happens on small embedded boxes... > Hmm, some of them look like good candidates for net_get_random_once. I don't > see such a problem with draining entropy pool, especially as they don't run > that early and they don't request so many random bits. I specifically did not use net_get_random_once once because checkentry is not a hotpath. I don't see why get_random_bytes use increases the security margin, especially considering none of these hashes have periodic run-time rehashing? But sure, if you think this change is a problem, Pablo can just revert 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
Hello! On Mon, Jan 06, 2014 at 12:54:36PM +0100, Florian Westphal wrote: > Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > > On Mon, Jan 06, 2014 at 12:12:55AM +0100, Pablo Neira Ayuso wrote: > > > From: Florian Westphal <fw@strlen.de> > > > > > > All these users need an initial seed value for jhash, prandom is > > > perfectly fine. This avoids draining the entropy pool where > > > its not strictly required. > > > > Secrets protecting hash tables should be rather strong. > > Yes, which is why e.g. conntrack hash is not converted. > > > prandom_u32() has two seeding points at boot-up. One is at late_initcall. > > Yes. None of these locations are executed via initcalls, they are all > in _checkentry (i.e., run when userspace iptables inserts a rule using > the target/match), except hashlimit where its delayed until the first > address is stored (so its even later). > > > Thanks to parallel boot-up this gets executed fairly early. The other one is > > when the RNG nonblocking pool is fully initialized. Only after this point we > > can assume prandom_u32() returns truely random values. In between, only > > get_random_bytes or net_get_random_once are safe for use. > > Can you elaborate? If entropy estimate is really really low > (because we're booting up), why would get_random_bytes() be a better > choice [ i understand net_get_random_once() is for delaying > the actual random_bytes call until a later point in time where we've > hopefully collected more entropy ] I hope, I answer that below. > > To get the impression when prandom_u32 gets truely seeded, watch out > > for the message "random: nonblocking pool is initialized" in dmesg. ;) > > It happens very very early on my machine, even before / is remounted > rw. I would be more interested in what happens on small embedded > boxes... On some of my small virtual machines (amd64) I even see this message while login on the console (small iptables set also loaded before). In the mean time prandom_u32() is still seeded with maybe 3 bits (I once measured it) at the beginning and won't get a refresh until the nonblocking pool is fully initialized. prandom_u32 will just iterate over its seed until it is renewed whereas get_random_bytes does try to stretch (with help of the twisted GFSR and SHA-1) the available entropy in case the nonblocking_pool is limited, thus it is more probable to get better random results. E.g. on an amd64 athlon x2 with two VMs: [Mon Jan 6 13:35:40 2014] Initializing cgroup subsys cpuset ... [Mon Jan 6 13:36:21 2014] random: nonblocking pool is initialized I normally get the message while typing in the password on the prompt of the serial console. Single integers are not so much of a problem. E.g. one problem in wireless code was, where get_random_bytes was called in a loop to fill a structure, that did hurt: f7d8ad81ca8c44 ("mac80211: minstrels: spare numerous useless calls to get_random_bytes"). > > Hmm, some of them look like good candidates for net_get_random_once. I don't > > see such a problem with draining entropy pool, especially as they don't run > > that early and they don't request so many random bits. > > I specifically did not use net_get_random_once once because checkentry is > not a hotpath. > > I don't see why get_random_bytes use increases the security margin, especially > considering none of these hashes have periodic run-time rehashing? > > But sure, if you think this change is a problem, Pablo can just revert it. I don't know if it is a real problem. Most of the time the initial seed should be enough, but I guess get_random_bytes would still be a more defensive choice. I would have used it. ;) Greetings, Hannes -- 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
On Mon, Jan 06, 2014 at 01:43:40PM +0100, Hannes Frederic Sowa wrote: [...] > > > Hmm, some of them look like good candidates for net_get_random_once. I don't > > > see such a problem with draining entropy pool, especially as they don't run > > > that early and they don't request so many random bits. > > > > I specifically did not use net_get_random_once once because checkentry is > > not a hotpath. > > > > I don't see why get_random_bytes use increases the security margin, especially > > considering none of these hashes have periodic run-time rehashing? > > > > But sure, if you think this change is a problem, Pablo can just revert it. > > I don't know if it is a real problem. Most of the time the initial seed > should be enough, but I guess get_random_bytes would still be a more > defensive choice. I would have used it. ;) OK, I have reverted this patch, thanks. -- 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
Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > On Mon, Jan 06, 2014 at 12:54:36PM +0100, Florian Westphal wrote: > > Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > > Can you elaborate? If entropy estimate is really really low > > (because we're booting up), why would get_random_bytes() be a better > > choice [ i understand net_get_random_once() is for delaying > > the actual random_bytes call until a later point in time where we've > > hopefully collected more entropy ] [..] > On some of my small virtual machines (amd64) I even see this message while > login on the console (small iptables set also loaded before). In the mean > time prandom_u32() is still seeded with maybe 3 bits (I once measured it) > at the beginning and won't get a refresh until the nonblocking pool is > fully initialized. I see. In this case it indeed could be a problem; I was doing this change with the assumption that prandom is useable at ->checkenty time. > > I specifically did not use net_get_random_once once because checkentry is > > not a hotpath. > > > > I don't see why get_random_bytes use increases the security margin, especially > > considering none of these hashes have periodic run-time rehashing? > > > > But sure, if you think this change is a problem, Pablo can just revert it. > > I don't know if it is a real problem. Most of the time the initial seed > should be enough, but I guess get_random_bytes would still be a more > defensive choice. I would have used it. ;) Alright. Given that this went into -next, I think we have a few weeks to investigate. I will check if the specific hash uses are problematic in their own right (due to lack of reseed) or if they are weakened by this change only. I'll follow up on this. Pablo/David, if you think this needs to be fixed RIGHT NOW then please just issue a revert for a42b99a6e329654d376b330de057eff87686d890. Thanks! -- 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
On Mon, Jan 06, 2014 at 02:04:51PM +0100, Florian Westphal wrote: > Pablo/David, if you think this needs to be fixed RIGHT NOW then please > just issue a revert for a42b99a6e329654d376b330de057eff87686d890. I just did it, thanks Florian. -- 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
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Mon, Jan 06, 2014 at 02:04:51PM +0100, Florian Westphal wrote: > > Pablo/David, if you think this needs to be fixed RIGHT NOW then please > > just issue a revert for a42b99a6e329654d376b330de057eff87686d890. > > I just did it, thanks Florian. Ok. Can you resurrect the nfnetlink_log change? That one is safe :) -- 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/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c index 3c4b69e..7d4254b 100644 --- a/net/netfilter/nfnetlink_log.c +++ b/net/netfilter/nfnetlink_log.c @@ -28,8 +28,6 @@ #include <linux/proc_fs.h> #include <linux/security.h> #include <linux/list.h> -#include <linux/jhash.h> -#include <linux/random.h> #include <linux/slab.h> #include <net/sock.h> #include <net/netfilter/nf_log.h> @@ -75,7 +73,6 @@ struct nfulnl_instance { }; #define INSTANCE_BUCKETS 16 -static unsigned int hash_init; static int nfnl_log_net_id __read_mostly; @@ -1066,11 +1063,6 @@ static int __init nfnetlink_log_init(void) { int status = -ENOMEM; - /* it's not really all that important to have a random value, so - * we can do this from the init function, even if there hasn't - * been that much entropy yet */ - get_random_bytes(&hash_init, sizeof(hash_init)); - netlink_register_notifier(&nfulnl_rtnl_notifier); status = nfnetlink_subsys_register(&nfulnl_subsys); if (status < 0) { diff --git a/net/netfilter/nft_hash.c b/net/netfilter/nft_hash.c index 3d3f8fc..6aae699 100644 --- a/net/netfilter/nft_hash.c +++ b/net/netfilter/nft_hash.c @@ -164,7 +164,7 @@ static int nft_hash_init(const struct nft_set *set, unsigned int cnt, i; if (unlikely(!nft_hash_rnd_initted)) { - get_random_bytes(&nft_hash_rnd, 4); + nft_hash_rnd = prandom_u32(); nft_hash_rnd_initted = true; } diff --git a/net/netfilter/xt_RATEEST.c b/net/netfilter/xt_RATEEST.c index 370adf6..190854b 100644 --- a/net/netfilter/xt_RATEEST.c +++ b/net/netfilter/xt_RATEEST.c @@ -100,7 +100,7 @@ static int xt_rateest_tg_checkentry(const struct xt_tgchk_param *par) int ret; if (unlikely(!rnd_inited)) { - get_random_bytes(&jhash_rnd, sizeof(jhash_rnd)); + jhash_rnd = prandom_u32(); rnd_inited = true; } diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c index c40b269..7671e82 100644 --- a/net/netfilter/xt_connlimit.c +++ b/net/netfilter/xt_connlimit.c @@ -229,7 +229,7 @@ static int connlimit_mt_check(const struct xt_mtchk_param *par) u_int32_t rand; do { - get_random_bytes(&rand, sizeof(rand)); + rand = prandom_u32(); } while (!rand); cmpxchg(&connlimit_rnd, 0, rand); } diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c index 9ff035c..a83a35c 100644 --- a/net/netfilter/xt_hashlimit.c +++ b/net/netfilter/xt_hashlimit.c @@ -177,7 +177,7 @@ dsthash_alloc_init(struct xt_hashlimit_htable *ht, /* initialize hash with random val at the time we allocate * the first hashtable entry */ if (unlikely(!ht->rnd_initialized)) { - get_random_bytes(&ht->rnd, sizeof(ht->rnd)); + ht->rnd = prandom_u32(); ht->rnd_initialized = true; } diff --git a/net/netfilter/xt_recent.c b/net/netfilter/xt_recent.c index 1e657cf..bfdc29f 100644 --- a/net/netfilter/xt_recent.c +++ b/net/netfilter/xt_recent.c @@ -334,7 +334,7 @@ static int recent_mt_check(const struct xt_mtchk_param *par, size_t sz; if (unlikely(!hash_rnd_inited)) { - get_random_bytes(&hash_rnd, sizeof(hash_rnd)); + hash_rnd = prandom_u32(); hash_rnd_inited = true; } if (info->check_set & ~XT_RECENT_VALID_FLAGS) {