* [PATCH v2] crypto: memzero_explicit - make sure to clear out sensitive data
@ 2014-09-07 21:23 Daniel Borkmann
2014-09-08 6:17 ` Hannes Frederic Sowa
2014-09-15 11:52 ` Herbert Xu
0 siblings, 2 replies; 6+ messages in thread
From: Daniel Borkmann @ 2014-09-07 21:23 UTC (permalink / raw)
To: herbert; +Cc: tytso, hannes, linux-crypto, gmazyland, Julia Lawall
Recently, in commit 13aa93c70e71 ("random: add and use memzero_explicit()
for clearing data"), we have found that GCC may optimize some memset()
cases away when it detects a stack variable is not being used anymore
and going out of scope. This can happen, for example, in cases when we
are clearing out sensitive information such as keying material or any
e.g. intermediate results from crypto computations, etc.
With the help of Coccinelle, we can figure out and fix such occurences
in the crypto subsytem as well. Julia Lawall provided the following
Coccinelle program:
@@
type T;
identifier x;
@@
T x;
... when exists
when any
-memset
+memzero_explicit
(&x,
-0,
...)
... when != x
when strict
@@
type T;
identifier x;
@@
T x[...];
... when exists
when any
-memset
+memzero_explicit
(x,
-0,
...)
... when != x
when strict
Therefore, make use of the drop-in replacement memzero_explicit() for
exactly such cases instead of using memset().
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Cc: Julia Lawall <julia.lawall@lip6.fr>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
v1 -> v2:
- There was one more non-array case in vmac.c
Herbert, Ted, I have posted my original patch that introduces
memzero_explicit() infrastructure along with its first users
that fixes such occurences to Ted's random tree [1].
That means, this one last patch on that regard would be on
top of it as an outcome of an audit in the crypto subsystem.
Would the two of you be okay, if Ted takes it to random as
well (since the random fix is not yet in mainline)? Changes
are minimal and rather uncomplicated, but I think it would be
quite important to get fixed. Thanks!
[1] https://git.kernel.org/cgit/linux/kernel/git/tytso/random.git/commit/?h=dev&id=13aa93c70e716b597d2451abee5e9f0b140b831c
crypto/cts.c | 3 ++-
crypto/sha1_generic.c | 2 +-
crypto/sha256_generic.c | 5 ++---
crypto/sha512_generic.c | 2 +-
crypto/tgr192.c | 4 ++--
crypto/vmac.c | 2 +-
crypto/wp512.c | 8 ++++----
7 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/crypto/cts.c b/crypto/cts.c
index 042223f..133f087 100644
--- a/crypto/cts.c
+++ b/crypto/cts.c
@@ -202,7 +202,8 @@ static int cts_cbc_decrypt(struct crypto_cts_ctx *ctx,
/* 5. Append the tail (BB - Ln) bytes of Xn (tmp) to Cn to create En */
memcpy(s + bsize + lastn, tmp + lastn, bsize - lastn);
/* 6. Decrypt En to create Pn-1 */
- memset(iv, 0, sizeof(iv));
+ memzero_explicit(iv, sizeof(iv));
+
sg_set_buf(&sgsrc[0], s + bsize, bsize);
sg_set_buf(&sgdst[0], d, bsize);
err = crypto_blkcipher_decrypt_iv(&lcldesc, sgdst, sgsrc, bsize);
diff --git a/crypto/sha1_generic.c b/crypto/sha1_generic.c
index 4279480..7bb0474 100644
--- a/crypto/sha1_generic.c
+++ b/crypto/sha1_generic.c
@@ -64,7 +64,7 @@ int crypto_sha1_update(struct shash_desc *desc, const u8 *data,
src = data + done;
} while (done + SHA1_BLOCK_SIZE <= len);
- memset(temp, 0, sizeof(temp));
+ memzero_explicit(temp, sizeof(temp));
partial = 0;
}
memcpy(sctx->buffer + partial, src, len - done);
diff --git a/crypto/sha256_generic.c b/crypto/sha256_generic.c
index 5433667..32c5e5e 100644
--- a/crypto/sha256_generic.c
+++ b/crypto/sha256_generic.c
@@ -210,10 +210,9 @@ static void sha256_transform(u32 *state, const u8 *input)
/* clear any sensitive info... */
a = b = c = d = e = f = g = h = t1 = t2 = 0;
- memset(W, 0, 64 * sizeof(u32));
+ memzero_explicit(W, 64 * sizeof(u32));
}
-
static int sha224_init(struct shash_desc *desc)
{
struct sha256_state *sctx = shash_desc_ctx(desc);
@@ -316,7 +315,7 @@ static int sha224_final(struct shash_desc *desc, u8 *hash)
sha256_final(desc, D);
memcpy(hash, D, SHA224_DIGEST_SIZE);
- memset(D, 0, SHA256_DIGEST_SIZE);
+ memzero_explicit(D, SHA256_DIGEST_SIZE);
return 0;
}
diff --git a/crypto/sha512_generic.c b/crypto/sha512_generic.c
index 6ed124f..04d295a 100644
--- a/crypto/sha512_generic.c
+++ b/crypto/sha512_generic.c
@@ -238,7 +238,7 @@ static int sha384_final(struct shash_desc *desc, u8 *hash)
sha512_final(desc, D);
memcpy(hash, D, 48);
- memset(D, 0, 64);
+ memzero_explicit(D, 64);
return 0;
}
diff --git a/crypto/tgr192.c b/crypto/tgr192.c
index 8740355..3c7af0d 100644
--- a/crypto/tgr192.c
+++ b/crypto/tgr192.c
@@ -612,7 +612,7 @@ static int tgr160_final(struct shash_desc *desc, u8 * out)
tgr192_final(desc, D);
memcpy(out, D, TGR160_DIGEST_SIZE);
- memset(D, 0, TGR192_DIGEST_SIZE);
+ memzero_explicit(D, TGR192_DIGEST_SIZE);
return 0;
}
@@ -623,7 +623,7 @@ static int tgr128_final(struct shash_desc *desc, u8 * out)
tgr192_final(desc, D);
memcpy(out, D, TGR128_DIGEST_SIZE);
- memset(D, 0, TGR192_DIGEST_SIZE);
+ memzero_explicit(D, TGR192_DIGEST_SIZE);
return 0;
}
diff --git a/crypto/vmac.c b/crypto/vmac.c
index 2eb11a3..d84c24b 100644
--- a/crypto/vmac.c
+++ b/crypto/vmac.c
@@ -613,7 +613,7 @@ static int vmac_final(struct shash_desc *pdesc, u8 *out)
}
mac = vmac(ctx->partial, ctx->partial_size, nonce, NULL, ctx);
memcpy(out, &mac, sizeof(vmac_t));
- memset(&mac, 0, sizeof(vmac_t));
+ memzero_explicit(&mac, sizeof(vmac_t));
memset(&ctx->__vmac_ctx, 0, sizeof(struct vmac_ctx));
ctx->partial_size = 0;
return 0;
diff --git a/crypto/wp512.c b/crypto/wp512.c
index 180f1d6..ec64e77 100644
--- a/crypto/wp512.c
+++ b/crypto/wp512.c
@@ -1102,8 +1102,8 @@ static int wp384_final(struct shash_desc *desc, u8 *out)
u8 D[64];
wp512_final(desc, D);
- memcpy (out, D, WP384_DIGEST_SIZE);
- memset (D, 0, WP512_DIGEST_SIZE);
+ memcpy(out, D, WP384_DIGEST_SIZE);
+ memzero_explicit(D, WP512_DIGEST_SIZE);
return 0;
}
@@ -1113,8 +1113,8 @@ static int wp256_final(struct shash_desc *desc, u8 *out)
u8 D[64];
wp512_final(desc, D);
- memcpy (out, D, WP256_DIGEST_SIZE);
- memset (D, 0, WP512_DIGEST_SIZE);
+ memcpy(out, D, WP256_DIGEST_SIZE);
+ memzero_explicit(D, WP512_DIGEST_SIZE);
return 0;
}
--
1.7.11.7
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] crypto: memzero_explicit - make sure to clear out sensitive data
2014-09-07 21:23 [PATCH v2] crypto: memzero_explicit - make sure to clear out sensitive data Daniel Borkmann
@ 2014-09-08 6:17 ` Hannes Frederic Sowa
2014-09-08 7:00 ` Julia Lawall
2014-09-15 11:52 ` Herbert Xu
1 sibling, 1 reply; 6+ messages in thread
From: Hannes Frederic Sowa @ 2014-09-08 6:17 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: herbert, tytso, linux-crypto, gmazyland, Julia Lawall
On So, 2014-09-07 at 23:23 +0200, Daniel Borkmann wrote:
> Recently, in commit 13aa93c70e71 ("random: add and use memzero_explicit()
> for clearing data"), we have found that GCC may optimize some memset()
> cases away when it detects a stack variable is not being used anymore
> and going out of scope. This can happen, for example, in cases when we
> are clearing out sensitive information such as keying material or any
> e.g. intermediate results from crypto computations, etc.
>
> With the help of Coccinelle, we can figure out and fix such occurences
> in the crypto subsytem as well. Julia Lawall provided the following
> Coccinelle program:
>
> @@
> type T;
> identifier x;
> @@
>
> T x;
> ... when exists
> when any
> -memset
> +memzero_explicit
> (&x,
> -0,
> ...)
> ... when != x
> when strict
>
> @@
> type T;
> identifier x;
> @@
>
> T x[...];
> ... when exists
> when any
> -memset
> +memzero_explicit
> (x,
> -0,
> ...)
> ... when != x
> when strict
I think this Coccinelle patch won't make it completely unnecessary for a
manual audit as it does not take optimizations (dead code eliminitation)
into account?
>
> Therefore, make use of the drop-in replacement memzero_explicit() for
> exactly such cases instead of using memset().
>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Cc: Julia Lawall <julia.lawall@lip6.fr>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org
Thanks,
Hannes
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] crypto: memzero_explicit - make sure to clear out sensitive data
2014-09-08 6:17 ` Hannes Frederic Sowa
@ 2014-09-08 7:00 ` Julia Lawall
2014-09-08 7:59 ` Daniel Borkmann
0 siblings, 1 reply; 6+ messages in thread
From: Julia Lawall @ 2014-09-08 7:00 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: Daniel Borkmann, herbert, tytso, linux-crypto, gmazyland
On Mon, 8 Sep 2014, Hannes Frederic Sowa wrote:
> On So, 2014-09-07 at 23:23 +0200, Daniel Borkmann wrote:
> > Recently, in commit 13aa93c70e71 ("random: add and use memzero_explicit()
> > for clearing data"), we have found that GCC may optimize some memset()
> > cases away when it detects a stack variable is not being used anymore
> > and going out of scope. This can happen, for example, in cases when we
> > are clearing out sensitive information such as keying material or any
> > e.g. intermediate results from crypto computations, etc.
> >
> > With the help of Coccinelle, we can figure out and fix such occurences
> > in the crypto subsytem as well. Julia Lawall provided the following
> > Coccinelle program:
> >
> > @@
> > type T;
> > identifier x;
> > @@
> >
> > T x;
> > ... when exists
> > when any
> > -memset
> > +memzero_explicit
> > (&x,
> > -0,
> > ...)
> > ... when != x
> > when strict
> >
> > @@
> > type T;
> > identifier x;
> > @@
> >
> > T x[...];
> > ... when exists
> > when any
> > -memset
> > +memzero_explicit
> > (x,
> > -0,
> > ...)
> > ... when != x
> > when strict
>
> I think this Coccinelle patch won't make it completely unnecessary for a
> manual audit as it does not take optimizations (dead code eliminitation)
> into account?
I agree. If you see something else that could be taken into account in
the semantic patch, please let me know.
julia
>
> >
> > Therefore, make use of the drop-in replacement memzero_explicit() for
> > exactly such cases instead of using memset().
> >
> > Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> > Cc: Julia Lawall <julia.lawall@lip6.fr>
> > Cc: Herbert Xu <herbert@gondor.apana.org.au>
> > Cc: Theodore Ts'o <tytso@mit.edu>
> > Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
>
> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org
>
> Thanks,
> Hannes
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] crypto: memzero_explicit - make sure to clear out sensitive data
2014-09-08 7:00 ` Julia Lawall
@ 2014-09-08 7:59 ` Daniel Borkmann
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2014-09-08 7:59 UTC (permalink / raw)
To: Julia Lawall
Cc: Hannes Frederic Sowa, herbert, tytso, linux-crypto, gmazyland
On 09/08/2014 09:00 AM, Julia Lawall wrote:
> On Mon, 8 Sep 2014, Hannes Frederic Sowa wrote:
>> On So, 2014-09-07 at 23:23 +0200, Daniel Borkmann wrote:
>>> Recently, in commit 13aa93c70e71 ("random: add and use memzero_explicit()
>>> for clearing data"), we have found that GCC may optimize some memset()
>>> cases away when it detects a stack variable is not being used anymore
>>> and going out of scope. This can happen, for example, in cases when we
>>> are clearing out sensitive information such as keying material or any
>>> e.g. intermediate results from crypto computations, etc.
>>>
>>> With the help of Coccinelle, we can figure out and fix such occurences
>>> in the crypto subsytem as well. Julia Lawall provided the following
>>> Coccinelle program:
>>>
>>> @@
>>> type T;
>>> identifier x;
>>> @@
>>>
>>> T x;
>>> ... when exists
>>> when any
>>> -memset
>>> +memzero_explicit
>>> (&x,
>>> -0,
>>> ...)
>>> ... when != x
>>> when strict
>>>
>>> @@
>>> type T;
>>> identifier x;
>>> @@
>>>
>>> T x[...];
>>> ... when exists
>>> when any
>>> -memset
>>> +memzero_explicit
>>> (x,
>>> -0,
>>> ...)
>>> ... when != x
>>> when strict
>>
>> I think this Coccinelle patch won't make it completely unnecessary for a
>> manual audit as it does not take optimizations (dead code eliminitation)
>> into account?
>
> I agree. If you see something else that could be taken into account in
> the semantic patch, please let me know.
Agreed, the code would fix the obvious candidates that would need
memzero_explicit() though. It also found the already fixed spots for
the random driver when I reran the script in that area, so I think
it's a good starting point nevertheless.
...
>>> Therefore, make use of the drop-in replacement memzero_explicit() for
>>> exactly such cases instead of using memset().
>>>
>>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>>> Cc: Julia Lawall <julia.lawall@lip6.fr>
>>> Cc: Herbert Xu <herbert@gondor.apana.org.au>
>>> Cc: Theodore Ts'o <tytso@mit.edu>
>>> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
>>
>> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org
>>
>> Thanks,
>> Hannes
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] crypto: memzero_explicit - make sure to clear out sensitive data
2014-09-07 21:23 [PATCH v2] crypto: memzero_explicit - make sure to clear out sensitive data Daniel Borkmann
2014-09-08 6:17 ` Hannes Frederic Sowa
@ 2014-09-15 11:52 ` Herbert Xu
2014-09-25 20:23 ` Daniel Borkmann
1 sibling, 1 reply; 6+ messages in thread
From: Herbert Xu @ 2014-09-15 11:52 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: tytso, hannes, linux-crypto, gmazyland, Julia Lawall
On Sun, Sep 07, 2014 at 11:23:38PM +0200, Daniel Borkmann wrote:
> Recently, in commit 13aa93c70e71 ("random: add and use memzero_explicit()
> for clearing data"), we have found that GCC may optimize some memset()
> cases away when it detects a stack variable is not being used anymore
> and going out of scope. This can happen, for example, in cases when we
> are clearing out sensitive information such as keying material or any
> e.g. intermediate results from crypto computations, etc.
>
> With the help of Coccinelle, we can figure out and fix such occurences
> in the crypto subsytem as well. Julia Lawall provided the following
> Coccinelle program:
>
> @@
> type T;
> identifier x;
> @@
>
> T x;
> ... when exists
> when any
> -memset
> +memzero_explicit
> (&x,
> -0,
> ...)
> ... when != x
> when strict
>
> @@
> type T;
> identifier x;
> @@
>
> T x[...];
> ... when exists
> when any
> -memset
> +memzero_explicit
> (x,
> -0,
> ...)
> ... when != x
> when strict
>
> Therefore, make use of the drop-in replacement memzero_explicit() for
> exactly such cases instead of using memset().
>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Cc: Julia Lawall <julia.lawall@lip6.fr>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] crypto: memzero_explicit - make sure to clear out sensitive data
2014-09-15 11:52 ` Herbert Xu
@ 2014-09-25 20:23 ` Daniel Borkmann
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2014-09-25 20:23 UTC (permalink / raw)
To: tytso; +Cc: Herbert Xu, hannes, linux-crypto, gmazyland, Julia Lawall
Hi Ted,
On 09/15/2014 01:52 PM, Herbert Xu wrote:
> On Sun, Sep 07, 2014 at 11:23:38PM +0200, Daniel Borkmann wrote:
>> Recently, in commit 13aa93c70e71 ("random: add and use memzero_explicit()
>> for clearing data"), we have found that GCC may optimize some memset()
>> cases away when it detects a stack variable is not being used anymore
>> and going out of scope. This can happen, for example, in cases when we
>> are clearing out sensitive information such as keying material or any
>> e.g. intermediate results from crypto computations, etc.
>>
>> With the help of Coccinelle, we can figure out and fix such occurences
>> in the crypto subsytem as well. Julia Lawall provided the following
>> Coccinelle program:
>>
>> @@
>> type T;
>> identifier x;
>> @@
>>
>> T x;
>> ... when exists
>> when any
>> -memset
>> +memzero_explicit
>> (&x,
>> -0,
>> ...)
>> ... when != x
>> when strict
>>
>> @@
>> type T;
>> identifier x;
>> @@
>>
>> T x[...];
>> ... when exists
>> when any
>> -memset
>> +memzero_explicit
>> (x,
>> -0,
>> ...)
>> ... when != x
>> when strict
>>
>> Therefore, make use of the drop-in replacement memzero_explicit() for
>> exactly such cases instead of using memset().
>>
>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>> Cc: Julia Lawall <julia.lawall@lip6.fr>
>> Cc: Herbert Xu <herbert@gondor.apana.org.au>
>> Cc: Theodore Ts'o <tytso@mit.edu>
>> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
>
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> Thanks,
Sorry for the noise, but given Herbert acked the patch and in the random
tree the infrastructure is at current present to fix this security issue,
could you take it through random tree as originally proposed between the
"---" and diffstat line of this patch [1]?
Thanks a lot,
Daniel
[1] http://www.spinics.net/lists/linux-crypto/msg11965.html
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-09-25 20:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-07 21:23 [PATCH v2] crypto: memzero_explicit - make sure to clear out sensitive data Daniel Borkmann
2014-09-08 6:17 ` Hannes Frederic Sowa
2014-09-08 7:00 ` Julia Lawall
2014-09-08 7:59 ` Daniel Borkmann
2014-09-15 11:52 ` Herbert Xu
2014-09-25 20:23 ` Daniel Borkmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).