From: Oliver Kurth Date: Fri, 26 Oct 2018 17:44:59 +0000 (-0700) Subject: Develop log APIs to fix security holes in the tools log messages. X-Git-Tag: stable-11.0.0~353 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d32b2f1f77af1246d50fd9003e15515ad0ba250a;p=thirdparty%2Fopen-vm-tools.git Develop log APIs to fix security holes in the tools log messages. Security artifacts such as command args on the host should not be logged in the VMX log files on the host. This change creates APIs so that different log messages can be used for host and guest. Refactored the log plumbing to minimize code duplication when calling the different implementation of logging to the vmx and logging in guest. Fixed one instance of security issue in vmbackup, to show how to use the new APIs. --- diff --git a/open-vm-tools/lib/include/vmware/tools/log.h b/open-vm-tools/lib/include/vmware/tools/log.h index 4b4b9dcfe..91e24478d 100644 --- a/open-vm-tools/lib/include/vmware/tools/log.h +++ b/open-vm-tools/lib/include/vmware/tools/log.h @@ -332,9 +332,44 @@ void VMTools_SetupVmxGuestLog(gboolean refreshRpcChannel, GKeyFile *cfg, const gchar *level); +typedef enum { + TO_HOST, + IN_GUEST +} LogWhere; + +void +VMTools_Log(LogWhere where, + GLogLevelFlags level, + const gchar *domain, + const gchar *fmt, + ...); + G_END_DECLS +#define host_warning(fmt, ...) \ + VMTools_Log(TO_HOST, G_LOG_LEVEL_WARNING, G_LOG_DOMAIN, fmt, ## __VA_ARGS__) + +#define guest_warning(fmt, ...) \ + VMTools_Log(IN_GUEST, G_LOG_LEVEL_WARNING, G_LOG_DOMAIN, fmt, ## __VA_ARGS__) + +#define host_message(fmt, ...) \ + VMTools_Log(TO_HOST, G_LOG_LEVEL_MESSAGE, G_LOG_DOMAIN, fmt, ## __VA_ARGS__) + +#define guest_message(fmt, ...) \ + VMTools_Log(IN_GUEST, G_LOG_LEVEL_MESSAGE, G_LOG_DOMAIN, fmt, ## __VA_ARGS__) + +#define host_info(fmt, ...) \ + VMTools_Log(TO_HOST, G_LOG_LEVEL_INFO, G_LOG_DOMAIN, fmt, ## __VA_ARGS__) + +#define guest_info(fmt, ...) \ + VMTools_Log(IN_GUEST, G_LOG_LEVEL_INFO, G_LOG_DOMAIN, fmt, ## __VA_ARGS__) + +#define host_debug(fmt, ...) \ + VMTools_Log(TO_HOST, G_LOG_LEVEL_DEBUG, G_LOG_DOMAIN, fmt, ## __VA_ARGS__) + +#define guest_debug(fmt, ...) \ + VMTools_Log(IN_GUEST, G_LOG_LEVEL_DEBUG, G_LOG_DOMAIN, fmt, ## __VA_ARGS__) + /** @} */ #endif /* _VMTOOLS_LOG_H_ */ - diff --git a/open-vm-tools/libvmtools/vmtoolsLog.c b/open-vm-tools/libvmtools/vmtoolsLog.c index 4722cfce0..f86d41e03 100644 --- a/open-vm-tools/libvmtools/vmtoolsLog.c +++ b/open-vm-tools/libvmtools/vmtoolsLog.c @@ -144,6 +144,12 @@ typedef struct LogEntry { } LogEntry; +static gboolean gLogInitialized = FALSE; +/* + * This lock protects most of the static global below. + * Note statically allocated GRecMutex does not need an explicit initialization. + */ +static GRecMutex gLogStateMutex; static gchar *gLogDomain = NULL; static GPtrArray *gCachedLogs = NULL; static guint gDroppedLogCount = 0; @@ -156,9 +162,6 @@ static LogHandler *gDefaultData; static LogHandler *gErrorData; static LogHandler *gErrorSyslog; static GPtrArray *gDomains = NULL; -static gboolean gLogInitialized = FALSE; -/* This lock protects the gGlibLoggingStopped and gLogHandlerEnabled flags */ -static GRecMutex gLogStateMutex; /* Whether to stop logging using glib logging functions */ static gboolean gGlibLoggingStopped = FALSE; /* Whether to enable proxy messages to log handler */ @@ -678,25 +681,24 @@ exit: /** - * Log handler function that glib invokes on each g_xxx(...) invocation. + * Helper function to acquire locks and log to host side. * * @param[in] domain Log domain. * @param[in] level Log level. * @param[in] message Message to log. - * @param[in] _data LogHandler pointer. */ static void -VMToolsLog(const gchar *domain, - GLogLevelFlags level, - const gchar *message, - gpointer _data) +LogToHost(const gchar *domain, + GLogLevelFlags level, + const gchar *message) { - if (!gLogEnabled) { + if (!gLogEnabled || !gUseVmxGuestLog) { return; } + /* - * To avoid nested logging inside of RpcChannel, we need to disable + * To avoid nested logging, such as a RpcChannel error, we need to disable * logging here. See bug 1069390. * This actually stop the glib based logging only. However, * log messages are saved directly to the file system using a lower @@ -705,33 +707,64 @@ VMToolsLog(const gchar *domain, VMTools_AcquireLogStateLock(); StopGlibLogging(); - if (gUseVmxGuestLog) { + /* + * Protect the RPC channel and friends data race + * Other thread might call g_xxx() or reinitialize the vmx guest logger. + * + * Recursive mutex is required as the set up function might calls + * g_xxx() functions to log messages which would call this function here. + * + * Always acquire the log state mutex first followed by + * the vmxGuestLog mutex to avoid a deadlock. + * + * This is because Debug()/Warning() functions call VMToolsLogWrapper() + * which acquire the log state mutex and call g_xxx function which ends + * up here. + * + * If the order is reversed here, we could end up deadlock + * between two threads, one calling g_xxx(), and the other calling + * Debug()/Warning() + */ - /* - * Protect the RPC channel and friends data race - * Other thread might call g_xxx() or reinitialize the vmx guest logger. - * - * Recursive mutex is required as the set up function might calls - * g_xxx() functions to log messages which would call this function here. - * - * Always acquire the log state mutex first followed by - * the vmxGuestLog mutex to avoid a deadlock. - * - * This is because Debug()/Warning() functions call VMToolsLogWrapper() - * which acquire the log state mutex and call g_xxx function which ends - * up here. - * - * If the order is reversed here, we could end up deadlock - * between two threads, one calling g_xxx(), and the other calling - * Debug()/Warning() - */ + g_rec_mutex_lock(&gVmxGuestLogMutex); + VmxGuestLog(NULL == domain ? gLogDomain : domain, + level, message); + g_rec_mutex_unlock(&gVmxGuestLogMutex); + + RestartGlibLogging(); + VMTools_ReleaseLogStateLock(); +} + + +/** + * Helper function to acquire lock and log on the guest side. + * + * @param[in] domain Log domain. + * @param[in] level Log level. + * @param[in] message Message to log. + * @param[in] _data LogHandler pointer. + */ - g_rec_mutex_lock(&gVmxGuestLogMutex); - VmxGuestLog(NULL == domain ? gLogDomain : domain, - level, message); - g_rec_mutex_unlock(&gVmxGuestLogMutex); +static void +LogToGuest(const gchar *domain, + GLogLevelFlags level, + const gchar *message, + gpointer _data) +{ + if (!gLogEnabled) { + return; } + /* + * Disable glib logging as this function could be invoked from the glib + * log handler. + * This is OK since the function is called either directly with no intention + * of writing log to host or we shall write the same log to the host + * seprately using the LogToHost() function. + */ + VMTools_AcquireLogStateLock(); + StopGlibLogging(); + /* Proxy to the LogHandler object pointed by _data. */ VMToolsLogInt(domain, level, message, _data); @@ -740,6 +773,26 @@ VMToolsLog(const gchar *domain, } +/** + * Log handler function that glib invokes on each g_xxx(...) invocation. + * + * @param[in] domain Log domain. + * @param[in] level Log level. + * @param[in] message Message to log. + * @param[in] _data LogHandler pointer. + */ + +static void +VMToolsLog(const gchar *domain, + GLogLevelFlags level, + const gchar *message, + gpointer _data) +{ + LogToHost(domain, level, message); + LogToGuest(domain, level, message, _data); +} + + /* ******************************************************************************* * VMToolsDefaultLogFilePath -- */ /** @@ -1265,7 +1318,6 @@ MarkLogInitialized(void) { if (!gLogInitialized) { gLogInitialized = TRUE; - g_rec_mutex_init(&gLogStateMutex); } } @@ -1308,6 +1360,7 @@ exit: /** + * Internal Helper function without holding the state lock. * Configures the logging system according to the configuration in the given * dictionary. * @@ -1323,11 +1376,11 @@ exit: * @param[in] reset Whether to reset the logging subsystem first. */ -void -VMTools_ConfigLogging(const gchar *defaultDomain, - GKeyFile *cfg, - gboolean force, - gboolean reset) +static void +VMToolsConfigLoggingInt(const gchar *defaultDomain, + GKeyFile *cfg, + gboolean force, + gboolean reset) { gboolean allocDict = (cfg == NULL); gchar **list; @@ -1514,6 +1567,36 @@ VMTools_ConfigLogging(const gchar *defaultDomain, } +/** + * Configures the logging system according to the configuration in the given + * dictionary. + * + * Optionally, it's possible to reset the logging subsystem; this will shut + * down all log handlers managed by the vmtools library before configuring + * the log system, which means that logging will behave as if the application + * was just started. A visible side-effect of this is that log files may be + * rotated (if they're not configure for appending). + * + * @param[in] defaultDomain Name of the default log domain. + * @param[in] cfg The configuration data. May be NULL. + * @param[in] force Whether to force logging to be enabled. + * @param[in] reset Whether to reset the logging subsystem first. + */ + +void +VMTools_ConfigLogging(const gchar *defaultDomain, + GKeyFile *cfg, + gboolean force, + gboolean reset) +{ + VMTools_AcquireLogStateLock(); + + VMToolsConfigLoggingInt(defaultDomain, cfg, force, reset); + + VMTools_ReleaseLogStateLock(); +} + + /* Wrappers for VMware's logging functions. */ /* @@ -2374,3 +2457,107 @@ done: VMTools_ReleaseLogStateLock(); } + + +/** + * Helper function to return the matching LogHandler for a domain name + * + * @param[in] domain Log domain. + * @return The matching LogHandler if found or + * the default LogHandler. + */ + +static LogHandler * +GetLogHandlerByDomain(const gchar *domain) +{ + guint i; + + if (NULL == gDomains) { + return gDefaultData; + } + + for (i = 0; i < gDomains->len; i++) { + LogHandler *data = g_ptr_array_index(gDomains, i); + + if (0 == strcmp(data->domain, domain)) { + return data; + } + } + + return gDefaultData; +} + + +/** + * Helper function to plumb the log to either host or guest. + * + * @param[in] where where the log should go. + * @param[in] level Log level. + * @param[in] domain Log domain. + * @param[in] fmt Log message output format. + * @param[in] args variadic format parameter list. + */ + +static void +LogWhereLevelV(LogWhere where, + GLogLevelFlags level, + const gchar *domain, + const gchar *fmt, + va_list args) +{ + gchar buf[4096]; + + g_vsnprintf(buf, sizeof buf, fmt, args); + buf[sizeof buf - 1] = '\0'; + + domain = (NULL != domain) ? domain : gLogDomain; + ASSERT(NULL != domain); + + switch (where) { + case TO_HOST: + LogToHost(domain, level, buf); + break; + + case IN_GUEST: { + LogHandler *data = GetLogHandlerByDomain(domain); + + if (NULL != data) { + LogToGuest(domain, level, buf, data); + } + break; + } + default: + NOT_REACHED(); + } +} + + +/** + ******************************************************************************* + * VMTools_Log -- */ /** + * + * Entry log function. + * + * Use this function to log different log messages on host and in guest. Usually + * a macro is defined to invoke this function once for the host side logging and + * the second time for the guest side logging. + * + * @param[in] where where the log should go. + * @param[in] level Log level. + * @param[in] domain Log domain. + * @param[in] fmt Log message output format. + */ + +void +VMTools_Log(LogWhere where, + GLogLevelFlags level, + const gchar *domain, + const gchar *fmt, + ...) +{ + va_list args; + + va_start(args, fmt); + LogWhereLevelV(where, level, domain, fmt, args); + va_end(args); +} diff --git a/open-vm-tools/services/plugins/vmbackup/scriptOps.c b/open-vm-tools/services/plugins/vmbackup/scriptOps.c index cfff69aef..fc4ad5c64 100644 --- a/open-vm-tools/services/plugins/vmbackup/scriptOps.c +++ b/open-vm-tools/services/plugins/vmbackup/scriptOps.c @@ -1,5 +1,5 @@ /********************************************************* - * Copyright (C) 2007-2016 VMware, Inc. All rights reserved. + * Copyright (C) 2007-2018 VMware, Inc. All rights reserved. * * This program is free software; you can redistribute it and/or modify it * under the terms of the GNU Lesser General Public License as published @@ -33,6 +33,8 @@ #include "procMgr.h" #include "str.h" #include "util.h" +#include "vmware/tools/log.h" + /* * These are legacy scripts used before the vmbackup-based backups. To @@ -163,7 +165,8 @@ VmBackupRunNextScript(VmBackupScriptOp *op) // IN/OUT scriptOp); } if (cmd != NULL) { - g_debug("Running script: %s\n", cmd); + host_debug("Running script: %s\n", scripts[index].path); + guest_debug("Running script: %s\n", cmd); scripts[index].proc = ProcMgr_ExecAsync(cmd, NULL); } else { g_debug("Failed to allocate memory to run script: %s\n",