[05/13] qcrypto-luks: clear the masterkey and password before freeing them always
diff mbox series

Message ID 20190814202219.1870-6-mlevitsk@redhat.com
State New
Headers show
Series
  • RFC: luks/encrypted qcow2 key management
Related show

Commit Message

Maxim Levitsky Aug. 14, 2019, 8:22 p.m. UTC
While there are other places where these are still stored in memory,
this is still one less key material area that can be sniffed with
various side channel attacks



Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 crypto/block-luks.c | 52 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 44 insertions(+), 8 deletions(-)

Comments

Max Reitz Aug. 20, 2019, 6:12 p.m. UTC | #1
On 14.08.19 22:22, Maxim Levitsky wrote:
> While there are other places where these are still stored in memory,
> this is still one less key material area that can be sniffed with
> various side channel attacks
> 
> 
> 

(Many empty lines here)

> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  crypto/block-luks.c | 52 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 44 insertions(+), 8 deletions(-)

Wouldn’t it make sense to introduce a dedicated function for this?

Max
Maxim Levitsky Aug. 21, 2019, 10:40 p.m. UTC | #2
On Tue, 2019-08-20 at 20:12 +0200, Max Reitz wrote:
> On 14.08.19 22:22, Maxim Levitsky wrote:
> > While there are other places where these are still stored in memory,
> > this is still one less key material area that can be sniffed with
> > various side channel attacks
> > 
> > 
> > 
> 
> (Many empty lines here)
> 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  crypto/block-luks.c | 52 ++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 44 insertions(+), 8 deletions(-)
> 
> Wouldn’t it make sense to introduce a dedicated function for this?

Absolutely. I was mostly focused on fixing all the cases first.
I usually refactor such ugly code at the end, but this time I forgot
to do so.

Plus I need to pick a place where to put such function (it can be useful in any place in qemu), 
and first check if maybe glib already has such free+scrub function implemented somewhere.

Best regards,
	Maxim Levitsky
Nir Soffer Aug. 21, 2019, 11:01 p.m. UTC | #3
On Wed, Aug 14, 2019, 23:23 Maxim Levitsky <mlevitsk@redhat.com> wrote:

