From 395c228bee78515155d7e73fbbcd35fffb0d7fa7 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 17 Apr 2021 17:59:43 +0100 Subject: [PATCH] Adopt a new universal implementation of smemclr(). This new implementation uses the same optimisation-barrier technique that I used in various places in testsc: have a no-op function, and a volatile function pointer pointing at it, and then call through the function pointer, so that nothing actually happens (apart from the physical call and return) but the compiler has to assume that _anything_ might have happened. Doing this just after a memset enforces that the compiler can't have thrown away the memset, because the called function might (for example) check that all the memory really is zero and abort if not. I've been turning this over in my mind ever since coming up with the technique for testsc. I think it's far more robust than the previous smemclr technique: so much so that I'm switching to using it _everywhere_, and no longer using platform alternatives like Windows's SecureZeroMemory(). --- CMakeLists.txt | 1 + cmake/platforms/windows.cmake | 3 --- defs.h | 28 ---------------------- unix/CMakeLists.txt | 1 - utils/buildinfo.c | 3 --- utils/smemclr.c | 44 +++++++++++++++++++++-------------- windows/CMakeLists.txt | 3 --- 7 files changed, 28 insertions(+), 55 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 1047773e..178c1baf 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -40,6 +40,7 @@ add_library(utils STATIC utils/seat_connection_fatal.c utils/sessprep.c utils/sk_free_peer_info.c + utils/smemclr.c utils/smemeq.c utils/ssh2_pick_fingerprint.c utils/sshutils.c diff --git a/cmake/platforms/windows.cmake b/cmake/platforms/windows.cmake index 2adda297..0760ec67 100644 --- a/cmake/platforms/windows.cmake +++ b/cmake/platforms/windows.cmake @@ -36,9 +36,6 @@ define_negation(NO_MULTIMON HAVE_MULTIMON_H) check_include_files("windows.h;htmlhelp.h" HAVE_HTMLHELP_H) define_negation(NO_HTMLHELP HAVE_HTMLHELP_H) -check_symbol_exists(SecureZeroMemory "windows.h" HAVE_SECUREZEROMEMORY) -define_negation(NO_SECUREZEROMEMORY HAVE_SECUREZEROMEMORY) - check_symbol_exists(strtoumax "inttypes.h" HAVE_STRTOUMAX) check_symbol_exists(AddDllDirectory "windows.h" HAVE_ADDDLLDIRECTORY) check_symbol_exists(SetDefaultDllDirectories "windows.h" diff --git a/defs.h b/defs.h index 86f8edde..f175d257 100644 --- a/defs.h +++ b/defs.h @@ -201,32 +201,4 @@ typedef struct PacketProtocolLayer PacketProtocolLayer; #define NORETURN #endif -/* ---------------------------------------------------------------------- - * Platform-specific definitions. - * - * Most of these live in the per-platform header files, of which - * puttyps.h selects the appropriate one. But some of the sources - * (particularly standalone test applications) would prefer not to - * have to include a per-platform header at all, because that makes it - * more portable to platforms not supported by the code base as a - * whole (for example, compiling purely computational parts of the - * code for specialist platforms for test and analysis purposes). So - * any definition that has to affect even _those_ modules will have to - * go here, with the key constraint being that this code has to come - * to _some_ decision even if the compilation platform is not a - * recognised one at all. - */ - -/* Purely computational code uses smemclr(), so we have to make the - * decision here about whether that's provided by utils.c or by a - * platform implementation. We define PLATFORM_HAS_SMEMCLR to suppress - * utils.c's definition. */ -#ifdef _WINDOWS -/* Windows provides the API function 'SecureZeroMemory', which we use - * unless the user has told us not to by defining NO_SECUREZEROMEMORY. */ -#ifndef NO_SECUREZEROMEMORY -#define PLATFORM_HAS_SMEMCLR -#endif -#endif - #endif /* PUTTY_DEFS_H */ diff --git a/unix/CMakeLists.txt b/unix/CMakeLists.txt index d77f7c9d..ebb77a55 100644 --- a/unix/CMakeLists.txt +++ b/unix/CMakeLists.txt @@ -18,7 +18,6 @@ add_platform_sources_to_library(utils utils/pollwrap.c utils/signal.c utils/x11_ignore_error.c - ../utils/smemclr.c # Compiled icon pixmap files xpmpucfg.c xpmputty.c xpmptcfg.c xpmpterm.c # We want the ISO C implementation of ltime(), because we don't have diff --git a/utils/buildinfo.c b/utils/buildinfo.c index 1db65a0d..2c40d6c2 100644 --- a/utils/buildinfo.c +++ b/utils/buildinfo.c @@ -130,9 +130,6 @@ char *buildinfo(const char *newline) #if defined _WINDOWS && defined MINEFIELD strbuf_catf(buf, "%sBuild option: MINEFIELD", newline); #endif -#ifdef NO_SECUREZEROMEMORY - strbuf_catf(buf, "%sBuild option: NO_SECUREZEROMEMORY", newline); -#endif #ifdef NO_IPV6 strbuf_catf(buf, "%sBuild option: NO_IPV6", newline); #endif diff --git a/utils/smemclr.c b/utils/smemclr.c index afe919d1..3cd7a761 100644 --- a/utils/smemclr.c +++ b/utils/smemclr.c @@ -6,18 +6,36 @@ * won't optimise away memsets on variables that are about to be freed * or go out of scope. See * https://buildsecurityin.us-cert.gov/bsi-rules/home/g1/771-BSI.html - * - * Some platforms (e.g. Windows) may provide their own version of this - * function. */ #include "defs.h" #include "misc.h" +/* + * Trivial function that is given a pointer to some memory and ignores + * it. + */ +static void no_op(void *ptr, size_t size) {} + +/* + * Function pointer that is given a pointer to some memory, and from + * the compiler's point of view, _might_ read it, or otherwise depend + * on its contents. + * + * In fact, this function pointer always points to no_op() above. But + * because the pointer itself is volatile-qualified, the compiler + * isn't allowed to optimise based on the assumption that that will + * always be the case. So it has to call through the function pointer + * anyway, on the basis that it _might_ have magically changed at run + * time into a pointer to some completely arbitrary function. And + * therefore it must also avoid optimising away any observable effect + * beforehand that a completely arbitrary function might depend on - + * such as the zeroing of our memory re3gion. + */ +static void (*const volatile maybe_read)(void *ptr, size_t size) = no_op; + void smemclr(void *b, size_t n) { - volatile char *vp; - if (b && n > 0) { /* * Zero out the memory. @@ -25,18 +43,10 @@ void smemclr(void *b, size_t n) memset(b, 0, n); /* - * Perform a volatile access to the object, forcing the - * compiler to admit that the previous memset was important. - * - * This while loop should in practice run for zero iterations - * (since we know we just zeroed the object out), but in - * theory (as far as the compiler knows) it might range over - * the whole object. (If we had just written, say, '*vp = - * *vp;', a compiler could in principle have 'helpfully' - * optimised the memset into only zeroing out the first byte. - * This should be robust.) + * Call the above function pointer, which (for all the + * compiler knows) might check that we've really zeroed the + * memory. */ - vp = b; - while (*vp) vp++; + maybe_read(b, n); } } diff --git a/windows/CMakeLists.txt b/windows/CMakeLists.txt index 41b617ab..b512f459 100644 --- a/windows/CMakeLists.txt +++ b/windows/CMakeLists.txt @@ -27,9 +27,6 @@ add_platform_sources_to_library(utils utils/version.c utils/win_strerror.c winucs.c) -if(NOT HAVE_SECUREZEROMEMORY) - add_platform_sources_to_library(utils ../utils/smemclr.c) -endif() if(NOT HAVE_STRTOUMAX) add_platform_sources_to_library(utils utils/strtoumax.c) endif()