[PATCH] Use memzero_explicit to clear local buffers

[PATCH] Use memzero_explicit to clear local buffers

[Giel van Schijndel]

When leaving a function use memzero_explicit instead of memset(0) to
clear locally allocated/owned buffers. memset(0) may be optimized away.

All of the affected buffers contain sensitive data, key material or
derivatives of one of those two.
---
 arch/x86/crypto/sha256_ssse3_glue.c | 2 +-
 drivers/usb/wusbcore/security.c     | 2 +-
 fs/cifs/smbencrypt.c                | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/crypto/sha256_ssse3_glue.c b/arch/x86/crypto/sha256_ssse3_glue.c
index 8fad72f..b616e63 100644
--- a/arch/x86/crypto/sha256_ssse3_glue.c
+++ b/arch/x86/crypto/sha256_ssse3_glue.c
@@ -164,7 +164,7 @@ static int sha256_ssse3_final(struct shash_desc *desc, u8 *out)
 		dst[i] = cpu_to_be32(sctx->state[i]);
 
 	/* Wipe context */
-	memset(sctx, 0, sizeof(*sctx));
+	memzero_explicit(sctx, sizeof(*sctx));
 
 	return 0;
 }
diff --git a/drivers/usb/wusbcore/security.c b/drivers/usb/wusbcore/security.c
index b66faaf..a25f4fe 100644
--- a/drivers/usb/wusbcore/security.c
+++ b/drivers/usb/wusbcore/security.c
@@ -521,7 +521,7 @@ error_wusbhc_set_ptk:
 error_hs3:
 error_hs2:
 error_hs1:
-	memset(hs, 0, 3*sizeof(hs[0]));
+	memzero_explicit(hs, 3*sizeof(hs[0]));
 	memzero_explicit(&keydvt_out, sizeof(keydvt_out));
 	memzero_explicit(&keydvt_in, sizeof(keydvt_in));
 	memzero_explicit(&ccm_n, sizeof(ccm_n));
diff --git a/fs/cifs/smbencrypt.c b/fs/cifs/smbencrypt.c
index 6c15663..a4232ec 100644
--- a/fs/cifs/smbencrypt.c
+++ b/fs/cifs/smbencrypt.c
@@ -221,7 +221,7 @@ E_md4hash(const unsigned char *passwd, unsigned char *p16,
 	}
 
 	rc = mdfour(p16, (unsigned char *) wpwd, len * sizeof(__le16));
-	memset(wpwd, 0, 129 * sizeof(__le16));
+	memzero_explicit(wpwd, sizeof(wpwd));
 
 	return rc;
 }
-- 
2.1.4

Re: [PATCH] Use memzero_explicit to clear local buffers

[Herbert Xu]

On Sun, Jan 04, 2015 at 07:05:40PM +0100, Giel van Schijndel wrote:
> When leaving a function use memzero_explicit instead of memset(0) to
> clear locally allocated/owned buffers. memset(0) may be optimized away.
> 
> All of the affected buffers contain sensitive data, key material or
> derivatives of one of those two.

Nack.
 
> diff --git a/arch/x86/crypto/sha256_ssse3_glue.c b/arch/x86/crypto/sha256_ssse3_glue.c
> index 8fad72f..b616e63 100644
> --- a/arch/x86/crypto/sha256_ssse3_glue.c
> +++ b/arch/x86/crypto/sha256_ssse3_glue.c
> @@ -164,7 +164,7 @@ static int sha256_ssse3_final(struct shash_desc *desc, u8 *out)
>  		dst[i] = cpu_to_be32(sctx->state[i]);
>  
>  	/* Wipe context */
> -	memset(sctx, 0, sizeof(*sctx));
> +	memzero_explicit(sctx, sizeof(*sctx));

sctx does not point to stack memory so this is bogus.

Only stack memory cleared just before it goes out of scope needs
memzero_explicit.

Cheers,
-- 
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

Re: [PATCH] Use memzero_explicit to clear local buffers

[Giel van Schijndel]

