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

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

[Jason A. Donenfeld]

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

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

[Jason A. Donenfeld]

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

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

[Kalle Valo]

"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

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

[Kees Cook]

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

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

[Emmanuel Grumbach]

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.

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

[Emil Lenngren]

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.

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

[Emmanuel Grumbach]

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.

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

[Arend van Spriel]

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

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

[Stephan Müller]

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

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

[Jason A. Donenfeld]

Hi Stephan,

On Sun, Jun 11, 2017 at 11:06 PM, Stephan M=C3=BCller <smueller@chronox.de>=
 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