From c4cc26ed3771814d302e397137c9fe8b2440e889 Mon Sep 17 00:00:00 2001 From: Jelte Fennema Date: Wed, 11 Mar 2020 20:05:36 +0100 Subject: [PATCH] =?UTF-8?q?Semmle:=20Ensure=20stack=20memory=20is=20not=20?= =?UTF-8?q?leaked=20through=20uninitialized=E2=80=A6=20(#3561)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New stack memory can contain anything including passwords/private keys. In these functions we return structs that can have their padding bytes uninitialized. By first zeroing out the struct fully, we try to ensure that any data that is in these padding bytes is at least overwritten once. It might not be zero anymore after setting the fields, but at least it shouldn't be private data anymore. --- .../distributed/executor/adaptive_executor.c | 13 ++++++++----- src/backend/distributed/metadata/node_metadata.c | 14 +++++++++----- src/include/distributed/citus_safe_lib.h | 2 ++ src/include/distributed/version_compat.h | 12 ++++++++---- 4 files changed, 27 insertions(+), 14 deletions(-) diff --git a/src/backend/distributed/executor/adaptive_executor.c b/src/backend/distributed/executor/adaptive_executor.c index a7dc7c67f..39e071f2d 100644 --- a/src/backend/distributed/executor/adaptive_executor.c +++ b/src/backend/distributed/executor/adaptive_executor.c @@ -135,6 +135,7 @@ #include "distributed/adaptive_executor.h" #include "distributed/cancel_utils.h" #include "distributed/citus_custom_scan.h" +#include "distributed/citus_safe_lib.h" #include "distributed/connection_management.h" #include "distributed/deparse_shard_query.h" #include "distributed/distributed_execution_locks.h" @@ -1002,11 +1003,13 @@ static TransactionProperties DecideTransactionPropertiesForTaskList(RowModifyLevel modLevel, List *taskList, bool exludeFromTransaction) { - TransactionProperties xactProperties = { - .errorOnAnyFailure = false, - .useRemoteTransactionBlocks = TRANSACTION_BLOCKS_ALLOWED, - .requires2PC = false - }; + TransactionProperties xactProperties; + + /* ensure uninitialized padding doesn't escape the function */ + memset_struct_0(xactProperties); + xactProperties.errorOnAnyFailure = false; + xactProperties.useRemoteTransactionBlocks = TRANSACTION_BLOCKS_ALLOWED; + xactProperties.requires2PC = false; if (taskList == NIL) { diff --git a/src/backend/distributed/metadata/node_metadata.c b/src/backend/distributed/metadata/node_metadata.c index 0e9fab79a..91cea7ab1 100644 --- a/src/backend/distributed/metadata/node_metadata.c +++ b/src/backend/distributed/metadata/node_metadata.c @@ -21,6 +21,7 @@ #include "catalog/namespace.h" #include "commands/sequence.h" #include "distributed/citus_acquire_lock.h" +#include "distributed/citus_safe_lib.h" #include "distributed/colocation_utils.h" #include "distributed/commands.h" #include "distributed/commands/utility_hook.h" @@ -109,11 +110,14 @@ PG_FUNCTION_INFO_V1(get_shard_id_for_distribution_column); static NodeMetadata DefaultNodeMetadata() { - NodeMetadata nodeMetadata = { - .nodeRack = WORKER_DEFAULT_RACK, - .shouldHaveShards = true, - .groupId = INVALID_GROUP_ID, - }; + NodeMetadata nodeMetadata; + + /* ensure uninitialized padding doesn't escape the function */ + memset_struct_0(nodeMetadata); + nodeMetadata.nodeRack = WORKER_DEFAULT_RACK; + nodeMetadata.shouldHaveShards = true; + nodeMetadata.groupId = INVALID_GROUP_ID; + return nodeMetadata; } diff --git a/src/include/distributed/citus_safe_lib.h b/src/include/distributed/citus_safe_lib.h index f1552fbe1..c934e931b 100644 --- a/src/include/distributed/citus_safe_lib.h +++ b/src/include/distributed/citus_safe_lib.h @@ -26,4 +26,6 @@ void * SafeBsearch(const void *key, const void *ptr, rsize_t count, rsize_t size int (*comp)(const void *, const void *)); int SafeSnprintf(char *str, rsize_t count, const char *fmt, ...); +#define memset_struct_0(variable) memset(&variable, 0, sizeof(variable)) + #endif diff --git a/src/include/distributed/version_compat.h b/src/include/distributed/version_compat.h index 5feaaa12a..4c9278ac4 100644 --- a/src/include/distributed/version_compat.h +++ b/src/include/distributed/version_compat.h @@ -12,8 +12,10 @@ #define VERSION_COMPAT_H #include "postgres.h" + #include "commands/explain.h" #include "catalog/namespace.h" +#include "distributed/citus_safe_lib.h" #include "nodes/parsenodes.h" #include "parser/parse_func.h" #if (PG_VERSION_NUM >= 120000) @@ -71,10 +73,12 @@ FileReadCompat(FileCompat *file, char *buffer, int amount, uint32 wait_event_inf static inline FileCompat FileCompatFromFileStart(File fileDesc) { - FileCompat fc = { - .fd = fileDesc, - .offset = 0 - }; + FileCompat fc; + + /* ensure uninitialized padding doesn't escape the function */ + memset_struct_0(fc); + fc.fd = fileDesc; + fc.offset = 0; return fc; }