[-- Attachment #1: Type: text/plain, Size: 2133 bytes --]

On Mon, Jan 05, 2015 at 08:35:38 +1100, Herbert Xu wrote:
> On Sun, Jan 04, 2015 at 07:05:40PM +0100, Giel van Schijndel wrote:
>> When leaving a function use memzero_explicit instead of memset(0) to
>> clear locally allocated/owned buffers. memset(0) may be optimized away.
>> 
>> All of the affected buffers contain sensitive data, key material or
>> derivatives of one of those two.
> 
> Nack.

Do you mean that the sample below doesn't contain sensitive data?
Or is there another buffer(s) in my patch that you believe doesn't
contain that?

(I contain a hash derived from secret material to be a "derivative of one of
those two", leaking of which could lead to a confirmation-attack).

>> diff --git a/arch/x86/crypto/sha256_ssse3_glue.c b/arch/x86/crypto/sha256_ssse3_glue.c
>> index 8fad72f..b616e63 100644
>> --- a/arch/x86/crypto/sha256_ssse3_glue.c
>> +++ b/arch/x86/crypto/sha256_ssse3_glue.c
>> @@ -164,7 +164,7 @@ static int sha256_ssse3_final(struct shash_desc *desc, u8 *out)
>>  		dst[i] = cpu_to_be32(sctx->state[i]);
>>  
>>  	/* Wipe context */
>> -	memset(sctx, 0, sizeof(*sctx));
>> +	memzero_explicit(sctx, sizeof(*sctx));
> 
> sctx does not point to stack memory so this is bogus.
> 
> Only stack memory cleared just before it goes out of scope needs
> memzero_explicit.

Is that because the compiler can't safely optimize memset(0) away for a
variable with greater-than-local scope?

Because I think using memzero_explicit() as an indicator that said
buffer contains data that really *needs* to be destroyed is enough of a
reason already. I believe any overhead is negligable because there's
only a single extra call involved and that's cheap for the extra clarity
it buys (i.e. "this piece of memory *really* needs to be destroyed
beyond this statement"). (Though this approach is only valid for memory
that can contain security-sensitive data IMO.)

-- 
Met vriendelijke groet,
With kind regards,
Giel van Schijndel
--
"Always code as if the guy who ends up maintaining your code will be a
 violent psychopath who knows where you live."
  -- Rick Osborne

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] Use memzero_explicit to clear local buffers

[Herbert Xu]

On Sun, Jan 04, 2015 at 11:49:09PM +0100, Giel van Schijndel wrote:
>
> > sctx does not point to stack memory so this is bogus.
> > 
> > Only stack memory cleared just before it goes out of scope needs
> > memzero_explicit.
> 
> Is that because the compiler can't safely optimize memset(0) away for a
> variable with greater-than-local scope?

Exactly.  memzero_explicit is not a marker for sensitive data.
Its only purpose is to prevent the compiler from optimising away
zeroing that occurs at the end of a scope.

Daniel, we should add a comment so that people stop sending bogus
patches with memzero_explicit.

Cheers,
-- 
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

Re: [PATCH] Use memzero_explicit to clear local buffers

[Giel van Schijndel]

[-- Attachment #1: Type: text/plain, Size: 1042 bytes --]

On Mon, Jan 05, 2015 at 10:36:37 +1100, Herbert Xu wrote:
> On Sun, Jan 04, 2015 at 11:49:09PM +0100, Giel van Schijndel wrote:
>>
>>> sctx does not point to stack memory so this is bogus.
>>> 
>>> Only stack memory cleared just before it goes out of scope needs
>>> memzero_explicit.
>> 
>> Is that because the compiler can't safely optimize memset(0) away for a
>> variable with greater-than-local scope?
> 
> Exactly.  memzero_explicit is not a marker for sensitive data.
> Its only purpose is to prevent the compiler from optimising away
> zeroing that occurs at the end of a scope.

Question: are you sure the compiler won't optimize the call to memset(0)
way if it's immediately followed by kfree()?

Because one of my changes concerns that situation.

Another actually does change a stack-allocated buffer, I'll split that
one off right away.

-- 
Met vriendelijke groet,
With kind regards,
Giel van Schijndel
--
"When all you have is a hammer, everything starts to look like a nail."
  -- Abraham Maslow

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] Use memzero_explicit to clear local buffers

[Herbert Xu]

On Tue, Jan 06, 2015 at 08:42:26PM +0100, Giel van Schijndel wrote:
> 
> Question: are you sure the compiler won't optimize the call to memset(0)
> way if it's immediately followed by kfree()?

Yes it won't be optimised away.  However, you could use kzfree.

> Another actually does change a stack-allocated buffer, I'll split that
> one off right away.

OK.

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

Re: [PATCH] Use memzero_explicit to clear local buffers

[Giel van Schijndel]

[-- Attachment #1: Type: text/plain, Size: 624 bytes --]

On Sun, Jan 04, 2015 at 19:05:40 +0100, Giel van Schijndel wrote:
> When leaving a function use memzero_explicit instead of memset(0) to
> clear locally allocated/owned buffers. memset(0) may be optimized away.
> 
> All of the affected buffers contain sensitive data, key material or
> derivatives of one of those two.
> ---

Forgot to:
Signed-off-by: Giel van Schijndel <me@mortis.eu>

-- 
Met vriendelijke groet,
With kind regards,
Giel van Schijndel
--
"Question: what do you call your programming methodology?
 Answer: Faith based development. You code and then pray that it works."
  -- John Spelner

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH RESEND] cifs: use memzero_explicit to clear stack buffer

[Giel van Schijndel]

When leaving a function use memzero_explicit instead of memset(0) to
clear stack allocated buffers. memset(0) may be optimized away.

This particular buffer is highly likely to contain sensitive data which
we shouldn't leak (it's named 'passwd' after all).