> While there are other places where these are still stored in memory,
> this is still one less key material area that can be sniffed with
> various side channel attacks
>
>
>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  crypto/block-luks.c | 52 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 44 insertions(+), 8 deletions(-)
>
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index e1a4df94b7..336e633df4 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -1023,8 +1023,18 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
>   cleanup:
>      qcrypto_ivgen_free(ivgen);
>      qcrypto_cipher_free(cipher);
> -    g_free(splitkey);
> -    g_free(possiblekey);
> +
> +    if (splitkey) {
> +        memset(splitkey, 0, splitkeylen);
>

I think we need memset_s() or similar replacement, see

https://www.cryptologie.net/article/419/zeroing-memory-compiler-optimizations-and-memset_s/

+        g_free(splitkey);
> +    }
> +
> +    if (possiblekey) {
> +        memset(possiblekey, 0, masterkeylen(luks));
> +        g_free(possiblekey);
> +
> +    }
> +
>      return ret;
>  }
>
> @@ -1161,16 +1171,34 @@ qcrypto_block_luks_open(QCryptoBlock *block,
>      block->sector_size = QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
>      block->payload_offset = luks->header.payload_offset *
> block->sector_size;
>
> -    g_free(masterkey);
> -    g_free(password);
> +    if (masterkey) {
> +        memset(masterkey, 0, masterkeylen(luks));
> +        g_free(masterkey);
> +    }
> +
> +    if (password) {
> +        memset(password, 0, strlen(password));
> +        g_free(password);
> +    }
> +
>      return 0;
>
>   fail:
> -    g_free(masterkey);
> +
> +    if (masterkey) {
> +        memset(masterkey, 0, masterkeylen(luks));
> +        g_free(masterkey);
> +    }
> +
> +    if (password) {
> +        memset(password, 0, strlen(password));
> +        g_free(password);
> +    }
> +
>      qcrypto_block_free_cipher(block);
>      qcrypto_ivgen_free(block->ivgen);
> +
>      g_free(luks);
> -    g_free(password);
>      return ret;
>  }
>
> @@ -1459,7 +1487,10 @@ qcrypto_block_luks_create(QCryptoBlock *block,
>
>      memset(masterkey, 0, luks->header.key_bytes);
>      g_free(masterkey);
> +
> +    memset(password, 0, strlen(password));
>      g_free(password);
> +
>      g_free(cipher_mode_spec);
>
>      return 0;
> @@ -1467,9 +1498,14 @@ qcrypto_block_luks_create(QCryptoBlock *block,
>   error:
>      if (masterkey) {
>          memset(masterkey, 0, luks->header.key_bytes);
> +        g_free(masterkey);
>      }
> -    g_free(masterkey);
> -    g_free(password);
> +
> +    if (password) {
> +        memset(password, 0, strlen(password));
> +        g_free(password);
> +    }
> +
>      g_free(cipher_mode_spec);
>
>      qcrypto_block_free_cipher(block);
> --
> 2.17.2
>
>
>
Maxim Levitsky Aug. 21, 2019, 11:11 p.m. UTC | #4
On Thu, 2019-08-22 at 02:01 +0300, Nir Soffer wrote:
> On Wed, Aug 14, 2019, 23:23 Maxim Levitsky <mlevitsk@redhat.com> wrote:
> 
> > While there are other places where these are still stored in memory,
> > this is still one less key material area that can be sniffed with
> > various side channel attacks
> > 
> > 
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  crypto/block-luks.c | 52 ++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 44 insertions(+), 8 deletions(-)
> > 
> > diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> > index e1a4df94b7..336e633df4 100644
> > --- a/crypto/block-luks.c
> > +++ b/crypto/block-luks.c
> > @@ -1023,8 +1023,18 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
> >   cleanup:
> >      qcrypto_ivgen_free(ivgen);
> >      qcrypto_cipher_free(cipher);
> > -    g_free(splitkey);
> > -    g_free(possiblekey);
> > +
> > +    if (splitkey) {
> > +        memset(splitkey, 0, splitkeylen);
> > 
> 
> I think we need memset_s() or similar replacement, see
> 
> https://www.cryptologie.net/article/419/zeroing-memory-compiler-optimizations-and-memset_s/

You raise a very very good point here! Thanks!!

Best regards,
	Maxim Levitsky
Daniel P. Berrangé Aug. 22, 2019, 10:49 a.m. UTC | #5
On Tue, Aug 20, 2019 at 08:12:51PM +0200, Max Reitz wrote:
> On 14.08.19 22:22, Maxim Levitsky wrote:
> > While there are other places where these are still stored in memory,
> > this is still one less key material area that can be sniffed with
> > various side channel attacks
> > 
> > 
> > 
> 
> (Many empty lines here)
> 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  crypto/block-luks.c | 52 ++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 44 insertions(+), 8 deletions(-)
> 
> Wouldn’t it make sense to introduce a dedicated function for this?

Yes, it would.

In fact I have a series pending which bumps min glib and introduces
use of auto-free functions in this code.

It would be desirable to have a autp-free func for memset+free
so we can just declare the variable

   q_autowipefree char *password = NULL;

and have it result in memset+free

Regards,
Daniel
Maxim Levitsky Aug. 22, 2019, 10:56 a.m. UTC | #6
On Thu, 2019-08-22 at 11:49 +0100, Daniel P. Berrangé wrote:
> On Tue, Aug 20, 2019 at 08:12:51PM +0200, Max Reitz wrote:
> > On 14.08.19 22:22, Maxim Levitsky wrote:
> > > While there are other places where these are still stored in memory,
> > > this is still one less key material area that can be sniffed with
> > > various side channel attacks
> > > 
> > > 
> > > 
> > 
> > (Many empty lines here)
> > 
> > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > > ---
> > >  crypto/block-luks.c | 52 ++++++++++++++++++++++++++++++++++++++-------
> > >  1 file changed, 44 insertions(+), 8 deletions(-)
> > 
> > Wouldn’t it make sense to introduce a dedicated function for this?
> 
> Yes, it would.
> 
> In fact I have a series pending which bumps min glib and introduces
> use of auto-free functions in this code.
> 
> It would be desirable to have a autp-free func for memset+free
> so we can just declare the variable
> 
>    q_autowipefree char *password = NULL;
> 
> and have it result in memset+free
> 

That is perfect.
When do you think you could post the series so that I could rebase
on top of it?

Best regards,
	Maxim Levitsky
Maxim Levitsky Aug. 25, 2019, 3:31 p.m. UTC | #7
On Thu, 2019-08-22 at 13:56 +0300, Maxim Levitsky wrote:
> On Thu, 2019-08-22 at 11:49 +0100, Daniel P. Berrangé wrote:
> > On Tue, Aug 20, 2019 at 08:12:51PM +0200, Max Reitz wrote:
> > > On 14.08.19 22:22, Maxim Levitsky wrote:
> > > > While there are other places where these are still stored in memory,
> > > > this is still one less key material area that can be sniffed with
> > > > various side channel attacks
> > > > 
> > > > 
> > > > 
> > > 
> > > (Many empty lines here)
> > > 
> > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > > > ---
> > > >  crypto/block-luks.c | 52 ++++++++++++++++++++++++++++++++++++++-------
> > > >  1 file changed, 44 insertions(+), 8 deletions(-)
> > > 
> > > Wouldn’t it make sense to introduce a dedicated function for this?
> > 
> > Yes, it would.
> > 
> > In fact I have a series pending which bumps min glib and introduces
> > use of auto-free functions in this code.
> > 
> > It would be desirable to have a autp-free func for memset+free
> > so we can just declare the variable
> > 
> >    q_autowipefree char *password = NULL;
> > 
> > and have it result in memset+free
> > 
> 
> That is perfect.
> When do you think you could post the series so that I could rebase
> on top of it?


I am thinking that I will keep my patch as is, just so that code is
consistent in memsetting the secrets (even though as Nir pointed out,
that these will be probably optimized away anyway).
And then when you send your patch you will just remove all
of these memsets.

Is this all right? 

Best regards,
	Maxim Levitsky
Maxim Levitsky Aug. 25, 2019, 5:15 p.m. UTC | #8
On Sun, 2019-08-25 at 18:31 +0300, Maxim Levitsky wrote:
> On Thu, 2019-08-22 at 13:56 +0300, Maxim Levitsky wrote:
> > On Thu, 2019-08-22 at 11:49 +0100, Daniel P. Berrangé wrote:
> > > On Tue, Aug 20, 2019 at 08:12:51PM +0200, Max Reitz wrote:
> > > > On 14.08.19 22:22, Maxim Levitsky wrote:
> > > > > While there are other places where these are still stored in memory,
> > > > > this is still one less key material area that can be sniffed with
> > > > > various side channel attacks
> > > > > 
> > > > > 
> > > > > 
> > > > 
> > > > (Many empty lines here)
> > > > 
> > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > > > > ---
> > > > >  crypto/block-luks.c | 52 ++++++++++++++++++++++++++++++++++++++-------
> > > > >  1 file changed, 44 insertions(+), 8 deletions(-)
> > > > 
> > > > Wouldn’t it make sense to introduce a dedicated function for this?
> > > 
> > > Yes, it would.
> > > 
> > > In fact I have a series pending which bumps min glib and introduces
> > > use of auto-free functions in this code.
> > > 
> > > It would be desirable to have a autp-free func for memset+free
> > > so we can just declare the variable
> > > 
> > >    q_autowipefree char *password = NULL;
> > > 
> > > and have it result in memset+free
> > > 
> > 
> > That is perfect.
> > When do you think you could post the series so that I could rebase
> > on top of it?
> 
> 
> I am thinking that I will keep my patch as is, just so that code is
> consistent in memsetting the secrets (even though as Nir pointed out,
> that these will be probably optimized away anyway).
> And then when you send your patch you will just remove all
> of these memsets.
> 
> Is this all right? 

I see that your series actually already got merged.
Can I now implement the 'q_autowipefree', or do I need another glib version bump
for that?

Best regards,
	Maxim Levitsky


> 
> Best regards,
> 	Maxim Levitsky
Daniel P. Berrangé Aug. 27, 2019, 8:55 a.m. UTC | #9
On Sun, Aug 25, 2019 at 06:31:02PM +0300, Maxim Levitsky wrote:
> On Thu, 2019-08-22 at 13:56 +0300, Maxim Levitsky wrote:
> > On Thu, 2019-08-22 at 11:49 +0100, Daniel P. Berrangé wrote:
> > > On Tue, Aug 20, 2019 at 08:12:51PM +0200, Max Reitz wrote:
> > > > On 14.08.19 22:22, Maxim Levitsky wrote:
> > > > > While there are other places where these are still stored in memory,
> > > > > this is still one less key material area that can be sniffed with
> > > > > various side channel attacks
> > > > > 
> > > > > 
> > > > > 
> > > > 
> > > > (Many empty lines here)
> > > > 
> > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > > > > ---
> > > > >  crypto/block-luks.c | 52 ++++++++++++++++++++++++++++++++++++++-------
> > > > >  1 file changed, 44 insertions(+), 8 deletions(-)
> > > > 
> > > > Wouldn’t it make sense to introduce a dedicated function for this?
> > > 
> > > Yes, it would.
> > > 
> > > In fact I have a series pending which bumps min glib and introduces
> > > use of auto-free functions in this code.
> > > 
> > > It would be desirable to have a autp-free func for memset+free
> > > so we can just declare the variable
> > > 
> > >    q_autowipefree char *password = NULL;
> > > 
> > > and have it result in memset+free
> > > 
> > 
> > That is perfect.
> > When do you think you could post the series so that I could rebase
> > on top of it?
> 
> 
> I am thinking that I will keep my patch as is, just so that code is
> consistent in memsetting the secrets (even though as Nir pointed out,
> that these will be probably optimized away anyway).
> And then when you send your patch you will just remove all
> of these memsets.

I'm fine with you continuing to use memset, since this is a pre-existing
problem in the code that you are not making worse. We'll figure out the
fix separately.

Regards,
Daniel

Patch
diff mbox series

diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index e1a4df94b7..336e633df4 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -1023,8 +1023,18 @@  qcrypto_block_luks_load_key(QCryptoBlock *block,
  cleanup:
     qcrypto_ivgen_free(ivgen);
     qcrypto_cipher_free(cipher);
-    g_free(splitkey);
-    g_free(possiblekey);
+
+    if (splitkey) {
+        memset(splitkey, 0, splitkeylen);
+        g_free(splitkey);
+    }
+
+    if (possiblekey) {
+        memset(possiblekey, 0, masterkeylen(luks));
+        g_free(possiblekey);
+
+    }
+
     return ret;
 }
 
@@ -1161,16 +1171,34 @@  qcrypto_block_luks_open(QCryptoBlock *block,
     block->sector_size = QCRYPTO_BLOCK_LUKS_SECTOR_SIZE;
     block->payload_offset = luks->header.payload_offset * block->sector_size;
 
-    g_free(masterkey);
-    g_free(password);
+    if (masterkey) {
+        memset(masterkey, 0, masterkeylen(luks));
+        g_free(masterkey);
+    }
+
+    if (password) {
+        memset(password, 0, strlen(password));
+        g_free(password);
+    }
+
     return 0;
 
  fail:
-    g_free(masterkey);
+
+    if (masterkey) {
+        memset(masterkey, 0, masterkeylen(luks));
+        g_free(masterkey);
+    }
+
+    if (password) {
+        memset(password, 0, strlen(password));
+        g_free(password);
+    }
+
     qcrypto_block_free_cipher(block);
     qcrypto_ivgen_free(block->ivgen);
+
     g_free(luks);
-    g_free(password);
     return ret;
 }
 
@@ -1459,7 +1487,10 @@  qcrypto_block_luks_create(QCryptoBlock *block,
 
     memset(masterkey, 0, luks->header.key_bytes);
     g_free(masterkey);
+
+    memset(password, 0, strlen(password));
     g_free(password);
+
     g_free(cipher_mode_spec);
 
     return 0;
@@ -1467,9 +1498,14 @@  qcrypto_block_luks_create(QCryptoBlock *block,
  error:
     if (masterkey) {
         memset(masterkey, 0, luks->header.key_bytes);
+        g_free(masterkey);
     }
-    g_free(masterkey);
-    g_free(password);
+
+    if (password) {
+        memset(password, 0, strlen(password));
+        g_free(password);
+    }
+
     g_free(cipher_mode_spec);
 
     qcrypto_block_free_cipher(block);