From a93cf0c544c15ec071db8ef681c4874f60cc8c80 Mon Sep 17 00:00:00 2001 From: Andrew Hutchings Date: Thu, 16 Oct 2025 14:27:58 +0100 Subject: [PATCH] Add file storage safety to wolfPKCS11 Ensure token storage validates configured directories, uses atomic temp files for updates, and returns errors when no secure path is available. Document the updated storage path resolution and adjust the storage tests to cover the missing-path failure case. --- README.md | 9 +- src/internal.c | 373 ++++++++++++++++++++++++++++++++-------- tests/token_path_test.c | 79 ++++----- wolfpkcs11/internal.h | 8 + 4 files changed, 351 insertions(+), 118 deletions(-) diff --git a/README.md b/README.md index cd3bc6a6..70b78556 100644 --- a/README.md +++ b/README.md @@ -98,8 +98,12 @@ NOTE: In the code, we have embedded a test key. This must be changed for ### WOLFPKCS11_TOKEN_PATH -Path into which files are stored that contain token data. -When not set, defaults to: /tmp +Path into which files are stored that contain token data. If unset, wolfPKCS11 +tries, in order, the directory specified by `WOLFPKCS11_TOKEN_PATH`, any store +directory configured by NSS, the user's home directory (`~/.wolfPKCS11` on +POSIX or `%APPDIR%\wolfPKCS11` on Windows), and finally the optional +`WOLFPKCS11_DEFAULT_TOKEN_PATH` build-time setting. There is no fallback to +`/tmp`; deployments must provide a secure storage location explicitly. ### WOLFPKCS11_NO_STORE @@ -287,4 +291,3 @@ Adds backend support for TPM 2.0 using wolfTPM. Adds AES CBC key wrap / unwrap s ### wolfPKCS11 Release 1.0 (October 20, 2021) * Initial PKCS11 support - diff --git a/src/internal.c b/src/internal.c index fb88358a..dac23e3f 100644 --- a/src/internal.c +++ b/src/internal.c @@ -51,11 +51,25 @@ #define MKDIR(path) _mkdir(path) #else #include - #include #define MKDIR(path) mkdir(path, 0700) #endif #endif +#if !defined(WOLFPKCS11_NO_STORE) && !defined(WOLFPKCS11_CUSTOM_STORE) + #include + #include + #if defined(_WIN32) || defined(_MSC_VER) + #include + #include + #include + #include + #else + #include + #include + #include + #endif +#endif + #include #include @@ -1057,6 +1071,183 @@ int WP11_SetStoreDir(const char *dir, size_t dirSz) } #endif +typedef struct WP11_FileStoreCtx { + XFILE file; + int is_write; + int has_temp; + char final_name[WP11_STORE_MAX_PATH]; + char temp_name[WP11_STORE_MAX_PATH]; +} WP11_FileStoreCtx; + +static void wolfPKCS11_StoreAbortTemp(WP11_FileStoreCtx* ctx) +{ + if (ctx != NULL && ctx->has_temp) { + remove(ctx->temp_name); + ctx->has_temp = 0; + } +} + +static int wolfPKCS11_StoreCommitTemp(WP11_FileStoreCtx* ctx) +{ + int ret = 0; + + if (ctx == NULL || ctx->has_temp == 0) { + return 0; + } + +#if defined(_WIN32) || defined(_MSC_VER) + if (!MoveFileExA(ctx->temp_name, ctx->final_name, + MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH)) { + ret = READ_ONLY_E; + } +#else + if (rename(ctx->temp_name, ctx->final_name) != 0) { + ret = READ_ONLY_E; + } +#endif + + if (ret == 0) { + ctx->has_temp = 0; + } + + return ret; +} + +static int wolfPKCS11_StoreValidateDir(const char* dirPath) +{ +#if defined(_WIN32) || defined(_MSC_VER) + DWORD attrs; + + attrs = GetFileAttributesA(dirPath); + if (attrs == INVALID_FILE_ATTRIBUTES) { + return READ_ONLY_E; + } + if ((attrs & FILE_ATTRIBUTE_DIRECTORY) == 0 || + (attrs & FILE_ATTRIBUTE_REPARSE_POINT) != 0) { + return READ_ONLY_E; + } + return 0; +#else + struct stat st; + + if (lstat(dirPath, &st) != 0) { + return READ_ONLY_E; + } + if (!S_ISDIR(st.st_mode)) { + return READ_ONLY_E; + } +#ifdef S_ISLNK + if (S_ISLNK(st.st_mode)) { + return READ_ONLY_E; + } +#endif + return 0; +#endif +} + +static int wolfPKCS11_StoreEnsureDir(const char* dirPath) +{ + int ret; + + if (dirPath == NULL || dirPath[0] == '\0') { + return READ_ONLY_E; + } + + ret = wolfPKCS11_StoreValidateDir(dirPath); + if (ret == 0) { + return 0; + } + + if (MKDIR(dirPath) == 0) { + return wolfPKCS11_StoreValidateDir(dirPath); + } + + if (errno == EEXIST) { + return wolfPKCS11_StoreValidateDir(dirPath); + } + + return READ_ONLY_E; +} + +static int wolfPKCS11_StoreCreateTempFile(WP11_FileStoreCtx* ctx, + const char* dirPath) +{ + if (ctx == NULL) { + return READ_ONLY_E; + } + +#if defined(_WIN32) || defined(_MSC_VER) + char templateBuf[WP11_STORE_MAX_PATH]; + int ret; + int fd; + + ret = XSNPRINTF(templateBuf, sizeof(templateBuf), + "%s/wp11_tmp_%08lx_%08lx_XXXXXX", dirPath, + (unsigned long)_getpid(), (unsigned long)GetTickCount()); + if (ret <= 0 || ret >= (int)sizeof(templateBuf)) { + return READ_ONLY_E; + } + + if (_mktemp_s(templateBuf, sizeof(templateBuf)) != 0) { + return READ_ONLY_E; + } + + fd = _open(templateBuf, _O_CREAT | _O_EXCL | _O_BINARY | _O_WRONLY, + _S_IREAD | _S_IWRITE); + if (fd == -1) { + return READ_ONLY_E; + } + + ctx->file = _fdopen(fd, "wb"); + if (ctx->file == NULL) { + _close(fd); + _unlink(templateBuf); + return READ_ONLY_E; + } + + XSTRNCPY(ctx->temp_name, templateBuf, sizeof(ctx->temp_name)); + ctx->temp_name[sizeof(ctx->temp_name) - 1] = '\0'; + ctx->has_temp = 1; + + return 0; +#else + char templateBuf[WP11_STORE_MAX_PATH]; + int fd; + int ret; + + ret = XSNPRINTF(templateBuf, sizeof(templateBuf), + "%s/wp11_tmp_%08lx_%08lx_XXXXXX", dirPath, + (unsigned long)getpid(), (unsigned long)time(NULL)); + if (ret <= 0 || ret >= (int)sizeof(templateBuf)) { + return READ_ONLY_E; + } + + fd = mkstemp(templateBuf); + if (fd < 0) { + return READ_ONLY_E; + } + + if (fchmod(fd, S_IRUSR | S_IWUSR) != 0) { + close(fd); + unlink(templateBuf); + return READ_ONLY_E; + } + + ctx->file = fdopen(fd, "wb"); + if (ctx->file == NULL) { + close(fd); + unlink(templateBuf); + return READ_ONLY_E; + } + + XSTRNCPY(ctx->temp_name, templateBuf, sizeof(ctx->temp_name)); + ctx->temp_name[sizeof(ctx->temp_name) - 1] = '\0'; + ctx->has_temp = 1; + + return 0; +#endif +} + static int wolfPKCS11_Store_Name(int type, CK_ULONG id1, CK_ULONG id2, char* name, int nameLen) { @@ -1074,9 +1265,8 @@ static int wolfPKCS11_Store_Name(int type, CK_ULONG id1, CK_ULONG id2, char* nam * 1. Environment variable WOLFPKCS11_TOKEN_PATH * 2. NSS store directory, if set using C_Initialize * 3. Home directory with .wolfPKCS11 (or APPDIR with wolfPKCS11 for - * Windows) + * Windows) * 4. WOLFPKCS11_DEFAULT_TOKEN_PATH, if set - * 5. /tmp in Linux, %TEMP% or C:\Windows\Temp in Windows */ #ifndef WOLFPKCS11_NO_ENV str = XGETENV("WOLFPKCS11_TOKEN_PATH"); @@ -1115,17 +1305,6 @@ static int wolfPKCS11_Store_Name(int type, CK_ULONG id1, CK_ULONG id2, char* nam if (str == NULL) { str = WC_STRINGIFY(WOLFPKCS11_DEFAULT_TOKEN_PATH); } -#else - if (str == NULL) { - #if defined(_WIN32) || defined(_MSC_VER) - str = XGETENV("%TEMP%"); - if (str == NULL) { - str = "C:\\Windows\\Temp"; - } - #else - str = "/tmp"; - #endif - } #endif /* 47 is maximum number of character to a filename and path separator. */ @@ -1209,7 +1388,7 @@ int wolfPKCS11_Store_Remove(int type, CK_ULONG id1, CK_ULONG id2) word32 nvIndex; WOLFTPM2_HANDLE parent; #else - char name[120] = "\0"; + char name[WP11_STORE_MAX_PATH] = "\0"; #endif #ifdef WOLFPKCS11_DEBUG_STORE @@ -1275,12 +1454,8 @@ int wolfPKCS11_Store_OpenSz(int type, CK_ULONG id1, CK_ULONG id2, int read, int maxSz; WOLFTPM2_HANDLE parent; #else -#ifdef WOLFPKCS11_NSS - char name[600] = "\0"; -#else - char name[120] = "\0"; -#endif - XFILE file = XBADFILE; + WP11_FileStoreCtx* ctx = NULL; + char name[WP11_STORE_MAX_PATH] = "\0"; #endif #ifdef WOLFPKCS11_DEBUG_STORE @@ -1355,64 +1530,97 @@ int wolfPKCS11_Store_OpenSz(int type, CK_ULONG id1, CK_ULONG id2, int read, /* build filename */ ret = wolfPKCS11_Store_Name(type, id1, id2, name, sizeof(name)); - /* Check that the string fits in name */ - if (ret > 0 && ret < (int) sizeof(name)) + if (ret > 0 && ret < (int)sizeof(name)) { ret = 0; - else + } + else { ret = -1; + } + + if (ret == 0) { + ctx = (WP11_FileStoreCtx*)XMALLOC(sizeof(*ctx), NULL, + DYNAMIC_TYPE_TMP_BUFFER); + if (ctx == NULL) { + ret = MEMORY_E; + } + } - /* Open file for read or write. */ if (ret == 0) { - if (read) { - file = XFOPEN(name, "r"); - if (file == NULL) { + char dirPath[WP11_STORE_MAX_PATH]; + const char* lastSlash = NULL; + size_t nameLen = XSTRLEN(name); + size_t i; + + XMEMSET(ctx, 0, sizeof(*ctx)); + ctx->file = XBADFILE; + ctx->is_write = (read == 0); + + { + size_t finalLen = XSTRLEN(name); + if (finalLen >= sizeof(ctx->final_name)) { + ret = READ_ONLY_E; + } + else { + XMEMCPY(ctx->final_name, name, finalLen + 1); + } + } + + if (ret == 0 && read) { + ctx->file = XFOPEN(name, "rb"); + if (ctx->file == NULL) { ret = NOT_AVAILABLE_E; } } - else { - file = XFOPEN(name, "w"); - if (file == NULL) { - /* Try to create directory if it doesn't exist */ - char* lastSlash = NULL; - char dirPath[120]; - int i; - - /* Find the last directory separator */ - for (i = 0; name[i] != '\0'; i++) { - if (name[i] == '/' || name[i] == '\\') { - lastSlash = (char*)&name[i]; - } + else if (ret == 0) { + for (i = 0; i < nameLen; i++) { + if (name[i] == '/' || name[i] == '\\') { + lastSlash = &name[i]; } + } - if (lastSlash != NULL) { - /* Extract directory path */ - int dirLen = (int)(lastSlash - name); - if (dirLen > 0 && dirLen < (int)sizeof(dirPath)) { - XMEMCPY(dirPath, name, dirLen); - dirPath[dirLen] = '\0'; - - /* Try to create the directory */ - if (MKDIR(dirPath) == 0 || errno == EEXIST) { - /* Directory created or already exists, try opening file again */ - file = XFOPEN(name, "w"); - } - } - } + if (lastSlash == NULL) { + ret = READ_ONLY_E; + } + else { + int dirLen = (int)(lastSlash - name); - if (file == NULL) { + if (dirLen <= 0 || dirLen >= (int)sizeof(dirPath)) { ret = READ_ONLY_E; } + else { + XMEMCPY(dirPath, name, dirLen); + dirPath[dirLen] = '\0'; + + ret = wolfPKCS11_StoreEnsureDir(dirPath); + if (ret == 0) { + ret = wolfPKCS11_StoreCreateTempFile(ctx, dirPath); + } + } } } } + if (ret == 0 && (ctx->file == NULL || ctx->file == XBADFILE)) { + ret = READ_ONLY_E; + } + if (ret == 0) { - /* Return the file pointer. */ - *store = file; + *store = ctx; } - (void)variableSz; /* not used */ + else if (ctx != NULL) { + if (ctx->file != NULL && ctx->file != XBADFILE) { + XFCLOSE(ctx->file); + } + if (ctx->has_temp) { + wolfPKCS11_StoreAbortTemp(ctx); + } + XFREE(ctx, NULL, DYNAMIC_TYPE_TMP_BUFFER); + ctx = NULL; + } + + (void)variableSz; /* not used in file-backed storage */ #ifdef WOLFPKCS11_DEBUG_STORE - printf("Store Open %p: ret %d, name %s\n", *store, ret, name); + printf("Store Open %p: ret %d, name %s\n", (ret == 0) ? *store : NULL, ret, name); #endif #endif return ret; @@ -1447,7 +1655,7 @@ void wolfPKCS11_Store_Close(void* store) #ifdef WOLFPKCS11_TPM_STORE WP11_TpmStore* tpmStore = (WP11_TpmStore*)store; #else - XFILE file = (XFILE)store; + WP11_FileStoreCtx* ctx = (WP11_FileStoreCtx*)store; #endif #ifdef WOLFPKCS11_DEBUG_STORE @@ -1458,9 +1666,30 @@ void wolfPKCS11_Store_Close(void* store) /* nothing to do for TPM */ (void)tpmStore; #else - /* Close a valid file pointer. */ - if (file != XBADFILE) { - XFCLOSE(file); + if (ctx != NULL) { + int commitRet = 0; + + if (ctx->file != XBADFILE && ctx->file != NULL) { + XFCLOSE(ctx->file); + ctx->file = XBADFILE; + } + + if (ctx->is_write && ctx->has_temp) { + commitRet = wolfPKCS11_StoreCommitTemp(ctx); + if (commitRet != 0) { + wolfPKCS11_StoreAbortTemp(ctx); +#ifdef WOLFPKCS11_DEBUG_STORE + printf("Store commit failed for %s (ret %d)\n", + ctx->final_name, commitRet); +#endif + } + } + else if (ctx->has_temp) { + wolfPKCS11_StoreAbortTemp(ctx); + } + + XMEMSET(ctx, 0, sizeof(*ctx)); + XFREE(ctx, NULL, DYNAMIC_TYPE_TMP_BUFFER); } #endif } @@ -1481,7 +1710,7 @@ int wolfPKCS11_Store_Read(void* store, unsigned char* buffer, int len) WP11_TpmStore* tpmStore = (WP11_TpmStore*)store; word32 readSize; #else - XFILE file = (XFILE)store; + WP11_FileStoreCtx* ctx = (WP11_FileStoreCtx*)store; #endif #ifdef WOLFPKCS11_DEBUG_STORE @@ -1509,9 +1738,8 @@ int wolfPKCS11_Store_Read(void* store, unsigned char* buffer, int len) } } #else - /* Read from a valid file pointer. */ - if (file != XBADFILE) { - ret = (int)XFREAD(buffer, 1, len, file); + if (ctx != NULL && ctx->file != XBADFILE && ctx->file != NULL) { + ret = (int)XFREAD(buffer, 1, len, ctx->file); } #endif return ret; @@ -1532,7 +1760,7 @@ int wolfPKCS11_Store_Write(void* store, unsigned char* buffer, int len) #ifdef WOLFPKCS11_TPM_STORE WP11_TpmStore* tpmStore = (WP11_TpmStore*)store; #else - XFILE file = (XFILE)store; + WP11_FileStoreCtx* ctx = (WP11_FileStoreCtx*)store; #endif #ifdef WOLFPKCS11_DEBUG_STORE @@ -1547,12 +1775,11 @@ int wolfPKCS11_Store_Write(void* store, unsigned char* buffer, int len) ret = len; } #else - /* Write to a valid file pointer. */ - if (store != XBADFILE) { - ret = (int)XFWRITE(buffer, 1, len, file); + if (ctx != NULL && ctx->file != XBADFILE && ctx->file != NULL) { + ret = (int)XFWRITE(buffer, 1, len, ctx->file); if (ret == len) { /* Ensure data makes it to storage. */ - (void)XFFLUSH(file); + (void)XFFLUSH(ctx->file); } } #endif diff --git a/tests/token_path_test.c b/tests/token_path_test.c index ac82fee8..57b7705f 100644 --- a/tests/token_path_test.c +++ b/tests/token_path_test.c @@ -160,7 +160,7 @@ static void pkcs11_final(void) #endif } -static CK_RV create_test_token(const char* test_name) +static CK_RV create_test_token(const char* test_name, int expect_success) { CK_RV ret; CK_SESSION_HANDLE session = CK_INVALID_HANDLE; @@ -174,6 +174,22 @@ static CK_RV create_test_token(const char* test_name) memcpy(label, tokenName, strlen(tokenName)); ret = funcList->C_InitToken(slot, soPin, soPinLen, label); + if (ret != CKR_OK) { + if (expect_success) { + CHECK_CKR(ret, "C_InitToken", CKR_OK); + } else { + printf("PASS: C_InitToken failed with %ld as expected\n", (long)ret); + test_passed++; + } + goto end; + } + + if (!expect_success) { + printf("FAIL: C_InitToken succeeded but expected failure\n"); + test_failed++; + goto end; + } + CHECK_CKR(ret, "C_InitToken", CKR_OK); /* Open session */ @@ -197,10 +213,10 @@ static CK_RV create_test_token(const char* test_name) } /* Close session */ +end: if (session != CK_INVALID_HANDLE) { funcList->C_CloseSession(session); } - return ret; } @@ -240,7 +256,7 @@ static int test_default_home_path(void) /* Clean up any existing test files */ cleanup_test_files(expected_dir); - ret = create_test_token("Testing default home directory path"); + ret = create_test_token("Testing default home directory path", 1); if (ret != CKR_OK) { return -1; } @@ -282,7 +298,7 @@ static int test_env_var_path(void) /* Clean up any existing test files */ cleanup_test_files(test_dir); - ret = create_test_token("Testing WOLFPKCS11_TOKEN_PATH environment variable"); + ret = create_test_token("Testing WOLFPKCS11_TOKEN_PATH environment variable", 1); if (ret != CKR_OK) { return -1; } @@ -305,56 +321,35 @@ static int test_env_var_path(void) return 0; } -static int test_temp_fallback_path(void) +static int test_missing_storage_path(void) { CK_RV ret; - char expected_dir[256]; - char test_file[512]; +#if defined(WOLFPKCS11_DEFAULT_TOKEN_PATH) + printf("SKIP: WOLFPKCS11_DEFAULT_TOKEN_PATH is defined; missing path scenario not applicable\n"); + return 0; +#endif + +#ifdef WOLFPKCS11_NO_ENV + printf("SKIP: Environment variables disabled; cannot clear token path\n"); + return 0; +#endif - /* Clear environment variable and simulate no home directory */ #ifndef WOLFPKCS11_NO_ENV unsetenv("WOLFPKCS11_TOKEN_PATH"); unsetenv("HOME"); - unsetenv("%APPDATA%"); -#endif - -#ifdef WOLFPKCS11_DEFAULT_TOKEN_PATH - strcpy(expected_dir, WC_STRINGIFY(WOLFPKCS11_DEFAULT_TOKEN_PATH)); -#else -#ifdef _WIN32 - const char* temp = getenv("TEMP"); - if (!temp) { - strcpy(expected_dir, "C:\\Windows\\Temp"); - } else { - strcpy(expected_dir, temp); - } -#else - strcpy(expected_dir, "/tmp"); +#if defined(_WIN32) + unsetenv("APPDIR"); + unsetenv("APPDATA"); #endif #endif - /* Clean up any existing test files */ - cleanup_test_files(expected_dir); - - ret = create_test_token("Testing directory fallback"); - if (ret != CKR_OK) { - return -1; - } - - /* Check if token files were created in expected location */ - snprintf(test_file, sizeof(test_file), "%s" PATH_SEP "wp11_token_0000000000000001", expected_dir); - if (!file_exists(test_file)) { - printf("FAIL: Token file not found at expected location: %s\n", test_file); + ret = create_test_token("Testing missing storage path handling", 0); + if (ret == CKR_OK) { + printf("FAIL: Token creation succeeded unexpectedly without a storage path\n"); test_failed++; return -1; } - printf("PASS: Token file found at: %s\n", test_file); - test_passed++; - - /* Clean up */ - cleanup_test_files(expected_dir); - return 0; } @@ -453,7 +448,7 @@ int main(int argc, char* argv[]) result = 1; } - if (test_temp_fallback_path() != 0) { + if (test_missing_storage_path() != 0) { result = 1; } diff --git a/wolfpkcs11/internal.h b/wolfpkcs11/internal.h index 81347f06..ebc224e2 100644 --- a/wolfpkcs11/internal.h +++ b/wolfpkcs11/internal.h @@ -60,6 +60,14 @@ extern "C" { #define WOLFPKCS11_NO_STORE #endif +#ifndef WP11_STORE_MAX_PATH + #ifdef WOLFPKCS11_NSS + #define WP11_STORE_MAX_PATH 600 + #else + #define WP11_STORE_MAX_PATH 120 + #endif +#endif + #if !defined(WOLFSSL_PUBLIC_MP) #error Please build wolfSSL using recommended options: \ ./configure --enable-aescfb --enable-rsapss --enable-keygen \