| Message ID | 20190814202219.1870-6-mlevitsk@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | RFC: luks/encrypted qcow2 key management | expand |
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
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
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 > > >
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
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
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
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
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
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
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);
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(-)