[PATCH v4 00/13] Unseeded In-Kernel Randomness Fixes

[PATCH v4 00/13] Unseeded In-Kernel Randomness Fixes

[Jason A. Donenfeld]

As discussed in [1], there is a problem with get_random_bytes being
used before the RNG has actually been seeded. The solution for fixing
this appears to be multi-pronged. One of those prongs involves adding
a simple blocking API so that modules that use the RNG in process
context can just sleep (in an interruptable manner) until the RNG is
ready to be used. This winds up being a very useful API that covers
a few use cases, several of which are included in this patch set.

[1] http://www.openwall.com/lists/kernel-hardening/2017/06/02/2

Changes v3->v4:
  - Mark one patch for stable
  - Operation ordering on batched entropy invalidation
  - Separate out big_key into its own patch to the keys mailing list
  - General cleanups

Jason A. Donenfeld (13):
  random: invalidate batched entropy after crng init
  random: add synchronous API for the urandom pool
  random: add get_random_{bytes,u32,u64,int,long,once}_wait family
  security/keys: ensure RNG is seeded before use
  crypto/rng: ensure that the RNG is ready before using
  iscsi: ensure RNG is seeded before use
  ceph: ensure RNG is seeded before using
  cifs: use get_random_u32 for 32-bit lock random
  rhashtable: use get_random_u32 for hash_rnd
  net/neighbor: use get_random_u32 for 32-bit hash random
  net/route: use get_random_int for random counter
  bluetooth/smp: ensure RNG is properly seeded before ECDH use
  random: warn when kernel uses unseeded randomness

 crypto/rng.c                              |  6 +-
 drivers/char/random.c                     | 93 +++++++++++++++++++++++++++----
 drivers/target/iscsi/iscsi_target_auth.c  | 14 ++++-
 drivers/target/iscsi/iscsi_target_login.c | 22 +++++---
 fs/cifs/cifsfs.c                          |  2 +-
 include/linux/net.h                       |  2 +
 include/linux/once.h                      |  2 +
 include/linux/random.h                    | 26 +++++++++
 lib/Kconfig.debug                         | 16 ++++++
 lib/rhashtable.c                          |  2 +-
 net/bluetooth/hci_request.c               |  6 ++
 net/bluetooth/smp.c                       | 18 ++++--
 net/ceph/ceph_common.c                    |  6 +-
 net/core/neighbour.c                      |  3 +-
 net/ipv4/route.c                          |  3 +-
 security/keys/encrypted-keys/encrypted.c  |  8 ++-
 security/keys/key.c                       | 16 +++---
 17 files changed, 198 insertions(+), 47 deletions(-)

-- 
2.13.0

[PATCH v4 01/13] random: invalidate batched entropy after crng init

[Jason A. Donenfeld]

It's possible that get_random_{u32,u64} is used before the crng has
initialized, in which case, its output might not be cryptographically
secure. For this problem, directly, this patch set is introducing the
*_wait variety of functions, but even with that, there's a subtle issue:
what happens to our batched entropy that was generated before
initialization. Prior to this commit, it'd stick around, supplying bad
numbers. After this commit, we force the entropy to be re-extracted
after each phase of the crng has initialized.

In order to avoid a race condition with the position counter, we
introduce a simple rwlock for this invalidation. Since it's only during
this awkward transition period, after things are all set up, we stop
using it, so that it doesn't have an impact on performance.

This should probably be backported to 4.11.

