jonathan/open-vm-tools
1
0
Fork 0
forked from rpms/open-vm-tools
Code Pull Requests Activity
a10-cve-2025-22247
open-vm-tools/0002-open-vm-tools-cve-2025-22247.patch

375 lines
12 KiB
Diff
Raw Permalink Blame History

From 7874e572b5aac5a418551dc5e3935c1e74bf6f1f Mon Sep 17 00:00:00 2001
From: John Wolfe <john.wolfe@broadcom.com>
Date: Mon, 5 May 2025 15:58:03 -0700
Subject: [PATCH] Validate user names and file paths
Prevent usage of illegal characters in user names and file paths.
Also, disallow unexpected symlinks in file paths.
This patch contains changes to common source files not applicable
to open-vm-tools.
All files being updated should be consider to have the copyright to
be updated to:
* Copyright (c) XXXX-2025 Broadcom. All Rights Reserved.
* The term "Broadcom" refers to Broadcom Inc. and/or its subsidiaries.
The 2025 Broadcom copyright information update is not part of this
patch set to allow the patch to be easily applied to previous
open-vm-tools source releases.
---
open-vm-tools/vgauth/common/VGAuthUtil.c | 33 +++++++++
open-vm-tools/vgauth/common/VGAuthUtil.h | 2 +
open-vm-tools/vgauth/common/prefs.h | 3 +
open-vm-tools/vgauth/common/usercheck.c | 23 +++++-
open-vm-tools/vgauth/serviceImpl/alias.c | 74 ++++++++++++++++++-
open-vm-tools/vgauth/serviceImpl/service.c | 27 +++++++
open-vm-tools/vgauth/serviceImpl/serviceInt.h | 1 +
7 files changed, 160 insertions(+), 3 deletions(-)
diff --git a/open-vm-tools/vgauth/common/VGAuthUtil.c b/open-vm-tools/vgauth/common/VGAuthUtil.c
index 76383c462..9c2adb8d0 100644
--- a/open-vm-tools/vgauth/common/VGAuthUtil.c
+++ b/open-vm-tools/vgauth/common/VGAuthUtil.c
@@ -309,3 +309,36 @@ Util_Assert(const char *cond,
#endif
g_assert(0);
}
+
+
+/*
+ ******************************************************************************
+ * Util_Utf8CaseCmp -- */ /**
+ *
+ * Case insensitive comparison for utf8 strings which can have non-ascii
+ * characters.
+ *
+ * @param[in] str1 Null terminated utf8 string.
+ * @param[in] str2 Null terminated utf8 string.
+ *
+ ******************************************************************************
+ */
+
+int
+Util_Utf8CaseCmp(const gchar *str1,
+ const gchar *str2)
+{
+ int ret;
+ gchar *str1Case;
+ gchar *str2Case;
+
+ str1Case = g_utf8_casefold(str1, -1);
+ str2Case = g_utf8_casefold(str2, -1);
+
+ ret = g_strcmp0(str1Case, str2Case);
+
+ g_free(str1Case);
+ g_free(str2Case);
+
+ return ret;
+}
diff --git a/open-vm-tools/vgauth/common/VGAuthUtil.h b/open-vm-tools/vgauth/common/VGAuthUtil.h
index f7f3aa216..ef32a91da 100644
--- a/open-vm-tools/vgauth/common/VGAuthUtil.h
+++ b/open-vm-tools/vgauth/common/VGAuthUtil.h
@@ -105,4 +105,6 @@ gboolean Util_CheckExpiration(const GTimeVal *start, unsigned int duration);
void Util_Assert(const char *cond, const char *file, int lineNum);
+int Util_Utf8CaseCmp(const gchar *str1, const gchar *str2);
+
#endif
diff --git a/open-vm-tools/vgauth/common/prefs.h b/open-vm-tools/vgauth/common/prefs.h
index 6c58f3f4b..3299eb26c 100644
--- a/open-vm-tools/vgauth/common/prefs.h
+++ b/open-vm-tools/vgauth/common/prefs.h
@@ -167,6 +167,9 @@ msgCatalog = /etc/vmware-tools/vgauth/messages
/** Where the localized version of the messages were installed. */
#define VGAUTH_PREF_LOCALIZATION_DIR "msgCatalog"
+/** If symlinks or junctions are allowed in alias store file path */
+#define VGAUTH_PREF_ALLOW_SYMLINKS "allowSymlinks"
+
/*
* Pref values
*/
diff --git a/open-vm-tools/vgauth/common/usercheck.c b/open-vm-tools/vgauth/common/usercheck.c
index 3beede2e8..340aa0411 100644
--- a/open-vm-tools/vgauth/common/usercheck.c
+++ b/open-vm-tools/vgauth/common/usercheck.c
@@ -78,6 +78,8 @@
* Solaris as well, but that path is untested.
*/
+#define MAX_USER_NAME_LEN 256
+
/*
* A single retry works for the LDAP case, but try more often in case NIS
* or something else has a related issue. Note that a bad username/uid won't
@@ -354,12 +356,29 @@ Usercheck_UsernameIsLegal(const gchar *userName)
* restricted list for local usernames.
*/
size_t len;
- char *illegalChars = "<>/";
+ size_t i = 0;
+ int backSlashCnt = 0;
+ /*
+ * As user names are used to generate its alias store file name/path, it
+ * should not contain path traversal characters ('/' and '\').
+ */
+ char *illegalChars = "<>/\\";
len = strlen(userName);
- if (strcspn(userName, illegalChars) != len) {
+ if (len > MAX_USER_NAME_LEN) {
return FALSE;
}
+
+ while ((i += strcspn(userName + i, illegalChars)) < len) {
+ /*
+ * One backward slash is allowed for domain\username separator.
+ */
+ if (userName[i] != '\\' || ++backSlashCnt > 1) {
+ return FALSE;
+ }
+ ++i;
+ }
+
return TRUE;
}
diff --git a/open-vm-tools/vgauth/serviceImpl/alias.c b/open-vm-tools/vgauth/serviceImpl/alias.c
index 4e170202c..c7040ebff 100644
--- a/open-vm-tools/vgauth/serviceImpl/alias.c
+++ b/open-vm-tools/vgauth/serviceImpl/alias.c
@@ -41,6 +41,7 @@
#include "certverify.h"
#include "VGAuthProto.h"
#include "vmxlog.h"
+#include "VGAuthUtil.h"
// puts the identity store in an easy to find place
#undef WIN_TEST_MODE
@@ -66,6 +67,7 @@
#define ALIASSTORE_FILE_PREFIX "user-"
#define ALIASSTORE_FILE_SUFFIX ".xml"
+static gboolean allowSymlinks = FALSE;
static gchar *aliasStoreRootDir = DEFAULT_ALIASSTORE_ROOT_DIR;
#ifdef _WIN32
@@ -252,6 +254,12 @@ mapping file layout:
*/
+#ifdef _WIN32
+#define ISPATHSEP(c) ((c) == '\\' || (c) == '/')
+#else
+#define ISPATHSEP(c) ((c) == '/')
+#endif
+
/*
******************************************************************************
@@ -466,6 +474,7 @@ ServiceLoadFileContentsWin(const gchar *fileName,
gunichar2 *fileNameW = NULL;
BOOL ok;
DWORD bytesRead;
+ gchar *realPath = NULL;
*fileSize = 0;
*contents = NULL;
@@ -622,6 +631,22 @@ ServiceLoadFileContentsWin(const gchar *fileName,
goto done;
}
+ if (!allowSymlinks) {
+ /*
+ * Check if fileName is real path.
+ */
+ if ((realPath = ServiceFileGetPathByHandle(hFile)) == NULL) {
+ err = VGAUTH_E_FAIL;
+ goto done;
+ }
+ if (Util_Utf8CaseCmp(realPath, fileName) != 0) {
+ Warning("%s: Real path (%s) is not same as file path (%s)\n",
+ __FUNCTION__, realPath, fileName);
+ err = VGAUTH_E_FAIL;
+ goto done;
+ }
+ }
+
/*
* Now finally read the contents.
*/
@@ -650,6 +675,7 @@ done:
CloseHandle(hFile);
}
g_free(fileNameW);
+ g_free(realPath);
return err;
}
@@ -672,6 +698,7 @@ ServiceLoadFileContentsPosix(const gchar *fileName,
gchar *buf;
gchar *bp;
int fd = -1;
+ gchar realPath[PATH_MAX] = { 0 };
*fileSize = 0;
*contents = NULL;
@@ -817,6 +844,23 @@ ServiceLoadFileContentsPosix(const gchar *fileName,
goto done;
}
+ if (!allowSymlinks) {
+ /*
+ * Check if fileName is real path.
+ */
+ if (realpath(fileName, realPath) == NULL) {
+ Warning("%s: realpath() failed. errno (%d)\n", __FUNCTION__, errno);
+ err = VGAUTH_E_FAIL;
+ goto done;
+ }
+ if (g_strcmp0(realPath, fileName) != 0) {
+ Warning("%s: Real path (%s) is not same as file path (%s)\n",
+ __FUNCTION__, realPath, fileName);
+ err = VGAUTH_E_FAIL;
+ goto done;
+ }
+ }
+
/*
* All confidence checks passed; read the bits.
*/
@@ -2803,8 +2847,13 @@ ServiceAliasRemoveAlias(const gchar *reqUserName,
/*
* We don't verify the user exists in a Remove operation, to allow
- * cleanup of deleted user's stores.
+ * cleanup of deleted user's stores, but we do check whether the
+ * user name is legal or not.
*/
+ if (!Usercheck_UsernameIsLegal(userName)) {
+ Warning("%s: Illegal user name '%s'\n", __FUNCTION__, userName);
+ return VGAUTH_E_FAIL;
+ }
if (!CertVerify_IsWellFormedPEMCert(pemCert)) {
return VGAUTH_E_INVALID_CERTIFICATE;
@@ -3036,6 +3085,16 @@ ServiceAliasQueryAliases(const gchar *userName,
}
#endif
+ /*
+ * We don't verify the user exists in a Query operation to allow
+ * cleaning up after a deleted user, but we do check whether the
+ * user name is legal or not.
+ */
+ if (!Usercheck_UsernameIsLegal(userName)) {
+ Warning("%s: Illegal user name '%s'\n", __FUNCTION__, userName);
+ return VGAUTH_E_FAIL;
+ }
+
err = AliasLoadAliases(userName, num, aList);
if (VGAUTH_E_OK != err) {
Warning("%s: failed to load Aliases for '%s'\n", __FUNCTION__, userName);
@@ -3294,6 +3353,7 @@ ServiceAliasInitAliasStore(void)
VGAuthError err = VGAUTH_E_OK;
gboolean saveBadDir = FALSE;
char *defaultDir = NULL;
+ size_t len;
#ifdef _WIN32
{
@@ -3324,6 +3384,10 @@ ServiceAliasInitAliasStore(void)
defaultDir = g_strdup(DEFAULT_ALIASSTORE_ROOT_DIR);
#endif
+ allowSymlinks = Pref_GetBool(gPrefs,
+ VGAUTH_PREF_ALLOW_SYMLINKS,
+ VGAUTH_PREF_GROUP_NAME_SERVICE,
+ FALSE);
/*
* Find the alias store directory. This allows an installer to put
* it somewhere else if necessary.
@@ -3337,6 +3401,14 @@ ServiceAliasInitAliasStore(void)
VGAUTH_PREF_GROUP_NAME_SERVICE,
defaultDir);
+ /*
+ * Remove the trailing separator if any from aliasStoreRootDir path.
+ */
+ len = strlen(aliasStoreRootDir);
+ if (ISPATHSEP(aliasStoreRootDir[len - 1])) {
+ aliasStoreRootDir[len - 1] = '\0';
+ }
+
Log("Using '%s' for alias store root directory\n", aliasStoreRootDir);
g_free(defaultDir);
diff --git a/open-vm-tools/vgauth/serviceImpl/service.c b/open-vm-tools/vgauth/serviceImpl/service.c
index d4716526c..e053ed0fa 100644
--- a/open-vm-tools/vgauth/serviceImpl/service.c
+++ b/open-vm-tools/vgauth/serviceImpl/service.c
@@ -28,6 +28,7 @@
#include "VGAuthUtil.h"
#ifdef _WIN32
#include "winUtil.h"
+#include <glib.h>
#endif
static ServiceStartListeningForIOFunc startListeningIOFunc = NULL;
@@ -283,9 +284,35 @@ static gchar *
ServiceUserNameToPipeName(const char *userName)
{
gchar *escapedName = ServiceEncodeUserName(userName);
+#ifdef _WIN32
+ /*
+ * Adding below pragma only in windows to suppress the compile time warning
+ * about unavailability of g_uuid_string_random() since compiler flag
+ * GLIB_VERSION_MAX_ALLOWED is defined to GLIB_VERSION_2_34.
+ * TODO: Remove below pragma when GLIB_VERSION_MAX_ALLOWED is bumped up to
+ * or greater than GLIB_VERSION_2_52.
+ */
+#pragma warning(suppress : 4996)
+ gchar *uuidStr = g_uuid_string_random();
+ /*
+ * Add a unique suffix to avoid a name collision with an existing named pipe
+ * created by someone else (intentionally or by accident).
+ * This is not needed for Linux; name collisions on sockets are already
+ * avoided there since (1) file system paths to VGAuthService sockets are in
+ * a directory that is writable only by root and (2) VGAuthService unlinks a
+ * socket path before binding it to a newly created socket.
+ */
+ gchar *pipeName = g_strdup_printf("%s-%s-%s",
+ SERVICE_PUBLIC_PIPE_NAME,
+ escapedName,
+ uuidStr);
+
+ g_free(uuidStr);
+#else
gchar *pipeName = g_strdup_printf("%s-%s",
SERVICE_PUBLIC_PIPE_NAME,
escapedName);
+#endif
g_free(escapedName);
return pipeName;
diff --git a/open-vm-tools/vgauth/serviceImpl/serviceInt.h b/open-vm-tools/vgauth/serviceImpl/serviceInt.h
index 5f420192b..f4f88547d 100644
--- a/open-vm-tools/vgauth/serviceImpl/serviceInt.h
+++ b/open-vm-tools/vgauth/serviceImpl/serviceInt.h
@@ -441,6 +441,7 @@ VGAuthError ServiceFileVerifyAdminGroupOwnedByHandle(const HANDLE hFile);
VGAuthError ServiceFileVerifyEveryoneReadableByHandle(const HANDLE hFile);
VGAuthError ServiceFileVerifyUserAccessByHandle(const HANDLE hFile,
const char *userName);
+gchar *ServiceFileGetPathByHandle(HANDLE hFile);
#else
VGAuthError ServiceFileVerifyFileOwnerAndPerms(const char *fileName,
const char *userName,
--
2.43.5
View Git Blame Copy Permalink