Whenever you're comparing two MACs, it's important to do this using
crypto_memneq instead of memcmp. With memcmp, you leak timing information,
which could then be used to iteratively forge a MAC. This is far too basic
of a mistake for us to have so pervasively in the year 2017, so let's begin
cleaning this stuff up. The following 6 locations were found with some
simple regex greps, but I'm sure more lurk below the surface. If you
maintain some code or know somebody who maintains some code that deals
with MACs, tell them to double check which comparison function they're
using.
Jason A. Donenfeld (6):
sunrpc: use constant time memory comparison for mac
net/ipv6: use constant time memory comparison for mac
ccree: use constant time memory comparison for macs and tags
security/keys: use constant time memory comparison for macs
bluetooth/smp: use constant time memory comparison for secret values
mac80211/wpa: use constant time memory comparison for MACs
Cc: Anna Schumaker <[email protected]>
Cc: David Howells <[email protected]>
Cc: David Safford <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Gilad Ben-Yossef <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Gustavo Padovan <[email protected]>
Cc: "J. Bruce Fields" <[email protected]>
Cc: Jeff Layton <[email protected]>
Cc: Johan Hedberg <[email protected]>
Cc: Johannes Berg <[email protected]>
Cc: Marcel Holtmann <[email protected]>
Cc: Mimi Zohar <[email protected]>
Cc: Trond Myklebust <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
drivers/staging/ccree/ssi_fips_ll.c | 17 ++++++++-------
net/bluetooth/smp.c | 39 ++++++++++++++++++-----------------
net/ipv6/seg6_hmac.c | 3 ++-
net/mac80211/wpa.c | 9 ++++----
net/sunrpc/auth_gss/gss_krb5_crypto.c | 3 ++-
security/keys/trusted.c | 7 ++++---
6 files changed, 42 insertions(+), 36 deletions(-)
--
2.13.1
Arend van Spriel <[email protected]> writes:
> On 6/11/2017 11:30 PM, Emil Lenngren wrote:
>> 2017-06-11 22:48 GMT+02:00 Emmanuel Grumbach <[email protected]>:
>>> On Sun, Jun 11, 2017 at 4:36 PM, Kees Cook <[email protected]> wrote:
>>>>
>>>> On Sun, Jun 11, 2017 at 1:13 AM, Kalle Valo <[email protected]> wrote:
>>>>> "Jason A. Donenfeld" <[email protected]> writes:
>>>>>
>>>>>> Whenever you're comparing two MACs, it's important to do this using
>>>>>> crypto_memneq instead of memcmp. With memcmp, you leak timing information,
>>>>>> which could then be used to iteratively forge a MAC.
>>>>>
>>>>> Do you have any pointers where I could learn more about this?
>>>>
>>>> While not using C specifically, this talks about the problem generally:
>>>> https://www.chosenplaintext.ca/articles/beginners-guide-constant-time-cryptography.html
>>>>
>>>
>>> Sorry for the stupid question, but the MAC address is in plaintext in
>>> the air anyway or easily accessible via user space tools. I fail to
>>> see what it is so secret about a MAC address in that code where that
>>> same MAC address is accessible via myriads of ways.
>>
>> I think you're mixing up Media Access Control (MAC) addresses with
>> Message Authentication Code (MAC). The second one is a cryptographic
>> signature of a message.
>
> While this may be obvious to those who are in the know this mixup is
> easily made outside the crypto domain and especially in the (wireless)
> networking domain (my mind wandered towards the same error path).
I did realise that this was about Message Authentication Code (yay!) but
I got lost because I thought this is somehow related to timestamps :)
Thanks to Kees I now understand this is about revealing execution time
to the attacker, not timestamps or anything like that.
> As this series is touching stuff outside crypto it is good to be
> explicit and not use such abbreviations that can be misinterpreted.
> The article Kees referred to is also useful to get into the proper
> context here and at least worth mentioning this or other useful
> references in the cover letter.
And the kernel documentation we have is not really helping much:
/**
* crypto_memneq - Compare two areas of memory without leaking
* timing information.
*
* @a: One area of memory
* @b: Another area of memory
* @size: The size of the area.
*
* Returns 0 when data is equal, 1 otherwise.
*/
For most people "leaking timing information" does not tell much. Adding
a sentence or two _why_ this function should be used would be very
helpful.
--
Kalle Valo
On 6/11/2017 11:30 PM, Emil Lenngren wrote:
> 2017-06-11 22:48 GMT+02:00 Emmanuel Grumbach <[email protected]>:
>> On Sun, Jun 11, 2017 at 4:36 PM, Kees Cook <[email protected]> wrote:
>>>
>>> On Sun, Jun 11, 2017 at 1:13 AM, Kalle Valo <[email protected]> wrote:
>>>> "Jason A. Donenfeld" <[email protected]> writes:
>>>>
>>>>> Whenever you're comparing two MACs, it's important to do this using
>>>>> crypto_memneq instead of memcmp. With memcmp, you leak timing information,
>>>>> which could then be used to iteratively forge a MAC.
>>>>
>>>> Do you have any pointers where I could learn more about this?
>>>
>>> While not using C specifically, this talks about the problem generally:
>>> https://www.chosenplaintext.ca/articles/beginners-guide-constant-time-cryptography.html
>>>
>>
>> Sorry for the stupid question, but the MAC address is in plaintext in
>> the air anyway or easily accessible via user space tools. I fail to
>> see what it is so secret about a MAC address in that code where that
>> same MAC address is accessible via myriads of ways.
>
> I think you're mixing up Media Access Control (MAC) addresses with
> Message Authentication Code (MAC). The second one is a cryptographic
> signature of a message.
While this may be obvious to those who are in the know this mixup is
easily made outside the crypto domain and especially in the (wireless)
networking domain (my mind wandered towards the same error path). As
this series is touching stuff outside crypto it is good to be explicit
and not use such abbreviations that can be misinterpreted. The article
Kees referred to is also useful to get into the proper context here and
at least worth mentioning this or other useful references in the cover
letter.
Regards,
Arend
On Mon, Jun 12, 2017 at 12:30 AM, Emil Lenngren <[email protected]> wrote:
> 2017-06-11 22:48 GMT+02:00 Emmanuel Grumbach <[email protected]>:
>> On Sun, Jun 11, 2017 at 4:36 PM, Kees Cook <[email protected]> wrote:
>>>
>>> On Sun, Jun 11, 2017 at 1:13 AM, Kalle Valo <[email protected]> wrote:
>>> > "Jason A. Donenfeld" <[email protected]> writes:
>>> >
>>> >> Whenever you're comparing two MACs, it's important to do this using
>>> >> crypto_memneq instead of memcmp. With memcmp, you leak timing information,
>>> >> which could then be used to iteratively forge a MAC.
>>> >
>>> > Do you have any pointers where I could learn more about this?
>>>
>>> While not using C specifically, this talks about the problem generally:
>>> https://www.chosenplaintext.ca/articles/beginners-guide-constant-time-cryptography.html
>>>
>>
>> Sorry for the stupid question, but the MAC address is in plaintext in
>> the air anyway or easily accessible via user space tools. I fail to
>> see what it is so secret about a MAC address in that code where that
>> same MAC address is accessible via myriads of ways.
>
> I think you're mixing up Media Access Control (MAC) addresses with
> Message Authentication Code (MAC). The second one is a cryptographic
> signature of a message.
Obviously... Sorry for the noise.
2017-06-11 22:48 GMT+02:00 Emmanuel Grumbach <[email protected]>:
> On Sun, Jun 11, 2017 at 4:36 PM, Kees Cook <[email protected]> wrote:
>>
>> On Sun, Jun 11, 2017 at 1:13 AM, Kalle Valo <[email protected]> wrote:
>> > "Jason A. Donenfeld" <[email protected]> writes:
>> >
>> >> Whenever you're comparing two MACs, it's important to do this using
>> >> crypto_memneq instead of memcmp. With memcmp, you leak timing information,
>> >> which could then be used to iteratively forge a MAC.
>> >
>> > Do you have any pointers where I could learn more about this?
>>
>> While not using C specifically, this talks about the problem generally:
>> https://www.chosenplaintext.ca/articles/beginners-guide-constant-time-cryptography.html
>>
>
> Sorry for the stupid question, but the MAC address is in plaintext in
> the air anyway or easily accessible via user space tools. I fail to
> see what it is so secret about a MAC address in that code where that
> same MAC address is accessible via myriads of ways.
I think you're mixing up Media Access Control (MAC) addresses with
Message Authentication Code (MAC). The second one is a cryptographic
signature of a message.
Hi Stephan,
On Sun, Jun 11, 2017 at 11:06 PM, Stephan M=C3=BCller <[email protected]>=
wrote:
> Are you planning to send an update to your patch set? If yes, there is an=
other
> one which should be converted too: crypto/rsa-pkcs1pad.c.
I just sent an update to this thread patching that, per your
suggestion. Since these issues are expected to be cherry picked by
their respective committer, I figure we can just pile on the patches
here, listing the 0/6 intro email as each patch's parent.
Jason
Am Samstag, 10. Juni 2017, 04:59:06 CEST schrieb Jason A. Donenfeld:
Hi Jason,
> Whenever you're comparing two MACs, it's important to do this using
> crypto_memneq instead of memcmp. With memcmp, you leak timing information,
> which could then be used to iteratively forge a MAC. This is far too basic
> of a mistake for us to have so pervasively in the year 2017, so let's begin
> cleaning this stuff up. The following 6 locations were found with some
> simple regex greps, but I'm sure more lurk below the surface. If you
> maintain some code or know somebody who maintains some code that deals
> with MACs, tell them to double check which comparison function they're
> using.
Are you planning to send an update to your patch set? If yes, there is another
one which should be converted too: crypto/rsa-pkcs1pad.c.
Otherwise, I will send a patch converting this one.
Thanks.
Ciao
Stephan
On Sun, Jun 11, 2017 at 4:36 PM, Kees Cook <[email protected]> wrote:
>
> On Sun, Jun 11, 2017 at 1:13 AM, Kalle Valo <[email protected]> wrote:
> > "Jason A. Donenfeld" <[email protected]> writes:
> >
> >> Whenever you're comparing two MACs, it's important to do this using
> >> crypto_memneq instead of memcmp. With memcmp, you leak timing information,
> >> which could then be used to iteratively forge a MAC.
> >
> > Do you have any pointers where I could learn more about this?
>
> While not using C specifically, this talks about the problem generally:
> https://www.chosenplaintext.ca/articles/beginners-guide-constant-time-cryptography.html
>
Sorry for the stupid question, but the MAC address is in plaintext in
the air anyway or easily accessible via user space tools. I fail to
see what it is so secret about a MAC address in that code where that
same MAC address is accessible via myriads of ways.
On Sun, Jun 11, 2017 at 1:13 AM, Kalle Valo <[email protected]> wrote:
> "Jason A. Donenfeld" <[email protected]> writes:
>
>> Whenever you're comparing two MACs, it's important to do this using
>> crypto_memneq instead of memcmp. With memcmp, you leak timing information,
>> which could then be used to iteratively forge a MAC.
>
> Do you have any pointers where I could learn more about this?
While not using C specifically, this talks about the problem generally:
https://www.chosenplaintext.ca/articles/beginners-guide-constant-time-cryptography.html
-Kees
--
Kees Cook
Pixel Security
"Jason A. Donenfeld" <[email protected]> writes:
> Whenever you're comparing two MACs, it's important to do this using
> crypto_memneq instead of memcmp. With memcmp, you leak timing information,
> which could then be used to iteratively forge a MAC.
Do you have any pointers where I could learn more about this?
--
Kalle Valo
Hi Jason,
> This file is filled with complex cryptography. Thus, the comparisons of
> MACs and secret keys and curve points and so forth should not add timing
> attacks, which could either result in a direct forgery, or, given the
> complexity, some other type of attack.
>
> Signed-off-by: Jason A. Donenfeld <[email protected]>
> Cc: Marcel Holtmann <[email protected]>
> Cc: Gustavo Padovan <[email protected]>
> Cc: Johan Hedberg <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> net/bluetooth/smp.c | 39 ++++++++++++++++++++-------------------
> 1 file changed, 20 insertions(+), 19 deletions(-)
patch has been applied to bluetooth-next tree.
Regards
Marcel
This file is filled with complex cryptography. Thus, the comparisons of
MACs and secret keys and curve points and so forth should not add timing
attacks, which could either result in a direct forgery, or, given the
complexity, some other type of attack.
Signed-off-by: Jason A. Donenfeld <[email protected]>
Cc: Marcel Holtmann <[email protected]>
Cc: Gustavo Padovan <[email protected]>
Cc: Johan Hedberg <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
net/bluetooth/smp.c | 39 ++++++++++++++++++++-------------------
1 file changed, 20 insertions(+), 19 deletions(-)
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 14585edc9439..a0ef89772c36 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -23,6 +23,7 @@
#include <linux/debugfs.h>
#include <linux/scatterlist.h>
#include <linux/crypto.h>
+#include <crypto/algapi.h>
#include <crypto/b128ops.h>
#include <crypto/hash.h>
@@ -523,7 +524,7 @@ bool smp_irk_matches(struct hci_dev *hdev, const u8 irk[16],
if (err)
return false;
- return !memcmp(bdaddr->b, hash, 3);
+ return !crypto_memneq(bdaddr->b, hash, 3);
}
int smp_generate_rpa(struct hci_dev *hdev, const u8 irk[16], bdaddr_t *rpa)
@@ -579,7 +580,7 @@ int smp_generate_oob(struct hci_dev *hdev, u8 hash[16], u8 rand[16])
/* This is unlikely, but we need to check that
* we didn't accidentially generate a debug key.
*/
- if (memcmp(smp->local_sk, debug_sk, 32))
+ if (crypto_memneq(smp->local_sk, debug_sk, 32))
break;
}
smp->debug_key = false;
@@ -993,7 +994,7 @@ static u8 smp_random(struct smp_chan *smp)
if (ret)
return SMP_UNSPECIFIED;
- if (memcmp(smp->pcnf, confirm, sizeof(smp->pcnf)) != 0) {
+ if (crypto_memneq(smp->pcnf, confirm, sizeof(smp->pcnf))) {
BT_ERR("Pairing failed (confirmation values mismatch)");
return SMP_CONFIRM_FAILED;
}
@@ -1512,7 +1513,7 @@ static u8 sc_passkey_round(struct smp_chan *smp, u8 smp_op)
smp->rrnd, r, cfm))
return SMP_UNSPECIFIED;
- if (memcmp(smp->pcnf, cfm, 16))
+ if (crypto_memneq(smp->pcnf, cfm, 16))
return SMP_CONFIRM_FAILED;
smp->passkey_round++;
@@ -1908,7 +1909,7 @@ static u8 sc_send_public_key(struct smp_chan *smp)
/* This is unlikely, but we need to check that
* we didn't accidentially generate a debug key.
*/
- if (memcmp(smp->local_sk, debug_sk, 32))
+ if (crypto_memneq(smp->local_sk, debug_sk, 32))
break;
}
}
@@ -2176,7 +2177,7 @@ static u8 smp_cmd_pairing_random(struct l2cap_conn *conn, struct sk_buff *skb)
if (err)
return SMP_UNSPECIFIED;
- if (memcmp(smp->pcnf, cfm, 16))
+ if (crypto_memneq(smp->pcnf, cfm, 16))
return SMP_CONFIRM_FAILED;
} else {
smp_send_cmd(conn, SMP_CMD_PAIRING_RANDOM, sizeof(smp->prnd),
@@ -2660,7 +2661,7 @@ static int smp_cmd_public_key(struct l2cap_conn *conn, struct sk_buff *skb)
if (err)
return SMP_UNSPECIFIED;
- if (memcmp(cfm.confirm_val, smp->pcnf, 16))
+ if (crypto_memneq(cfm.confirm_val, smp->pcnf, 16))
return SMP_CONFIRM_FAILED;
}
@@ -2693,7 +2694,7 @@ static int smp_cmd_public_key(struct l2cap_conn *conn, struct sk_buff *skb)
else
hcon->pending_sec_level = BT_SECURITY_FIPS;
- if (!memcmp(debug_pk, smp->remote_pk, 64))
+ if (!crypto_memneq(debug_pk, smp->remote_pk, 64))
set_bit(SMP_FLAG_DEBUG_KEY, &smp->flags);
if (smp->method == DSP_PASSKEY) {
@@ -2792,7 +2793,7 @@ static int smp_cmd_dhkey_check(struct l2cap_conn *conn, struct sk_buff *skb)
if (err)
return SMP_UNSPECIFIED;
- if (memcmp(check->e, e, 16))
+ if (crypto_memneq(check->e, e, 16))
return SMP_DHKEY_CHECK_FAILED;
if (!hcon->out) {
@@ -3506,10 +3507,10 @@ static int __init test_debug_key(void)
if (!generate_ecdh_keys(pk, sk))
return -EINVAL;
- if (memcmp(sk, debug_sk, 32))
+ if (crypto_memneq(sk, debug_sk, 32))
return -EINVAL;
- if (memcmp(pk, debug_pk, 64))
+ if (crypto_memneq(pk, debug_pk, 64))
return -EINVAL;
return 0;
@@ -3529,7 +3530,7 @@ static int __init test_ah(struct crypto_cipher *tfm_aes)
if (err)
return err;
- if (memcmp(res, exp, 3))
+ if (crypto_memneq(res, exp, 3))
return -EINVAL;
return 0;
@@ -3559,7 +3560,7 @@ static int __init test_c1(struct crypto_cipher *tfm_aes)
if (err)
return err;
- if (memcmp(res, exp, 16))
+ if (crypto_memneq(res, exp, 16))
return -EINVAL;
return 0;
@@ -3584,7 +3585,7 @@ static int __init test_s1(struct crypto_cipher *tfm_aes)
if (err)
return err;
- if (memcmp(res, exp, 16))
+ if (crypto_memneq(res, exp, 16))
return -EINVAL;
return 0;
@@ -3616,7 +3617,7 @@ static int __init test_f4(struct crypto_shash *tfm_cmac)
if (err)
return err;
- if (memcmp(res, exp, 16))
+ if (crypto_memneq(res, exp, 16))
return -EINVAL;
return 0;
@@ -3650,10 +3651,10 @@ static int __init test_f5(struct crypto_shash *tfm_cmac)
if (err)
return err;
- if (memcmp(mackey, exp_mackey, 16))
+ if (crypto_memneq(mackey, exp_mackey, 16))
return -EINVAL;
- if (memcmp(ltk, exp_ltk, 16))
+ if (crypto_memneq(ltk, exp_ltk, 16))
return -EINVAL;
return 0;
@@ -3686,7 +3687,7 @@ static int __init test_f6(struct crypto_shash *tfm_cmac)
if (err)
return err;
- if (memcmp(res, exp, 16))
+ if (crypto_memneq(res, exp, 16))
return -EINVAL;
return 0;
@@ -3740,7 +3741,7 @@ static int __init test_h6(struct crypto_shash *tfm_cmac)
if (err)
return err;
- if (memcmp(res, exp, 16))
+ if (crypto_memneq(res, exp, 16))
return -EINVAL;
return 0;
--
2.13.1