Signed-off-by: Giel van Schijndel <me@mortis.eu>
Reported-at: http://www.viva64.com/en/b/0299/
Reported-by: Andrey Karpov
Reported-by: Svyatoslav Razmyslov
---
 fs/cifs/smbencrypt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/smbencrypt.c b/fs/cifs/smbencrypt.c
index 6c15663..a4232ec 100644
--- a/fs/cifs/smbencrypt.c
+++ b/fs/cifs/smbencrypt.c
@@ -221,7 +221,7 @@ E_md4hash(const unsigned char *passwd, unsigned char *p16,
 	}
 
 	rc = mdfour(p16, (unsigned char *) wpwd, len * sizeof(__le16));
-	memset(wpwd, 0, 129 * sizeof(__le16));
+	memzero_explicit(wpwd, sizeof(wpwd));
 
 	return rc;
 }
-- 
2.1.4

Re: [PATCH RESEND] cifs: use memzero_explicit to clear stack buffer

[Herbert Xu]

On Tue, Jan 06, 2015 at 10:37:00PM +0100, Giel van Schijndel wrote:
> When leaving a function use memzero_explicit instead of memset(0) to
> clear stack allocated buffers. memset(0) may be optimized away.
> 
> This particular buffer is highly likely to contain sensitive data which
> we shouldn't leak (it's named 'passwd' after all).
> 
> Signed-off-by: Giel van Schijndel <me@mortis.eu>
> Reported-at: http://www.viva64.com/en/b/0299/
> Reported-by: Andrey Karpov
> Reported-by: Svyatoslav Razmyslov

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

Re: [PATCH RESEND] cifs: use memzero_explicit to clear stack buffer

[Steve French]

merged into cifs-2.6.git for-next

On Tue, Jan 6, 2015 at 4:59 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Tue, Jan 06, 2015 at 10:37:00PM +0100, Giel van Schijndel wrote:
>> When leaving a function use memzero_explicit instead of memset(0) to
>> clear stack allocated buffers. memset(0) may be optimized away.
>>
>> This particular buffer is highly likely to contain sensitive data which
>> we shouldn't leak (it's named 'passwd' after all).
>>
>> Signed-off-by: Giel van Schijndel <me@mortis.eu>
>> Reported-at: http://www.viva64.com/en/b/0299/
>> Reported-by: Andrey Karpov
>> Reported-by: Svyatoslav Razmyslov
>
> 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



-- 
Thanks,

Steve

Re: [PATCH RESEND] cifs: use memzero_explicit to clear stack buffer

[Steve French]

Looks fine to me - will merge into cifs-2.6.git

On Tue, Jan 6, 2015 at 3:37 PM, Giel van Schijndel <me@mortis.eu> wrote:
> When leaving a function use memzero_explicit instead of memset(0) to
> clear stack allocated buffers. memset(0) may be optimized away.
>
> This particular buffer is highly likely to contain sensitive data which
> we shouldn't leak (it's named 'passwd' after all).
>
> Signed-off-by: Giel van Schijndel <me@mortis.eu>
> Reported-at: http://www.viva64.com/en/b/0299/
> Reported-by: Andrey Karpov
> Reported-by: Svyatoslav Razmyslov
> ---
>  fs/cifs/smbencrypt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/cifs/smbencrypt.c b/fs/cifs/smbencrypt.c
> index 6c15663..a4232ec 100644
> --- a/fs/cifs/smbencrypt.c
> +++ b/fs/cifs/smbencrypt.c
> @@ -221,7 +221,7 @@ E_md4hash(const unsigned char *passwd, unsigned char *p16,
>         }
>
>         rc = mdfour(p16, (unsigned char *) wpwd, len * sizeof(__le16));
> -       memset(wpwd, 0, 129 * sizeof(__le16));
> +       memzero_explicit(wpwd, sizeof(wpwd));
>
>         return rc;
>  }
> --
> 2.1.4
>



-- 
Thanks,

Steve