From c39d555d0951942e890181936d8b3a88762e19d6 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 29 Jul 2026 09:00:17 -0700 Subject: [PATCH] Fix TOCTOU defect in test_ConfigSavePID() - Scenarios 1 and 5 fopen() once and fstat() that handle instead of stat()ing the path a second time. - A failed open is now a logged error rather than an indirect rd == 0. - The FIFO lstat() and failPath existence test stay as they are: neither has a descriptor to stat, and mkdtemp()'s 0700 directory makes them unraceable. Issues: CID-651701 --- apps/wolfsshd/test/test_configuration.c | 32 +++++++++++++++---------- 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/apps/wolfsshd/test/test_configuration.c b/apps/wolfsshd/test/test_configuration.c index ce2976ee..1fc371a5 100644 --- a/apps/wolfsshd/test/test_configuration.c +++ b/apps/wolfsshd/test/test_configuration.c @@ -2824,6 +2824,9 @@ static int test_ConfigSavePID(void) void (*prevXfsz)(int); #endif + /* Every path below lives in this directory, which mkdtemp() creates mode + * 0700. No other user can traverse it, so none of the path-based checks + * here can be raced. */ if (mkdtemp(base) == NULL) { Log(" mkdtemp failed.\n"); ret = WS_FATAL_ERROR; @@ -2850,15 +2853,16 @@ static int test_ConfigSavePID(void) ret = pidSave(conf, pidPath); } if (ret == WS_SUCCESS) { - if (stat(pidPath, &st) != 0) { + /* fstat() the open handle instead of stat()ing the path a second + * time, so the mode asserted below belongs to the same file the PID + * was read from. */ + f = fopen(pidPath, "r"); + if (f == NULL || fstat(fileno(f), &st) != 0) { + Log(" Could not open or stat %s.\n", pidPath); ret = WS_FATAL_ERROR; } else { - f = fopen(pidPath, "r"); - rd = (f != NULL) ? fscanf(f, "%ld", &pid) : 0; - if (f != NULL) { - fclose(f); - } + rd = fscanf(f, "%ld", &pid); ret = smExpect("normal PID file written with our PID", (rd == 1 && pid == (long)getpid()) ? WS_SUCCESS : WS_FATAL_ERROR, 1); @@ -2868,6 +2872,9 @@ static int test_ConfigSavePID(void) : WS_SUCCESS, 1); } } + if (f != NULL) { + fclose(f); + } } /* Scenario 2: a symlink at the PID path is refused, link target untouched. @@ -2974,15 +2981,13 @@ static int test_ConfigSavePID(void) } if (ret == WS_SUCCESS) { pid = -1; - if (stat(stale, &st) != 0) { + f = fopen(stale, "r"); + if (f == NULL || fstat(fileno(f), &st) != 0) { + Log(" Could not open or stat %s.\n", stale); ret = WS_FATAL_ERROR; } else { - f = fopen(stale, "r"); - rd = (f != NULL) ? fscanf(f, "%ld", &pid) : 0; - if (f != NULL) { - fclose(f); - } + rd = fscanf(f, "%ld", &pid); ret = smExpect("existing PID file rewritten with our PID", (rd == 1 && pid == (long)getpid()) ? WS_SUCCESS : WS_FATAL_ERROR, 1); @@ -2993,6 +2998,9 @@ static int test_ConfigSavePID(void) : WS_SUCCESS, 1); } } + if (f != NULL) { + fclose(f); + } } /* Scenario 6: a PID file owned by another user is refused instead of being