diff --git a/memory.c b/memory.c index 448d9bd8..b6fb325d 100644 --- a/memory.c +++ b/memory.c @@ -8,6 +8,7 @@ #include "defs.h" #include "puttymem.h" +#include "misc.h" void *safemalloc(size_t n, size_t size) { @@ -72,7 +73,7 @@ void safefree(void *ptr) } void *safegrowarray(void *ptr, size_t *allocated, size_t eltsize, - size_t oldlen, size_t extralen) + size_t oldlen, size_t extralen, bool secret) { /* The largest value we can safely multiply by eltsize */ assert(eltsize > 0); @@ -108,7 +109,15 @@ void *safegrowarray(void *ptr, size_t *allocated, size_t eltsize, increment = maxincr; size_t newsize = oldsize + increment; - void *toret = saferealloc(ptr, newsize, eltsize); + void *toret; + if (secret) { + toret = safemalloc(newsize, eltsize); + memcpy(toret, ptr, oldsize * eltsize); + smemclr(ptr, oldsize * eltsize); + sfree(ptr); + } else { + toret = saferealloc(ptr, newsize, eltsize); + } *allocated = newsize; return toret; } diff --git a/misc.h b/misc.h index 1adaf161..f97f6730 100644 --- a/misc.h +++ b/misc.h @@ -34,14 +34,26 @@ char *dupprintf(const char *fmt, ...) char *dupvprintf(const char *fmt, va_list ap); void burnstr(char *string); +/* + * The visible part of a strbuf structure. There's a surrounding + * implementation struct in misc.c, which isn't exposed to client + * code. + */ struct strbuf { char *s; unsigned char *u; size_t len; BinarySink_IMPLEMENTATION; - /* (also there's a surrounding implementation struct in misc.c) */ }; + +/* strbuf constructors: strbuf_new_nm and strbuf_new differ in that a + * strbuf constructed using the _nm version will resize itself by + * alloc/copy/smemclr/free instead of realloc. Use that version for + * data sensitive enough that it's worth costing performance to + * avoid copies of it lingering in process memory. */ strbuf *strbuf_new(void); +strbuf *strbuf_new_nm(void); + void strbuf_free(strbuf *buf); void *strbuf_append(strbuf *buf, size_t len); char *strbuf_to_str(strbuf *buf); /* does free buf, but you must free result */ diff --git a/puttymem.h b/puttymem.h index ddb04a5c..85184a14 100644 --- a/puttymem.h +++ b/puttymem.h @@ -82,12 +82,28 @@ void safefree(void *); * function outside your control, which would either fill your buffer * and return success, or else return a 'too big' error without * telling you how much bigger it needed to be. + * + * The _nm variants of the macro set the 'private' flag in the + * underlying function, which forces array resizes to be done by a + * manual allocate/copy/free instead of realloc, with careful clearing + * of the previous memory block before we free it. This costs + * performance, but if the block contains important secrets such as + * private keys or passwords, it avoids the risk that a realloc that + * moves the memory block might leave a copy of the data visible in + * the freed memory at the previous location. */ void *safegrowarray(void *array, size_t *size, size_t eltsize, - size_t oldlen, size_t extralen); -#define sgrowarrayn(array, size, n, m) \ - ((array) = safegrowarray(array, &(size), sizeof(*array), n, m)) -#define sgrowarray(array, size, n) sgrowarrayn(array, size, n, 1) + size_t oldlen, size_t extralen, bool private); + +/* The master macro wrapper, of which all others are special cases */ +#define sgrowarray_general(array, size, n, m, priv) \ + ((array) = safegrowarray(array, &(size), sizeof(*array), n, m, priv)) + +/* The special-case macros that are easier to use in most situations */ +#define sgrowarrayn( a, s, n, m) sgrowarray_general(a, s, n, m, false) +#define sgrowarray( a, s, n ) sgrowarray_general(a, s, n, 1, false) +#define sgrowarrayn_nm(a, s, n, m) sgrowarray_general(a, s, n, m, true ) +#define sgrowarray_nm( a, s, n ) sgrowarray_general(a, s, n, 1, true ) /* * This function is called by the innermost safemalloc/saferealloc diff --git a/utils.c b/utils.c index c601d87c..283702d8 100644 --- a/utils.c +++ b/utils.c @@ -386,6 +386,7 @@ char *dupprintf(const char *fmt, ...) struct strbuf_impl { size_t size; struct strbuf visible; + bool nm; /* true if we insist on non-moving buffer resizes */ }; #define STRBUF_SET_UPTR(buf) \ @@ -397,7 +398,8 @@ void *strbuf_append(strbuf *buf_o, size_t len) { struct strbuf_impl *buf = container_of(buf_o, struct strbuf_impl, visible); char *toret; - sgrowarrayn(buf->visible.s, buf->size, buf->visible.len + 1, len); + sgrowarray_general( + buf->visible.s, buf->size, buf->visible.len + 1, len, buf->nm); STRBUF_SET_UPTR(buf); toret = buf->visible.s + buf->visible.len; buf->visible.len += len; @@ -412,16 +414,19 @@ static void strbuf_BinarySink_write( memcpy(strbuf_append(buf_o, len), data, len); } -strbuf *strbuf_new(void) +static strbuf *strbuf_new_general(bool nm) { struct strbuf_impl *buf = snew(struct strbuf_impl); BinarySink_INIT(&buf->visible, strbuf_BinarySink_write); buf->visible.len = 0; buf->size = 512; + buf->nm = nm; STRBUF_SET_PTR(buf, snewn(buf->size, char)); *buf->visible.s = '\0'; return &buf->visible; } +strbuf *strbuf_new(void) { return strbuf_new_general(false); } +strbuf *strbuf_new_nm(void) { return strbuf_new_general(true); } void strbuf_free(strbuf *buf_o) { struct strbuf_impl *buf = container_of(buf_o, struct strbuf_impl, visible);