[v2,01/13] introduce g_autowipe
diff mbox series

Message ID 20190826135103.22410-2-mlevitsk@redhat.com
State New
Headers show
Series
  • RFC crypto/luks: preparation for encryption key managment
Related show

Commit Message

Maxim Levitsky Aug. 26, 2019, 1:50 p.m. UTC
Marking a pointer with g_autowipe, will
not only free it at the scope exit, but also
erase the data it points to just prior to freeing it.

This is first attempt to implement this feature,
as suggested by Daniel and Nir.

The things that need to be verified prior to merging this is

1. Can we just always use memset_s (defined in C++)
 or some alternative.

2. is it portable enought for us to use malloc_usable_size
to get the size of malloced pointer in the autofree callback?
This function is aviable in glibc (but no wrapper in glib).

Thanks for Daniel for the g_autowipe and to Nir for the
information about the fact that plain memset is usually
optimized away.

Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Suggested-by: Nir Soffer <nsoffer@redhat.com>
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 include/autowipe.h | 52 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 include/autowipe.h

Comments

Tony Nguyen Aug. 27, 2019, 10:46 a.m. UTC | #1
Hi Maxim,

On Mon, Aug 26, 2019 at 04:50:51PM +0300, Maxim Levitsky wrote:
> 2. is it portable enought for us to use malloc_usable_size
> to get the size of malloced pointer in the autofree callback?
> This function is aviable in glibc (but no wrapper in glib).

We will also need to consider host portability: malloc_usable_size
for Linux, malloc_size for MacOS, _msize for Windows, etc.

[...]

> +#include <stddef.h>
> +#include <malloc.h>

Again host portability, <malloc.h> is not present on MacOS.

Regards,
Tony
Daniel P. Berrangé Aug. 27, 2019, 10:52 a.m. UTC | #2
On Mon, Aug 26, 2019 at 04:50:51PM +0300, Maxim Levitsky wrote:
> Marking a pointer with g_autowipe, will
> not only free it at the scope exit, but also
> erase the data it points to just prior to freeing it.
> 
> This is first attempt to implement this feature,
> as suggested by Daniel and Nir.
> 
> The things that need to be verified prior to merging this is
> 
> 1. Can we just always use memset_s (defined in C++)
>  or some alternative.
> 
> 2. is it portable enought for us to use malloc_usable_size
> to get the size of malloced pointer in the autofree callback?
> This function is aviable in glibc (but no wrapper in glib).

Urgh, no, we can't rely on that.

I completely forgot that we would need to know the size during
the deallocate function.  The portable way to deal with this
will be to change all our code that handles passwords to use
GString instead, since that is a struct carrying around the
allocated size.

As mentioned in v1, I'm fine if you just let this series use
memset as this is a pre-existing problem & we can fix it
in separate yseries.


Regards,
Daniel
Maxim Levitsky Aug. 27, 2019, 11:24 a.m. UTC | #3
On Tue, 2019-08-27 at 11:52 +0100, Daniel P. Berrangé wrote:
> On Mon, Aug 26, 2019 at 04:50:51PM +0300, Maxim Levitsky wrote:
> > Marking a pointer with g_autowipe, will
> > not only free it at the scope exit, but also
> > erase the data it points to just prior to freeing it.
> > 
> > This is first attempt to implement this feature,
> > as suggested by Daniel and Nir.
> > 
> > The things that need to be verified prior to merging this is
> > 
> > 1. Can we just always use memset_s (defined in C++)
> >  or some alternative.
> > 
> > 2. is it portable enought for us to use malloc_usable_size
> > to get the size of malloced pointer in the autofree callback?
> > This function is aviable in glibc (but no wrapper in glib).
> 
> Urgh, no, we can't rely on that.
> 
> I completely forgot that we would need to know the size during
> the deallocate function.  The portable way to deal with this
> will be to change all our code that handles passwords to use
> GString instead, since that is a struct carrying around the
> allocated size.
> 
> As mentioned in v1, I'm fine if you just let this series use
> memset as this is a pre-existing problem & we can fix it
> in separate yseries.

All right, I *was* afraid of that, but I guess it was worth a try anyway.
So I think I'll keep that patch that adds few missing memsets, 
just to consistency/documentation purposes since anyway
in addtion to these there are lot of other places that keys are kept,
like the ciphers itself, secrets (which aren't even freed  usually
as long as VM is running)

The purpose was that I just that memsetting caught my eye and
I wanted to make it at least consistent.

Thanks for the review,
	Best regards,
		Maxim Levitsky

Patch
diff mbox series

diff --git a/include/autowipe.h b/include/autowipe.h
new file mode 100644
index 0000000000..1ed4eaf3ba
--- /dev/null
+++ b/include/autowipe.h
@@ -0,0 +1,52 @@ 
+/*
+ * g_autowipe implementation for crypto secret wiping
+ *
+ * Copyright (c) 2019 Red Hat, Inc.
+ * Copyright (c) 2019 Maxim Levitsky
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+
+#include <stddef.h>
+#include <malloc.h>
+#include <glib.h>
+
+
+/*
+ * based on
+ * https://www.cryptologie.net/article/419/zeroing-memory-compiler-optimizations-and-memset_s/
+ */
+
+static inline void memerase(void *pointer, size_t size)
+{
+#ifdef __STDC_LIB_EXT1__
+    memset_s(pointer, size, 0, size);
+#else
+    /*volatile used to force compiler to not optimize the code away*/
+    volatile unsigned char *p = pointer;
+    while (size--) {
+        *p++ = 0;
+    }
+#endif
+}
+
+static void g_autoptr_cleanup_generic_wipe_gfree(void *p)
+{
+    void **pp = (void **)p;
+    size_t size = malloc_usable_size(*pp);
+    memerase(*pp, size);
+    g_free(*pp);
+}
+
+#define g_autowipe _GLIB_CLEANUP(g_autoptr_cleanup_generic_wipe_gfree)