(Also: adding my copyright to the top. With the patch series from
January, this patch, and then the ones that come after, I think there's
a relevant amount of code in here to add my name to the top.)

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/char/random.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 0ab024918907..2291e6224ed3 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1,6 +1,9 @@
 /*
  * random.c -- A strong random number generator
  *
+ * Copyright (C) 2017 Jason A. Donenfeld <Jason@zx2c4.com>. All
+ * Rights Reserved.
+ *
  * Copyright Matt Mackall <mpm@selenic.com>, 2003, 2004, 2005
  *
  * Copyright Theodore Ts'o, 1994, 1995, 1996, 1997, 1998, 1999.  All
@@ -762,6 +765,8 @@ static DECLARE_WAIT_QUEUE_HEAD(crng_init_wait);
 static struct crng_state **crng_node_pool __read_mostly;
 #endif
 
+static void invalidate_batched_entropy(void);
+
 static void crng_initialize(struct crng_state *crng)
 {
 	int		i;
@@ -799,6 +804,7 @@ static int crng_fast_load(const char *cp, size_t len)
 		cp++; crng_init_cnt++; len--;
 	}
 	if (crng_init_cnt >= CRNG_INIT_CNT_THRESH) {
+		invalidate_batched_entropy();
 		crng_init = 1;
 		wake_up_interruptible(&crng_init_wait);
 		pr_notice("random: fast init done\n");
@@ -836,6 +842,7 @@ static void crng_reseed(struct crng_state *crng, struct entropy_store *r)
 	memzero_explicit(&buf, sizeof(buf));
 	crng->init_time = jiffies;
 	if (crng == &primary_crng && crng_init < 2) {
+		invalidate_batched_entropy();
 		crng_init = 2;
 		process_random_ready_list();
 		wake_up_interruptible(&crng_init_wait);
@@ -2019,6 +2026,7 @@ struct batched_entropy {
 	};
 	unsigned int position;
 };
+static rwlock_t batched_entropy_reset_lock = __RW_LOCK_UNLOCKED(batched_entropy_reset_lock);
 
 /*
  * Get a random word for internal kernel use only. The quality of the random
@@ -2029,6 +2037,8 @@ static DEFINE_PER_CPU(struct batched_entropy, batched_entropy_u64);
 u64 get_random_u64(void)
 {
 	u64 ret;
+	bool use_lock = crng_init < 2;
+	unsigned long flags;
 	struct batched_entropy *batch;
 
 #if BITS_PER_LONG == 64
@@ -2041,11 +2051,15 @@ u64 get_random_u64(void)
 #endif
 
 	batch = &get_cpu_var(batched_entropy_u64);
+	if (use_lock)
+		read_lock_irqsave(&batched_entropy_reset_lock, flags);
 	if (batch->position % ARRAY_SIZE(batch->entropy_u64) == 0) {
 		extract_crng((u8 *)batch->entropy_u64);
 		batch->position = 0;
 	}
 	ret = batch->entropy_u64[batch->position++];
+	if (use_lock)
+		read_unlock_irqrestore(&batched_entropy_reset_lock, flags);
 	put_cpu_var(batched_entropy_u64);
 	return ret;
 }
@@ -2055,22 +2069,45 @@ static DEFINE_PER_CPU(struct batched_entropy, batched_entropy_u32);
 u32 get_random_u32(void)
 {
 	u32 ret;
+	bool use_lock = crng_init < 2;
+	unsigned long flags;
 	struct batched_entropy *batch;
 
 	if (arch_get_random_int(&ret))
 		return ret;
 
 	batch = &get_cpu_var(batched_entropy_u32);
+	if (use_lock)
+		read_lock_irqsave(&batched_entropy_reset_lock, flags);
 	if (batch->position % ARRAY_SIZE(batch->entropy_u32) == 0) {
 		extract_crng((u8 *)batch->entropy_u32);
 		batch->position = 0;
 	}
 	ret = batch->entropy_u32[batch->position++];
+	if (use_lock)
+		read_unlock_irqrestore(&batched_entropy_reset_lock, flags);
 	put_cpu_var(batched_entropy_u32);
 	return ret;
 }
 EXPORT_SYMBOL(get_random_u32);
 
+/* It's important to invalidate all potential batched entropy that might
+ * be stored before the crng is initialized, which we can do lazily by
+ * simply resetting the counter to zero so that it's re-extracted on the
+ * next usage. */
+static void invalidate_batched_entropy(void)
+{
+	int cpu;
+	unsigned long flags;
+
+	write_lock_irqsave(&batched_entropy_reset_lock, flags);
+	for_each_possible_cpu (cpu) {
+		per_cpu_ptr(&batched_entropy_u32, cpu)->position = 0;
+		per_cpu_ptr(&batched_entropy_u64, cpu)->position = 0;
+	}
+	write_unlock_irqrestore(&batched_entropy_reset_lock, flags);
+}
+
 /**
  * randomize_page - Generate a random, page aligned address
  * @start:	The smallest acceptable address the caller will take.
-- 
2.13.0

Re: [PATCH v4 01/13] random: invalidate batched entropy after crng init

[Theodore Ts'o]

On Tue, Jun 06, 2017 at 07:47:52PM +0200, Jason A. Donenfeld wrote:
> It's possible that get_random_{u32,u64} is used before the crng has
> initialized, in which case, its output might not be cryptographically
> secure. For this problem, directly, this patch set is introducing the
> *_wait variety of functions, but even with that, there's a subtle issue:
> what happens to our batched entropy that was generated before
> initialization. Prior to this commit, it'd stick around, supplying bad
> numbers. After this commit, we force the entropy to be re-extracted
> after each phase of the crng has initialized.
> 
> In order to avoid a race condition with the position counter, we
> introduce a simple rwlock for this invalidation. Since it's only during
> this awkward transition period, after things are all set up, we stop
> using it, so that it doesn't have an impact on performance.
> 
> This should probably be backported to 4.11.
> 
> (Also: adding my copyright to the top. With the patch series from
> January, this patch, and then the ones that come after, I think there's
> a relevant amount of code in here to add my name to the top.)
> 
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Thanks, applied.  This will be on the for_stable that I will be
sending to Linus sometime during 4.12-rcX.

					- Ted

Re: [PATCH v4 01/13] random: invalidate batched entropy after crng init

[Jason A. Donenfeld]

On Thu, Jun 8, 2017 at 1:58 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> Thanks, applied.  This will be on the for_stable that I will be
> sending to Linus sometime during 4.12-rcX.

I think you might have just missed the kbuild test robot complaining
about an incorrect compiler warning, when using an ancient gcc for the
sh platform. I fixed that bug in v5, which I posted about an hour
ago. The only change was a single commit, so you can just cherry-pick
that as you wish:

https://git.kernel.org/pub/scm/linux/kernel/git/zx2c4/linux.git/patch/?id=2f390fd961d1f97738d7af4f4797542e05f4607e

Other than that one commit change, it was the same.

Or, if it's easier, the tag for-random-4.12 in
git://git.kernel.org/pub/scm/linux/kernel/git/tytso/random.git is for you.

[PATCH v4 02/13] random: add synchronous API for the urandom pool

[Jason A. Donenfeld]

This enables users of get_random_{bytes,u32,u64,int,long} to wait until
the pool is ready before using this function, in case they actually want
to have reliable randomness.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/char/random.c  | 41 +++++++++++++++++++++++++++++++----------
 include/linux/random.h |  1 +
 2 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 2291e6224ed3..36cdb2406610 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -851,11 +851,6 @@ static void crng_reseed(struct crng_state *crng, struct entropy_store *r)
 	spin_unlock_irqrestore(&primary_crng.lock, flags);
 }
 
-static inline void crng_wait_ready(void)
-{
-	wait_event_interruptible(crng_init_wait, crng_ready());
-}
-
 static void _extract_crng(struct crng_state *crng,
 			  __u8 out[CHACHA20_BLOCK_SIZE])
 {
@@ -1473,7 +1468,10 @@ static ssize_t extract_entropy_user(struct entropy_store *r, void __user *buf,
  * number of good random numbers, suitable for key generation, seeding
  * TCP sequence numbers, etc.  It does not rely on the hardware random
  * number generator.  For random bytes direct from the hardware RNG
- * (when available), use get_random_bytes_arch().
+ * (when available), use get_random_bytes_arch(). In order to ensure
+ * that the randomness provided by this function is okay, the function
+ * wait_for_random_bytes() should be called and return 0 at least once
+ * at any point prior.
  */
 void get_random_bytes(void *buf, int nbytes)
 {
@@ -1503,6 +1501,24 @@ void get_random_bytes(void *buf, int nbytes)
 EXPORT_SYMBOL(get_random_bytes);
 
 /*
+ * Wait for the urandom pool to be seeded and thus guaranteed to supply
+ * cryptographically secure random numbers. This applies to: the /dev/urandom
+ * device, the get_random_bytes function, and the get_random_{u32,u64,int,long}
+ * family of functions. Using any of these functions without first calling
+ * this function forfeits the guarantee of security.
+ *
+ * Returns: 0 if the urandom pool has been seeded.
+ *          -ERESTARTSYS if the function was interrupted by a signal.
+ */
+int wait_for_random_bytes(void)
+{
+	if (likely(crng_ready()))
+		return 0;
+	return wait_event_interruptible(crng_init_wait, crng_ready());
+}
+EXPORT_SYMBOL(wait_for_random_bytes);
+
+/*
  * Add a callback function that will be invoked when the nonblocking
  * pool is initialised.
  *
@@ -1856,6 +1872,8 @@ const struct file_operations urandom_fops = {
 SYSCALL_DEFINE3(getrandom, char __user *, buf, size_t, count,
 		unsigned int, flags)
 {
+	int ret;
+
 	if (flags & ~(GRND_NONBLOCK|GRND_RANDOM))
 		return -EINVAL;
 
@@ -1868,9 +1886,9 @@ SYSCALL_DEFINE3(getrandom, char __user *, buf, size_t, count,
 	if (!crng_ready()) {
 		if (flags & GRND_NONBLOCK)
 			return -EAGAIN;
-		crng_wait_ready();
-		if (signal_pending(current))
-			return -ERESTARTSYS;
+		ret = wait_for_random_bytes();
+		if (unlikely(ret))
+			return ret;
 	}
 	return urandom_read(NULL, buf, count, NULL);
 }
@@ -2031,7 +2049,10 @@ static rwlock_t batched_entropy_reset_lock = __RW_LOCK_UNLOCKED(batched_entropy_
 /*
  * Get a random word for internal kernel use only. The quality of the random
  * number is either as good as RDRAND or as good as /dev/urandom, with the
- * goal of being quite fast and not depleting entropy.
+ * goal of being quite fast and not depleting entropy. In order to ensure
+ * that the randomness provided by this function is okay, the function
+ * wait_for_random_bytes() should be called and return 0 at least once
+ * at any point prior.
  */
 static DEFINE_PER_CPU(struct batched_entropy, batched_entropy_u64);
 u64 get_random_u64(void)
diff --git a/include/linux/random.h b/include/linux/random.h
index ed5c3838780d..e29929347c95 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -34,6 +34,7 @@ extern void add_input_randomness(unsigned int type, unsigned int code,
 extern void add_interrupt_randomness(int irq, int irq_flags) __latent_entropy;
 
 extern void get_random_bytes(void *buf, int nbytes);
+extern int wait_for_random_bytes(void);
 extern int add_random_ready_callback(struct random_ready_callback *rdy);
 extern void del_random_ready_callback(struct random_ready_callback *rdy);
 extern void get_random_bytes_arch(void *buf, int nbytes);
-- 
2.13.0

Re: [PATCH v4 02/13] random: add synchronous API for the urandom pool

[Theodore Ts'o]

On Tue, Jun 06, 2017 at 07:47:53PM +0200, Jason A. Donenfeld wrote:
> This enables users of get_random_{bytes,u32,u64,int,long} to wait until
> the pool is ready before using this function, in case they actually want
> to have reliable randomness.
> 
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

Thanks, applied for the dev branch of random.git.  (I changed the
patch summary slightly; it now reads: "random: add
wait_for_random_bytes() API").

						- Ted

[PATCH v4 03/13] random: add get_random_{bytes,u32,u64,int,long,once}_wait family

[Jason A. Donenfeld]

These functions are simple convenience wrappers that call
wait_for_random_bytes before calling the respective get_random_*
function.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 include/linux/net.h    |  2 ++
 include/linux/once.h   |  2 ++
 include/linux/random.h | 25 +++++++++++++++++++++++++
 3 files changed, 29 insertions(+)

diff --git a/include/linux/net.h b/include/linux/net.h
index abcfa46a2bd9..dda2cc939a53 100644
--- a/include/linux/net.h
+++ b/include/linux/net.h
@@ -274,6 +274,8 @@ do {									\
 
 #define net_get_random_once(buf, nbytes)			\
 	get_random_once((buf), (nbytes))
+#define net_get_random_once_wait(buf, nbytes)			\
+	get_random_once_wait((buf), (nbytes))
 
 int kernel_sendmsg(struct socket *sock, struct msghdr *msg, struct kvec *vec,
 		   size_t num, size_t len);
diff --git a/include/linux/once.h b/include/linux/once.h
index 285f12cb40e6..9c98aaa87cbc 100644
--- a/include/linux/once.h
+++ b/include/linux/once.h
@@ -53,5 +53,7 @@ void __do_once_done(bool *done, struct static_key *once_key,
 
 #define get_random_once(buf, nbytes)					     \
 	DO_ONCE(get_random_bytes, (buf), (nbytes))
+#define get_random_once_wait(buf, nbytes)                                    \
+	DO_ONCE(get_random_bytes_wait, (buf), (nbytes))                      \
 
 #endif /* _LINUX_ONCE_H */
diff --git a/include/linux/random.h b/include/linux/random.h
index e29929347c95..4aecc339558d 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -58,6 +58,31 @@ static inline unsigned long get_random_long(void)
 #endif
 }
 
+/* Calls wait_for_random_bytes() and then calls get_random_bytes(buf, nbytes).
+ * Returns the result of the call to wait_for_random_bytes. */
+static inline int get_random_bytes_wait(void *buf, int nbytes)
+{
+	int ret = wait_for_random_bytes();
+	if (unlikely(ret))
+		return ret;
+	get_random_bytes(buf, nbytes);
+	return 0;
+}
+
+#define declare_get_random_var_wait(var) \
+	static inline int get_random_ ## var ## _wait(var *out) { \
+		int ret = wait_for_random_bytes(); \
+		if (unlikely(ret)) \
+			return ret; \
+		*out = get_random_ ## var(); \
+		return 0; \
+	}
+declare_get_random_var_wait(u32)
+declare_get_random_var_wait(u64)
+declare_get_random_var_wait(int)
+declare_get_random_var_wait(long)
+#undef declare_get_random_var
+
 unsigned long randomize_page(unsigned long start, unsigned long range);
 
 u32 prandom_u32(void);
-- 
2.13.0

Re: [kernel-hardening] [PATCH v4 03/13] random: add get_random_{bytes,u32,u64,int,long,once}_wait family

[Theodore Ts'o]

On Tue, Jun 06, 2017 at 07:47:54PM +0200, Jason A. Donenfeld wrote:
> These functions are simple convenience wrappers that call
> wait_for_random_bytes before calling the respective get_random_*
> function.
> 
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

Thanks, applied to the dev branch.

					- Ted

[PATCH v4 04/13] security/keys: ensure RNG is seeded before use

[Jason A. Donenfeld]

Otherwise, we might use bad random numbers which, particularly in the
case of IV generation, could be quite bad. It makes sense to use the
synchronous API here, because we're always in process context (as the
code is littered with GFP_KERNEL and the like). However, we can't change
to using a blocking function in key serial allocation, because this will
block booting in some configurations, so here we use the more
appropriate get_random_u32, which will use RDRAND if available.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: David Safford <safford@us.ibm.com>
---
 security/keys/encrypted-keys/encrypted.c |  8 +++++---
 security/keys/key.c                      | 16 ++++++++--------
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index 0010955d7876..d51a28fc5cd5 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -777,10 +777,12 @@ static int encrypted_init(struct encrypted_key_payload *epayload,
 
 	__ekey_init(epayload, format, master_desc, datalen);
 	if (!hex_encoded_iv) {
-		get_random_bytes(epayload->iv, ivsize);
+		ret = get_random_bytes_wait(epayload->iv, ivsize);
+		if (unlikely(ret))
+			return ret;
 
-		get_random_bytes(epayload->decrypted_data,
-				 epayload->decrypted_datalen);
+		ret = get_random_bytes_wait(epayload->decrypted_data,
+					    epayload->decrypted_datalen);
 	} else
 		ret = encrypted_key_decrypt(epayload, format, hex_encoded_iv);
 	return ret;
diff --git a/security/keys/key.c b/security/keys/key.c
index 455c04d80bbb..b72078e532f2 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -134,17 +134,15 @@ void key_user_put(struct key_user *user)
  * Allocate a serial number for a key.  These are assigned randomly to avoid
  * security issues through covert channel problems.
  */
-static inline void key_alloc_serial(struct key *key)
+static inline int key_alloc_serial(struct key *key)
 {
 	struct rb_node *parent, **p;
 	struct key *xkey;
 
-	/* propose a random serial number and look for a hole for it in the
-	 * serial number tree */
+	/* propose a non-negative random serial number and look for a hole for
+	 * it in the serial number tree */
 	do {
-		get_random_bytes(&key->serial, sizeof(key->serial));
-
-		key->serial >>= 1; /* negative numbers are not permitted */
+		key->serial = get_random_u32() >> 1;
 	} while (key->serial < 3);
 
 	spin_lock(&key_serial_lock);
@@ -170,7 +168,7 @@ static inline void key_alloc_serial(struct key *key)
 	rb_insert_color(&key->serial_node, &key_serial_tree);
 
 	spin_unlock(&key_serial_lock);
-	return;
+	return 0;
 
 	/* we found a key with the proposed serial number - walk the tree from
 	 * that point looking for the next unused serial number */
@@ -314,7 +312,9 @@ struct key *key_alloc(struct key_type *type, const char *desc,
 
 	/* publish the key by giving it a serial number */
 	atomic_inc(&user->nkeys);
-	key_alloc_serial(key);
+	ret = key_alloc_serial(key);
+	if (ret < 0)
+		goto security_error;
 
 error:
 	return key;
-- 
2.13.0

Re: [PATCH v4 04/13] security/keys: ensure RNG is seeded before use

[Theodore Ts'o]

On Tue, Jun 06, 2017 at 07:47:55PM +0200, Jason A. Donenfeld wrote:
> -static inline void key_alloc_serial(struct key *key)
> +static inline int key_alloc_serial(struct key *key)

> @@ -170,7 +168,7 @@ static inline void key_alloc_serial(struct key *key)
>  	rb_insert_color(&key->serial_node, &key_serial_tree);
>  
>  	spin_unlock(&key_serial_lock);
> -	return;
> +	return 0;
>  
>  	/* we found a key with the proposed serial number - walk the tree from
>  	 * that point looking for the next unused serial number */

> @@ -314,7 +312,9 @@ struct key *key_alloc(struct key_type *type, const char *desc,
>  
>  	/* publish the key by giving it a serial number */
>  	atomic_inc(&user->nkeys);
> -	key_alloc_serial(key);
> +	ret = key_alloc_serial(key);
> +	if (ret < 0)
> +		goto security_error;
>  
>  error:
>  	return key;

I'm guessing you changed key_alloc_serial() to return an int back when
you were thinking that you might use get_random_bytes_wait(), which
could return -ERESTARTSYS.

Now that you're not doing this, but using get_random_u32() instead,
there's no point to change the function signature of
key_alloc_serial() and add an error check in key_alloc() that will
never fail, right?  That's just adding a dead code path.  Which the
compiler can probably optimize away, but why make the code slightly
harder to read than necessasry?

						- Ted

Re: [PATCH v4 04/13] security/keys: ensure RNG is seeded before use

[Jason A. Donenfeld]

On Thu, Jun 8, 2017 at 2:31 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> I'm guessing you changed key_alloc_serial() to return an int back when
> you were thinking that you might use get_random_bytes_wait(), which
> could return -ERESTARTSYS.
>
> Now that you're not doing this, but using get_random_u32() instead,
> there's no point to change the function signature of
> key_alloc_serial() and add an error check in key_alloc() that will
> never fail, right?  That's just adding a dead code path.  Which the
> compiler can probably optimize away, but why make the code slightly
> harder to read than necessasry?

Good catch, and thanks for reading these so thoroughly that you caught
the churn artifacts. Do you want me to clean this up and resubmit, or
are you planning on adjusting it in the dev branch?

Re: [PATCH v4 04/13] security/keys: ensure RNG is seeded before use

[Jason A. Donenfeld]

On Thu, Jun 8, 2017 at 2:50 AM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> On Thu, Jun 8, 2017 at 2:31 AM, Theodore Ts'o <tytso@mit.edu> wrote:
>> I'm guessing you changed key_alloc_serial() to return an int back when
>> you were thinking that you might use get_random_bytes_wait(), which
>> could return -ERESTARTSYS.
>>
>> Now that you're not doing this, but using get_random_u32() instead,
>> there's no point to change the function signature of
>> key_alloc_serial() and add an error check in key_alloc() that will
>> never fail, right?  That's just adding a dead code path.  Which the
>> compiler can probably optimize away, but why make the code slightly
>> harder to read than necessasry?
>
> Good catch, and thanks for reading these so thoroughly that you caught
> the churn artifacts. Do you want me to clean this up and resubmit, or
> are you planning on adjusting it in the dev branch?

Fixed it up here if you just want to grab this instead:

https://git.kernel.org/pub/scm/linux/kernel/git/zx2c4/linux.git/patch/?id=a0361e55bce30ace529ed8b28bd452e3ac0ee91f

[PATCH v4 05/13] crypto/rng: ensure that the RNG is ready before using

[Jason A. Donenfeld]

Otherwise, we might be seeding the RNG using bad randomness, which is
dangerous. The one use of this function from within the kernel -- not
from userspace -- is being removed (keys/big_key), so that call site
isn't relevant in assessing this.

Cc: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 crypto/rng.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/crypto/rng.c b/crypto/rng.c
index f46dac5288b9..e042437e64b4 100644
--- a/crypto/rng.c
+++ b/crypto/rng.c
@@ -48,12 +48,14 @@ int crypto_rng_reset(struct crypto_rng *tfm, const u8 *seed, unsigned int slen)
 		if (!buf)
 			return -ENOMEM;
 
-		get_random_bytes(buf, slen);
+		err = get_random_bytes_wait(buf, slen);
+		if (err)
+			goto out;
 		seed = buf;
 	}
 
 	err = crypto_rng_alg(tfm)->seed(tfm, seed, slen);
-
+out:
 	kzfree(buf);
 	return err;
 }
-- 
2.13.0

Re: [kernel-hardening] [PATCH v4 05/13] crypto/rng: ensure that the RNG is ready before using

[Theodore Ts'o]

On Tue, Jun 06, 2017 at 07:47:56PM +0200, Jason A. Donenfeld wrote:
> Otherwise, we might be seeding the RNG using bad randomness, which is
> dangerous. The one use of this function from within the kernel -- not
> from userspace -- is being removed (keys/big_key), so that call site
> isn't relevant in assessing this.

The use in keys/big_key is _being_ removed, so this commit is
dependent on that commit landing, correct?  (Order matters, because
otherwise we don't want to potentially screw up doing a kernel bisect
and causing their kernel to deadlock during the boot while they are
trying to track down an unreleated problem.)

       	  	     		   - Ted

Re: [kernel-hardening] [PATCH v4 05/13] crypto/rng: ensure that the RNG is ready before using

[Jason A. Donenfeld]

On Thu, Jun 8, 2017 at 2:41 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> The use in keys/big_key is _being_ removed, so this commit is
> dependent on that commit landing, correct?  (Order matters, because
> otherwise we don't want to potentially screw up doing a kernel bisect
> and causing their kernel to deadlock during the boot while they are
> trying to track down an unreleated problem.)

Yes. It's actually landing with get_random_bytes, to avoid a
dependency problem when merging. After these both lands, I'll submit a
third changing that over to get_random_bytes_wait in the right place.

[PATCH v4 06/13] iscsi: ensure RNG is seeded before use

[Jason A. Donenfeld]

It's not safe to use weak random data here, especially for the challenge
response randomness. Since we're always in process context, it's safe to
simply wait until we have enough randomness to carry out the
authentication correctly.

While we're at it, we clean up a small memleak during an error
condition.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: "Nicholas A. Bellinger" <nab@linux-iscsi.org>
Cc: Lee Duncan <lduncan@suse.com>
Cc: Chris Leech <cleech@redhat.com>
---
 drivers/target/iscsi/iscsi_target_auth.c  | 14 +++++++++++---
 drivers/target/iscsi/iscsi_target_login.c | 22 ++++++++++++++--------
 2 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_auth.c b/drivers/target/iscsi/iscsi_target_auth.c
index 903b667f8e01..f9bc8ec6fb6b 100644
--- a/drivers/target/iscsi/iscsi_target_auth.c
+++ b/drivers/target/iscsi/iscsi_target_auth.c
@@ -47,18 +47,21 @@ static void chap_binaryhex_to_asciihex(char *dst, char *src, int src_len)
 	}
 }
 
-static void chap_gen_challenge(
+static int chap_gen_challenge(
 	struct iscsi_conn *conn,
 	int caller,
 	char *c_str,
 	unsigned int *c_len)
 {
+	int ret;
 	unsigned char challenge_asciihex[CHAP_CHALLENGE_LENGTH * 2 + 1];
 	struct iscsi_chap *chap = conn->auth_protocol;
 
 	memset(challenge_asciihex, 0, CHAP_CHALLENGE_LENGTH * 2 + 1);
 
-	get_random_bytes(chap->challenge, CHAP_CHALLENGE_LENGTH);
+	ret = get_random_bytes_wait(chap->challenge, CHAP_CHALLENGE_LENGTH);
+	if (unlikely(ret))
+		return ret;
 	chap_binaryhex_to_asciihex(challenge_asciihex, chap->challenge,
 				CHAP_CHALLENGE_LENGTH);
 	/*
@@ -69,6 +72,7 @@ static void chap_gen_challenge(
 
 	pr_debug("[%s] Sending CHAP_C=0x%s\n\n", (caller) ? "server" : "client",
 			challenge_asciihex);
+	return 0;
 }
 
 static int chap_check_algorithm(const char *a_str)
@@ -143,6 +147,7 @@ static struct iscsi_chap *chap_server_open(
 	case CHAP_DIGEST_UNKNOWN:
 	default:
 		pr_err("Unsupported CHAP_A value\n");
+		kfree(conn->auth_protocol);
 		return NULL;
 	}
 
@@ -156,7 +161,10 @@ static struct iscsi_chap *chap_server_open(
 	/*
 	 * Generate Challenge.
 	 */
-	chap_gen_challenge(conn, 1, aic_str, aic_len);
+	if (chap_gen_challenge(conn, 1, aic_str, aic_len) < 0) {
+		kfree(conn->auth_protocol);
+		return NULL;
+	}
 
 	return chap;
 }
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index 66238477137b..5ef028c11738 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -245,22 +245,26 @@ int iscsi_check_for_session_reinstatement(struct iscsi_conn *conn)
 	return 0;
 }
 
-static void iscsi_login_set_conn_values(
+static int iscsi_login_set_conn_values(
 	struct iscsi_session *sess,
 	struct iscsi_conn *conn,
 	__be16 cid)
 {
+	int ret;
 	conn->sess		= sess;
 	conn->cid		= be16_to_cpu(cid);
 	/*
 	 * Generate a random Status sequence number (statsn) for the new
 	 * iSCSI connection.
 	 */
-	get_random_bytes(&conn->stat_sn, sizeof(u32));
+	ret = get_random_bytes_wait(&conn->stat_sn, sizeof(u32));
+	if (unlikely(ret))
+		return ret;
 
 	mutex_lock(&auth_id_lock);
 	conn->auth_id		= iscsit_global->auth_id++;
 	mutex_unlock(&auth_id_lock);
+	return 0;
 }
 
 __printf(2, 3) int iscsi_change_param_sprintf(
@@ -306,7 +310,11 @@ static int iscsi_login_zero_tsih_s1(
 		return -ENOMEM;
 	}
 
-	iscsi_login_set_conn_values(sess, conn, pdu->cid);
+	ret = iscsi_login_set_conn_values(sess, conn, pdu->cid);
+	if (unlikely(ret)) {
+		kfree(sess);
+		return ret;
+	}
 	sess->init_task_tag	= pdu->itt;
 	memcpy(&sess->isid, pdu->isid, 6);
 	sess->exp_cmd_sn	= be32_to_cpu(pdu->cmdsn);
@@ -497,8 +505,7 @@ static int iscsi_login_non_zero_tsih_s1(
 {
 	struct iscsi_login_req *pdu = (struct iscsi_login_req *)buf;
 
-	iscsi_login_set_conn_values(NULL, conn, pdu->cid);
-	return 0;
+	return iscsi_login_set_conn_values(NULL, conn, pdu->cid);
 }
 
 /*
@@ -554,9 +561,8 @@ static int iscsi_login_non_zero_tsih_s2(
 		atomic_set(&sess->session_continuation, 1);
 	spin_unlock_bh(&sess->conn_lock);
 
-	iscsi_login_set_conn_values(sess, conn, pdu->cid);
-
-	if (iscsi_copy_param_list(&conn->param_list,
+	if (iscsi_login_set_conn_values(sess, conn, pdu->cid) < 0 ||
+	    iscsi_copy_param_list(&conn->param_list,
 			conn->tpg->param_list, 0) < 0) {
 		iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
 				ISCSI_LOGIN_STATUS_NO_RESOURCES);
-- 
2.13.0

Re: [PATCH v4 06/13] iscsi: ensure RNG is seeded before use

[Theodore Ts'o]

On Tue, Jun 06, 2017 at 07:47:57PM +0200, Jason A. Donenfeld wrote:
> It's not safe to use weak random data here, especially for the challenge
> response randomness. Since we're always in process context, it's safe to
> simply wait until we have enough randomness to carry out the
> authentication correctly.
> 
> While we're at it, we clean up a small memleak during an error
> condition.

What was the testing that was done for commit?  It looks safe, but I'm
unfamiliar enough with how the iSCSI authentication works that I'd
prefer getting an ack'ed by from the iSCSI maintainers or
alternativel, information about how to kick off some kind of automated
test suite ala xfstests for file systems.

Thanks,

					- Ted

Re: [kernel-hardening] Re: [PATCH v4 06/13] iscsi: ensure RNG is seeded before use

[Jason A. Donenfeld]

[-- Attachment #1: Type: text/plain, Size: 515 bytes --]

On Thu, Jun 8, 2017 at 4:43 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> What was the testing that was done for commit?  It looks safe, but I'm
> unfamiliar enough with how the iSCSI authentication works that I'd
> prefer getting an ack'ed by from the iSCSI maintainers or
> alternativel, information about how to kick off some kind of automated
> test suite ala xfstests for file systems.

Only very basic testing from my end.

I'm thus adding the iSCSI list to see if they'll have a look (patch reattached).

Jason

[-- Attachment #2: 0001-iscsi-ensure-RNG-is-seeded-before-use.patch --]
[-- Type: text/x-patch, Size: 4642 bytes --]

From 1adecf785526a2a96104767807140b9e1a9e2a27 Mon Sep 17 00:00:00 2001
From: "Jason A. Donenfeld" <Jason@zx2c4.com>
Date: Mon, 5 Jun 2017 05:09:54 +0200
Subject: [PATCH] iscsi: ensure RNG is seeded before use

It's not safe to use weak random data here, especially for the challenge
response randomness. Since we're always in process context, it's safe to
simply wait until we have enough randomness to carry out the
authentication correctly.

While we're at it, we clean up a small memleak during an error
condition.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: "Nicholas A. Bellinger" <nab@linux-iscsi.org>
Cc: Lee Duncan <lduncan@suse.com>
Cc: Chris Leech <cleech@redhat.com>
---
 drivers/target/iscsi/iscsi_target_auth.c  | 14 +++++++++++---
 drivers/target/iscsi/iscsi_target_login.c | 22 ++++++++++++++--------
 2 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_auth.c b/drivers/target/iscsi/iscsi_target_auth.c
index 903b667f8e01..f9bc8ec6fb6b 100644
--- a/drivers/target/iscsi/iscsi_target_auth.c
+++ b/drivers/target/iscsi/iscsi_target_auth.c
@@ -47,18 +47,21 @@ static void chap_binaryhex_to_asciihex(char *dst, char *src, int src_len)
 	}
 }
 
-static void chap_gen_challenge(
+static int chap_gen_challenge(
 	struct iscsi_conn *conn,
 	int caller,
 	char *c_str,
 	unsigned int *c_len)
 {
+	int ret;
 	unsigned char challenge_asciihex[CHAP_CHALLENGE_LENGTH * 2 + 1];
 	struct iscsi_chap *chap = conn->auth_protocol;
 
 	memset(challenge_asciihex, 0, CHAP_CHALLENGE_LENGTH * 2 + 1);
 
-	get_random_bytes(chap->challenge, CHAP_CHALLENGE_LENGTH);
+	ret = get_random_bytes_wait(chap->challenge, CHAP_CHALLENGE_LENGTH);
+	if (unlikely(ret))
+		return ret;
 	chap_binaryhex_to_asciihex(challenge_asciihex, chap->challenge,
 				CHAP_CHALLENGE_LENGTH);
 	/*
@@ -69,6 +72,7 @@ static void chap_gen_challenge(
 
 	pr_debug("[%s] Sending CHAP_C=0x%s\n\n", (caller) ? "server" : "client",
 			challenge_asciihex);
+	return 0;
 }
 
 static int chap_check_algorithm(const char *a_str)
@@ -143,6 +147,7 @@ static struct iscsi_chap *chap_server_open(
 	case CHAP_DIGEST_UNKNOWN:
 	default:
 		pr_err("Unsupported CHAP_A value\n");
+		kfree(conn->auth_protocol);
 		return NULL;
 	}
 
@@ -156,7 +161,10 @@ static struct iscsi_chap *chap_server_open(
 	/*
 	 * Generate Challenge.
 	 */
-	chap_gen_challenge(conn, 1, aic_str, aic_len);
+	if (chap_gen_challenge(conn, 1, aic_str, aic_len) < 0) {
+		kfree(conn->auth_protocol);
+		return NULL;
+	}
 
 	return chap;
 }
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index 92b96b51d506..e9bdc8b86e7d 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -245,22 +245,26 @@ int iscsi_check_for_session_reinstatement(struct iscsi_conn *conn)
 	return 0;
 }
 
-static void iscsi_login_set_conn_values(
+static int iscsi_login_set_conn_values(
 	struct iscsi_session *sess,
 	struct iscsi_conn *conn,
 	__be16 cid)
 {
+	int ret;
 	conn->sess		= sess;
 	conn->cid		= be16_to_cpu(cid);
 	/*
 	 * Generate a random Status sequence number (statsn) for the new
 	 * iSCSI connection.
 	 */
-	get_random_bytes(&conn->stat_sn, sizeof(u32));
+	ret = get_random_bytes_wait(&conn->stat_sn, sizeof(u32));
+	if (unlikely(ret))
+		return ret;
 
 	mutex_lock(&auth_id_lock);
 	conn->auth_id		= iscsit_global->auth_id++;
 	mutex_unlock(&auth_id_lock);
+	return 0;
 }
 
 __printf(2, 3) int iscsi_change_param_sprintf(
@@ -306,7 +310,11 @@ static int iscsi_login_zero_tsih_s1(
 		return -ENOMEM;
 	}
 
-	iscsi_login_set_conn_values(sess, conn, pdu->cid);
+	ret = iscsi_login_set_conn_values(sess, conn, pdu->cid);
+	if (unlikely(ret)) {
+		kfree(sess);
+		return ret;
+	}
 	sess->init_task_tag	= pdu->itt;
 	memcpy(&sess->isid, pdu->isid, 6);
 	sess->exp_cmd_sn	= be32_to_cpu(pdu->cmdsn);
@@ -497,8 +505,7 @@ static int iscsi_login_non_zero_tsih_s1(
 {
 	struct iscsi_login_req *pdu = (struct iscsi_login_req *)buf;
 
-	iscsi_login_set_conn_values(NULL, conn, pdu->cid);
-	return 0;
+	return iscsi_login_set_conn_values(NULL, conn, pdu->cid);
 }
 
 /*
@@ -554,9 +561,8 @@ static int iscsi_login_non_zero_tsih_s2(
 		atomic_set(&sess->session_continuation, 1);
 	spin_unlock_bh(&sess->conn_lock);
 
-	iscsi_login_set_conn_values(sess, conn, pdu->cid);
-
-	if (iscsi_copy_param_list(&conn->param_list,
+	if (iscsi_login_set_conn_values(sess, conn, pdu->cid) < 0 ||
+	    iscsi_copy_param_list(&conn->param_list,
 			conn->tpg->param_list, 0) < 0) {
 		iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
 				ISCSI_LOGIN_STATUS_NO_RESOURCES);
-- 
2.13.0

Re: [kernel-hardening] Re: [PATCH v4 06/13] iscsi: ensure RNG is seeded before use

[Lee Duncan]

On 06/08/2017 05:09 AM, Jason A. Donenfeld wrote:
> On Thu, Jun 8, 2017 at 4:43 AM, Theodore Ts'o <tytso@mit.edu> wrote:
>> What was the testing that was done for commit?  It looks safe, but I'm
>> unfamiliar enough with how the iSCSI authentication works that I'd
>> prefer getting an ack'ed by from the iSCSI maintainers or
>> alternativel, information about how to kick off some kind of automated
>> test suite ala xfstests for file systems.
> 
> Only very basic testing from my end.
> 
> I'm thus adding the iSCSI list to see if they'll have a look (patch reattached).
> 
> Jason
> 

It seems like what you are doing is basically "good", i.e. if there is
not enough random data, don't use it. But what happens in that case? The
authentication fails? How does the user know to wait and try again?
-- 
Lee Duncan
SUSE Labs

Re: [kernel-hardening] Re: [PATCH v4 06/13] iscsi: ensure RNG is seeded before use

[Jason A. Donenfeld]

Hi Lee,

On Fri, Jun 16, 2017 at 11:58 PM, Lee Duncan <lduncan@suse.com> wrote:
> It seems like what you are doing is basically "good", i.e. if there is
> not enough random data, don't use it. But what happens in that case? The
> authentication fails? How does the user know to wait and try again?

The process just remains in interruptible (kill-able) sleep until
there is enough entropy, so the process doesn't need to do anything.
If the waiting is interrupted by a signal, it returns -ESYSRESTART,
which follows the usual semantics of restartable syscalls.

Jason

Re: [kernel-hardening] Re: [PATCH v4 06/13] iscsi: ensure RNG is seeded before use

[Lee Duncan]

On 06/16/2017 05:41 PM, Jason A. Donenfeld wrote:
> Hi Lee,
> 
> On Fri, Jun 16, 2017 at 11:58 PM, Lee Duncan <lduncan@suse.com> wrote:
>> It seems like what you are doing is basically "good", i.e. if there is
>> not enough random data, don't use it. But what happens in that case? The
>> authentication fails? How does the user know to wait and try again?
> 
> The process just remains in interruptible (kill-able) sleep until
> there is enough entropy, so the process doesn't need to do anything.
> If the waiting is interrupted by a signal, it returns -ESYSRESTART,
> which follows the usual semantics of restartable syscalls.
> 
> Jason
> 

In your testing, how long might a process have to wait? Are we talking
seconds? Longer? What about timeouts?

Sorry, but your changing something that isn't exactly broken, so I just
want to be sure we're not introducing some regression, like clients
can't connect the first 5 minutes are a reboot.
-- 
Lee Duncan

Re: [kernel-hardening] Re: [PATCH v4 06/13] iscsi: ensure RNG is seeded before use

[Jeffrey Walton]

On Fri, Jun 16, 2017 at 11:45 PM, Lee Duncan <lduncan@suse.com> wrote:
> On 06/16/2017 05:41 PM, Jason A. Donenfeld wrote:
>> Hi Lee,
>>
>> On Fri, Jun 16, 2017 at 11:58 PM, Lee Duncan <lduncan@suse.com> wrote:
>>> It seems like what you are doing is basically "good", i.e. if there is
>>> not enough random data, don't use it. But what happens in that case? The
>>> authentication fails? How does the user know to wait and try again?
>>
>> The process just remains in interruptible (kill-able) sleep until
>> there is enough entropy, so the process doesn't need to do anything.
>> If the waiting is interrupted by a signal, it returns -ESYSRESTART,
>> which follows the usual semantics of restartable syscalls.
>>
> In your testing, how long might a process have to wait? Are we talking
> seconds? Longer? What about timeouts?
>
> Sorry, but your changing something that isn't exactly broken, so I just
> want to be sure we're not introducing some regression, like clients
> can't connect the first 5 minutes are a reboot.

CHAP (https://www.rfc-editor.org/rfc/rfc1994.txt) and iSCSI
(https://www.ietf.org/rfc/rfc3720.txt) require random values. If iSCSI
is operating without them, it seems like something is broken. From RFC
3720, Section 8.2.1, CHAP Considerations:

   When CHAP is performed over a non-encrypted channel, it is vulnerable
   to an off-line dictionary attack.  Implementations MUST support use
   of up to 128 bit random CHAP secrets, including the means to generate
   such secrets and to accept them from an external generation source.
   Implementations MUST NOT provide secret generation (or expansion)
   means other than random generation.

CHAP actually has a weaker requirement since it only requires _unique_
(and not _random_). From RFC 1994, Section 2.3, Design Requirements:

   Each challenge value SHOULD be unique, since repetition of a
   challenge value in conjunction with the same secret would permit an
   attacker to reply with a previously intercepted response.  Since it
   is expected that the same secret MAY be used to authenticate with
   servers in disparate geographic regions, the challenge SHOULD exhibit
   global and temporal uniqueness.

But its not clear to me how to ensure uniqueness when its based on
randomness from the generators.

Jeff

Re: [kernel-hardening] [PATCH v4 06/13] iscsi: ensure RNG is seeded before use

[Paul Koning]

> On Jun 17, 2017, at 10:23 AM, Jeffrey Walton <noloader@gmail.com> wrote:
> 
> On Fri, Jun 16, 2017 at 11:45 PM, Lee Duncan <lduncan@suse.com> wrote:
>> On 06/16/2017 05:41 PM, Jason A. Donenfeld wrote:
>>> Hi Lee,
>>> 
>>> On Fri, Jun 16, 2017 at 11:58 PM, Lee Duncan <lduncan@suse.com> wrote:
>>>> It seems like what you are doing is basically "good", i.e. if there is
>>>> not enough random data, don't use it. But what happens in that case? The
>>>> authentication fails? How does the user know to wait and try again?
>>> 
>>> The process just remains in interruptible (kill-able) sleep until
>>> there is enough entropy, so the process doesn't need to do anything.
>>> If the waiting is interrupted by a signal, it returns -ESYSRESTART,
>>> which follows the usual semantics of restartable syscalls.
>>> 
>> In your testing, how long might a process have to wait? Are we talking
>> seconds? Longer? What about timeouts?
>> 
>> Sorry, but your changing something that isn't exactly broken, so I just
>> want to be sure we're not introducing some regression, like clients
>> can't connect the first 5 minutes are a reboot.
> 
> CHAP (https://www.rfc-editor.org/rfc/rfc1994.txt) and iSCSI
> (https://www.ietf.org/rfc/rfc3720.txt) require random values. If iSCSI
> is operating without them, it seems like something is broken. From RFC
> 3720, Section 8.2.1, CHAP Considerations:
> 
>   When CHAP is performed over a non-encrypted channel, it is vulnerable
>   to an off-line dictionary attack.  Implementations MUST support use
>   of up to 128 bit random CHAP secrets, including the means to generate
>   such secrets and to accept them from an external generation source.
>   Implementations MUST NOT provide secret generation (or expansion)
>   means other than random generation.

That only applies to the generation of the secret, which is configured into iscsi, not created by it.  A utility to generate the secret might be supplied, of course, just as one might have utilities to generate strong passwords, but it's not a component of the iSCSI protocol.

> CHAP actually has a weaker requirement since it only requires _unique_
> (and not _random_). From RFC 1994, Section 2.3, Design Requirements:
> 
>   Each challenge value SHOULD be unique, since repetition of a
>   challenge value in conjunction with the same secret would permit an
>   attacker to reply with a previously intercepted response.  Since it
>   is expected that the same secret MAY be used to authenticate with
>   servers in disparate geographic regions, the challenge SHOULD exhibit
>   global and temporal uniqueness.
> 
> But its not clear to me how to ensure uniqueness when its based on
> randomness from the generators.

A strong RNG of length n will produce numbers likely to be unique until you approach the birtday limit 2^(n/2).  So, say, a 128 bit challenge will be adequate.

	paul

Antw: Re: [kernel-hardening] Re: [PATCH v4 06/13] iscsi: ensure RNG is seeded before use

[Ulrich Windl]

>>> Jeffrey Walton <noloader@gmail.com> schrieb am 17.06.2017 um 16:23 in Nachricht
<CAH8yC8nHX2r9cfQ0gNeJAUrgSfAS8V16dVHv35BRnLn-YprZCg@mail.gmail.com>:

[...]
> But its not clear to me how to ensure uniqueness when its based on
> randomness from the generators.

Even with a perfect random generator non-unique values are possible (that's why it's random). It's unlikely, but it can happen. The question is whether the probability of non-unique values from /dev/urandom is any higher than that for values read from /dev/random. One _might_ be able to predict the values from /dev/urandom.

Regards,
Ulrich

> 
> Jeff
> 
> -- 
> You received this message because you are subscribed to the Google Groups 
> "open-iscsi" group.
> To unsubscribe from this group and stop receiving emails from it, send an 
> email to open-iscsi+unsubscribe@googlegroups.com.
> To post to this group, send email to open-iscsi@googlegroups.com.
> Visit this group at https://groups.google.com/group/open-iscsi.
> For more options, visit https://groups.google.com/d/optout.

Re: Antw: Re: [kernel-hardening] Re: [PATCH v4 06/13] iscsi: ensure RNG is seeded before use

[Paul Koning]

> On Jul 5, 2017, at 3:08 AM, Ulrich Windl <Ulrich.Windl@rz.uni-regensburg.de> wrote:
> 
>>>> Jeffrey Walton <noloader@gmail.com> schrieb am 17.06.2017 um 16:23 in Nachricht
> <CAH8yC8nHX2r9cfQ0gNeJAUrgSfAS8V16dVHv35BRnLn-YprZCg@mail.gmail.com>:
> 
> [...]
>> But its not clear to me how to ensure uniqueness when its based on
>> randomness from the generators.
> 
> Even with a perfect random generator non-unique values are possible (that's why it's random). It's unlikely, but it can happen. The question is whether the probability of non-unique values from /dev/urandom is any higher than that for values read from /dev/random. One _might_ be able to predict the values from /dev/urandom.

In the implementations I know, /dev/random and /dev/urandom are the same driver, the only difference is that when you read from /dev/random there's a check for the current entropy level.

If you haven't fed enough entropy yet to the driver since startup, and you read /dev/urandom, you get a value that isn't sufficiently secure.  

If you have a properly constructed RNG, as soon as it's been fed enough entropy it is secure (at least for the next 2^64 bits or so).  The notion of "using up entropy" is not meaningful for a good generator.   See Bruce Schneier's "Yarrow" paper for the details.

	paul

Re: Antw: Re: [kernel-hardening] Re: [PATCH v4 06/13] iscsi: ensure RNG is seeded before use

[Theodore Ts'o]

On Wed, Jul 05, 2017 at 09:16:09AM -0400, Paul Koning wrote:
> 
> In the implementations I know, /dev/random and /dev/urandom are the
> same driver, the only difference is that when you read from
> /dev/random there's a check for the current entropy level.

It's in the same driver but /dev/random and /dev/urandom are different
beasts.  Pre-4.9 we use the same SHA-1 based generator, but we use
different state pools, and we periodically "catastrophically reseed"
(ala Yarrow) from the random pool to the urandom pool.  Getrandom(2)
uses the urandom pool.

In 4.9 and later kernels, /dev/urandom and getrandom(2) use a ChaCha20
based generator which provides for more speed.  We still use the SHA-1
pool for the random pool because it allows for a much larger "state
pool" to store entropy.

> If you have a properly constructed RNG, as soon as it's been fed
> enough entropy it is secure (at least for the next 2^64 bits or so).
> The notion of "using up entropy" is not meaningful for a good
> generator.  See Bruce Schneier's "Yarrow" paper for the details.

A lot of this depends on whether or not you trust your crypto
primitives or not.  The /dev/random driver was the very first OS-based
random generator, and back then, export restrictions were still a
thing (which is why we only used MD5 and SHA-1 as crypto primitives),
and our cryptoanalytic understanding of what makes for a good crypto
hash or encryption algorithm was quite limited.  So trying to
accumulate a large amount of entropy pool of entropy is a good thing,
because even if there was a minor weakness in the crypto hash (and for
a while we were using MD5), relying on the adversary not being able to
guess all of the environmental noise harvested by the kernel would
cover for a lot of sins.

Remember, in the kernel we have access to a large amount of
environmental noise, so it makes a lot of sense to take advantage of
that as much as possible.  So by having a huge state pool for the
/dev/random entropy pool, we can harvest and store as much of that
environmental noise as possible.  This buys us a large amount of
safety margin, which is good thing because somewhere there might be
some Linux 2.0 or Linux 2.2 based router sitting in someone's home
where /dev/random is using MD5.  Those ancient kernels are probably
riddled with zero-day security holes, but the safety margin of using a
large entropy pool is such that even though there are many known
attacks against MD5, I'm actually pretty confident that the large
state pool mitigates the weakness of MD5 as used by /dev/random and
/dev/urandom.  At the very least, it will be much easier for the NSA
to use some other zero-day to attack the said router with the ancient
kernel, well before they try to reverse engineer its /dev/urandom
output.  :-)

						- Ted

Re: [kernel-hardening] Re: [PATCH v4 06/13] iscsi: ensure RNG is seeded before use

[Stephan Müller]

Am Samstag, 17. Juni 2017, 05:45:57 CEST schrieb Lee Duncan:

Hi Lee,

> In your testing, how long might a process have to wait? Are we talking
> seconds? Longer? What about timeouts?
>

In current kernels (starting with 4.8) this timeout should clear within a few 
seconds after boot.

In older kernels (pre 4.8), my KVM takes up to 90 seconds to reach that 
seeding point. I have heard that on IBM System Z this trigger point requires 
minutes to be reached.

Ciao
Stephan

Re: [kernel-hardening] Re: [PATCH v4 06/13] iscsi: ensure RNG is seeded before use

[Nicholas A. Bellinger]

Hi Stephan, Lee & Jason,

(Adding target-devel CC')

Apologies for coming late to the discussion.  Comments below.

On Sun, 2017-06-18 at 10:04 +0200, Stephan Müller wrote:
> Am Samstag, 17. Juni 2017, 05:45:57 CEST schrieb Lee Duncan:
> 
> Hi Lee,
> 
> > In your testing, how long might a process have to wait? Are we talking
> > seconds? Longer? What about timeouts?
> >
> 
> In current kernels (starting with 4.8) this timeout should clear within a few 
> seconds after boot.
> 
> In older kernels (pre 4.8), my KVM takes up to 90 seconds to reach that 
> seeding point. I have heard that on IBM System Z this trigger point requires 
> minutes to be reached.
> 

I share the same concern as Lee wrt to introducing latency into the
existing iscsi-target login sequence.

Namely in the use-cases where a single node is supporting ~1K unique
iscsi-target IQNs, and fail-over + re-balancing of IQNs where 100s of
login attempts are expected to occur in parallel.

If environments exist that require non trivial amounts of time for RNG
seeding to be ready for iscsi-target usage, then enforcing this
requirement at iscsi login time can open up problems, especially when
iscsi host environments may be sensitive to login timeouts, I/O
timeouts, et al.

That said, I'd prefer to simply wait for RNG to be seeded at modprobe
iscsi_target_mod time, instead of trying to enforce randomness during
login.

This way, those of use who have distributed storage platforms can know
to avoid putting a node into a state to accept iscsi-target IQN export
migration, before modprobe iscsi_target_mod has successfully loaded and
RNG seeding has been confirmed.

WDYT..?

Re: [kernel-hardening] Re: [PATCH v4 06/13] iscsi: ensure RNG is seeded before use

[Stephan Müller]

Am Montag, 26. Juni 2017, 03:23:09 CEST schrieb Nicholas A. Bellinger:

Hi Nicholas,

> Hi Stephan, Lee & Jason,
> 
> (Adding target-devel CC')
> 
> Apologies for coming late to the discussion.  Comments below.
> 
> On Sun, 2017-06-18 at 10:04 +0200, Stephan Müller wrote:
> > Am Samstag, 17. Juni 2017, 05:45:57 CEST schrieb Lee Duncan:
> > 
> > Hi Lee,
> > 
> > > In your testing, how long might a process have to wait? Are we talking
> > > seconds? Longer? What about timeouts?
> > 
> > In current kernels (starting with 4.8) this timeout should clear within a
> > few seconds after boot.
> > 
> > In older kernels (pre 4.8), my KVM takes up to 90 seconds to reach that
> > seeding point. I have heard that on IBM System Z this trigger point
> > requires minutes to be reached.
> 
> I share the same concern as Lee wrt to introducing latency into the
> existing iscsi-target login sequence.
> 
> Namely in the use-cases where a single node is supporting ~1K unique
> iscsi-target IQNs, and fail-over + re-balancing of IQNs where 100s of
> login attempts are expected to occur in parallel.
> 
> If environments exist that require non trivial amounts of time for RNG
> seeding to be ready for iscsi-target usage, then enforcing this
> requirement at iscsi login time can open up problems, especially when
> iscsi host environments may be sensitive to login timeouts, I/O
> timeouts, et al.
> 
> That said, I'd prefer to simply wait for RNG to be seeded at modprobe
> iscsi_target_mod time, instead of trying to enforce randomness during
> login.
> 
> This way, those of use who have distributed storage platforms can know
> to avoid putting a node into a state to accept iscsi-target IQN export
> migration, before modprobe iscsi_target_mod has successfully loaded and
> RNG seeding has been confirmed.
> 
> WDYT..?

We may have a chicken and egg problem when the wait is at the modprobe time. 
Assume the modprobe happens during initramfs time to get access to the root 
file system. In this case, you entire boot process will lock up for an 
indefinite amount of time. The reason is that in order to obtain events 
detected by the RNG, devices need to be initialized and working. Such devices 
commonly start working after the the root partition is mounted as it contains 
all drivers, all configuration information etc.

Note, during the development of my /dev/random implementation, I added the 
getrandom-like blocking behavior to /dev/urandom (which is the equivalent to 
Jason's patch except that it applies to user space). The boot process locked 
up since systemd wanted data from /dev/urandom while it processed the 
initramfs. As it did not get any, the boot process did not commence that could 
deliver new events to be picked up by the RNG.

As I do not have such an iscsi system, I cannot test Jason's patch. But maybe 
the mentioned chicken-and-egg problem I mentioned above is already visible 
with the current patch as it will lead to a blocking of the mounting of the 
root partition in case the root partition is on an iscsi target.

Ciao
Stephan

Re: [kernel-hardening] Re: [PATCH v4 06/13] iscsi: ensure RNG is seeded before use

[Nicholas A. Bellinger]

On Mon, 2017-06-26 at 19:38 +0200, Stephan Müller wrote:
> Am Montag, 26. Juni 2017, 03:23:09 CEST schrieb Nicholas A. Bellinger:
> 
> Hi Nicholas,
> 
> > Hi Stephan, Lee & Jason,
> > 
> > (Adding target-devel CC')
> > 
> > Apologies for coming late to the discussion.  Comments below.
> > 
> > On Sun, 2017-06-18 at 10:04 +0200, Stephan Müller wrote:
> > > Am Samstag, 17. Juni 2017, 05:45:57 CEST schrieb Lee Duncan:
> > > 
> > > Hi Lee,
> > > 
> > > > In your testing, how long might a process have to wait? Are we talking
> > > > seconds? Longer? What about timeouts?
> > > 
> > > In current kernels (starting with 4.8) this timeout should clear within a
> > > few seconds after boot.
> > > 
> > > In older kernels (pre 4.8), my KVM takes up to 90 seconds to reach that
> > > seeding point. I have heard that on IBM System Z this trigger point
> > > requires minutes to be reached.
> > 
> > I share the same concern as Lee wrt to introducing latency into the
> > existing iscsi-target login sequence.
> > 
> > Namely in the use-cases where a single node is supporting ~1K unique
> > iscsi-target IQNs, and fail-over + re-balancing of IQNs where 100s of
> > login attempts are expected to occur in parallel.
> > 
> > If environments exist that require non trivial amounts of time for RNG
> > seeding to be ready for iscsi-target usage, then enforcing this
> > requirement at iscsi login time can open up problems, especially when
> > iscsi host environments may be sensitive to login timeouts, I/O
> > timeouts, et al.
> > 
> > That said, I'd prefer to simply wait for RNG to be seeded at modprobe
> > iscsi_target_mod time, instead of trying to enforce randomness during
> > login.
> > 
> > This way, those of use who have distributed storage platforms can know
> > to avoid putting a node into a state to accept iscsi-target IQN export
> > migration, before modprobe iscsi_target_mod has successfully loaded and
> > RNG seeding has been confirmed.
> > 
> > WDYT..?
> 
> We may have a chicken and egg problem when the wait is at the modprobe time. 
> Assume the modprobe happens during initramfs time to get access to the root 
> file system. In this case, you entire boot process will lock up for an 
> indefinite amount of time. The reason is that in order to obtain events 
> detected by the RNG, devices need to be initialized and working. Such devices 
> commonly start working after the the root partition is mounted as it contains 
> all drivers, all configuration information etc.
> 
> Note, during the development of my /dev/random implementation, I added the 
> getrandom-like blocking behavior to /dev/urandom (which is the equivalent to 
> Jason's patch except that it applies to user space). The boot process locked 
> up since systemd wanted data from /dev/urandom while it processed the 
> initramfs. As it did not get any, the boot process did not commence that could 
> deliver new events to be picked up by the RNG.
> 
> As I do not have such an iscsi system, I cannot test Jason's patch. But maybe 
> the mentioned chicken-and-egg problem I mentioned above is already visible 
> with the current patch as it will lead to a blocking of the mounting of the 
> root partition in case the root partition is on an iscsi target.

AFAIK, there are no distro initramfs dependencies for iscsi_target_mod,
and every environment I've ever seen loads iscsi_target_mod after
switching to the real rootfs.

For an iscsi initiator that might not been the case, especially if the
rootfs is running atop a iscsi LUN.

But at least for iscsi-target mode, any blocking during modprobe waiting
for RNG seeding would happen outside of initramfs.

Antw: Re: [kernel-hardening] Re: [PATCH v4 06/13] iscsi: ensure RNG is seeded before use

[Ulrich Windl]

>>> Stephan Müller <smueller@chronox.de> schrieb am 26.06.2017 um 19:38 in
Nachricht <1678474.GnYBdSlWgs@tauon.chronox.de>:
> Am Montag, 26. Juni 2017, 03:23:09 CEST schrieb Nicholas A. Bellinger:
> 
> Hi Nicholas,
> 
>> Hi Stephan, Lee & Jason,
>> 
>> (Adding target-devel CC')
>> 
>> Apologies for coming late to the discussion.  Comments below.
>> 
>> On Sun, 2017-06-18 at 10:04 +0200, Stephan Müller wrote:
>> > Am Samstag, 17. Juni 2017, 05:45:57 CEST schrieb Lee Duncan:
>> > 
>> > Hi Lee,
>> > 
>> > > In your testing, how long might a process have to wait? Are we talking
>> > > seconds? Longer? What about timeouts?
>> > 
>> > In current kernels (starting with 4.8) this timeout should clear within
a
>> > few seconds after boot.
>> > 
>> > In older kernels (pre 4.8), my KVM takes up to 90 seconds to reach that
>> > seeding point. I have heard that on IBM System Z this trigger point
>> > requires minutes to be reached.
>> 
>> I share the same concern as Lee wrt to introducing latency into the
>> existing iscsi-target login sequence.
>> 
>> Namely in the use-cases where a single node is supporting ~1K unique
>> iscsi-target IQNs, and fail-over + re-balancing of IQNs where 100s of
>> login attempts are expected to occur in parallel.
>> 
>> If environments exist that require non trivial amounts of time for RNG
>> seeding to be ready for iscsi-target usage, then enforcing this
>> requirement at iscsi login time can open up problems, especially when
>> iscsi host environments may be sensitive to login timeouts, I/O
>> timeouts, et al.
>> 
>> That said, I'd prefer to simply wait for RNG to be seeded at modprobe
>> iscsi_target_mod time, instead of trying to enforce randomness during
>> login.
>> 
>> This way, those of use who have distributed storage platforms can know
>> to avoid putting a node into a state to accept iscsi-target IQN export
>> migration, before modprobe iscsi_target_mod has successfully loaded and
>> RNG seeding has been confirmed.
>> 
>> WDYT..?
> 
> We may have a chicken and egg problem when the wait is at the modprobe time.

> 
> Assume the modprobe happens during initramfs time to get access to the root

> file system. In this case, you entire boot process will lock up for an 
> indefinite amount of time. The reason is that in order to obtain events 
> detected by the RNG, devices need to be initialized and working. Such 
> devices 
> commonly start working after the the root partition is mounted as it 
> contains 
> all drivers, all configuration information etc.
> 
> Note, during the development of my /dev/random implementation, I added the 
> getrandom-like blocking behavior to /dev/urandom (which is the equivalent to

> 
> Jason's patch except that it applies to user space). The boot process locked


I thought reads from urandom never block by definition. An older manual page
(man urandom) also says: "A  read  from  the  /dev/urandom device will not
block waiting for more entropy."

Regards,
Ulrich

> 
> up since systemd wanted data from /dev/urandom while it processed the 
> initramfs. As it did not get any, the boot process did not commence that 
> could 
> deliver new events to be picked up by the RNG.
> 
> As I do not have such an iscsi system, I cannot test Jason's patch. But 
> maybe 
> the mentioned chicken-and-egg problem I mentioned above is already visible 
> with the current patch as it will lead to a blocking of the mounting of the

> root partition in case the root partition is on an iscsi target.
> 
> Ciao
> Stephan
> 
> -- 
> You received this message because you are subscribed to the Google Groups 
> "open-iscsi" group.
> To unsubscribe from this group and stop receiving emails from it, send an 
> email to open-iscsi+unsubscribe@googlegroups.com.
> To post to this group, send email to open-iscsi@googlegroups.com.
> Visit this group at https://groups.google.com/group/open-iscsi.
> For more options, visit https://groups.google.com/d/optout.

Re: Antw: Re: [kernel-hardening] Re: [PATCH v4 06/13] iscsi: ensure RNG is seeded before use

[Theodore Ts'o]

On Wed, Jul 05, 2017 at 09:03:43AM +0200, Ulrich Windl wrote:
> > Note, during the development of my /dev/random implementation, I added the 
> > getrandom-like blocking behavior to /dev/urandom (which is the equivalent to
> > Jason's patch except that it applies to user space). The boot process locked
> 
> I thought reads from urandom never block by definition. An older manual page
> (man urandom) also says: "A  read  from  the  /dev/urandom device will not
> block waiting for more entropy."

As I said in my original message, I *tried* this as an experiment.
Because lots of security-obsessed people were disputing my
intelligence, my judgement, and in some cases, my paternity becuase I
wouldn't change /dev/urandom not to block.

So I did the experiment so I could give them hard data about why we
couldn't go down that path.

> > up since systemd wanted data from /dev/urandom while it processed the 
> > initramfs. As it did not get any, the boot process did not commence that 
> > could 
> > deliver new events to be picked up by the RNG.

And indeed, making this change brick'ed at least one version of Ubuntu
and one version of CeroWRT, as reported by the kernel's 0-day testing
system.  As a result, this patch (which was always a proof of concept,
not anything I thought had any chance of going upstream), was dropped.

Since in the kernel, We Do Not Break Backwards Compatibility, this is
why we have a new interface --- getrandom(2) --- instead of changing
an existing interface.  (Well, there were multiple good reasons for
getrandom, but this was definitely one of them.)

	    	       	   	    - Ted

[PATCH v4 07/13] ceph: ensure RNG is seeded before using

[Jason A. Donenfeld]

Ceph uses the RNG for various nonce generations, and it shouldn't accept
using bad randomness. So, we wait for the RNG to be properly seeded. We
do this by calling wait_for_random_bytes() in a function that is
certainly called in process context, early on, so that all subsequent
calls to get_random_bytes are necessarily acceptable.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Ilya Dryomov <idryomov@gmail.com>
Cc: "Yan, Zheng" <zyan@redhat.com>
Cc: Sage Weil <sage@redhat.com>
---
 net/ceph/ceph_common.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c
index 4fd02831beed..26ab58665f77 100644
--- a/net/ceph/ceph_common.c
+++ b/net/ceph/ceph_common.c
@@ -611,7 +611,11 @@ struct ceph_client *ceph_create_client(struct ceph_options *opt, void *private)
 {
 	struct ceph_client *client;
 	struct ceph_entity_addr *myaddr = NULL;
-	int err = -ENOMEM;
+	int err;
+
+	err = wait_for_random_bytes();
+	if (err < 0)
+		return ERR_PTR(err);
 
 	client = kzalloc(sizeof(*client), GFP_KERNEL);
 	if (client == NULL)
-- 
2.13.0

Re: [kernel-hardening] [PATCH v4 07/13] ceph: ensure RNG is seeded before using

[Theodore Ts'o]

On Tue, Jun 06, 2017 at 07:47:58PM +0200, Jason A. Donenfeld wrote:
> Ceph uses the RNG for various nonce generations, and it shouldn't accept
> using bad randomness. So, we wait for the RNG to be properly seeded. We
> do this by calling wait_for_random_bytes() in a function that is
> certainly called in process context, early on, so that all subsequent
> calls to get_random_bytes are necessarily acceptable.
> 
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Cc: Ilya Dryomov <idryomov@gmail.com>
> Cc: "Yan, Zheng" <zyan@redhat.com>
> Cc: Sage Weil <sage@redhat.com>

Thanks, applied to the dev branch.

					- Ted

[PATCH v4 08/13] cifs: use get_random_u32 for 32-bit lock random

[Jason A. Donenfeld]

Using get_random_u32 here is faster, more fitting of the use case, and
just as cryptographically secure. It also has the benefit of providing
better randomness at early boot, which is sometimes when this is used.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Steve French <sfrench@samba.org>
---
 fs/cifs/cifsfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 9a1667e0e8d6..fe0c8dcc7dc7 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -1359,7 +1359,7 @@ init_cifs(void)
 	spin_lock_init(&cifs_tcp_ses_lock);
 	spin_lock_init(&GlobalMid_Lock);
 
-	get_random_bytes(&cifs_lock_secret, sizeof(cifs_lock_secret));
+	cifs_lock_secret = get_random_u32();
 
 	if (cifs_max_pending < 2) {
 		cifs_max_pending = 2;
-- 
2.13.0

Re: [kernel-hardening] [PATCH v4 08/13] cifs: use get_random_u32 for 32-bit lock random

[Theodore Ts'o]

On Tue, Jun 06, 2017 at 07:47:59PM +0200, Jason A. Donenfeld wrote:
> Using get_random_u32 here is faster, more fitting of the use case, and
> just as cryptographically secure. It also has the benefit of providing
> better randomness at early boot, which is sometimes when this is used.
> 
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Cc: Steve French <sfrench@samba.org>

There's a bigger problem here, which is that cifs_lock_secret is a
32-bit value which is being used to obscure flock->fl_owner before it
is sent across the wire.  But flock->fl_owner is a pointer to the
struct file *, so 64-bit architecture, the high 64-bits of a kernel
pointer is being exposed to anyone using tcpdump.  (Oops, I'm showing
my age; I guess all the cool kids are using Wireshark these days.)

Worse, the obscuring is being done using XOR.  How an active attacker
might be able to trivially reverse engineer the 32-bit "secret" is
left as an exercise to the reader.  The bottom line is if the goal is
to hide the memory location of a struct file from an attacker,
cifs_lock_secret is about as useful as a TSA agent doing security
theatre at an airport.  Which is to say, it makes the civilians feel
good.  :-)

BTW, Jason, this is why it's *good* to audit all of the uses of
get_random_bytes().  It only took me about 30 seconds in the first
patch in your series that changes a caller of get_random_bytes(), and
look what I was able to find by just taking a quick look.  Not waiting
for the CRNG to be fully initialized is the *least* of its problems.

Anyway, I'll include this commit in the dev branch of the random tree,
since it's not going to make things worse.

						- Ted

Re: [kernel-hardening] [PATCH v4 08/13] cifs: use get_random_u32 for 32-bit lock random

[Jason A. Donenfeld]

On Thu, Jun 8, 2017 at 2:25 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> There's a bigger problem here, which is that cifs_lock_secret is a
> 32-bit value which is being used to obscure flock->fl_owner before it
> is sent across the wire.  But flock->fl_owner is a pointer to the
> struct file *, so 64-bit architecture, the high 64-bits of a kernel
> pointer is being exposed to anyone using tcpdump.  (Oops, I'm showing
> my age; I guess all the cool kids are using Wireshark these days.)
>
> Worse, the obscuring is being done using XOR.  How an active attacker
> might be able to trivially reverse engineer the 32-bit "secret" is
> left as an exercise to the reader.  The bottom line is if the goal is
> to hide the memory location of a struct file from an attacker,
> cifs_lock_secret is about as useful as a TSA agent doing security
> theatre at an airport.  Which is to say, it makes the civilians feel
> good.  :-)

High five for taking the deep dive and actually reading how this all
works. Nice bug!

> Not waiting
> for the CRNG to be fully initialized is the *least* of its problems.

The kernel is vast and filled with tons of bugs of many sorts. On this
reasoning, maybe I should spend my time auditing web apps instead,
which are usually the "front door" of bugs? I like the puzzles of
random.c. I also had a real world need for wait_for_random_bytes() in
a module I'm writing.

But anyway, your general point is a really good one. Tons of callers
of the random functions are doing it wrong in one way or another.
Spending time looking at those is probably a good idea...

> Anyway, I'll include this commit in the dev branch of the random tree,
> since it's not going to make things worse.

Great, thanks.

Re: [kernel-hardening] [PATCH v4 08/13] cifs: use get_random_u32 for 32-bit lock random

[Jason A. Donenfeld]

On Thu, Jun 8, 2017 at 2:25 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> There's a bigger problem here, which is that cifs_lock_secret is a
> 32-bit value which is being used to obscure flock->fl_owner before it
> is sent across the wire.  But flock->fl_owner is a pointer to the
> struct file *, so 64-bit architecture, the high 64-bits of a kernel
> pointer is being exposed to anyone using tcpdump.  (Oops, I'm showing
> my age; I guess all the cool kids are using Wireshark these days.)
>
> Worse, the obscuring is being done using XOR.  How an active attacker
> might be able to trivially reverse engineer the 32-bit "secret" is
> left as an exercise to the reader.  The bottom line is if the goal is
> to hide the memory location of a struct file from an attacker,
> cifs_lock_secret is about as useful as a TSA agent doing security
> theatre at an airport.  Which is to say, it makes the civilians feel
> good.  :-)

High five for taking the deep dive and actually reading how this all
works. Nice bug!

> Not waiting
> for the CRNG to be fully initialized is the *least* of its problems.

The kernel is vast and filled with tons of bugs of many sorts. On this
reasoning, maybe I should spend my time auditing web apps instead,
which are usually the "front door" of bugs? I like the puzzles of
random.c. I also had a real world need for wait_for_random_bytes() in
a module I'm writing.

But anyway, your general point is a really good one. Tons of callers
of the random functions are doing it wrong in one way or another.
Spending time looking at those is probably a good idea...

> Anyway, I'll include this commit in the dev branch of the random tree,
> since it's not going to make things worse.

Great, thanks.

[PATCH v4 09/13] rhashtable: use get_random_u32 for hash_rnd

[Jason A. Donenfeld]

This is much faster and just as secure. It also has the added benefit of
probably returning better randomness at early-boot on systems with
architectural RNGs.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Thomas Graf <tgraf@suug.ch>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
---
 lib/rhashtable.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index d9e7274a04cd..a1eb7c947f46 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -235,7 +235,7 @@ static struct bucket_table *bucket_table_alloc(struct rhashtable *ht,
 
 	INIT_LIST_HEAD(&tbl->walkers);
 
-	get_random_bytes(&tbl->hash_rnd, sizeof(tbl->hash_rnd));
+	tbl->hash_rnd = get_random_u32();
 
 	for (i = 0; i < nbuckets; i++)
 		INIT_RHT_NULLS_HEAD(tbl->buckets[i], ht, i);
-- 
2.13.0

Re: [PATCH v4 09/13] rhashtable: use get_random_u32 for hash_rnd

[Theodore Ts'o]

On Tue, Jun 06, 2017 at 07:48:00PM +0200, Jason A. Donenfeld wrote:
> This is much faster and just as secure. It also has the added benefit of
> probably returning better randomness at early-boot on systems with
> architectural RNGs.
> 
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Cc: Thomas Graf <tgraf@suug.ch>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>

Thanks, applied to the random.git dev branch.

		       		  - Ted

[PATCH v4 10/13] net/neighbor: use get_random_u32 for 32-bit hash random

[Jason A. Donenfeld]

Using get_random_u32 here is faster, more fitting of the use case, and
just as cryptographically secure. It also has the benefit of providing
better randomness at early boot, which is when many of these structures
are assigned.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: David Miller <davem@davemloft.net>
---
 net/core/neighbour.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index d274f81fcc2c..9784133b0cdb 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -312,8 +312,7 @@ static struct neighbour *neigh_alloc(struct neigh_table *tbl, struct net_device
 
 static void neigh_get_hash_rnd(u32 *x)
 {
-	get_random_bytes(x, sizeof(*x));
-	*x |= 1;
+	*x = get_random_u32() | 1;
 }
 
 static struct neigh_hash_table *neigh_hash_alloc(unsigned int shift)
-- 
2.13.0

Re: [PATCH v4 10/13] net/neighbor: use get_random_u32 for 32-bit hash random

[Theodore Ts'o]

On Tue, Jun 06, 2017 at 07:48:01PM +0200, Jason A. Donenfeld wrote:
> Using get_random_u32 here is faster, more fitting of the use case, and
> just as cryptographically secure. It also has the benefit of providing
> better randomness at early boot, which is when many of these structures
> are assigned.
> 
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Cc: David Miller <davem@davemloft.net>

Applied to the random.git dev branch, thanks.

	       		      - Ted

[PATCH v4 11/13] net/route: use get_random_int for random counter

[Jason A. Donenfeld]

Using get_random_int here is faster, more fitting of the use case, and
just as cryptographically secure. It also has the benefit of providing
better randomness at early boot, which is when many of these structures
are assigned.

Also, semantically, it's not really proper to have been assigning an
atomic_t in this way before, even if in practice it works fine.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: David Miller <davem@davemloft.net>
---
 net/ipv4/route.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 655d9eebe43e..11e001a42094 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -2936,8 +2936,7 @@ static __net_init int rt_genid_init(struct net *net)
 {
 	atomic_set(&net->ipv4.rt_genid, 0);
 	atomic_set(&net->fnhe_genid, 0);
-	get_random_bytes(&net->ipv4.dev_addr_genid,
-			 sizeof(net->ipv4.dev_addr_genid));
+	atomic_set(&net->ipv4.dev_addr_genid, get_random_int());
 	return 0;
 }
 
-- 
2.13.0

Re: [PATCH v4 11/13] net/route: use get_random_int for random counter

[Theodore Ts'o]

On Tue, Jun 06, 2017 at 07:48:02PM +0200, Jason A. Donenfeld wrote:
> Using get_random_int here is faster, more fitting of the use case, and
> just as cryptographically secure. It also has the benefit of providing
> better randomness at early boot, which is when many of these structures
> are assigned.
> 
> Also, semantically, it's not really proper to have been assigning an
> atomic_t in this way before, even if in practice it works fine.
> 
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Cc: David Miller <davem@davemloft.net>

Applied to the dev branch of the random.git branch, thanks.

	       	   	     	 	    - Ted

[PATCH v4 12/13] bluetooth/smp: ensure RNG is properly seeded before ECDH use

[Jason A. Donenfeld]

This protocol uses lots of complex cryptography that relies on securely
generated random numbers. Thus, it's important that the RNG is actually
seeded before use. Fortuantely, it appears we're always operating in
process context (there are many GFP_KERNEL allocations and other
sleeping operations), and so we can simply demand that the RNG is seeded
before we use it.

We take two strategies in this commit. The first is for the library code
that's called from other modules like hci or mgmt: here we just change
the call to get_random_bytes_wait, and return the result of the wait to
the caller, along with the other error codes of those functions like
usual. Then there's the SMP protocol handler itself, which makes many
many many calls to get_random_bytes during different phases. For this,
rather than have to change all the calls to get_random_bytes_wait and
propagate the error result, it's actually enough to just put a single
call to wait_for_random_bytes() at the beginning of the handler, to
ensure that all the subsequent invocations are safe, without having to
actually change them. Likewise, for the random address changing
function, we'd rather know early on in the function whether the RNG
initialization has been interrupted, rather than later, so we call
wait_for_random_bytes() at the top, so that later on the call to
get_random_bytes() is acceptable.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: Johan Hedberg <johan.hedberg@gmail.com>
---
 net/bluetooth/hci_request.c |  6 ++++++
 net/bluetooth/smp.c         | 18 ++++++++++++++----
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index b5faff458d8b..4078057c4fd7 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -1406,6 +1406,12 @@ int hci_update_random_address(struct hci_request *req, bool require_privacy,
 	struct hci_dev *hdev = req->hdev;
 	int err;
 
+	if (require_privacy) {
+		err = wait_for_random_bytes();
+		if (unlikely(err))
+			return err;
+	}
+
 	/* If privacy is enabled use a resolvable private address. If
 	 * current RPA has expired or there is something else than
 	 * the current RPA in use, then generate a new one.
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 14585edc9439..5fef1bc96f42 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -537,7 +537,9 @@ int smp_generate_rpa(struct hci_dev *hdev, const u8 irk[16], bdaddr_t *rpa)
 
 	smp = chan->data;
 
-	get_random_bytes(&rpa->b[3], 3);
+	err = get_random_bytes_wait(&rpa->b[3], 3);
+	if (unlikely(err))
+		return err;
 
 	rpa->b[5] &= 0x3f;	/* Clear two most significant bits */
 	rpa->b[5] |= 0x40;	/* Set second most significant bit */
@@ -570,7 +572,9 @@ int smp_generate_oob(struct hci_dev *hdev, u8 hash[16], u8 rand[16])
 	} else {
 		while (true) {
 			/* Seed private key with random number */
-			get_random_bytes(smp->local_sk, 32);
+			err = get_random_bytes_wait(smp->local_sk, 32);
+			if (unlikely(err))
+				return err;
 
 			/* Generate local key pair for Secure Connections */
 			if (!generate_ecdh_keys(smp->local_pk, smp->local_sk))
@@ -589,7 +593,9 @@ int smp_generate_oob(struct hci_dev *hdev, u8 hash[16], u8 rand[16])
 	SMP_DBG("OOB Public Key Y: %32phN", smp->local_pk + 32);
 	SMP_DBG("OOB Private Key:  %32phN", smp->local_sk);
 
-	get_random_bytes(smp->local_rand, 16);
+	err = get_random_bytes_wait(smp->local_rand, 16);
+	if (unlikely(err))
+		return err;
 
 	err = smp_f4(smp->tfm_cmac, smp->local_pk, smp->local_pk,
 		     smp->local_rand, 0, hash);
@@ -2831,7 +2837,11 @@ static int smp_sig_channel(struct l2cap_chan *chan, struct sk_buff *skb)
 	struct hci_conn *hcon = conn->hcon;
 	struct smp_chan *smp;
 	__u8 code, reason;
-	int err = 0;
+	int err;
+
+	err = wait_for_random_bytes();
+	if (unlikely(err))
+		return err;
 
 	if (skb->len < 1)
 		return -EILSEQ;
-- 
2.13.0

Re: [PATCH v4 12/13] bluetooth/smp: ensure RNG is properly seeded before ECDH use

[Theodore Ts'o]

On Tue, Jun 06, 2017 at 07:48:03PM +0200, Jason A. Donenfeld wrote:
> This protocol uses lots of complex cryptography that relies on securely
> generated random numbers. Thus, it's important that the RNG is actually
> seeded before use. Fortuantely, it appears we're always operating in
> process context (there are many GFP_KERNEL allocations and other
> sleeping operations), and so we can simply demand that the RNG is seeded
> before we use it.
> 
> We take two strategies in this commit. The first is for the library code
> that's called from other modules like hci or mgmt: here we just change
> the call to get_random_bytes_wait, and return the result of the wait to
> the caller, along with the other error codes of those functions like
> usual. Then there's the SMP protocol handler itself, which makes many
> many many calls to get_random_bytes during different phases. For this,
> rather than have to change all the calls to get_random_bytes_wait and
> propagate the error result, it's actually enough to just put a single
> call to wait_for_random_bytes() at the beginning of the handler, to
> ensure that all the subsequent invocations are safe, without having to
> actually change them. Likewise, for the random address changing
> function, we'd rather know early on in the function whether the RNG
> initialization has been interrupted, rather than later, so we call
> wait_for_random_bytes() at the top, so that later on the call to
> get_random_bytes() is acceptable.

Do we need to do all of this?  Bluetooth folks, is it fair to assume
that hci_power_on() has to be called before any bluetooth functions
can be done?  If so, adding a wait_for_random_bytes() in
hci_power_on() might be all that is necessary.

	       	     	    	    - Ted

Re: [PATCH v4 12/13] bluetooth/smp: ensure RNG is properly seeded before ECDH use

[Marcel Holtmann]

Hi Ted,

>> This protocol uses lots of complex cryptography that relies on securely
>> generated random numbers. Thus, it's important that the RNG is actually
>> seeded before use. Fortuantely, it appears we're always operating in
>> process context (there are many GFP_KERNEL allocations and other
>> sleeping operations), and so we can simply demand that the RNG is seeded
>> before we use it.
>> 
>> We take two strategies in this commit. The first is for the library code
>> that's called from other modules like hci or mgmt: here we just change
>> the call to get_random_bytes_wait, and return the result of the wait to
>> the caller, along with the other error codes of those functions like
>> usual. Then there's the SMP protocol handler itself, which makes many
>> many many calls to get_random_bytes during different phases. For this,
>> rather than have to change all the calls to get_random_bytes_wait and
>> propagate the error result, it's actually enough to just put a single
>> call to wait_for_random_bytes() at the beginning of the handler, to
>> ensure that all the subsequent invocations are safe, without having to
>> actually change them. Likewise, for the random address changing
>> function, we'd rather know early on in the function whether the RNG
>> initialization has been interrupted, rather than later, so we call
>> wait_for_random_bytes() at the top, so that later on the call to
>> get_random_bytes() is acceptable.
> 
> Do we need to do all of this?  Bluetooth folks, is it fair to assume
> that hci_power_on() has to be called before any bluetooth functions
> can be done?  If so, adding a wait_for_random_bytes() in
> hci_power_on() might be all that is necessary.

yes, there are plenty of commands needed before a controller becomes usable. When plugging in new Bluetooth hardware, we have to power it up and read the initial settings and configuration out of.

Also all the cryptographic features only apply to LE enabled controllers. The classic BR/EDR controllers have this all in hardware. So if you are not LE enabled, then there is not even a point in waiting for any seeding. However that said, also all LE controllers have an extra random number function we could call if we need extra seeding. We never bothered to hook this up since we thought that the kernel has enough sources.

Regards

Marcel

Re: [PATCH v4 12/13] bluetooth/smp: ensure RNG is properly seeded before ECDH use

[Jason A. Donenfeld]

On Thu, Jun 8, 2017 at 7:04 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
>> Do we need to do all of this?  Bluetooth folks, is it fair to assume
>> that hci_power_on() has to be called before any bluetooth functions
>> can be done?  If so, adding a wait_for_random_bytes() in
>> hci_power_on() might be all that is necessary.

Maybe, but that could hassle bluetooth users who don't want to use any
of the fancy bluetooth crypto and don't want to bother waiting for
their RNG to initialize.

Re: [PATCH v4 12/13] bluetooth/smp: ensure RNG is properly seeded before ECDH use

[Jason A. Donenfeld]

Hello Marcel,

On Thu, Jun 8, 2017 at 7:04 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
> yes, there are plenty of commands needed before a controller becomes usable.

That doesn't clearly address with precision what Ted was wondering.
Specifically, the inquiry is: can you confirm with certainty whether
or not all calls to get_random_bytes() in the bluetooth directory are
*necessarily* going to come after a call to hci_power_on()?

Jason

Re: [PATCH v4 12/13] bluetooth/smp: ensure RNG is properly seeded before ECDH use

[Marcel Holtmann]

Hi Jason,

>> yes, there are plenty of commands needed before a controller becomes usable.
> 
> That doesn't clearly address with precision what Ted was wondering.
> Specifically, the inquiry is: can you confirm with certainty whether
> or not all calls to get_random_bytes() in the bluetooth directory are
> *necessarily* going to come after a call to hci_power_on()?

on a powered down controller, you can not do any crypto. SMP is only during a connection and the RPAs are only generated when needed. So yes, doing this once in hci_power_on is plenty. However we might want to limit this to LE capable controllers since for BR/EDR only controllers this is not needed. For A2MP I need to check that we need the random numbers seeded there. However this hidden behind the high speed feature.

Regards

Marcel

Re: [PATCH v4 12/13] bluetooth/smp: ensure RNG is properly seeded before ECDH use

[Jason A. Donenfeld]

On Thu, Jun 8, 2017 at 7:05 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> on a powered down controller, you can not do any crypto. SMP is only during a connection and the RPAs are only generated when needed. So yes, doing this once in hci_power_on is plenty. However we might want to limit this to LE capable controllers since for BR/EDR only controllers this is not needed. For A2MP I need to check that we need the random numbers seeded there. However this hidden behind the high speed feature.

Okay so it sounds like certain controllers will use this and certain
won't, and so it might be slightly complicated to follow the mouse
through the tube ideally. In that case, I'd recommend continuing with
this current patchset, which just adds the wait (which is a no-op if
it's already seeded) close to the actual call sites.

Can you review that patch and give your Signed-off-by if it looks good?

Jason

[PATCH] bluetooth: ensure RNG is properly seeded before powerup

[Jason A. Donenfeld]

The SMP protocol uses lots of complex cryptography that relies on securely
generated random numbers. Thus, it's important that the RNG is actually
seeded before use. Ted and the bluetooth maintainers seem to think that
it's sufficient to wait_for_random_bytes before powering up, and this
will ensure that all subsequent get_random_bytes calls occur after this
single wait_for_random_bytes. (I'm not completely convinced, but time
will tell.) Therefore, we put this call to wait_for_random_bytes, which
is running in a workqueue, so it can sleep while waiting. We also are
required to pass the potential error code back through the failure
setter, like the rest of the function does.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: Johan Hedberg <johan.hedberg@gmail.com>
---
Ted -- this would be instead of my other bluetooth patch. I think my other
one might be a bit more robust, but if you prefer this strategy instead,
here's the code for it.

 net/bluetooth/hci_core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 05686776a5fb..d940e22f365d 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2017,6 +2017,18 @@ static void hci_power_on(struct work_struct *work)
 
 	BT_DBG("%s", hdev->name);
 
+	/* Bluetooth is a big user of cryptography and thus needs to have a
+	 * good random number generator, especially for the SMP protocol.
+	 * Thus, we ensure we have good randomness before powering up.
+	 */
+	err = wait_for_random_bytes();
+	if (err < 0) {
+		hci_dev_lock(hdev);
+		mgmt_set_powered_failed(hdev, err);
+		hci_dev_unlock(hdev);
+		return;
+	}
+
 	if (test_bit(HCI_UP, &hdev->flags) &&
 	    hci_dev_test_flag(hdev, HCI_MGMT) &&
 	    hci_dev_test_and_clear_flag(hdev, HCI_AUTO_OFF)) {
-- 
2.13.1

[PATCH v4 13/13] random: warn when kernel uses unseeded randomness

[Jason A. Donenfeld]

This enables an important dmesg notification about when drivers have
used the crng without it being seeded first. Prior, these errors would
occur silently, and so there hasn't been a great way of diagnosing these
types of bugs for obscure setups. By adding this as a config option, we
can leave it on by default, so that we learn where these issues happen,
in the field, will still allowing some people to turn it off, if they
really know what they're doing and do not want the log entries.

However, we don't leave it _completely_ by default. An earlier version
of this patch simply had `default y`. I'd really love that, but it turns
out, this problem with unseeded randomness being used is really quite
present and is going to take a long time to fix. Thus, as a compromise
between log-messages-for-all and nobody-knows, this is `default y`,
except it is also `depends on DEBUG_KERNEL`. This will ensure that the
curious see the messages while others don't have to.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/char/random.c | 15 +++++++++++++--
 lib/Kconfig.debug     | 16 ++++++++++++++++
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 36cdb2406610..33a9ec86d101 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -288,7 +288,6 @@
 #define SEC_XFER_SIZE		512
 #define EXTRACT_SIZE		10
 
-#define DEBUG_RANDOM_BOOT 0
 
 #define LONGS(x) (((x) + sizeof(unsigned long) - 1)/sizeof(unsigned long))
 
@@ -1477,7 +1476,7 @@ void get_random_bytes(void *buf, int nbytes)
 {
 	__u8 tmp[CHACHA20_BLOCK_SIZE];
 
-#if DEBUG_RANDOM_BOOT > 0
+#ifdef CONFIG_WARN_UNSEEDED_RANDOM
 	if (!crng_ready())
 		printk(KERN_NOTICE "random: %pF get_random_bytes called "
 		       "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
@@ -2071,6 +2070,12 @@ u64 get_random_u64(void)
 	    return ret;
 #endif
 
+#ifdef CONFIG_WARN_UNSEEDED_RANDOM
+	if (!crng_ready())
+		printk(KERN_NOTICE "random: %pF get_random_u64 called "
+		       "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
+#endif
+
 	batch = &get_cpu_var(batched_entropy_u64);
 	if (use_lock)
 		read_lock_irqsave(&batched_entropy_reset_lock, flags);
@@ -2097,6 +2102,12 @@ u32 get_random_u32(void)
 	if (arch_get_random_int(&ret))
 		return ret;
 
+#ifdef CONFIG_WARN_UNSEEDED_RANDOM
+	if (!crng_ready())
+		printk(KERN_NOTICE "random: %pF get_random_u32 called "
+		       "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
+#endif
+
 	batch = &get_cpu_var(batched_entropy_u32);
 	if (use_lock)
 		read_lock_irqsave(&batched_entropy_reset_lock, flags);
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index e4587ebe52c7..c4159605bfbf 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1209,6 +1209,22 @@ config STACKTRACE
 	  It is also used by various kernel debugging features that require
 	  stack trace generation.
 
+config WARN_UNSEEDED_RANDOM
+	bool "Warn when kernel uses unseeded randomness"
+	default y
+	depends on DEBUG_KERNEL
+	help
+	  Some parts of the kernel contain bugs relating to their use of
+	  cryptographically secure random numbers before it's actually possible
+	  to generate those numbers securely. This setting ensures that these
+	  flaws don't go unnoticed, by enabling a message, should this ever
+	  occur. This will allow people with obscure setups to know when things
+	  are going wrong, so that they might contact developers about fixing
+	  it.
+
+	  Say Y here, unless you simply do not care about using unseeded
+	  randomness and do not want a potential warning message in your logs.
+
 config DEBUG_KOBJECT
 	bool "kobject debugging"
 	depends on DEBUG_KERNEL
-- 
2.13.0

Re: [PATCH v4 13/13] random: warn when kernel uses unseeded randomness

[Theodore Ts'o]

On Tue, Jun 06, 2017 at 07:48:04PM +0200, Jason A. Donenfeld wrote:
> This enables an important dmesg notification about when drivers have
> used the crng without it being seeded first. Prior, these errors would
> occur silently, and so there hasn't been a great way of diagnosing these
> types of bugs for obscure setups. By adding this as a config option, we
> can leave it on by default, so that we learn where these issues happen,
> in the field, will still allowing some people to turn it off, if they
> really know what they're doing and do not want the log entries.
> 
> However, we don't leave it _completely_ by default. An earlier version
> of this patch simply had `default y`. I'd really love that, but it turns
> out, this problem with unseeded randomness being used is really quite
> present and is going to take a long time to fix. Thus, as a compromise
> between log-messages-for-all and nobody-knows, this is `default y`,
> except it is also `depends on DEBUG_KERNEL`. This will ensure that the
> curious see the messages while others don't have to.
> 
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

This patch is pretty spammy.  On my KVM test kernel:

random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
random: neigh_hash_alloc+0x77/0x8f get_random_u32 called with crng_init = 0
random: neigh_hash_alloc+0x77/0x8f get_random_u32 called with crng_init = 0
random: neigh_hash_alloc+0x77/0x8f get_random_u32 called with crng_init = 0
random: neigh_hash_alloc+0x77/0x8f get_random_u32 called with crng_init = 0
random: rt_genid_init+0x24/0x2f get_random_u32 called with crng_init = 0
random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0

At the very least we probably should do a logical "uniq" on the output
(e.g., if we have complained about the previous callsite, don't whinge
about it again).

						- Ted

commit 9d9035bc6d7871a73d7f9aada4e63cb190874a68
Author: Theodore Ts'o <tytso@mit.edu>
Date:   Thu Jun 8 04:16:59 2017 -0400

    random: suppress duplicate crng_init=0 warnings
    
    Suppress duplicate CONFIG_WARN_UNSEEDED_RANDOM warnings to avoid
    spamming dmesg.
    
    Signed-off-by: Theodore Ts'o <tytso@mit.edu>

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 798f353f0d3c..3bdeef13afda 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1481,9 +1481,14 @@ void get_random_bytes(void *buf, int nbytes)
 	__u8 tmp[CHACHA20_BLOCK_SIZE];
 
 #ifdef CONFIG_WARN_UNSEEDED_RANDOM
-	if (!crng_ready())
+	static void *previous = NULL;
+	void *caller = (void *) _RET_IP_;
+
+	if (!crng_ready() && (READ_ONCE(previous) != caller)) {
 		printk(KERN_NOTICE "random: %pF get_random_bytes called "
-		       "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
+		       "with crng_init=%d\n", caller, crng_init);
+		WRITE_ONCE(previous, caller);
+	}
 #endif
 	trace_get_random_bytes(nbytes, _RET_IP_);
 
@@ -2064,6 +2069,10 @@ u64 get_random_u64(void)
 	bool use_lock = crng_init < 2;
 	unsigned long flags;
 	struct batched_entropy *batch;
+#ifdef CONFIG_WARN_UNSEEDED_RANDOM
+	static void *previous = NULL;
+	void *caller = (void *) _RET_IP_;
+#endif
 
 #if BITS_PER_LONG == 64
 	if (arch_get_random_long((unsigned long *)&ret))
@@ -2075,9 +2084,11 @@ u64 get_random_u64(void)
 #endif
 
 #ifdef CONFIG_WARN_UNSEEDED_RANDOM
-	if (!crng_ready())
+	if (!crng_ready() && (READ_ONCE(previous) != caller))  {
 		printk(KERN_NOTICE "random: %pF get_random_u64 called "
-		       "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
+		       "with crng_init=%d\n", caller, crng_init);
+		WRITE_ONCE(previous, caller);
+	}
 #endif
 
 	batch = &get_cpu_var(batched_entropy_u64);
@@ -2102,14 +2113,20 @@ u32 get_random_u32(void)
 	bool use_lock = crng_init < 2;
 	unsigned long flags;
 	struct batched_entropy *batch;
+#ifdef CONFIG_WARN_UNSEEDED_RANDOM
+	static void *previous = NULL;
+	void *caller = (void *) _RET_IP_;
+#endif
 
 	if (arch_get_random_int(&ret))
 		return ret;
 
 #ifdef CONFIG_WARN_UNSEEDED_RANDOM
-	if (!crng_ready())
+	if (!crng_ready() && READ_ONCE(previous) != caller) {
 		printk(KERN_NOTICE "random: %pF get_random_u32 called "
-		       "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
+		       "with crng_init=%d\n", caller, crng_init);
+		WRITE_ONCE(previous, caller);
+	}
 #endif
 
 	batch = &get_cpu_var(batched_entropy_u32);

Re: [PATCH v4 13/13] random: warn when kernel uses unseeded randomness

[Jason A. Donenfeld]

On Thu, Jun 8, 2017 at 10:19 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> At the very least we probably should do a logical "uniq" on the output
> (e.g., if we have complained about the previous callsite, don't whinge
> about it again).

That seems okay to me.

Re: [kernel-hardening] Re: [PATCH v4 13/13] random: warn when kernel uses unseeded randomness

[Michael Ellerman]

Theodore Ts'o <tytso@mit.edu> writes:
> On Tue, Jun 06, 2017 at 07:48:04PM +0200, Jason A. Donenfeld wrote:
>> This enables an important dmesg notification about when drivers have
>> used the crng without it being seeded first. Prior, these errors would
>> occur silently, and so there hasn't been a great way of diagnosing these
>> types of bugs for obscure setups. By adding this as a config option, we
>> can leave it on by default, so that we learn where these issues happen,
>> in the field, will still allowing some people to turn it off, if they
>> really know what they're doing and do not want the log entries.
...
>
> This patch is pretty spammy.  On my KVM test kernel:
>
> random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
> random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
> random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
> random: bucket_table_alloc+0x15f/0x190 get_random_u32 called with crng_init = 0
...
>
> At the very least we probably should do a logical "uniq" on the output
> (e.g., if we have complained about the previous callsite, don't whinge
> about it again).
>
> commit 9d9035bc6d7871a73d7f9aada4e63cb190874a68
> Author: Theodore Ts'o <tytso@mit.edu>
> Date:   Thu Jun 8 04:16:59 2017 -0400
>
>     random: suppress duplicate crng_init=0 warnings
>     
>     Suppress duplicate CONFIG_WARN_UNSEEDED_RANDOM warnings to avoid
>     spamming dmesg.
>     
>     Signed-off-by: Theodore Ts'o <tytso@mit.edu>

Even with this patch, it's still pretty spammy (today's linux-next):

 random: load_elf_binary+0x57c/0x1550 get_random_u64 called with crng_init=0
 random: arch_randomize_brk+0xa4/0xd0 get_random_u64 called with crng_init=0
 random: arch_mmap_rnd+0x78/0xb0 get_random_u64 called with crng_init=0
 random: load_elf_binary+0x57c/0x1550 get_random_u64 called with crng_init=0
 random: arch_randomize_brk+0xa4/0xd0 get_random_u64 called with crng_init=0
 random: arch_mmap_rnd+0x78/0xb0 get_random_u64 called with crng_init=0
 random: load_elf_binary+0x57c/0x1550 get_random_u64 called with crng_init=0
 random: arch_randomize_brk+0xa4/0xd0 get_random_u64 called with crng_init=0
 random: arch_mmap_rnd+0x78/0xb0 get_random_u64 called with crng_init=0
 random: load_elf_binary+0x57c/0x1550 get_random_u64 called with crng_init=0
 random: arch_randomize_brk+0xa4/0xd0 get_random_u64 called with crng_init=0
 random: arch_mmap_rnd+0x78/0xb0 get_random_u64 called with crng_init=0
 random: load_elf_binary+0x57c/0x1550 get_random_u64 called with crng_init=0
 random: arch_mmap_rnd+0x78/0xb0 get_random_u64 called with crng_init=0
 random: load_elf_binary+0x57c/0x1550 get_random_u64 called with crng_init=0
 random: arch_randomize_brk+0xa4/0xd0 get_random_u64 called with crng_init=0
 Initializing random number generator... random: arch_mmap_rnd+0x78/0xb0 get_random_u64 called with crng_init=0
 random: load_elf_binary+0x57c/0x1550 get_random_u64 called with crng_init=0
 random: arch_randomize_brk+0xa4/0xd0 get_random_u64 called with crng_init=0
 random: arch_mmap_rnd+0x78/0xb0 get_random_u64 called with crng_init=0
 random: load_elf_binary+0x57c/0x1550 get_random_u64 called with crng_init=0
 random: arch_randomize_brk+0xa4/0xd0 get_random_u64 called with crng_init=0
 random: arch_mmap_rnd+0x78/0xb0 get_random_u64 called with crng_init=0
 random: load_elf_binary+0x57c/0x1550 get_random_u64 called with crng_init=0
 random: arch_randomize_brk+0xa4/0xd0 get_random_u64 called with crng_init=0


Do I need to be doing anything to fix these? (this is on powerpc)

cheers

Re: [kernel-hardening] Re: [PATCH v4 13/13] random: warn when kernel uses unseeded randomness

[Stephan Müller]

Am Donnerstag, 15. Juni 2017, 13:03:48 CEST schrieb Michael Ellerman:

Hi Michael,

> 
> Even with this patch, it's still pretty spammy (today's linux-next):
> 

I would think that the issue regarding the logging is relevant for 
cryptographic use cases or use cases requiring strong random numbers only. 
Only those use cases should be fixed eventually to wait for a fully seeded 
DRNG.

The logged messages you present here indicate use cases where no strong 
security is required. It looks like that the logs show ASLR related use of 
random numbers. Those do not require a fully seeded ChaCha20 DRNG.

IMHO, users using the get_random_u64 or get_random_u32 are use cases that do 
not require a fully seeded DRNG thus do not need a cryptographically strong 
random number. Hence, I would think that the logging should be removed from 
get_random_u32/u64.

Yet, logging should remain for get_random_bytes which should be denominated as 
the interface for use cases where cryptographically strong random numbers are 
required.

Ciao
Stephan

Re: [kernel-hardening] Re: [PATCH v4 13/13] random: warn when kernel uses unseeded randomness

[Theodore Ts'o]

On Thu, Jun 15, 2017 at 01:59:43PM +0200, Stephan Müller wrote:
> I would think that the issue regarding the logging is relevant for 
> cryptographic use cases or use cases requiring strong random numbers only. 
> Only those use cases should be fixed eventually to wait for a fully seeded 
> DRNG.
> 
> The logged messages you present here indicate use cases where no strong 
> security is required. It looks like that the logs show ASLR related use of 
> random numbers. Those do not require a fully seeded ChaCha20 DRNG.

I suspect there is a range of opinions aobut whether or not ASLR
requires strongly secure random numbers or not.  It seems pretty clear
that if we proposed using prandom_u32 for ASLR, people would object
very strongly indeed, since that would make it trivially easy for
attackers to circumvent ASLR protections.

> IMHO, users using the get_random_u64 or get_random_u32 are use cases that do 
> not require a fully seeded DRNG thus do not need a cryptographically strong 
> random number. Hence, I would think that the logging should be removed from 
> get_random_u32/u64.

You are effectively proposing that there ought to be a middle range of
security between prandom_32, get_random_u32/get_random_u64 and
get_random_bytes().  I think that's going to lead to all sorts of
complexity and bugs from people not understanding when they should use
get_random_u32 vs get_random_bytes versus prandom_u32.  And then we'll
end up needing to audit all of the callsites for get_random_u32() so
they don't violate this new usage rule that you are proposing.

     	   	   	    	       - Ted

Re: [kernel-hardening] Re: [PATCH v4 13/13] random: warn when kernel uses unseeded randomness

[Stephan Müller]

Am Sonntag, 18. Juni 2017, 17:46:25 CEST schrieb Theodore Ts'o:

Hi Theodore,

> > IMHO, users using the get_random_u64 or get_random_u32 are use cases that
> > do not require a fully seeded DRNG thus do not need a cryptographically
> > strong random number. Hence, I would think that the logging should be
> > removed from get_random_u32/u64.
> 
> You are effectively proposing that there ought to be a middle range of
> security between prandom_32, get_random_u32/get_random_u64 and
> get_random_bytes().  I think that's going to lead to all sorts of
> complexity and bugs from people not understanding when they should use
> get_random_u32 vs get_random_bytes versus prandom_u32.  And then we'll
> end up needing to audit all of the callsites for get_random_u32() so
> they don't violate this new usage rule that you are proposing.

I only proposed to get rid of the log messages indicating a non-seeded DRNG.

But you bring up an interesting point: if it is true you say that it is hard 
for people to use differnent types of APIs regarding entropy and random 
numbers right (which I would concur with), and considering that you imply that 
get_random_bytes, get_random_u32 and get_random_u64 have the same security 
strength, why do we have these three APIs to begin with? The get_random_bytes 
API would then be more than enough.

Ciao
Stephan

Re: [kernel-hardening] Re: [PATCH v4 13/13] random: warn when kernel uses unseeded randomness

[Jason A. Donenfeld]

On Sun, Jun 18, 2017 at 7:55 PM, Stephan Müller <smueller@chronox.de> wrote:
> But you bring up an interesting point: if it is true you say that it is hard
> for people to use differnent types of APIs regarding entropy and random
> numbers right (which I would concur with), and considering that you imply that
> get_random_bytes, get_random_u32 and get_random_u64 have the same security
> strength, why do we have these three APIs to begin with? The get_random_bytes
> API would then be more than enough.

Because there are efficiences we can benefit from for getting integer
sized outputs.

Use get_random_{u32,u64} when you want a secure random number.
Use get_random_bytes when you want a longer secure random bytestring.

Re: [kernel-hardening] Re: [PATCH v4 13/13] random: warn when kernel uses unseeded randomness

[Jason A. Donenfeld]

On Sun, Jun 18, 2017 at 5:46 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> You are effectively proposing that there ought to be a middle range of
> security between prandom_32, get_random_u32/get_random_u64 and
> get_random_bytes().  I think that's going to lead to all sorts of
> complexity and bugs from people not understanding when they should use
> get_random_u32 vs get_random_bytes versus prandom_u32.  And then we'll
> end up needing to audit all of the callsites for get_random_u32() so
> they don't violate this new usage rule that you are proposing.

I agree with you wholeheartedly.

get_random_* provides the secure random numbers.
prandom_* provides the insecure random numbers.

Introducing some kind of middle ground will result in needless
complexity and inevitable bugs.

Re: [PATCH v4 13/13] random: warn when kernel uses unseeded randomness

[Jeffrey Walton]

On Tue, Jun 6, 2017 at 1:48 PM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> This enables an important dmesg notification about when drivers have
> used the crng without it being seeded first. Prior, these errors would
> occur silently, and so there hasn't been a great way of diagnosing these
> types of bugs for obscure setups. By adding this as a config option, we
> can leave it on by default, so that we learn where these issues happen,
> in the field, will still allowing some people to turn it off, if they
> really know what they're doing and do not want the log entries.
>
> However, we don't leave it _completely_ by default. An earlier version
> of this patch simply had `default y`. I'd really love that, but it turns
> out, this problem with unseeded randomness being used is really quite
> present and is going to take a long time to fix. Thus, as a compromise
> between log-messages-for-all and nobody-knows, this is `default y`,
> except it is also `depends on DEBUG_KERNEL`. This will ensure that the
> curious see the messages while others don't have to.

Please forgive my ignorance... What do the last two sentence mean exactly?

If I am running a production Debian, Fedora or Ubuntu kernel, will a
messages be present if a violation occurs? A "violation" is a policy
violation, and I mean a generator is used before its operational.

Sunlight is the best disinfectant. At least one message should be
logged to ensure the issue is known. But its not clear to me if that's
what happens when trying to parse the last two sentences.

Jeff

Re: [PATCH v4 00/13] Unseeded In-Kernel Randomness Fixes

[Jason A. Donenfeld]

Hi Ted,

Could I get your Signed-off-by on this patchset, so that somebody can
add it to their tree?

Thanks,
Jason