Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1662851 > unrolled thread

[PATCH 0/6] Constant Time Memory Comparisons Are Important

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/6] Constant Time Memory Comparisons Are Important "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-10 05:00 +0200
    [PATCH 3/6] ccree: use constant time memory comparison for macs and tags "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-10 05:00 +0200
      Re: [PATCH 3/6] ccree: use constant time memory comparison for macs  and tags Gilad Ben-Yossef <gilad@benyossef.com> - 2017-06-10 09:50 +0200
        Re: [PATCH 3/6] ccree: use constant time memory comparison for macs  and tags "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-10 13:00 +0200
          Re: [PATCH 3/6] ccree: use constant time memory comparison for macs  and tags Henrique de Moraes Holschuh <hmh@hmh.eng.br> - 2017-06-10 23:50 +0200
    [PATCH 2/6] net/ipv6: use constant time memory comparison for mac "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-10 05:10 +0200
    [PATCH 4/6] security/keys: use constant time memory comparison for macs "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-10 05:10 +0200
      Re: [kernel-hardening] [PATCH 4/6] security/keys: use constant time  memory comparison for macs James Morris <jmorris@namei.org> - 2017-06-14 11:00 +0200
    [PATCH 1/6] sunrpc: use constant time memory comparison for mac "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-10 05:10 +0200
    [PATCH 5/6] bluetooth/smp: use constant time memory comparison for secret values "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-10 05:10 +0200
      Re: [PATCH 5/6] bluetooth/smp: use constant time memory comparison  for secret values Marcel Holtmann <marcel@holtmann.org> - 2017-06-10 15:50 +0200
    [PATCH 6/6] mac80211/wpa: use constant time memory comparison for MACs "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-10 05:10 +0200
      Re: [PATCH 6/6] mac80211/wpa: use constant time memory comparison  for MACs Johannes Berg <johannes@sipsolutions.net> - 2017-06-13 10:30 +0200
        Re: [PATCH 6/6] mac80211/wpa: use constant time memory comparison for MACs "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-13 15:30 +0200
    Re: [PATCH 0/6] Constant Time Memory Comparisons Are Important Kalle Valo <kvalo@codeaurora.org> - 2017-06-11 10:20 +0200
      Re: [PATCH 0/6] Constant Time Memory Comparisons Are Important Kees Cook <keescook@chromium.org> - 2017-06-11 15:40 +0200
        Re: [PATCH 0/6] Constant Time Memory Comparisons Are Important Emmanuel Grumbach <egrumbach@gmail.com> - 2017-06-11 22:50 +0200
          Re: [PATCH 0/6] Constant Time Memory Comparisons Are Important Emil Lenngren <emil.lenngren@gmail.com> - 2017-06-11 23:40 +0200
            Re: [PATCH 0/6] Constant Time Memory Comparisons Are Important Emmanuel Grumbach <egrumbach@gmail.com> - 2017-06-12 07:10 +0200
            Re: [PATCH 0/6] Constant Time Memory Comparisons Are Important Arend van Spriel <arend.vanspriel@broadcom.com> - 2017-06-12 09:40 +0200
    Re: [PATCH 0/6] Constant Time Memory Comparisons Are Important Stephan Müller <smueller@chronox.de> - 2017-06-11 23:10 +0200
      Re: [PATCH 0/6] Constant Time Memory Comparisons Are Important "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-11 23:30 +0200
    [PATCH] rsa-pkcs1pad: use constant time memory comparison for MACs "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-11 23:30 +0200

Page 1 of 2  [1] 2  Next page →

#1662851 — [PATCH 0/6] Constant Time Memory Comparisons Are Important

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 <anna.schumaker@netapp.com>
Cc: David Howells <dhowells@redhat.com>
Cc: David Safford <safford@us.ibm.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Gilad Ben-Yossef <gilad@benyossef.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: "J. Bruce Fields" <bfields@fieldses.org>
Cc: Jeff Layton <jlayton@poochiereds.net>
Cc: Johan Hedberg <johan.hedberg@gmail.com>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: Trond Myklebust <trond.myklebust@primarydata.com>
Cc: keyrings@vger.kernel.org
Cc: linux-bluetooth@vger.kernel.org
Cc: linux-nfs@vger.kernel.org
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org

 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

