Extra-secure versions of sgrowarray and strbuf.

These versions, distinguished by the _nm suffix on their names, avoid
using realloc to grow the array, in case it moves the block and leaves
a copy of the data in the freed memory at the old address. (The suffix
'nm' stands for 'no moving'.) Instead, the array is grown by making a
new allocation, manually copying the data over, and carefully clearing
the old block before freeing it.

(An alternative would be to give this code base its own custom heap in
which the ordinary realloc takes care about this kind of thing, but I
don't really feel like going to that much effort!)

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;
 }

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 */

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

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);