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