[PATCH] memset_s() and talloc_set_secure()
Andreas Schneider
asn at samba.org
Thu Oct 11 10:17:55 UTC 2018
Hello,
the attached patch adds memset_s() [1] and talloc_set_secure(). It will make
sure that memory is zeroed/erased before freeing to not keep secrets around.
Consider the following code:
memset(ptr, '\0', size);
free(ptr);
The optimizer detects that after the memset() the code is freed and will drop
it.
To make sure that memory is really zeroed, we need to tell the optimizer. The
attached patchset will do that.
I've looked into talloc ABI updating but it fails to compile.
A share library has normaly 3 version numbers.
MAJOR
MINOR
PATCH
MAJOR is bumped if there is an API/ABI breakage
MINOR is bumped if the ABI changes but there is no breakage (e.g. adding a
function)
PATCH is bumped if there are only code fixes but the API/ABI didn't change
In the meantime I use abimap [2] in my project for managing SO versions.
Please review and someone who knows how to change the version of talloc
correctly, please push.
Thanks,
Andreas
[1] https://en.cppreference.com/w/c/string/byte/memset
[2] https://github.com/ansasaki/abimap
--
Andreas Schneider asn at samba.org
Samba Team www.samba.org
GPG-ID: 8DFF53E18F2ABC8D8F3C92237EE0FC4DCC014E3D
-------------- next part --------------
>From c95e28c4be2ca057061db23237ff6303059c1b13 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Wed, 10 Oct 2018 16:05:46 +0200
Subject: [PATCH 1/5] replace: Add memset_s() if not available
See https://en.cppreference.com/w/c/string/byte/memset
Signed-off-by: Andreas Schneider <asn at samba.org>
---
lib/replace/replace.c | 31 +++++++++++++++++++++++++++++++
lib/replace/replace.h | 5 +++++
lib/replace/wscript | 17 +++++++++++++++++
3 files changed, 53 insertions(+)
diff --git a/lib/replace/replace.c b/lib/replace/replace.c
index dc81e9cd5ab..113137c2992 100644
--- a/lib/replace/replace.c
+++ b/lib/replace/replace.c
@@ -947,3 +947,34 @@ void rep_setproctitle_init(int argc, char *argv[], char *envp[])
{
}
#endif
+
+#ifndef HAVE_MEMSET_S
+# ifndef RSIZE_MAX
+# define RSIZE_MAX (SIZE_MAX >> 1)
+# endif
+
+int rep_memset_s(void *dest, size_t destsz, int ch, size_t count)
+{
+ if (dest == NULL) {
+ return EINVAL;
+ }
+
+ if (destsz > RSIZE_MAX ||
+ count > RSIZE_MAX ||
+ count > destsz) {
+ return ERANGE;
+ }
+
+#if defined(HAVE_MEMSET_EXPLICIT)
+ memset_explicit(dest, destsz, ch, count);
+#else /* HAVE_MEMSET_EXPLICIT */
+ memset(dest, ch, count);
+# if defined(HAVE_GCC_VOLATILE_MEMORY_PROTECTION)
+ /* See http://llvm.org/bugs/show_bug.cgi?id=15495 */
+ __asm__ volatile("" : : "g"(dest) : "memory");
+# endif /* HAVE_GCC_VOLATILE_MEMORY_PROTECTION */
+#endif /* HAVE_MEMSET_EXPLICIT */
+
+ return 0;
+}
+#endif /* HAVE_MEMSET_S */
diff --git a/lib/replace/replace.h b/lib/replace/replace.h
index 626d3053029..260ca8df7a0 100644
--- a/lib/replace/replace.h
+++ b/lib/replace/replace.h
@@ -925,6 +925,11 @@ void rep_setproctitle(const char *fmt, ...) PRINTF_ATTRIBUTE(1, 2);
void rep_setproctitle_init(int argc, char *argv[], char *envp[]);
#endif
+#ifndef HAVE_MEMSET_S
+#define memset_s rep_memset_s
+int rep_memset_s(void *dest, size_t destsz, int ch, size_t count);
+#endif
+
#ifndef FALL_THROUGH
# ifdef HAVE_FALLTHROUGH_ATTRIBUTE
# define FALL_THROUGH __attribute__ ((fallthrough))
diff --git a/lib/replace/wscript b/lib/replace/wscript
index 8e2873424be..8adfffe9584 100644
--- a/lib/replace/wscript
+++ b/lib/replace/wscript
@@ -195,6 +195,23 @@ def configure(conf):
'socket nsl', checklibc=True,
headers='sys/socket.h netinet/in.h arpa/inet.h netdb.h')
+ conf.CHECK_FUNCS('memset_s memset_explicit')
+
+ conf.CHECK_CODE('''
+ #include <string.h>
+
+ int main(void)
+ {
+ char buf[] = "This is some content";
+ memset(buf, '\0', sizeof(buf)); __asm__ volatile("" : : "g"(&buf) : "memory");
+ return 0;
+ }
+ ''',
+ define='HAVE_GCC_VOLATILE_MEMORY_PROTECTION',
+ addmain=False,
+ msg='Checking for volatile memory protection',
+ local_include=False)
+
# Some old Linux systems have broken header files and
# miss the IPV6_V6ONLY define in netinet/in.h,
# but have it in linux/in6.h.
--
2.19.0
>From 53141c5a0a7a51ee8d1b0276970eb8127bbb7194 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Wed, 10 Oct 2018 16:09:32 +0200
Subject: [PATCH 2/5] s3:lib:popt: Use explicit_bzero() to burn string
Signed-off-by: Andreas Schneider <asn at samba.org>
---
source3/lib/popt_common_cmdline.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/source3/lib/popt_common_cmdline.c b/source3/lib/popt_common_cmdline.c
index fe23f84c547..971234257e6 100644
--- a/source3/lib/popt_common_cmdline.c
+++ b/source3/lib/popt_common_cmdline.c
@@ -217,7 +217,7 @@ void popt_burn_cmdline_password(int argc, char *argv[])
p = strchr_m(p, '%');
if (p != NULL) {
- memset(p, '\0', strlen(p));
+ memset_s(p, strlen(p), '\0', strlen(p));
}
found = false;
}
--
2.19.0
>From 74603f3046d13f0f08982e70e0f07e47385f9ed9 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Thu, 11 Oct 2018 07:54:41 +0200
Subject: [PATCH 3/5] talloc: Add support for secure talloc chunks
Signed-off-by: Andreas Schneider <asn at samba.org>
---
lib/talloc/talloc.c | 16 ++++++++++++++++
lib/talloc/talloc.h | 11 +++++++++++
2 files changed, 27 insertions(+)
diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c
index 54be63495ae..4d6e7e92af0 100644
--- a/lib/talloc/talloc.c
+++ b/lib/talloc/talloc.c
@@ -65,6 +65,7 @@
#define TALLOC_FLAG_LOOP 0x02
#define TALLOC_FLAG_POOL 0x04 /* This is a talloc pool */
#define TALLOC_FLAG_POOLMEM 0x08 /* This is allocated in a pool */
+#define TALLOC_FLAG_SECURE 0x10
/*
* Bits above this are random, used to make it harder to fake talloc
@@ -1091,6 +1092,7 @@ static inline int _tc_free_internal(struct talloc_chunk *tc,
{
void *ptr_to_free;
void *ptr = TC_PTR_FROM_CHUNK(tc);
+ size_t ptr_size = 0;
if (unlikely(tc->refs)) {
int is_child;
@@ -1186,8 +1188,10 @@ static inline int _tc_free_internal(struct talloc_chunk *tc,
* to be freed as poolmem, else it needs to be just freed.
*/
ptr_to_free = pool;
+ ptr_size = pool->poolsize;
} else {
ptr_to_free = tc;
+ ptr_size = tc->size;
}
if (tc->flags & TALLOC_FLAG_POOLMEM) {
@@ -1198,6 +1202,12 @@ static inline int _tc_free_internal(struct talloc_chunk *tc,
tc_memlimit_update_on_free(tc);
TC_INVALIDATE_FULL_CHUNK(tc);
+ if ((tc->flags & TALLOC_FLAG_SECURE) &&
+ (ptr_size > 0)) {
+ uint8_t *zero_ptr = ptr_to_free;
+
+ memset_s((void *)(zero_ptr + TC_HDR_SIZE), ptr_size, '\0', ptr_size);
+ }
free(ptr_to_free);
return 0;
}
@@ -1503,6 +1513,12 @@ _PUBLIC_ const char *talloc_set_name(const void *ptr, const char *fmt, ...)
return name;
}
+_PUBLIC_ void talloc_set_secure(const void *ptr)
+{
+ struct talloc_chunk *tc = talloc_chunk_from_ptr(ptr);
+
+ tc->flags |= TALLOC_FLAG_SECURE;
+}
/*
create a named talloc pointer. Any talloc pointer can be named, and
diff --git a/lib/talloc/talloc.h b/lib/talloc/talloc.h
index 7372df16fe8..f4ef2c8ae97 100644
--- a/lib/talloc/talloc.h
+++ b/lib/talloc/talloc.h
@@ -392,6 +392,17 @@ void *_talloc_steal_loc(const void *new_ctx, const void *ptr, const char *locati
*/
const char *talloc_set_name(const void *ptr, const char *fmt, ...) PRINTF_ATTRIBUTE(2,3);
+/**
+ * @brief Mark a buffer as secure buffer to be zeroed before freeing.
+ *
+ * This can be used to define a buffer as a secure buffer if it holds passwords
+ * or other secrets for examples. The buffer will be zeroed before is being
+ * freed.
+ *
+ * @param[in] ptr The talloc chunk to mark as secure.
+ */
+void talloc_set_secure(const void *ptr);
+
#ifdef DOXYGEN
/**
* @brief Change a talloc chunk's parent.
--
2.19.0
>From abd04961766dcb30ab0a97eb3c82e8964949d745 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Thu, 11 Oct 2018 08:25:22 +0200
Subject: [PATCH 4/5] talloc: Add test for talloc_set_secure()
Signed-off-by: Andreas Schneider <asn at samba.org>
---
lib/talloc/tests/test_talloc_secure.c | 52 +++++++++++++++++++++++++++
lib/talloc/wscript | 6 ++++
selftest/tests.py | 6 ++++
3 files changed, 64 insertions(+)
create mode 100644 lib/talloc/tests/test_talloc_secure.c
diff --git a/lib/talloc/tests/test_talloc_secure.c b/lib/talloc/tests/test_talloc_secure.c
new file mode 100644
index 00000000000..bc1a44e7321
--- /dev/null
+++ b/lib/talloc/tests/test_talloc_secure.c
@@ -0,0 +1,52 @@
+#include <stdarg.h>
+#include <stddef.h>
+#include <stdint.h>
+#include <setjmp.h>
+#include <cmocka.h>
+
+#include <string.h>
+#include <talloc.h>
+
+int rep_memset_s(void *dest, size_t destsz, int ch, size_t count);
+
+int rep_memset_s(void *dest, size_t destsz, int ch, size_t count)
+{
+ check_expected_ptr(dest);
+ check_expected(destsz);
+ check_expected(ch);
+ check_expected(count);
+
+ return 0;
+}
+
+/* A test case that does nothing and succeeds. */
+static void test_talloc_set_secure(void **state)
+{
+ TALLOC_CTX *mem_ctx = NULL;
+ char *password = NULL;
+
+ mem_ctx = talloc_new(NULL);
+ assert_non_null(mem_ctx);
+
+ password = talloc_strdup(mem_ctx, "secret");
+ assert_non_null(password);
+ talloc_set_secure(password);
+
+ expect_string(rep_memset_s, dest, "secret");
+ expect_value(rep_memset_s, destsz, strlen(password) + 1);
+ expect_value(rep_memset_s, ch, (int)'\0');
+ expect_value(rep_memset_s, count, strlen(password) + 1);
+
+ talloc_free(mem_ctx);
+}
+
+int main(void)
+{
+ const struct CMUnitTest tests[] = {
+ cmocka_unit_test(test_talloc_set_secure),
+ };
+
+ cmocka_set_message_output(CM_OUTPUT_SUBUNIT);
+
+ return cmocka_run_group_tests(tests, NULL, NULL);
+}
diff --git a/lib/talloc/wscript b/lib/talloc/wscript
index a5841b46e4e..93dda3e402d 100644
--- a/lib/talloc/wscript
+++ b/lib/talloc/wscript
@@ -139,6 +139,12 @@ def build(bld):
private_library=private_library,
manpages='man/talloc.3')
+ # We only check for cmocka in the Samba tree
+ bld.SAMBA_BINARY('test_talloc_secure',
+ 'tests/test_talloc_secure.c',
+ 'talloc cmocka',
+ install=False)
+
if not bld.CONFIG_SET('USING_SYSTEM_PYTALLOC_UTIL'):
for env in bld.gen_python_environments(['PKGCONFIGDIR']):
name = bld.pyembed_libname('pytalloc-util')
diff --git a/selftest/tests.py b/selftest/tests.py
index daa3bb7390c..484ee447925 100644
--- a/selftest/tests.py
+++ b/selftest/tests.py
@@ -41,6 +41,8 @@ have_man_pages_support = ("XSLTPROC_MANPAGES" in config_hash)
with_pam = ("WITH_PAM" in config_hash)
pam_wrapper_so_path = config_hash["LIBPAM_WRAPPER_SO_PATH"]
pam_set_items_so_path = config_hash["PAM_SET_ITEMS_SO_PATH"]
+with_talloc = ("TALLOC_BUILD_VERSION_MAJOR" in config_hash)
+with_memset_s = ("HAVE_MEMSET_S" in config_hash)
planpythontestsuite("none", "samba.tests.source", py3_compatible=True)
if have_man_pages_support:
@@ -193,6 +195,10 @@ plantestsuite("samba.unittests.lib_util_modules", "none",
plantestsuite("samba.unittests.smb1cli_session", "none",
[os.path.join(bindir(), "default/libcli/smb/test_smb1cli_session")])
+if with_talloc and not with_memset_s:
+ plantestsuite("talloc.set_secure", "none",
+ [os.path.join(bindir(), "default/lib/talloc/test_talloc_secure")])
+
plantestsuite("samba.unittests.tldap", "none",
[os.path.join(bindir(), "default/source3/test_tldap")])
plantestsuite("samba.unittests.rfc1738", "none",
--
2.19.0
>From 3dfa917513eedb89de5e4ba3832fcf627d527cf4 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Thu, 11 Oct 2018 08:46:26 +0200
Subject: [PATCH 5/5] ABI
It is strange that you get an error if you bump the minor version.
Because this is what you need to do if you add a function to a library.
The patch level is only bumped if there is a code change which doesn't
change the ABI.
The new version should be 2.2.0
---
lib/talloc/ABI/talloc-2.1.15.sigs | 66 +++++++++++++++++++++++++++++++
lib/talloc/wscript | 2 +-
2 files changed, 67 insertions(+), 1 deletion(-)
create mode 100644 lib/talloc/ABI/talloc-2.1.15.sigs
diff --git a/lib/talloc/ABI/talloc-2.1.15.sigs b/lib/talloc/ABI/talloc-2.1.15.sigs
new file mode 100644
index 00000000000..985b2e569ca
--- /dev/null
+++ b/lib/talloc/ABI/talloc-2.1.15.sigs
@@ -0,0 +1,66 @@
+_talloc: void *(const void *, size_t)
+_talloc_array: void *(const void *, size_t, unsigned int, const char *)
+_talloc_free: int (void *, const char *)
+_talloc_get_type_abort: void *(const void *, const char *, const char *)
+_talloc_memdup: void *(const void *, const void *, size_t, const char *)
+_talloc_move: void *(const void *, const void *)
+_talloc_pooled_object: void *(const void *, size_t, const char *, unsigned int, size_t)
+_talloc_realloc: void *(const void *, void *, size_t, const char *)
+_talloc_realloc_array: void *(const void *, void *, size_t, unsigned int, const char *)
+_talloc_reference_loc: void *(const void *, const void *, const char *)
+_talloc_set_destructor: void (const void *, int (*)(void *))
+_talloc_steal_loc: void *(const void *, const void *, const char *)
+_talloc_zero: void *(const void *, size_t, const char *)
+_talloc_zero_array: void *(const void *, size_t, unsigned int, const char *)
+talloc_asprintf: char *(const void *, const char *, ...)
+talloc_asprintf_append: char *(char *, const char *, ...)
+talloc_asprintf_append_buffer: char *(char *, const char *, ...)
+talloc_autofree_context: void *(void)
+talloc_check_name: void *(const void *, const char *)
+talloc_disable_null_tracking: void (void)
+talloc_enable_leak_report: void (void)
+talloc_enable_leak_report_full: void (void)
+talloc_enable_null_tracking: void (void)
+talloc_enable_null_tracking_no_autofree: void (void)
+talloc_find_parent_byname: void *(const void *, const char *)
+talloc_free_children: void (void *)
+talloc_get_name: const char *(const void *)
+talloc_get_size: size_t (const void *)
+talloc_increase_ref_count: int (const void *)
+talloc_init: void *(const char *, ...)
+talloc_is_parent: int (const void *, const void *)
+talloc_named: void *(const void *, size_t, const char *, ...)
+talloc_named_const: void *(const void *, size_t, const char *)
+talloc_parent: void *(const void *)
+talloc_parent_name: const char *(const void *)
+talloc_pool: void *(const void *, size_t)
+talloc_realloc_fn: void *(const void *, void *, size_t)
+talloc_reference_count: size_t (const void *)
+talloc_reparent: void *(const void *, const void *, const void *)
+talloc_report: void (const void *, FILE *)
+talloc_report_depth_cb: void (const void *, int, int, void (*)(const void *, int, int, int, void *), void *)
+talloc_report_depth_file: void (const void *, int, int, FILE *)
+talloc_report_full: void (const void *, FILE *)
+talloc_set_abort_fn: void (void (*)(const char *))
+talloc_set_log_fn: void (void (*)(const char *))
+talloc_set_log_stderr: void (void)
+talloc_set_memlimit: int (const void *, size_t)
+talloc_set_name: const char *(const void *, const char *, ...)
+talloc_set_name_const: void (const void *, const char *)
+talloc_set_secure: void (const void *)
+talloc_show_parents: void (const void *, FILE *)
+talloc_strdup: char *(const void *, const char *)
+talloc_strdup_append: char *(char *, const char *)
+talloc_strdup_append_buffer: char *(char *, const char *)
+talloc_strndup: char *(const void *, const char *, size_t)
+talloc_strndup_append: char *(char *, const char *, size_t)
+talloc_strndup_append_buffer: char *(char *, const char *, size_t)
+talloc_test_get_magic: int (void)
+talloc_total_blocks: size_t (const void *)
+talloc_total_size: size_t (const void *)
+talloc_unlink: int (const void *, void *)
+talloc_vasprintf: char *(const void *, const char *, va_list)
+talloc_vasprintf_append: char *(char *, const char *, va_list)
+talloc_vasprintf_append_buffer: char *(char *, const char *, va_list)
+talloc_version_major: int (void)
+talloc_version_minor: int (void)
diff --git a/lib/talloc/wscript b/lib/talloc/wscript
index 93dda3e402d..0b3a7ab65ef 100644
--- a/lib/talloc/wscript
+++ b/lib/talloc/wscript
@@ -1,7 +1,7 @@
#!/usr/bin/env python
APPNAME = 'talloc'
-VERSION = '2.1.14'
+VERSION = '2.1.15'
import os
import sys
--
2.19.0
More information about the samba-technical
mailing list