pull-request: mac80211 2017-10-16

pull-request: mac80211 2017-10-16

[Johannes Berg]

Hi Dave,

Here's a fix, for a small part of the "KRACK" announced today
(krackattacks.com).

Please pull and let me know if there's any problem.

Thanks,
johannes



The following changes since commit c0576e3975084d4699b7bfef578613fb8e1144f6:

  net: call cgroup_sk_alloc() earlier in sk_clone_lock() (2017-10-10 20:24:29 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git tags/mac80211-for-davem-2017-10-16

for you to fetch changes up to fdf7cb4185b60c68e1a75e61691c4afdc15dea0e:

  mac80211: accept key reinstall without changing anything (2017-10-16 13:02:03 +0200)

----------------------------------------------------------------
Just a single fix, for a WoWLAN-related part of CVE-2017-13080.

----------------------------------------------------------------
Johannes Berg (1):
      mac80211: accept key reinstall without changing anything

 net/mac80211/key.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

pull-request: mac80211 2017-10-16

[Johannes Berg]

Hi Dave,

Here's a fix, for a small part of the "KRACK" announced today
(krackattacks.com).

Please pull and let me know if there's any problem.

Thanks,
johannes



The following changes since commit c0576e3975084d4699b7bfef578613fb8e1144f6:

  net: call cgroup_sk_alloc() earlier in sk_clone_lock() (2017-10-10 20:24:29 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git tags/mac80211-for-davem-2017-10-16

for you to fetch changes up to fdf7cb4185b60c68e1a75e61691c4afdc15dea0e:

  mac80211: accept key reinstall without changing anything (2017-10-16 13:02:03 +0200)

----------------------------------------------------------------
Just a single fix, for a WoWLAN-related part of CVE-2017-13080.

----------------------------------------------------------------
Johannes Berg (1):
      mac80211: accept key reinstall without changing anything

 net/mac80211/key.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

Re: pull-request: mac80211 2017-10-16

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Mon, 16 Oct 2017 15:46:17 +0200

> Here's a fix, for a small part of the "KRACK" announced today
> (krackattacks.com).
> 
> Please pull and let me know if there's any problem.

Pulled.

Re: pull-request: mac80211 2017-10-16

[Jason A. Donenfeld]

Mobile phone right now, so not able to write patch, but you probably
should be using crypto_memneq for comparing those two keys, not
memcmp.

Jason

Re: pull-request: mac80211 2017-10-16

[Johannes Berg]

On Tue, 2017-10-17 at 01:30 +0200, Jason A. Donenfeld wrote:
> Mobile phone right now, so not able to write patch, but you probably
> should be using crypto_memneq for comparing those two keys, not
> memcmp.

I know that's a gut instinct, but I really don't see the point.

If you actually get this to evaluate to "equal", you're already being
attacked, and the attacker already knows they're trying to get the same
key to be installed again. If it's not equal, you execute so much code
beneath, going to the driver etc., that I'd think this particular time
is in the noise.

Also, this isn't something that the attacker can directly observe
anyway, and if they can, then surely the fact that we *skip* a large
amount of code when returning 0 is also observable.

Besides, the only thing that can observe the timing is hostapd or
wpa_supplicant, which obviously know the key (and in the fixed versions
will already have done the comparison themselves).

And lastly, if you wanted to use this as an oracle to get the key back
from the kernel, then you already have admin permissions (in your
network namespace), and can just sniff netlink traffic anyway.

johannes

Re: pull-request: mac80211 2017-10-16

[Johannes Berg]

On Tue, 2017-10-17 at 01:30 +0200, Jason A. Donenfeld wrote:
> Mobile phone right now, so not able to write patch, but you probably
> should be using crypto_memneq for comparing those two keys, not
> memcmp.

I know that's a gut instinct, but I really don't see the point.

If you actually get this to evaluate to "equal", you're already being
attacked, and the attacker already knows they're trying to get the same
key to be installed again. If it's not equal, you execute so much code
beneath, going to the driver etc., that I'd think this particular time
is in the noise.

Also, this isn't something that the attacker can directly observe
anyway, and if they can, then surely the fact that we *skip* a large
amount of code when returning 0 is also observable.

Besides, the only thing that can observe the timing is hostapd or
wpa_supplicant, which obviously know the key (and in the fixed versions
will already have done the comparison themselves).

And lastly, if you wanted to use this as an oracle to get the key back
from the kernel, then you already have admin permissions (in your
network namespace), and can just sniff netlink traffic anyway.

johannes

Re: pull-request: mac80211 2017-10-16

[Jason A. Donenfeld]

On Tue, Oct 17, 2017 at 7:46 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> If it's not equal, you execute so much code
> beneath, going to the driver etc., that I'd think this particular time
> is in the noise.

Usually presumptions like this get you in trouble when some crafty
academic has a smart idea about that noise. I'll send a patch.

[PATCH] mac80211: use constant time comparison with keys

[Jason A. Donenfeld]

Otherwise we risk leaking information via timing side channel.

Fixes: fdf7cb4185b6 ("mac80211: accept key reinstall without changing anything")
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 net/mac80211/key.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index ae995c8480db..035d16fe926e 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -19,6 +19,7 @@
 #include <linux/slab.h>
 #include <linux/export.h>
 #include <net/mac80211.h>
+#include <crypto/algapi.h>
 #include <asm/unaligned.h>
 #include "ieee80211_i.h"
 #include "driver-ops.h"
@@ -635,7 +636,7 @@ int ieee80211_key_link(struct ieee80211_key *key,
 	 * new version of the key to avoid nonce reuse or replay issues.
 	 */
 	if (old_key && key->conf.keylen == old_key->conf.keylen &&
-	    !memcmp(key->conf.key, old_key->conf.key, key->conf.keylen)) {
+	    !crypto_memneq(key->conf.key, old_key->conf.key, key->conf.keylen)) {
 		ieee80211_key_free_unused(key);
 		ret = 0;
 		goto out;
-- 
2.14.2

[PATCH] mac80211: use constant time comparison with keys

[Jason A. Donenfeld]

Otherwise we risk leaking information via timing side channel.

Fixes: fdf7cb4185b6 ("mac80211: accept key reinstall without changing anything")
Signed-off-by: Jason A. Donenfeld <Jason-OnJsPKxuuEcAvxtiuMwx3w@public.gmane.org>
---
 net/mac80211/key.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index ae995c8480db..035d16fe926e 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -19,6 +19,7 @@
 #include <linux/slab.h>
 #include <linux/export.h>
 #include <net/mac80211.h>
+#include <crypto/algapi.h>
 #include <asm/unaligned.h>
 #include "ieee80211_i.h"
 #include "driver-ops.h"
@@ -635,7 +636,7 @@ int ieee80211_key_link(struct ieee80211_key *key,
 	 * new version of the key to avoid nonce reuse or replay issues.
 	 */
 	if (old_key && key->conf.keylen == old_key->conf.keylen &&
-	    !memcmp(key->conf.key, old_key->conf.key, key->conf.keylen)) {
+	    !crypto_memneq(key->conf.key, old_key->conf.key, key->conf.keylen)) {
 		ieee80211_key_free_unused(key);
 		ret = 0;
 		goto out;
-- 
2.14.2

Re: [PATCH] mac80211: use constant time comparison with keys

[Johannes Berg]

On Tue, 2017-10-17 at 20:32 +0200, Jason A. Donenfeld wrote:
> Otherwise we risk leaking information via timing side channel.
> 
Applied.

johannes