Mailing List Archive

[PATCH] Wipe potentially sensitive stack memory.
* src/util.h: Add wipememory macro from cJSON.c.
* src/data.c (_gpgme_data_inbound_handler): Wipe buffer before return.
* src/engine-assuan.c (inquire_cb): Ditto.

Signed-off-by: Ben Kibbey <bjk@luxsci.net>
---
src/cJSON.c | 10 +---------
src/data.c | 2 ++
src/engine-assuan.c | 3 +++
src/util.h | 9 +++++++++
4 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/src/cJSON.c b/src/cJSON.c
index 7769b0eb..f73e25a3 100644
--- a/src/cJSON.c
+++ b/src/cJSON.c
@@ -49,6 +49,7 @@
#include <gpg-error.h>

#include "cJSON.h"
+#include "util.h"

/* Only use calloc. */
#define CALLOC_ONLY 1
@@ -56,15 +57,6 @@
/* Maximum recursion depth */
#define MAX_DEPTH 512

-/* To avoid that a compiler optimizes certain memset calls away, these
- macros may be used instead. */
-#define wipememory2(_ptr,_set,_len) do { \
- volatile char *_vptr=(volatile char *)(_ptr); \
- size_t _vlen=(_len); \
- while(_vlen) { *_vptr=(_set); _vptr++; _vlen--; } \
- } while(0)
-#define wipememory(_ptr,_len) wipememory2(_ptr,0,_len)
-
/* We use malloc function wrappers from gpgrt (aka libgpg-error). */
#include <gpgrt.h>
#define xtrycalloc(a,b) gpgrt_calloc ((a), (b))
diff --git a/src/data.c b/src/data.c
index 70595907..07e279a9 100644
--- a/src/data.c
+++ b/src/data.c
@@ -594,6 +594,8 @@ _gpgme_data_inbound_handler (void *opaque, int fd)
buflen -= amt;
}
while (buflen > 0);
+
+ wipememory (buffer, sizeof (buffer));
return TRACE_ERR (0);
}

diff --git a/src/engine-assuan.c b/src/engine-assuan.c
index ab9d05a9..a8c65dca 100644
--- a/src/engine-assuan.c
+++ b/src/engine-assuan.c
@@ -467,6 +467,9 @@ inquire_cb (engine_llass_t llass, const char *keyword, const char *args)
if (err)
break;
}
+
+ wipememory(buf, sizeof(buf));
+
/* Tell the caller that we are finished with the data
* object. The error code from assuan_send_data has
* priority over the one from the cleanup function. */
diff --git a/src/util.h b/src/util.h
index bc78c9b8..97bc1da2 100644
--- a/src/util.h
+++ b/src/util.h
@@ -41,6 +41,15 @@

#define DIM(v) (sizeof(v)/sizeof((v)[0]))

+
+/* To avoid that a compiler optimizes certain memset calls away, these
+ macros may be used instead. */
+#define wipememory2(_ptr,_set,_len) do { \
+ volatile char *_vptr=(volatile char *)(_ptr); \
+ size_t _vlen=(_len); \
+ while(_vlen) { *_vptr=(_set); _vptr++; _vlen--; } \
+ } while(0)
+#define wipememory(_ptr,_len) wipememory2(_ptr,0,_len)


/*-- {posix,w32}-util.c --*/
--
2.30.2


_______________________________________________
Gnupg-devel mailing list
Gnupg-devel@gnupg.org
http://lists.gnupg.org/mailman/listinfo/gnupg-devel
Re: [PATCH] Wipe potentially sensitive stack memory. [ In reply to ]
Hi Ben,

On Tue, 8 Jun 2021 20:51, Ben Kibbey said:
> * src/data.c (_gpgme_data_inbound_handler): Wipe buffer before return.

Is that intended for passphrase callbacks or secret key export? Would a
flag flagging such a data object holding sensitive data not be better?

BTW, I plan to allow for lager buffers in this function to reduce the
overhead for certain callers which don't work well with small data
blocks. Thus a new data object flag will anyway be added.


Salam-Shalom,

Werner

--
Die Gedanken sind frei. Ausnahmen regelt ein Bundesgesetz.
Re: [PATCH] Wipe potentially sensitive stack memory. [ In reply to ]
On Fri, Jun 11, 2021 at 07:56:57AM +0200, Werner Koch wrote:
> Hi Ben,
>
> On Tue, 8 Jun 2021 20:51, Ben Kibbey said:
> > * src/data.c (_gpgme_data_inbound_handler): Wipe buffer before return.
>
> Is that intended for passphrase callbacks or secret key export? Would a
> flag flagging such a data object holding sensitive data not be better?

It is used for gpg IO during gpgme_op_decrypt_*() and other app
engines. Although normally not key material, what remains in the buffer is
decrypted data which could be anything including key material for some
other purpose. I didn't push the patch because I wasn't sure what you or
others thought about the cost/overhead of wipememory() on every call.
But I think (I'm not an ASM or CPU expert) most CPU's have a register
(MMX, SSE, etc) to help with zeroing things out and the overhead of
doing so is not very much, if any. Although it may all depend on the
libc implementation, too.

> BTW, I plan to allow for lager buffers in this function to reduce the
> overhead for certain callers which don't work well with small data
> blocks. Thus a new data object flag will anyway be added.

OK cool.

--
Ben Kibbey
Re: [PATCH] Wipe potentially sensitive stack memory. [ In reply to ]
On Fri, 11 Jun 2021 20:25, Ben Kibbey said:

> It is used for gpg IO during gpgme_op_decrypt_*() and other app
> engines. Although normally not key material, what remains in the buffer is
> decrypted data which could be anything including key material for some
> other purpose. I didn't push the patch because I wasn't sure what you or

Okay. Most work will however go into the caller to keep it safe there ;-)

>> BTW, I plan to allow for lager buffers in this function to reduce the
>> overhead for certain callers which don't work well with small data
>> blocks. Thus a new data object flag will anyway be added.
>
> OK cool.

So let me do it in the course of that patch. I guess I can do that next
week.


Shalom-Salam,

Werner


--
Die Gedanken sind frei. Ausnahmen regelt ein Bundesgesetz.