From 34a1847276c26d06418ee68b632e0eb13a74110f Mon Sep 17 00:00:00 2001 From: Daniel Lange Date: Sun, 30 Aug 2026 14:33:40 +0200 Subject: [PATCH 1/2] Make htop_history reside in $XDG_STATE_HOME and harden accessing the file against symlink attacks Fixes GHSA-46qw-qjgv-jhrw, thank you 0xseiryuu. Resolves #1944, thank you @Explorer09. Resolves #2089. Assisted-by: OpenCode Zen --- CommandLine.c | 15 ++----- History.c | 60 ++++++++++++++++++++++++--- History.h | 1 + Settings.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++--- Settings.h | 6 +++ htop.1.in | 10 +++++ 6 files changed, 180 insertions(+), 22 deletions(-) diff --git a/CommandLine.c b/CommandLine.c index 1ca40c046..e7541685e 100644 --- a/CommandLine.c +++ b/CommandLine.c @@ -472,19 +472,10 @@ int CommandLine_run(int argc, char** argv) { if (flags.commFilter) setCommFilter(&state, &(flags.commFilter)); - /* Set up shared search/filter history, stored next to the config file */ - const char* rcPath = settings->filename; - const char* lastSlash = strrchr(rcPath, '/'); - char historyPath[PATH_MAX]; - if (lastSlash) { - int dirLen = (int)(lastSlash - rcPath + 1); - xSnprintf(historyPath, sizeof(historyPath), "%.*s" "htop_history", dirLen, rcPath); - } else { - /* no history file saved unless we have a sane rcPath */ - historyPath[0] = '\0'; - } - + /* Set up shared search/filter history, stored below the XDG state directory */ + char* historyPath = Settings_getHistoryFile(); IncSet_setHistoryFile(panel->inc, historyPath); + free(historyPath); ScreenManager* scr = ScreenManager_new(header, host, &state, true); ScreenManager_add(scr, (Panel*) panel, -1); diff --git a/History.c b/History.c index 50320abfd..10a75c6d4 100644 --- a/History.c +++ b/History.c @@ -9,23 +9,58 @@ in the source distribution for its full text. #include "History.h" +#include #include #include #include #include #include +#include #include "Macros.h" #include "XUtils.h" +/* Determine whether the history file is safe to (over)write, mirroring the + checks Settings_read() applies to htoprc: the file must be a regular file + owned by the effective user with owner-write permission. The O_NOFOLLOW + flag guards the final path component against symlink attacks. */ static void History_load(History* this) { if (!this->filename) return; - FILE* fp = fopen(this->filename, "r"); - if (!fp) + + int fd = -1; + do { + fd = open(this->filename, O_RDWR | O_NOCTTY | O_NOFOLLOW); + } while (fd < 0 && errno == EINTR); + + if (fd < 0) { + this->writeHistory = (errno == ENOENT); + if (errno != EACCES && errno != EPERM && errno != EROFS) + return; + } else { + struct stat sb; + int err = fstat(fd, &sb); + this->writeHistory = !err && S_ISREG(sb.st_mode) && (sb.st_mode & S_IWUSR) && sb.st_uid == geteuid(); + } + + /* If opening read & write is not possible, open read only. + There is no risk of following a symlink in this case. */ + if (fd < 0) { + do { + fd = open(this->filename, O_RDONLY | O_NOCTTY); + } while (fd < 0 && errno == EINTR); + } + + if (fd < 0) return; + FILE* fp = fdopen(fd, "r"); + if (!fp) { + close(fd); + return; + } + char line[LINEEDITOR_MAX + 2]; while (fgets(line, sizeof(line), fp)) { size_t len = strlen(line); @@ -48,6 +83,7 @@ History* History_new(const char* filename) { this->position = 0; this->saved[0] = '\0'; this->filename = filename ? xStrdup(filename) : NULL; + this->writeHistory = true; if (this->filename) History_load(this); @@ -66,12 +102,26 @@ void History_delete(History* this) { } void History_save(const History* this) { - if (!this->filename) + if (!this->filename || !this->writeHistory) return; - /* Settings_write writes things via a temp file & rename, we do it less robust but faster here: */ - int fd = open(this->filename, O_WRONLY | O_CREAT | O_TRUNC, 0600); + /* Settings_write writes things via a temp file & rename, we do it less robust but faster here. + O_NOFOLLOW guards against a symlink planted at the final path component, + and the fstat() re-check closes a race between open and the owner verification. */ + int fd = open(this->filename, O_WRONLY | O_NOCTTY | O_CREAT | O_NOFOLLOW, 0600); if (fd == -1) return; + + struct stat sb; + if (fstat(fd, &sb) != 0 || !S_ISREG(sb.st_mode) || !(sb.st_mode & S_IWUSR) || sb.st_uid != geteuid()) { + close(fd); + return; + } + + if (ftruncate(fd, 0) != 0) { + close(fd); + return; + } + FILE* fp = fdopen(fd, "w"); if (!fp) { close(fd); // fd not consumed on failure, so close it diff --git a/History.h b/History.h index d2989d7fc..d187ed7e5 100644 --- a/History.h +++ b/History.h @@ -22,6 +22,7 @@ typedef struct History_ { size_t position; /* current browse position: count = "at new input" */ char saved[LINEEDITOR_MAX + 1]; /* saved current input while browsing */ char* filename; /* path to history file (may be NULL = no read / write) */ + bool writeHistory; /* whether the history file may be (over)written */ } History; /* Create a new History, loading from the given file (may be NULL = init new history) */ diff --git a/Settings.c b/Settings.c index 65c7292b8..453201357 100644 --- a/Settings.c +++ b/Settings.c @@ -910,6 +910,110 @@ int Settings_write(const Settings* this, bool onCrash) { return r; } +static const char* Settings_getHome(void) { + const char* home = getenv("HOME"); + if (!home || home[0] != '/') { + const struct passwd* pw = getpwuid(getuid()); + return (pw && pw->pw_dir && pw->pw_dir[0] == '/') ? pw->pw_dir : ""; + } + return home; +} + +static bool Settings_mkdirp(const char* path, mode_t mode) { + char* copy = xStrdup(path); + bool ok = true; + for (char* p = copy + (copy[0] == '/' ? 1 : 0); *p; p++) { + if (*p != '/') + continue; + *p = '\0'; + if (mkdir(copy, mode) != 0 && errno != EEXIST) + ok = false; + *p = '/'; + } + if (mkdir(copy, mode) != 0 && errno != EEXIST) + ok = false; + free(copy); + return ok; +} + +static void Settings_migrateHistory(const char* fromPath, const char* toPath) { + if (access(toPath, F_OK) == 0) + return; + + struct stat sb; + if (lstat(fromPath, &sb) != 0 || !S_ISREG(sb.st_mode) || sb.st_uid != geteuid()) + return; + + int fromFd = open(fromPath, O_RDONLY | O_NOFOLLOW); + if (fromFd < 0) + return; + int toFd = open(toPath, O_WRONLY | O_CREAT | O_EXCL | O_NOFOLLOW, 0600); + if (toFd < 0) { + close(fromFd); + return; + } + + bool ok = true; + char buf[4096]; + for (;;) { + ssize_t n = read(fromFd, buf, sizeof(buf)); + if (n < 0) { + if (errno == EINTR) + continue; + ok = false; + break; + } + if (n == 0) + break; + if (full_write(toFd, buf, (size_t)n) != n) { + ok = false; + break; + } + } + if (close(fromFd) != 0) + ok = false; + if (close(toFd) != 0 || !ok) { + unlink(toPath); + } else { + (void) unlink(fromPath); + } +} + +char* Settings_getHistoryFile(void) { + const char* xdgStateHome = getenv("XDG_STATE_HOME"); + const char* xdgConfigHome = getenv("XDG_CONFIG_HOME"); + const char* home = Settings_getHome(); + + if ((!xdgStateHome || xdgStateHome[0] != '/') && !home[0]) + return NULL; + + char* stateHtopDir; + if (xdgStateHome && xdgStateHome[0] == '/') + stateHtopDir = String_cat(xdgStateHome, "/htop"); + else + stateHtopDir = String_cat(home, "/.local/state/htop"); + + char* historyFile = String_cat(stateHtopDir, "/htop_history"); + if (!Settings_mkdirp(stateHtopDir, 0700)) { + free(stateHtopDir); + free(historyFile); + return NULL; + } + free(stateHtopDir); + + char* legacyDir; + if (xdgConfigHome && xdgConfigHome[0] == '/') + legacyDir = String_cat(xdgConfigHome, "/htop"); + else + legacyDir = String_cat(home, CONFIGDIR "/htop"); + char* legacyFile = String_cat(legacyDir, "/htop_history"); + free(legacyDir); + Settings_migrateHistory(legacyFile, historyFile); + free(legacyFile); + + return historyFile; +} + Settings* Settings_new(const Machine* host, Hashtable* dynamicMeters, Hashtable* dynamicColumns, Hashtable* dynamicScreens) { Settings* this = xCalloc(1, sizeof(Settings)); @@ -963,11 +1067,7 @@ Settings* Settings_new(const Machine* host, Hashtable* dynamicMeters, Hashtable* if (rcfile) { this->initialFilename = xStrdup(rcfile); } else { - const char* home = getenv("HOME"); - if (!home || home[0] != '/') { - const struct passwd* pw = getpwuid(getuid()); - home = (pw && pw->pw_dir && pw->pw_dir[0] == '/') ? pw->pw_dir : ""; - } + const char* home = Settings_getHome(); const char* xdgConfigHome = getenv("XDG_CONFIG_HOME"); char* configDir = NULL; char* htopDir = NULL; diff --git a/Settings.h b/Settings.h index e1642ab0b..ab44d070a 100644 --- a/Settings.h +++ b/Settings.h @@ -151,4 +151,10 @@ bool Settings_isReadonly(void); void Settings_setHeaderLayout(Settings* this, HeaderLayout hLayout); +/* Return the path of the search/filter history file, located below the XDG + state directory. Ensures the directory exists and migrates a legacy history + file from the configuration directory. Returns NULL if no suitable home + directory can be determined. */ +char* Settings_getHistoryFile(void); + #endif diff --git a/htop.1.in b/htop.1.in index 818e3ce77..d81984e6e 100644 --- a/htop.1.in +++ b/htop.1.in @@ -739,6 +739,16 @@ tries to read the system-wide configuration from .I @sysconfdir@/htoprc and as a last resort, falls back to its hard coded defaults. .LP +The search and filter history is stored below the XDG state directory, in +.IR $XDG_STATE_HOME/htop/htop_history , +defaulting to +.IR ~/.local/state/htop/htop_history +when the +.IR $XDG_STATE_HOME +variable is not set. +A history file left over from an older version next to the configuration +file is migrated to this location automatically on the next start. +.LP You may override the location of the configuration file using the $HTOPRC environment variable (so you can have multiple configurations for different machines that share the same home directory, for example). From 1217436913cd540e4efbcf78eca799c0f0af2e2c Mon Sep 17 00:00:00 2001 From: Daniel Lange Date: Sun, 30 Aug 2026 15:47:56 +0200 Subject: [PATCH 2/2] Address review feedback from CodeQL and CodeRabbitAI Assisted-by: CodeRabbitAI, OpenCode Zen --- CommandLine.c | 2 +- History.c | 22 +++++++++++++------ Settings.c | 58 ++++++++++++++++++++++++++++++++++----------------- Settings.h | 6 +++--- htop.1.in | 5 +++-- 5 files changed, 62 insertions(+), 31 deletions(-) diff --git a/CommandLine.c b/CommandLine.c index e7541685e..f79106f4a 100644 --- a/CommandLine.c +++ b/CommandLine.c @@ -473,7 +473,7 @@ int CommandLine_run(int argc, char** argv) { setCommFilter(&state, &(flags.commFilter)); /* Set up shared search/filter history, stored below the XDG state directory */ - char* historyPath = Settings_getHistoryFile(); + char* historyPath = Settings_getHistoryFile(settings->filename); IncSet_setHistoryFile(panel->inc, historyPath); free(historyPath); diff --git a/History.c b/History.c index 10a75c6d4..c3f255319 100644 --- a/History.c +++ b/History.c @@ -24,14 +24,15 @@ in the source distribution for its full text. /* Determine whether the history file is safe to (over)write, mirroring the checks Settings_read() applies to htoprc: the file must be a regular file owned by the effective user with owner-write permission. The O_NOFOLLOW - flag guards the final path component against symlink attacks. */ + flag guards the final path component against symlink attacks, while + O_NONBLOCK keeps an owned FIFO from blocking the read-only fallback. */ static void History_load(History* this) { if (!this->filename) return; int fd = -1; do { - fd = open(this->filename, O_RDWR | O_NOCTTY | O_NOFOLLOW); + fd = open(this->filename, O_RDWR | O_NOCTTY | O_NOFOLLOW | O_NONBLOCK); } while (fd < 0 && errno == EINTR); if (fd < 0) { @@ -45,16 +46,24 @@ static void History_load(History* this) { } /* If opening read & write is not possible, open read only. - There is no risk of following a symlink in this case. */ + O_NOFOLLOW rejects a planted symlink, O_NONBLOCK avoids blocking on + non-regular files such as FIFOs when no writer is present. */ if (fd < 0) { do { - fd = open(this->filename, O_RDONLY | O_NOCTTY); + fd = open(this->filename, O_RDONLY | O_NOCTTY | O_NOFOLLOW | O_NONBLOCK); } while (fd < 0 && errno == EINTR); } if (fd < 0) return; + /* Only read regular files; reading a FIFO would block. */ + struct stat sb; + if (fstat(fd, &sb) != 0 || !S_ISREG(sb.st_mode)) { + close(fd); + return; + } + FILE* fp = fdopen(fd, "r"); if (!fp) { close(fd); @@ -106,8 +115,9 @@ void History_save(const History* this) { return; /* Settings_write writes things via a temp file & rename, we do it less robust but faster here. O_NOFOLLOW guards against a symlink planted at the final path component, - and the fstat() re-check closes a race between open and the owner verification. */ - int fd = open(this->filename, O_WRONLY | O_NOCTTY | O_CREAT | O_NOFOLLOW, 0600); + O_NONBLOCK avoids hanging on an existing FIFO, and the fstat() re-check + closes a race between open and the owner verification. */ + int fd = open(this->filename, O_WRONLY | O_NOCTTY | O_CREAT | O_NOFOLLOW | O_NONBLOCK, 0600); if (fd == -1) return; diff --git a/Settings.c b/Settings.c index 453201357..092c3f6bb 100644 --- a/Settings.c +++ b/Settings.c @@ -937,17 +937,24 @@ static bool Settings_mkdirp(const char* path, mode_t mode) { } static void Settings_migrateHistory(const char* fromPath, const char* toPath) { - if (access(toPath, F_OK) == 0) + /* O_NOFOLLOW and O_NONBLOCK ensure a symlink or FIFO planted at the path + cannot redirect the copy or block it. */ + int fromFd = open(fromPath, O_RDONLY | O_NOFOLLOW | O_NONBLOCK); + if (fromFd < 0) return; + /* Validate the descriptor we actually opened: regular file owned by the + effective user. O_NOFOLLOW rejects a symlink at the final path component, + and fstat() rules out a swap between open() and here. */ struct stat sb; - if (lstat(fromPath, &sb) != 0 || !S_ISREG(sb.st_mode) || sb.st_uid != geteuid()) + if (fstat(fromFd, &sb) != 0 || !S_ISREG(sb.st_mode) || sb.st_uid != geteuid()) { + close(fromFd); return; + } - int fromFd = open(fromPath, O_RDONLY | O_NOFOLLOW); - if (fromFd < 0) - return; - int toFd = open(toPath, O_WRONLY | O_CREAT | O_EXCL | O_NOFOLLOW, 0600); + /* O_EXCL guarantees the destination is never overwritten; once the state + file exists it takes precedence over the legacy copy. */ + int toFd = open(toPath, O_WRONLY | O_CREAT | O_EXCL | O_NOFOLLOW | O_NONBLOCK, 0600); if (toFd < 0) { close(fromFd); return; @@ -974,14 +981,29 @@ static void Settings_migrateHistory(const char* fromPath, const char* toPath) { ok = false; if (close(toFd) != 0 || !ok) { unlink(toPath); - } else { - (void) unlink(fromPath); + return; } + + /* Remove the legacy file only if it still refers to the entry we copied. */ + struct stat sbPath; + if (lstat(fromPath, &sbPath) == 0 && sbPath.st_dev == sb.st_dev && sbPath.st_ino == sb.st_ino) + (void) unlink(fromPath); } -char* Settings_getHistoryFile(void) { +/* Return the legacy history path as stored beside a configuration file + (the old history location), or NULL when the file has no directory part. */ +static char* Settings_legacyHistoryFile(const char* configFile) { + const char* lastSlash = strrchr(configFile, '/'); + if (!lastSlash) + return NULL; + char* dir = xStrndup(configFile, (size_t)(lastSlash - configFile + 1)); + char* file = String_cat(dir, "htop_history"); + free(dir); + return file; +} + +char* Settings_getHistoryFile(const char* configFile) { const char* xdgStateHome = getenv("XDG_STATE_HOME"); - const char* xdgConfigHome = getenv("XDG_CONFIG_HOME"); const char* home = Settings_getHome(); if ((!xdgStateHome || xdgStateHome[0] != '/') && !home[0]) @@ -1001,15 +1023,13 @@ char* Settings_getHistoryFile(void) { } free(stateHtopDir); - char* legacyDir; - if (xdgConfigHome && xdgConfigHome[0] == '/') - legacyDir = String_cat(xdgConfigHome, "/htop"); - else - legacyDir = String_cat(home, CONFIGDIR "/htop"); - char* legacyFile = String_cat(legacyDir, "/htop_history"); - free(legacyDir); - Settings_migrateHistory(legacyFile, historyFile); - free(legacyFile); + /* The search/filter history used to be stored next to the + htoprc file; migrate it if present. */ + char* legacyFile = Settings_legacyHistoryFile(configFile); + if (legacyFile) { + Settings_migrateHistory(legacyFile, historyFile); + free(legacyFile); + } return historyFile; } diff --git a/Settings.h b/Settings.h index ab44d070a..637f2de00 100644 --- a/Settings.h +++ b/Settings.h @@ -153,8 +153,8 @@ void Settings_setHeaderLayout(Settings* this, HeaderLayout hLayout); /* Return the path of the search/filter history file, located below the XDG state directory. Ensures the directory exists and migrates a legacy history - file from the configuration directory. Returns NULL if no suitable home - directory can be determined. */ -char* Settings_getHistoryFile(void); + file stored next to the active configuration file. Returns NULL if no + suitable home directory can be determined. */ +char* Settings_getHistoryFile(const char* configFile); #endif diff --git a/htop.1.in b/htop.1.in index d81984e6e..c69a42356 100644 --- a/htop.1.in +++ b/htop.1.in @@ -746,8 +746,9 @@ defaulting to when the .IR $XDG_STATE_HOME variable is not set. -A history file left over from an older version next to the configuration -file is migrated to this location automatically on the next start. +A history file left over from an older version stored next to the htoprc +configuration file is migrated to this location automatically on the next +start. .LP You may override the location of the configuration file using the $HTOPRC environment variable (so you can have multiple configurations for different