[toc] | [next] | [standalone]

#1662852 — [PATCH 3/6] ccree: use constant time memory comparison for macs and tags

Otherwise, we enable several different forgeries via timing attack.

While the C inside this file is nearly incomprehensible, I did notice a
high volume of "FIPS" and "NIST", which makes this kind of bug slightly
more embarrassing.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Gilad Ben-Yossef <gilad@benyossef.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: stable@vger.kernel.org
---
 drivers/staging/ccree/ssi_fips_ll.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/ccree/ssi_fips_ll.c b/drivers/staging/ccree/ssi_fips_ll.c
index d573574bbb98..3310997d8e3e 100644
--- a/drivers/staging/ccree/ssi_fips_ll.c
+++ b/drivers/staging/ccree/ssi_fips_ll.c
@@ -19,6 +19,7 @@ This file defines the driver FIPS Low Level implmentaion functions,
 that executes the KAT.
 ***************************************************************/
 #include <linux/kernel.h>
+#include <crypto/algapi.h>
 
 #include "ssi_driver.h"
 #include "ssi_fips_local.h"
@@ -462,7 +463,7 @@ ssi_cipher_fips_power_up_tests(struct ssi_drvdata *drvdata, void *cpu_addr_buffe
 		}
 
 		/* compare actual dout to expected */
