From 5e6e4b42f2b1fcc1e4ef1b920e463bfa55da8b45 Mon Sep 17 00:00:00 2001 From: Pavel Kohout Date: Tue, 13 Jan 2026 00:00:00 +0000 Subject: [PATCH] Fix symlink-following vulnerabilities (CWE-59) Multiple symlink-following vulnerabilities exist in OpenCryptoki that run in privileged contexts. These allow a token-group user to redirect file operations to arbitrary filesystem targets by planting symlinks in group-writable token directories, resulting in privilege escalation or data exposure. Affected components: 1. pkcstok_admin: set_file_permissions() uses stat() which follows symlinks, then applies chmod/chown to the symlink target. 2. pkcstok_migrate: fopen() follows symlinks, then set_perm() modifies the target permissions. 3. loadsave.c: Multiple wrapper functions use fopen() followed by set_perm(). 4. hsm_mk_change.c: hsm_mk_change_op_open() uses fopen() followed by hsm_mk_change_op_set_perm(). 5. pbkdf.c: fopen() followed by set_perms() in two locations. This fix: - Introduces fopen_nofollow() helper in platform.h - Checks for O_NOFOLLOW at compile time (not hardcoded per-platform) - On platforms with O_NOFOLLOW: uses open(O_NOFOLLOW) + fdopen() for atomic symlink rejection (race-condition free) - On platforms without O_NOFOLLOW: falls back to lstat() + fopen() and emits a compiler warning so the unsafe fallback doesn't go unnoticed - Updates all affected wrapper functions to use fopen_nofollow() - pkcstok_admin: Uses lstat() instead of stat() and skips symlinks Reported-by: Pavel Kohout, Aisle Research, www.aisle.com Signed-off-by: Pavel Kohout Signed-off-by: Ingo Franzki --- usr/lib/common/loadsave.c | 81 +++++++++++++++++---- usr/lib/common/platform.h | 82 +++++++++++++++++++++- usr/lib/hsm_mk_change/hsm_mk_change.c | 8 ++- usr/lib/icsf_stdll/pbkdf.c | 17 +++-- usr/sbin/pkcstok_admin/pkcstok_admin.c | 9 ++- usr/sbin/pkcstok_migrate/pkcstok_migrate.c | 23 ++++-- 6 files changed, 194 insertions(+), 26 deletions(-) diff --git a/usr/lib/common/loadsave.c b/usr/lib/common/loadsave.c index 18b8aa044..f9c0cc7f0 100644 --- a/usr/lib/common/loadsave.c +++ b/usr/lib/common/loadsave.c @@ -68,9 +68,17 @@ static FILE *open_token_object_path(char *buf, size_t buflen, STDLL_TokData_t *tokdata, const char *path, const char *mode) { + FILE *fp; + if (get_token_object_path(buf, buflen, tokdata, path, NULL) < 0) return NULL; - return fopen(buf, mode); + + /* CWE-59 fix: Use fopen_nofollow to prevent symlink attacks */ + fp = fopen_nofollow(buf, mode); + if (fp == NULL && errno == ELOOP) + TRACE_ERROR("Refusing to follow symlink: %s\n", buf); + + return fp; } static FILE *open_token_object_path_new(char *newbuf, size_t newbuflen, @@ -78,11 +86,19 @@ static FILE *open_token_object_path_new(char *newbuf, size_t newbuflen, STDLL_TokData_t *tokdata, const char *path, const char *mode) { + FILE *fp; + if (get_token_object_path(newbuf, newbuflen, tokdata, path, ".TMP") < 0) return NULL; if (get_token_object_path(basebuf, basebuflen, tokdata, path, NULL) < 0) return NULL; - return fopen(newbuf, mode); + + /* CWE-59 fix: Use fopen_nofollow to prevent symlink attacks */ + fp = fopen_nofollow(newbuf, mode); + if (fp == NULL && errno == ELOOP) + TRACE_ERROR("Refusing to follow symlink: %s\n", newbuf); + + return fp; } static int get_token_data_store_path(char *buf, size_t buflen, @@ -101,9 +117,17 @@ static FILE *open_token_data_store_path(char *buf, size_t buflen, STDLL_TokData_t *tokdata, const char *path, const char *mode) { + FILE *fp; + if (get_token_data_store_path(buf, buflen, tokdata, path, NULL) < 0) return NULL; - return fopen(buf, mode); + + /* CWE-59 fix: Use fopen_nofollow to prevent symlink attacks */ + fp = fopen_nofollow(buf, mode); + if (fp == NULL && errno == ELOOP) + TRACE_ERROR("Refusing to follow symlink: %s\n", buf); + + return fp; } static FILE *open_token_data_store_path_new(char *newbuf, size_t newbuflen, @@ -111,11 +135,19 @@ static FILE *open_token_data_store_path_new(char *newbuf, size_t newbuflen, STDLL_TokData_t *tokdata, const char *path, const char *mode) { + FILE *fp; + if (get_token_data_store_path(newbuf, newbuflen, tokdata, path, ".TMP") < 0) return NULL; if (get_token_data_store_path(basebuf, basebuflen, tokdata, path, NULL) < 0) return NULL; - return fopen(newbuf, mode); + + /* CWE-59 fix: Use fopen_nofollow to prevent symlink attacks */ + fp = fopen_nofollow(newbuf, mode); + if (fp == NULL && errno == ELOOP) + TRACE_ERROR("Refusing to follow symlink: %s\n", newbuf); + + return fp; } static FILE *open_token_object_index(char *buf, size_t buflen, @@ -127,17 +159,27 @@ static FILE *open_token_object_index(char *buf, size_t buflen, static FILE *open_token_nvdat(char *buf, size_t buflen, STDLL_TokData_t *tokdata, const char *mode) { + FILE *fp; + if (ock_snprintf(buf, buflen, "%s/" PK_LITE_NV, tokdata->data_store)) { TRACE_ERROR("NVDAT.TOK file name buffer overflow\n"); return NULL; } - return fopen(buf, mode); + + /* CWE-59 fix: Use fopen_nofollow to prevent symlink attacks */ + fp = fopen_nofollow(buf, mode); + if (fp == NULL && errno == ELOOP) + TRACE_ERROR("Refusing to follow symlink: %s\n", buf); + + return fp; } static FILE *open_token_nvdat_new(char *newbuf, size_t newbuflen, char *basebuf, size_t basebuflen, STDLL_TokData_t *tokdata, const char *mode) { + FILE *fp; + if (ock_snprintf(newbuf, newbuflen, "%s/" PK_LITE_NV ".TMP", tokdata->data_store)) { TRACE_ERROR("NVDAT.TOK file name buffer overflow\n"); @@ -148,7 +190,13 @@ static FILE *open_token_nvdat_new(char *newbuf, size_t newbuflen, TRACE_ERROR("NVDAT.TOK file name buffer overflow\n"); return NULL; } - return fopen(newbuf, mode); + + /* CWE-59 fix: Use fopen_nofollow to prevent symlink attacks */ + fp = fopen_nofollow(newbuf, mode); + if (fp == NULL && errno == ELOOP) + TRACE_ERROR("Refusing to follow symlink: %s\n", newbuf); + + return fp; } static CK_RV close_token_file_new(FILE * fp, CK_RV rc, @@ -289,9 +337,12 @@ CK_RV save_token_object(STDLL_TokData_t *tokdata, OBJECT *obj) // we didn't find it...either the index file doesn't exist or this // is a new object... // - fp = fopen(fname, "a"); + fp = fopen_nofollow(fname, "a"); if (!fp) { - TRACE_ERROR("fopen(%s): %s\n", fname, strerror(errno)); + if (errno == ELOOP) + TRACE_ERROR("Refusing to follow symlink: %s\n", fname); + else + TRACE_ERROR("fopen(%s): %s\n", fname, strerror(errno)); return CKR_FUNCTION_FAILED; } @@ -663,11 +714,14 @@ CK_RV load_token_data_old(STDLL_TokData_t *tokdata, CK_SLOT_ID slot_id) if (errno == ENOENT) { init_token_data(tokdata, slot_id); - fp = fopen(fname, "r"); + fp = fopen_nofollow(fname, "r"); if (!fp) { // were really hosed here since the created // did not occur - TRACE_ERROR("fopen(%s): %s\n", fname, strerror(errno)); + if (errno == ELOOP) + TRACE_ERROR("Refusing to follow symlink: %s\n", fname); + else + TRACE_ERROR("fopen(%s): %s\n", fname, strerror(errno)); rc = CKR_FUNCTION_FAILED; goto out_unlock; } @@ -2345,11 +2399,14 @@ CK_RV load_token_data(STDLL_TokData_t *tokdata, CK_SLOT_ID slot_id) if (errno == ENOENT) { init_token_data(tokdata, slot_id); - fp = fopen(fname, "r"); + fp = fopen_nofollow(fname, "r"); if (!fp) { // were really hosed here since the created // did not occur - TRACE_ERROR("fopen(%s): %s\n", fname, strerror(errno)); + if (errno == ELOOP) + TRACE_ERROR("Refusing to follow symlink: %s\n", fname); + else + TRACE_ERROR("fopen(%s): %s\n", fname, strerror(errno)); rc = CKR_FUNCTION_FAILED; goto out_unlock; } diff --git a/usr/lib/common/platform.h b/usr/lib/common/platform.h index 799821b57..51cc1c737 100644 --- a/usr/lib/common/platform.h +++ b/usr/lib/common/platform.h @@ -7,7 +7,16 @@ * found in the file LICENSE file or at * https://opensource.org/licenses/cpl1.0.php */ +#ifndef PLATFORM_H +#define PLATFORM_H + #include +#include +#include +#include +#include +#include +#include #if defined(_AIX) #include "aix/getopt.h" @@ -30,10 +39,81 @@ /* for htobexx, htolexx, bexxtoh and lexxtoh macros */ #include /* macros from bsdlog and friends */ -#include #include #define OCK_API_LIBNAME "libopencryptoki.so" #define DYNLIB_LDFLAGS (RTLD_NOW) #endif /* _AIX */ + +/* + * Check for O_NOFOLLOW support at compile time. + * If not available, fall back to lstat() + fopen() (has TOCTOU race). + */ +#ifndef O_NOFOLLOW +#define OCK_NO_O_NOFOLLOW 1 +#warning "O_NOFOLLOW not supported, symlink protection uses racy lstat() fallback!" +#endif + +/* + * CWE-59 fix: Open file without following symlinks. + * + * On platforms with O_NOFOLLOW support: + * Uses open(O_NOFOLLOW) + fdopen() for atomic symlink rejection. + * + * On platforms without O_NOFOLLOW (e.g., older AIX): + * Falls back to lstat() + fopen(). This has a TOCTOU race condition, + * but still catches pre-planted symlinks which is the common attack + * scenario. Better than no protection at all. + * + * Returns NULL with errno=ELOOP if path is a symlink. + */ +static inline FILE *fopen_nofollow(const char *path, const char *mode) +{ +#ifdef OCK_NO_O_NOFOLLOW + /* + * Fallback for platforms without O_NOFOLLOW: use lstat() check. + * This has a TOCTOU race but catches pre-planted symlinks. + */ + struct stat sb; + + if (lstat(path, &sb) == 0) { + if (S_ISLNK(sb.st_mode)) { + errno = ELOOP; + return NULL; + } + } + /* Note: if lstat fails (e.g., file doesn't exist for "w" mode), + * we proceed with fopen() which will handle the error appropriately */ + return fopen(path, mode); +#else + /* Preferred: atomic symlink rejection via O_NOFOLLOW */ + int flags = O_NOFOLLOW; + int fd; + FILE *fp; + + /* Determine flags based on mode */ + if (mode[0] == 'r') { + flags |= (mode[1] == '+') ? O_RDWR : O_RDONLY; + } else if (mode[0] == 'w') { + flags |= O_CREAT | O_TRUNC | ((mode[1] == '+') ? O_RDWR : O_WRONLY); + } else if (mode[0] == 'a') { + flags |= O_CREAT | O_APPEND | ((mode[1] == '+') ? O_RDWR : O_WRONLY); + } else { + return NULL; + } + + fd = open(path, flags, 0600); + if (fd < 0) + return NULL; + + fp = fdopen(fd, mode); + if (fp == NULL) { + close(fd); + return NULL; + } + return fp; +#endif +} + +#endif /* PLATFORM_H */ diff --git a/usr/lib/hsm_mk_change/hsm_mk_change.c b/usr/lib/hsm_mk_change/hsm_mk_change.c index f40dfb43e..8c66546f6 100644 --- a/usr/lib/hsm_mk_change/hsm_mk_change.c +++ b/usr/lib/hsm_mk_change/hsm_mk_change.c @@ -623,9 +623,13 @@ static FILE* hsm_mk_change_op_open(const char *id, CK_SLOT_ID slot_id, TRACE_DEVEL("file to open: %s mode: %s\n", hsm_mk_change_file, mode); - fp = fopen(hsm_mk_change_file, mode); + /* CWE-59 fix: Use fopen_nofollow to prevent symlink attacks */ + fp = fopen_nofollow(hsm_mk_change_file, mode); if (fp == NULL) { - TRACE_ERROR("%s fopen(%s, %s): %s\n", __func__, + if (errno == ELOOP) + TRACE_ERROR("Refusing to follow symlink: %s\n", hsm_mk_change_file); + else + TRACE_ERROR("%s fopen(%s, %s): %s\n", __func__, hsm_mk_change_file, mode, strerror(errno)); } diff --git a/usr/lib/icsf_stdll/pbkdf.c b/usr/lib/icsf_stdll/pbkdf.c index 47d1b97c3..91230804f 100644 --- a/usr/lib/icsf_stdll/pbkdf.c +++ b/usr/lib/icsf_stdll/pbkdf.c @@ -26,6 +26,7 @@ #include "h_extern.h" #include "pbkdf.h" #include "trace.h" +#include "platform.h" CK_RV get_randombytes(unsigned char *output, int bytes) @@ -546,9 +547,13 @@ CK_RV secure_racf(STDLL_TokData_t *tokdata, totallen = outputlen + AES_INIT_VECTOR_SIZE; snprintf(fname, sizeof(fname), "%s/%s/%s", CONFIG_PATH, tokname, RACFFILE); - fp = fopen(fname, "w"); + /* CWE-59 fix: Use fopen_nofollow to prevent symlink attacks */ + fp = fopen_nofollow(fname, "w"); if (!fp) { - TRACE_ERROR("fopen failed: %s\n", strerror(errno)); + if (errno == ELOOP) + TRACE_ERROR("Refusing to follow symlink: %s\n", fname); + else + TRACE_ERROR("fopen failed: %s\n", strerror(errno)); return CKR_FUNCTION_FAILED; } @@ -619,9 +624,13 @@ CK_RV secure_masterkey(STDLL_TokData_t *tokdata, /* get the total length */ totallen = outputlen + SALTSIZE; - fp = fopen(fname, "w"); + /* CWE-59 fix: Use fopen_nofollow to prevent symlink attacks */ + fp = fopen_nofollow(fname, "w"); if (!fp) { - TRACE_ERROR("fopen failed: %s\n", strerror(errno)); + if (errno == ELOOP) + TRACE_ERROR("Refusing to follow symlink: %s\n", fname); + else + TRACE_ERROR("fopen failed: %s\n", strerror(errno)); return CKR_FUNCTION_FAILED; } diff --git a/usr/sbin/pkcstok_admin/pkcstok_admin.c b/usr/sbin/pkcstok_admin/pkcstok_admin.c index 9912804ee..d144cc04c 100644 --- a/usr/sbin/pkcstok_admin/pkcstok_admin.c +++ b/usr/sbin/pkcstok_admin/pkcstok_admin.c @@ -336,11 +336,18 @@ static int set_file_permissions(const char *fname, const struct group *group, pr_verbose("Setting permissions for '%s' with group '%s'", fname, group->gr_name); - if (stat(fname, &sb) != 0) { + /* CWE-59 fix: Use lstat to detect symlinks */ + if (lstat(fname, &sb) != 0) { warnx("'%s' does not exist.", fname); return -1; } + /* Only process regular files and directories (CWE-59 fix) */ + if (!S_ISREG(sb.st_mode) && !S_ISDIR(sb.st_mode)) { + warnx("Skipping '%s': not a regular file or directory.", fname); + return 0; + } + if (sb.st_uid != 0) { /* owner is not root */ pwd = getpwuid(sb.st_uid); diff --git a/usr/sbin/pkcstok_migrate/pkcstok_migrate.c b/usr/sbin/pkcstok_migrate/pkcstok_migrate.c index 12b605b5b..9579e2364 100644 --- a/usr/sbin/pkcstok_migrate/pkcstok_migrate.c +++ b/usr/sbin/pkcstok_migrate/pkcstok_migrate.c @@ -48,6 +48,7 @@ #include "local_types.h" #include "h_extern.h" #include "slotmgr.h" // for ock_snprintf +#include "platform.h" #define OCK_TOOL #include "pkcs_utils.h" @@ -77,9 +78,14 @@ static FILE *open_datastore_file(char *buf, size_t buflen, TRACE_ERROR("Path overflow for datastore file %s\n", file); return NULL; } - res = fopen(buf, mode); - if (!res) - TRACE_ERROR("fopen(%s) failed, errno=%s\n", buf, strerror(errno)); + /* CWE-59 fix: Use fopen_nofollow to prevent symlink attacks */ + res = fopen_nofollow(buf, mode); + if (!res) { + if (errno == ELOOP) + TRACE_ERROR("Refusing to follow symlink: %s\n", buf); + else + TRACE_ERROR("fopen(%s) failed, errno=%s\n", buf, strerror(errno)); + } return res; } @@ -94,9 +100,14 @@ static FILE *open_tokenobject(char *buf, size_t buflen, file, tokenobj); return NULL; } - res = fopen(buf, mode); - if (!res) - TRACE_ERROR("fopen(%s) failed, errno=%s\n", buf, strerror(errno)); + /* CWE-59 fix: Use fopen_nofollow to prevent symlink attacks */ + res = fopen_nofollow(buf, mode); + if (!res) { + if (errno == ELOOP) + TRACE_ERROR("Refusing to follow symlink: %s\n", buf); + else + TRACE_ERROR("fopen(%s) failed, errno=%s\n", buf, strerror(errno)); + } return res; }