From bab0f268d3b07a20347a72d5aa48bb077ab5262b 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. (cherry picked from commit c4cc26ed3771814d302e397137c9fe8b2440e889) --- .../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 08c450c60..b9dc5ab32 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" @@ -982,11 +983,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 a9a87e51c..a2594173c 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; }