-		if (memcmp(virt_ctx->dout, cipherData->dataOut, cipherData->dataInSize) != 0)
+		if (crypto_memneq(virt_ctx->dout, cipherData->dataOut, cipherData->dataInSize))
 		{
 			FIPS_LOG("dout comparison error %d - oprMode=%d, isAes=%d\n", i, cipherData->oprMode, cipherData->isAes);
 			FIPS_LOG("  i  expected   received \n");
@@ -586,7 +587,7 @@ ssi_cmac_fips_power_up_tests(struct ssi_drvdata *drvdata, void *cpu_addr_buffer,
 		}
 
 		/* compare actual mac result to expected */
-		if (memcmp(virt_ctx->mac_res, cmac_data->mac_res, cmac_data->mac_res_size) != 0)
+		if (crypto_memneq(virt_ctx->mac_res, cmac_data->mac_res, cmac_data->mac_res_size))
 		{
 			FIPS_LOG("comparison error %d - digest_size=%d \n", i, cmac_data->mac_res_size);
 			FIPS_LOG("  i  expected   received \n");
@@ -760,7 +761,7 @@ ssi_hash_fips_power_up_tests(struct ssi_drvdata *drvdata, void *cpu_addr_buffer,
                 }
 
 		/* compare actual mac result to expected */
-		if (memcmp(virt_ctx->mac_res, hash_data->mac_res, digest_size) != 0)
+		if (crypto_memneq(virt_ctx->mac_res, hash_data->mac_res, digest_size))
 		{
 			FIPS_LOG("comparison error %d - hash_mode=%d digest_size=%d \n", i, hash_data->hash_mode, digest_size);
 			FIPS_LOG("  i  expected   received \n");
@@ -1093,7 +1094,7 @@ ssi_hmac_fips_power_up_tests(struct ssi_drvdata *drvdata, void *cpu_addr_buffer,
 		}
 
 		/* compare actual mac result to expected */
-		if (memcmp(virt_ctx->mac_res, hmac_data->mac_res, digest_size) != 0)
+		if (crypto_memneq(virt_ctx->mac_res, hmac_data->mac_res, digest_size))
 		{
 			FIPS_LOG("comparison error %d - hash_mode=%d digest_size=%d \n", i, hmac_data->hash_mode, digest_size);
 			FIPS_LOG("  i  expected   received \n");
@@ -1310,7 +1311,7 @@ ssi_ccm_fips_power_up_tests(struct ssi_drvdata *drvdata, void *cpu_addr_buffer,
 		}
 
 		/* compare actual dout to expected */
-		if (memcmp(virt_ctx->dout, ccmData->dataOut, ccmData->dataInSize) != 0)
+		if (crypto_memneq(virt_ctx->dout, ccmData->dataOut, ccmData->dataInSize))
 		{
 			FIPS_LOG("dout comparison error %d - size=%d \n", i, ccmData->dataInSize);
                         error = CC_REE_FIPS_ERROR_AESCCM_PUT;
@@ -1318,7 +1319,7 @@ ssi_ccm_fips_power_up_tests(struct ssi_drvdata *drvdata, void *cpu_addr_buffer,
                 }
 
 		/* compare actual mac result to expected */
-		if (memcmp(virt_ctx->mac_res, ccmData->macResOut, ccmData->tagSize) != 0)
+		if (crypto_memneq(virt_ctx->mac_res, ccmData->macResOut, ccmData->tagSize))
 		{
 			FIPS_LOG("mac_res comparison error %d - mac_size=%d \n", i, ccmData->tagSize);
 			FIPS_LOG("  i  expected   received \n");
@@ -1633,7 +1634,7 @@ ssi_gcm_fips_power_up_tests(struct ssi_drvdata *drvdata, void *cpu_addr_buffer,
 
 		if (gcmData->direction == DRV_CRYPTO_DIRECTION_ENCRYPT) {
 			/* compare actual dout to expected */
-			if (memcmp(virt_ctx->dout, gcmData->dataOut, gcmData->dataInSize) != 0)
+			if (crypto_memneq(virt_ctx->dout, gcmData->dataOut, gcmData->dataInSize))
 			{
 				FIPS_LOG("dout comparison error %d - size=%d \n", i, gcmData->dataInSize);
 				FIPS_LOG("  i  expected   received \n");
@@ -1649,7 +1650,7 @@ ssi_gcm_fips_power_up_tests(struct ssi_drvdata *drvdata, void *cpu_addr_buffer,
 		}
 
 		/* compare actual mac result to expected */
-		if (memcmp(virt_ctx->mac_res, gcmData->macResOut, gcmData->tagSize) != 0)
+		if (crypto_memneq(virt_ctx->mac_res, gcmData->macResOut, gcmData->tagSize))
 		{
 			FIPS_LOG("mac_res comparison error %d - mac_size=%d \n", i, gcmData->tagSize);
 			FIPS_LOG("  i  expected   received \n");
-- 
2.13.1

[toc] | [prev] | [next] | [standalone]

#1662896 — Re: [PATCH 3/6] ccree: use constant time memory comparison for macs and tags

Thank you Jason,

I think what you are doing is very important.

On Sat, Jun 10, 2017 at 5:59 AM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> Otherwise, we enable several different forgeries via timing attack.
>
> While the C inside this file is nearly incomprehensible, I did notice a
> high volume of "FIPS" and "NIST", which makes this kind of bug slightly
> more embarrassing.
>

The code you are referring to implements, as the function name states,
FIPS power up tests[*].
Specifically, this is the code that compares computed results to known
good results.

As far as I understand the purpose of timing and memory side channel
attacks is to deduce
key material by measurement of time and/or memory usage. However, this
being a FIPS power
up test, the key material is actually part of the source code, so not
much use here.

So, unless I've missed something, I'm going to NAK this one. Your
patch however did inspire me
to look in the ccree driver for other places where not using these
mechanisms is more dangerous,
so thank you for that.

[*] whose implementation inside the driver itself is questionable and
will probably go away as part
of staging clean-ups.

Thanks,
Gilad


> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Cc: Gilad Ben-Yossef <gilad@benyossef.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: stable@vger.kernel.org
> ---
>  drivers/staging/ccree/ssi_fips_ll.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/ccree/ssi_fips_ll.c b/drivers/staging/ccree/ssi_fips_ll.c
> index d573574bbb98..3310997d8e3e 100644
> --- a/drivers/staging/ccree/ssi_fips_ll.c
> +++ b/drivers/staging/ccree/ssi_fips_ll.c
> @@ -19,6 +19,7 @@ This file defines the driver FIPS Low Level implmentaion functions,
>  that executes the KAT.
>  ***************************************************************/
>  #include <linux/kernel.h>
> +#include <crypto/algapi.h>
>
>  #include "ssi_driver.h"
>  #include "ssi_fips_local.h"
> @@ -462,7 +463,7 @@ ssi_cipher_fips_power_up_tests(struct ssi_drvdata *drvdata, void *cpu_addr_buffe
>                 }
>
>                 /* compare actual dout to expected */
> -               if (memcmp(virt_ctx->dout, cipherData->dataOut, cipherData->dataInSize) != 0)
> +               if (crypto_memneq(virt_ctx->dout, cipherData->dataOut, cipherData->dataInSize))
>                 {
>                         FIPS_LOG("dout comparison error %d - oprMode=%d, isAes=%d\n", i, cipherData->oprMode, cipherData->isAes);
>                         FIPS_LOG("  i  expected   received \n");
> @@ -586,7 +587,7 @@ ssi_cmac_fips_power_up_tests(struct ssi_drvdata *drvdata, void *cpu_addr_buffer,
>                 }
>
>                 /* compare actual mac result to expected */
> -               if (memcmp(virt_ctx->mac_res, cmac_data->mac_res, cmac_data->mac_res_size) != 0)
> +               if (crypto_memneq(virt_ctx->mac_res, cmac_data->mac_res, cmac_data->mac_res_size))
>                 {
>                         FIPS_LOG("comparison error %d - digest_size=%d \n", i, cmac_data->mac_res_size);
>                         FIPS_LOG("  i  expected   received \n");
> @@ -760,7 +761,7 @@ ssi_hash_fips_power_up_tests(struct ssi_drvdata *drvdata, void *cpu_addr_buffer,
>                  }
>
>                 /* compare actual mac result to expected */
> -               if (memcmp(virt_ctx->mac_res, hash_data->mac_res, digest_size) != 0)
> +               if (crypto_memneq(virt_ctx->mac_res, hash_data->mac_res, digest_size))
>                 {
>                         FIPS_LOG("comparison error %d - hash_mode=%d digest_size=%d \n", i, hash_data->hash_mode, digest_size);
>                         FIPS_LOG("  i  expected   received \n");
> @@ -1093,7 +1094,7 @@ ssi_hmac_fips_power_up_tests(struct ssi_drvdata *drvdata, void *cpu_addr_buffer,
>                 }
>
>                 /* compare actual mac result to expected */
> -               if (memcmp(virt_ctx->mac_res, hmac_data->mac_res, digest_size) != 0)
> +               if (crypto_memneq(virt_ctx->mac_res, hmac_data->mac_res, digest_size))
>                 {
>                         FIPS_LOG("comparison error %d - hash_mode=%d digest_size=%d \n", i, hmac_data->hash_mode, digest_size);
>                         FIPS_LOG("  i  expected   received \n");
> @@ -1310,7 +1311,7 @@ ssi_ccm_fips_power_up_tests(struct ssi_drvdata *drvdata, void *cpu_addr_buffer,
>                 }
>
>                 /* compare actual dout to expected */
> -               if (memcmp(virt_ctx->dout, ccmData->dataOut, ccmData->dataInSize) != 0)
> +               if (crypto_memneq(virt_ctx->dout, ccmData->dataOut, ccmData->dataInSize))
>                 {
>                         FIPS_LOG("dout comparison error %d - size=%d \n", i, ccmData->dataInSize);
>                          error = CC_REE_FIPS_ERROR_AESCCM_PUT;
> @@ -1318,7 +1319,7 @@ ssi_ccm_fips_power_up_tests(struct ssi_drvdata *drvdata, void *cpu_addr_buffer,
>                  }
>
>                 /* compare actual mac result to expected */
> -               if (memcmp(virt_ctx->mac_res, ccmData->macResOut, ccmData->tagSize) != 0)
> +               if (crypto_memneq(virt_ctx->mac_res, ccmData->macResOut, ccmData->tagSize))
>                 {
>                         FIPS_LOG("mac_res comparison error %d - mac_size=%d \n", i, ccmData->tagSize);
>                         FIPS_LOG("  i  expected   received \n");
> @@ -1633,7 +1634,7 @@ ssi_gcm_fips_power_up_tests(struct ssi_drvdata *drvdata, void *cpu_addr_buffer,
>
>                 if (gcmData->direction == DRV_CRYPTO_DIRECTION_ENCRYPT) {
>                         /* compare actual dout to expected */
> -                       if (memcmp(virt_ctx->dout, gcmData->dataOut, gcmData->dataInSize) != 0)
> +                       if (crypto_memneq(virt_ctx->dout, gcmData->dataOut, gcmData->dataInSize))
>                         {
>                                 FIPS_LOG("dout comparison error %d - size=%d \n", i, gcmData->dataInSize);
>                                 FIPS_LOG("  i  expected   received \n");
> @@ -1649,7 +1650,7 @@ ssi_gcm_fips_power_up_tests(struct ssi_drvdata *drvdata, void *cpu_addr_buffer,
>                 }
>
>                 /* compare actual mac result to expected */
> -               if (memcmp(virt_ctx->mac_res, gcmData->macResOut, gcmData->tagSize) != 0)
> +               if (crypto_memneq(virt_ctx->mac_res, gcmData->macResOut, gcmData->tagSize))
>                 {
>                         FIPS_LOG("mac_res comparison error %d - mac_size=%d \n", i, gcmData->tagSize);
>                         FIPS_LOG("  i  expected   received \n");
> --
> 2.13.1
>



-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

[toc] | [prev] | [next] | [standalone]

#1662936 — Re: [PATCH 3/6] ccree: use constant time memory comparison for macs and tags

Hey Gilad,

That's fine. As I mentioned, I really have no clue what this code's
trying to do. If this is just part of some test that doesn't deal with
actual messages that could be forged, then of course there's nothing
that needs to be done and this can be NAKd.

Jason

[toc] | [prev] | [next] | [standalone]

#1663012 — Re: [PATCH 3/6] ccree: use constant time memory comparison for macs and tags

On Sat, 10 Jun 2017, Jason A. Donenfeld wrote:
> That's fine. As I mentioned, I really have no clue what this code's
> trying to do. If this is just part of some test that doesn't deal with
> actual messages that could be forged, then of course there's nothing
> that needs to be done and this can be NAKd.

Well, it is *testing* things, so you might want to use whatever the
module(s) will actually use.  Maybe test with both?

-- 
  Henrique Holschuh

[toc] | [prev] | [next] | [standalone]

#1662854 — [PATCH 2/6] net/ipv6: use constant time memory comparison for mac

Otherwise, we enable a MAC forgery via timing attack.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: stable@vger.kernel.org
---
 net/ipv6/seg6_hmac.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/seg6_hmac.c b/net/ipv6/seg6_hmac.c
index f950cb53d5e3..54213c83b44e 100644
--- a/net/ipv6/seg6_hmac.c
+++ b/net/ipv6/seg6_hmac.c
@@ -38,6 +38,7 @@
 #include <net/xfrm.h>
 
 #include <linux/cryptohash.h>
+#include <crypto/algapi.h>
 #include <crypto/hash.h>
 #include <crypto/sha.h>
 #include <net/seg6.h>
@@ -274,7 +275,7 @@ bool seg6_hmac_validate_skb(struct sk_buff *skb)
 	if (seg6_hmac_compute(hinfo, srh, &ipv6_hdr(skb)->saddr, hmac_output))
 		return false;
 
-	if (memcmp(hmac_output, tlv->hmac, SEG6_HMAC_FIELD_LEN) != 0)
+	if (crypto_memneq(hmac_output, tlv->hmac, SEG6_HMAC_FIELD_LEN))
 		return false;
 
 	return true;
-- 
2.13.1

[toc] | [prev] | [next] | [standalone]

#1662856 — [PATCH 4/6] security/keys: use constant time memory comparison for macs

Otherwise, we enable a MAC forgery via timing attack.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: David Safford <safford@us.ibm.com>
Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: David Howells <dhowells@redhat.com>
Cc: keyrings@vger.kernel.org
Cc: stable@vger.kernel.org
---
 security/keys/trusted.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index 2ae31c5a87de..df7d30b0a6f7 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -11,6 +11,7 @@
  * See Documentation/security/keys-trusted-encrypted.txt
  */
 
+#include <crypto/algapi.h>
 #include <crypto/hash_info.h>
 #include <linux/uaccess.h>
 #include <linux/module.h>
@@ -243,7 +244,7 @@ static int TSS_checkhmac1(unsigned char *buffer,
 	if (ret < 0)
 		goto out;
 
-	if (memcmp(testhmac, authdata, SHA1_DIGEST_SIZE))
+	if (crypto_memneq(testhmac, authdata, SHA1_DIGEST_SIZE))
 		ret = -EINVAL;
 out:
 	kfree(sdesc);
@@ -335,7 +336,7 @@ static int TSS_checkhmac2(unsigned char *buffer,
 			  TPM_NONCE_SIZE, ononce, 1, continueflag1, 0, 0);
 	if (ret < 0)
 		goto out;
-	if (memcmp(testhmac1, authdata1, SHA1_DIGEST_SIZE)) {
+	if (crypto_memneq(testhmac1, authdata1, SHA1_DIGEST_SIZE)) {
 		ret = -EINVAL;
 		goto out;
 	}
@@ -344,7 +345,7 @@ static int TSS_checkhmac2(unsigned char *buffer,
 			  TPM_NONCE_SIZE, ononce, 1, continueflag2, 0, 0);
 	if (ret < 0)
 		goto out;
-	if (memcmp(testhmac2, authdata2, SHA1_DIGEST_SIZE))
+	if (crypto_memneq(testhmac2, authdata2, SHA1_DIGEST_SIZE))
 		ret = -EINVAL;
 out:
 	kfree(sdesc);
-- 
2.13.1

[toc] | [prev] | [next] | [standalone]

#1665588 — Re: [kernel-hardening] [PATCH 4/6] security/keys: use constant time memory comparison for macs

On Sat, 10 Jun 2017, Jason A. Donenfeld wrote:

> Otherwise, we enable a MAC forgery via timing attack.
> 
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Cc: David Safford <safford@us.ibm.com>
> Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
> Cc: David Howells <dhowells@redhat.com>
> Cc: keyrings@vger.kernel.org
> Cc: stable@vger.kernel.org


Reviewed-by: James Morris <james.l.morris@oracle.com>


-- 
James Morris
<jmorris@namei.org>

[toc] | [prev] | [next] | [standalone]

#1662857 — [PATCH 1/6] sunrpc: use constant time memory comparison for mac

Otherwise, we enable a MAC forgery via timing attack.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: "J. Bruce Fields" <bfields@fieldses.org>
Cc: Jeff Layton <jlayton@poochiereds.net>
Cc: Trond Myklebust <trond.myklebust@primarydata.com>
Cc: Anna Schumaker <anna.schumaker@netapp.com>
Cc: linux-nfs@vger.kernel.org
Cc: stable@vger.kernel.org
---
 net/sunrpc/auth_gss/gss_krb5_crypto.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c
index fb39284ec174..12649c9fedab 100644
--- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
+++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
@@ -34,6 +34,7 @@
  * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR PURPOSE.
  */
 
+#include <crypto/algapi.h>
 #include <crypto/hash.h>
 #include <crypto/skcipher.h>
 #include <linux/err.h>
@@ -927,7 +928,7 @@ gss_krb5_aes_decrypt(struct krb5_ctx *kctx, u32 offset, struct xdr_buf *buf,
 	if (ret)
 		goto out_err;
 
-	if (memcmp(pkt_hmac, our_hmac, kctx->gk5e->cksumlength) != 0) {
+	if (crypto_memneq(pkt_hmac, our_hmac, kctx->gk5e->cksumlength) != 0) {
 		ret = GSS_S_BAD_SIG;
 		goto out_err;
 	}
-- 
2.13.1

[toc] | [prev] | [next] | [standalone]

#1662858 — [PATCH 5/6] bluetooth/smp: use constant time memory comparison for secret values

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 <Jason@zx2c4.com>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: Johan Hedberg <johan.hedberg@gmail.com>
Cc: linux-bluetooth@vger.kernel.org
Cc: stable@vger.kernel.org
---
 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

[toc] | [prev] | [next] | [standalone]

#1662957 — Re: [PATCH 5/6] bluetooth/smp: use constant time memory comparison for secret values

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 <Jason@zx2c4.com>
> Cc: Marcel Holtmann <marcel@holtmann.org>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: Johan Hedberg <johan.hedberg@gmail.com>
> Cc: linux-bluetooth@vger.kernel.org
> Cc: stable@vger.kernel.org
> ---
> net/bluetooth/smp.c | 39 ++++++++++++++++++++-------------------
> 1 file changed, 20 insertions(+), 19 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

[toc] | [prev] | [next] | [standalone]

#1662859 — [PATCH 6/6] mac80211/wpa: use constant time memory comparison for MACs

Otherwise, we enable all sorts of forgeries via timing attack.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: linux-wireless@vger.kernel.org
Cc: stable@vger.kernel.org
---
 net/mac80211/wpa.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/mac80211/wpa.c b/net/mac80211/wpa.c
index c1ef22df865f..cc19614ff4e6 100644
--- a/net/mac80211/wpa.c
+++ b/net/mac80211/wpa.c
@@ -17,6 +17,7 @@
 #include <asm/unaligned.h>
 #include <net/mac80211.h>
 #include <crypto/aes.h>
+#include <crypto/algapi.h>
 
 #include "ieee80211_i.h"
 #include "michael.h"
@@ -153,7 +154,7 @@ ieee80211_rx_h_michael_mic_verify(struct ieee80211_rx_data *rx)
 	data_len = skb->len - hdrlen - MICHAEL_MIC_LEN;
 	key = &rx->key->conf.key[NL80211_TKIP_DATA_OFFSET_RX_MIC_KEY];
 	michael_mic(key, hdr, data, data_len, mic);
-	if (memcmp(mic, data + data_len, MICHAEL_MIC_LEN) != 0)
+	if (crypto_memneq(mic, data + data_len, MICHAEL_MIC_LEN))
 		goto mic_fail;
 
 	/* remove Michael MIC from payload */
@@ -1048,7 +1049,7 @@ ieee80211_crypto_aes_cmac_decrypt(struct ieee80211_rx_data *rx)
 		bip_aad(skb, aad);
 		ieee80211_aes_cmac(key->u.aes_cmac.tfm, aad,
 				   skb->data + 24, skb->len - 24, mic);
-		if (memcmp(mic, mmie->mic, sizeof(mmie->mic)) != 0) {
+		if (crypto_memneq(mic, mmie->mic, sizeof(mmie->mic))) {
 			key->u.aes_cmac.icverrors++;
 			return RX_DROP_UNUSABLE;
 		}
@@ -1098,7 +1099,7 @@ ieee80211_crypto_aes_cmac_256_decrypt(struct ieee80211_rx_data *rx)
 		bip_aad(skb, aad);
 		ieee80211_aes_cmac_256(key->u.aes_cmac.tfm, aad,
 				       skb->data + 24, skb->len - 24, mic);
-		if (memcmp(mic, mmie->mic, sizeof(mmie->mic)) != 0) {
+		if (crypto_memneq(mic, mmie->mic, sizeof(mmie->mic))) {
 			key->u.aes_cmac.icverrors++;
 			return RX_DROP_UNUSABLE;
 		}
@@ -1202,7 +1203,7 @@ ieee80211_crypto_aes_gmac_decrypt(struct ieee80211_rx_data *rx)
 		if (ieee80211_aes_gmac(key->u.aes_gmac.tfm, aad, nonce,
 				       skb->data + 24, skb->len - 24,
 				       mic) < 0 ||
-		    memcmp(mic, mmie->mic, sizeof(mmie->mic)) != 0) {
+		    crypto_memneq(mic, mmie->mic, sizeof(mmie->mic))) {
 			key->u.aes_gmac.icverrors++;
 			return RX_DROP_UNUSABLE;
 		}
-- 
2.13.1

[toc] | [prev] | [next] | [standalone]

#1664592 — Re: [PATCH 6/6] mac80211/wpa: use constant time memory comparison for MACs

On Sat, 2017-06-10 at 04:59 +0200, Jason A. Donenfeld wrote:
> Otherwise, we enable all sorts of forgeries via timing attack.

I'm not really sure that this is actually true, since you don't get
much feedback on your frame that's dropped, especially if you're
attacking from remote. Basically, I don't see how you can observe the
timing of this operation?

Anyway, applied.

johannes

[toc] | [prev] | [next] | [standalone]

#1664850 — Re: [PATCH 6/6] mac80211/wpa: use constant time memory comparison for MACs

On Tue, Jun 13, 2017 at 10:20 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> I'm not really sure that this is actually true, since you don't get
> much feedback on your frame that's dropped, especially if you're
> attacking from remote. Basically, I don't see how you can observe the
> timing of this operation?

There have been practical attacks published before that relied on
jitter coming from simultaneous operations.

> Anyway, applied.

Great, thanks.

[toc] | [prev] | [next] | [standalone]

#1663075

"Jason A. Donenfeld" <Jason@zx2c4.com> 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

[toc] | [prev] | [next] | [standalone]

#1663103

On Sun, Jun 11, 2017 at 1:13 AM, Kalle Valo <kvalo@codeaurora.org> wrote:
> "Jason A. Donenfeld" <Jason@zx2c4.com> 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

[toc] | [prev] | [next] | [standalone]

#1663146

On Sun, Jun 11, 2017 at 4:36 PM, Kees Cook <keescook@chromium.org> wrote:
>
> On Sun, Jun 11, 2017 at 1:13 AM, Kalle Valo <kvalo@codeaurora.org> wrote:
> > "Jason A. Donenfeld" <Jason@zx2c4.com> 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.

[toc] | [prev] | [next] | [standalone]

#1663155

2017-06-11 22:48 GMT+02:00 Emmanuel Grumbach <egrumbach@gmail.com>:
> On Sun, Jun 11, 2017 at 4:36 PM, Kees Cook <keescook@chromium.org> wrote:
>>
>> On Sun, Jun 11, 2017 at 1:13 AM, Kalle Valo <kvalo@codeaurora.org> wrote:
>> > "Jason A. Donenfeld" <Jason@zx2c4.com> 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.

[toc] | [prev] | [next] | [standalone]

#1663245

On Mon, Jun 12, 2017 at 12:30 AM, Emil Lenngren <emil.lenngren@gmail.com> wrote:
> 2017-06-11 22:48 GMT+02:00 Emmanuel Grumbach <egrumbach@gmail.com>:
>> On Sun, Jun 11, 2017 at 4:36 PM, Kees Cook <keescook@chromium.org> wrote:
>>>
>>> On Sun, Jun 11, 2017 at 1:13 AM, Kalle Valo <kvalo@codeaurora.org> wrote:
>>> > "Jason A. Donenfeld" <Jason@zx2c4.com> 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.

[toc] | [prev] | [next] | [standalone]

#1663290

On 6/11/2017 11:30 PM, Emil Lenngren wrote:
> 2017-06-11 22:48 GMT+02:00 Emmanuel Grumbach <egrumbach@gmail.com>:
>> On Sun, Jun 11, 2017 at 4:36 PM, Kees Cook <keescook@chromium.org> wrote:
>>>
>>> On Sun, Jun 11, 2017 at 1:13 AM, Kalle Valo <kvalo@codeaurora.org> wrote:
>>>> "Jason A. Donenfeld" <Jason@zx2c4.com> 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

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web