From: Tobias Brunner <tobias@strongswan.org>
Date: Thu, 17 Oct 2019 11:09:54 +0000 (+0200)
Subject: utils: Handle NULL consistently if memwipe() is implemented via explicit_bzero()
X-Git-Tag: 5.8.2dr2~22
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=cf98706bb8c48327ca65b08f849ec2292b239a57;p=thirdparty%2Fstrongswan.git

utils: Handle NULL consistently if memwipe() is implemented via explicit_bzero()

Our own implementation ignores NULL values, however, explicit_bzero()
can't handle that, as indicated by the `__nonnull ((1))` attribute in the
function's signature in string.h, and causes a segmentation fault.  This
was noticed in one of the unit tests for NewHope.  Since we usually use
memwipe() via chunk_clear(), which already ignores NULL pointers, this
is not that much of an issue in practice.

Fixes: 149d1bbb055a ("memory: Use explicit_bzero() as memwipe() if available")
---

diff --git a/src/libstrongswan/tests/suites/test_utils.c b/src/libstrongswan/tests/suites/test_utils.c
index 27343349e5..9060f57b58 100644
--- a/src/libstrongswan/tests/suites/test_utils.c
+++ b/src/libstrongswan/tests/suites/test_utils.c
@@ -547,6 +547,63 @@ START_TEST(test_memstr)
 }
 END_TEST
 
+/*******************************************************************************
+ * memwipe
+ */
+
+START_TEST(test_memwipe_null)
+{
+	memwipe(NULL, 16);
+}
+END_TEST
+
+static inline bool test_wiped_memory(char *buf, size_t len)
+{
+	int i;
+
+	/* comparing two bytes at once reduces the chances of conflicts if memory
+	 * got overwritten already */
+	for (i = 0; i < len; i += 2)
+	{
+		if (buf[i] != 0 && buf[i] == i &&
+			buf[i+1] != 0 && buf[i+1] == i+1)
+		{
+			return FALSE;
+		}
+	}
+	return TRUE;
+}
+
+START_TEST(test_memwipe_stack)
+{
+	char buf[64];
+	int i;
+
+	for (i = 0; i < sizeof(buf); i++)
+	{
+		buf[i] = i;
+	}
+	memwipe(buf, sizeof(buf));
+	ck_assert(test_wiped_memory(buf, sizeof(buf)));
+}
+END_TEST
+
+START_TEST(test_memwipe_heap)
+{
+	size_t len = 64;
+	char *buf = malloc(len);
+	int i;
+
+	for (i = 0; i < len; i++)
+	{
+		buf[i] = i;
+	}
+	memwipe(buf, len);
+	ck_assert(test_wiped_memory(buf, len));
+	free(buf);
+}
+END_TEST
+
 /*******************************************************************************
  * utils_memrchr
  */
@@ -1132,6 +1189,12 @@ Suite *utils_suite_create()
 	tcase_add_loop_test(tc, test_memstr, 0, countof(memstr_data));
 	suite_add_tcase(s, tc);
 
+	tc = tcase_create("memwipe");
+	tcase_add_test(tc, test_memwipe_null);
+	tcase_add_test(tc, test_memwipe_stack);
+	tcase_add_test(tc, test_memwipe_heap);
+	suite_add_tcase(s, tc);
+
 	tc = tcase_create("utils_memrchr");
 	tcase_add_loop_test(tc, test_utils_memrchr, 0, countof(memrchr_data));
 	suite_add_tcase(s, tc);
diff --git a/src/libstrongswan/utils/utils/memory.h b/src/libstrongswan/utils/utils/memory.h
index c257b1eca7..eea19588ef 100644
--- a/src/libstrongswan/utils/utils/memory.h
+++ b/src/libstrongswan/utils/utils/memory.h
@@ -87,7 +87,13 @@ static inline void *memset_noop(void *s, int c, size_t n)
 void memxor(uint8_t dest[], const uint8_t src[], size_t n);
 
 #ifdef HAVE_EXPLICIT_BZERO
-#define memwipe(ptr, n) explicit_bzero(ptr, n)
+static inline void memwipe(void *ptr, size_t n)
+{
+	if (ptr)
+	{
+		explicit_bzero(ptr, n);
+	}
+}
 #else /* HAVE_EXPLICIT_BZERO */
 /**
  * Safely overwrite n bytes of memory at ptr with zero, non-inlining variant.