From d18e9a4895599a479df264a6c7380b8805abb434 Mon Sep 17 00:00:00 2001 From: Michael Klein Date: Wed, 2 Dec 2015 22:15:46 +0100 Subject: [PATCH 1/7] add some security checks when running SUID root on Darwin, htop needs to run with root privileges to display information about other users processes. This commit makes running htop SUID root a bit more safe. --- Process.c | 17 +++++++++++------ TraceScreen.c | 1 + 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/Process.c b/Process.c index 4999bfcb..a1c2079c 100644 --- a/Process.c +++ b/Process.c @@ -513,12 +513,16 @@ void Process_toggleTag(Process* this) { } bool Process_setPriority(Process* this, int priority) { - int old_prio = getpriority(PRIO_PROCESS, this->pid); - int err = setpriority(PRIO_PROCESS, this->pid, priority); - if (err == 0 && old_prio != getpriority(PRIO_PROCESS, this->pid)) { - this->nice = priority; + if ( Process_getuid == 0 || Process_getuid == (int) this->st_uid ) { + int old_prio = getpriority(PRIO_PROCESS, this->pid); + int err = setpriority(PRIO_PROCESS, this->pid, priority); + if (err == 0 && old_prio != getpriority(PRIO_PROCESS, this->pid)) { + this->nice = priority; + } + return (err == 0); } - return (err == 0); + else + return false; } bool Process_changePriorityBy(Process* this, size_t delta) { @@ -526,7 +530,8 @@ bool Process_changePriorityBy(Process* this, size_t delta) { } void Process_sendSignal(Process* this, size_t sgn) { - kill(this->pid, (int) sgn); + if ( Process_getuid == 0 || Process_getuid == (int) this->st_uid ) + kill(this->pid, (int) sgn); } long Process_pidCompare(const void* v1, const void* v2) { diff --git a/TraceScreen.c b/TraceScreen.c index ecd0c0ab..3a62eb63 100644 --- a/TraceScreen.c +++ b/TraceScreen.c @@ -86,6 +86,7 @@ void TraceScreen_run(TraceScreen* this) { int child = fork(); if (child == -1) return; if (child == 0) { + seteuid(getuid()); dup2(fdpair[1], STDERR_FILENO); int ok = fcntl(fdpair[1], F_SETFL, O_NONBLOCK); if (ok != -1) { From 42b08f223395eb8cfcd0d36389803f329ea493f7 Mon Sep 17 00:00:00 2001 From: Michael Klein Date: Wed, 2 Dec 2015 23:42:10 +0100 Subject: [PATCH 2/7] drop privileges during Settings_read()/Settings_write() --- Settings.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/Settings.c b/Settings.c index 65e49bfc..6a674549 100644 --- a/Settings.c +++ b/Settings.c @@ -154,7 +154,12 @@ static void readFields(ProcessField* fields, int* flags, const char* line) { } static bool Settings_read(Settings* this, const char* fileName) { - FILE* fd = fopen(fileName, "r"); + FILE* fd; + uid_t euid = geteuid(); + + seteuid(getuid()); + fd = fopen(fileName, "w"); + seteuid(euid); if (!fd) return false; @@ -260,7 +265,11 @@ static void writeMeterModes(Settings* this, FILE* fd, int column) { bool Settings_write(Settings* this) { FILE* fd; + uid_t euid = geteuid(); + + seteuid(getuid()); fd = fopen(this->filename, "w"); + seteuid(euid); if (fd == NULL) { return false; } @@ -345,6 +354,8 @@ Settings* Settings_new(int cpuCount) { htopDir = String_cat(home, "/.config/htop"); } legacyDotfile = String_cat(home, "/.htoprc"); + uid_t euid = geteuid(); + seteuid(getuid()); (void) mkdir(configDir, 0700); (void) mkdir(htopDir, 0700); free(htopDir); @@ -357,6 +368,7 @@ Settings* Settings_new(int cpuCount) { free(legacyDotfile); legacyDotfile = NULL; } + seteuid(euid); } this->colorScheme = 0; this->changed = false; From ab3a7c2fa826932c9c297885b0ea33f1d880cc01 Mon Sep 17 00:00:00 2001 From: Michael Klein Date: Mon, 7 Dec 2015 20:10:09 +0100 Subject: [PATCH 3/7] drop privileges before changing process priority or sending signals - replaces uid check from d18e9a4895599a479df264a6c7380b8805abb434 --- Process.c | 23 ++++++++++++----------- Process.h | 2 ++ 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/Process.c b/Process.c index a1c2079c..8046daf1 100644 --- a/Process.c +++ b/Process.c @@ -513,16 +513,15 @@ void Process_toggleTag(Process* this) { } bool Process_setPriority(Process* this, int priority) { - if ( Process_getuid == 0 || Process_getuid == (int) this->st_uid ) { - int old_prio = getpriority(PRIO_PROCESS, this->pid); - int err = setpriority(PRIO_PROCESS, this->pid, priority); - if (err == 0 && old_prio != getpriority(PRIO_PROCESS, this->pid)) { - this->nice = priority; - } - return (err == 0); + uid_t euid = geteuid(); + seteuid(getuid()); + int old_prio = getpriority(PRIO_PROCESS, this->pid); + int err = setpriority(PRIO_PROCESS, this->pid, priority); + seteuid(euid); + if (err == 0 && old_prio != getpriority(PRIO_PROCESS, this->pid)) { + this->nice = priority; } - else - return false; + return (err == 0); } bool Process_changePriorityBy(Process* this, size_t delta) { @@ -530,8 +529,10 @@ bool Process_changePriorityBy(Process* this, size_t delta) { } void Process_sendSignal(Process* this, size_t sgn) { - if ( Process_getuid == 0 || Process_getuid == (int) this->st_uid ) - kill(this->pid, (int) sgn); + uid_t euid = geteuid(); + seteuid(getuid()); + kill(this->pid, (int) sgn); + seteuid(euid); } long Process_pidCompare(const void* v1, const void* v2) { diff --git a/Process.h b/Process.h index 841b1291..d856c035 100644 --- a/Process.h +++ b/Process.h @@ -158,6 +158,8 @@ typedef struct ProcessClass_ { #define ONE_DECIMAL_M (ONE_DECIMAL_K * ONE_DECIMAL_K) #define ONE_DECIMAL_G (ONE_DECIMAL_M * ONE_DECIMAL_K) +extern char Process_pidFormat[20]; + void Process_setupColumnWidths(); void Process_humanNumber(RichString* str, unsigned long number, bool coloring); From 84783bd6f0b9da40d3ce92d0a81e56d276d24eca Mon Sep 17 00:00:00 2001 From: Michael Klein Date: Wed, 9 Dec 2015 20:34:11 +0100 Subject: [PATCH 4/7] Fix fopen mode in Settings_read() --- Settings.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Settings.c b/Settings.c index 6a674549..8eb5bbd7 100644 --- a/Settings.c +++ b/Settings.c @@ -158,7 +158,7 @@ static bool Settings_read(Settings* this, const char* fileName) { uid_t euid = geteuid(); seteuid(getuid()); - fd = fopen(fileName, "w"); + fd = fopen(fileName, "r"); seteuid(euid); if (!fd) return false; From e595f6865e5d58cc6996124dac9df8384519ce20 Mon Sep 17 00:00:00 2001 From: Michael McConville Date: Mon, 4 Jan 2016 16:20:51 -0500 Subject: [PATCH 5/7] Rename variable for consistency Suggested by Hisham. --- openbsd/OpenBSDProcessList.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/openbsd/OpenBSDProcessList.c b/openbsd/OpenBSDProcessList.c index 9757ef91..6da38efc 100644 --- a/openbsd/OpenBSDProcessList.c +++ b/openbsd/OpenBSDProcessList.c @@ -49,8 +49,8 @@ ProcessList* ProcessList_new(UsersTable* usersTable, Hashtable* pidWhiteList, ui int mib[] = { CTL_HW, HW_NCPU }; int fmib[] = { CTL_KERN, KERN_FSCALE }; int i, e; - OpenBSDProcessList* fpl = calloc(1, sizeof(OpenBSDProcessList)); - ProcessList* pl = (ProcessList*) fpl; + OpenBSDProcessList* opl = calloc(1, sizeof(OpenBSDProcessList)); + ProcessList* pl = (ProcessList*) opl; size_t size = sizeof(pl->cpuCount); ProcessList_init(pl, Class(OpenBSDProcess), usersTable, pidWhiteList, userId); @@ -58,31 +58,31 @@ ProcessList* ProcessList_new(UsersTable* usersTable, Hashtable* pidWhiteList, ui if (e == -1 || pl->cpuCount < 1) { pl->cpuCount = 1; } - fpl->cpus = realloc(fpl->cpus, pl->cpuCount * sizeof(CPUData)); + opl->cpus = realloc(opl->cpus, pl->cpuCount * sizeof(CPUData)); size = sizeof(fscale); if (sysctl(fmib, 2, &fscale, &size, NULL, 0) < 0) err(1, "fscale sysctl call failed"); for (i = 0; i < pl->cpuCount; i++) { - fpl->cpus[i].totalTime = 1; - fpl->cpus[i].totalPeriod = 1; + opl->cpus[i].totalTime = 1; + opl->cpus[i].totalPeriod = 1; } pageSizeKb = PAGE_SIZE_KB; // XXX: last arg should eventually be an errbuf - fpl->kd = kvm_open(NULL, NULL, NULL, KVM_NO_FILES, NULL); - assert(fpl->kd); + opl->kd = kvm_open(NULL, NULL, NULL, KVM_NO_FILES, NULL); + assert(opl->kd); return pl; } void ProcessList_delete(ProcessList* this) { - const OpenBSDProcessList* fpl = (OpenBSDProcessList*) this; - if (fpl->kd) kvm_close(fpl->kd); + const OpenBSDProcessList* opl = (OpenBSDProcessList*) this; + if (opl->kd) kvm_close(opl->kd); - free(fpl->cpus); + free(opl->cpus); ProcessList_done(this); free(this); @@ -102,7 +102,7 @@ static inline void OpenBSDProcessList_scanMemoryInfo(ProcessList* pl) { pl->totalMem = uvmexp.npages * pageSizeKb; /* - const OpenBSDProcessList* fpl = (OpenBSDProcessList*) pl; + const OpenBSDProcessList* opl = (OpenBSDProcessList*) pl; size_t len = sizeof(pl->totalMem); sysctl(MIB_hw_physmem, 2, &(pl->totalMem), &len, NULL, 0); @@ -114,7 +114,7 @@ static inline void OpenBSDProcessList_scanMemoryInfo(ProcessList* pl) { pl->cachedMem *= pageSizeKb; struct kvm_swap swap[16]; - int nswap = kvm_getswapinfo(fpl->kd, swap, sizeof(swap)/sizeof(swap[0]), 0); + int nswap = kvm_getswapinfo(opl->kd, swap, sizeof(swap)/sizeof(swap[0]), 0); pl->totalSwap = 0; pl->usedSwap = 0; for (int i = 0; i < nswap; i++) { @@ -180,7 +180,7 @@ double getpcpu(const struct kinfo_proc *kp) { } void ProcessList_goThroughEntries(ProcessList* this) { - OpenBSDProcessList* fpl = (OpenBSDProcessList*) this; + OpenBSDProcessList* opl = (OpenBSDProcessList*) this; Settings* settings = this->settings; bool hideKernelThreads = settings->hideKernelThreads; bool hideUserlandThreads = settings->hideUserlandThreads; @@ -194,7 +194,7 @@ void ProcessList_goThroughEntries(ProcessList* this) { OpenBSDProcessList_scanMemoryInfo(this); // use KERN_PROC_KTHREAD to also include kernel threads - struct kinfo_proc* kprocs = kvm_getprocs(fpl->kd, KERN_PROC_ALL, 0, sizeof(struct kinfo_proc), &count); + struct kinfo_proc* kprocs = kvm_getprocs(opl->kd, KERN_PROC_ALL, 0, sizeof(struct kinfo_proc), &count); //struct kinfo_proc* kprocs = getprocs(KERN_PROC_ALL, 0, &count); for (i = 0; i < count; i++) { @@ -218,11 +218,11 @@ void ProcessList_goThroughEntries(ProcessList* this) { proc->starttime_ctime = kproc->p_ustart_sec; proc->user = UsersTable_getRef(this->usersTable, proc->st_uid); ProcessList_add((ProcessList*)this, proc); - proc->comm = OpenBSDProcessList_readProcessName(fpl->kd, kproc, &proc->basenameOffset); + proc->comm = OpenBSDProcessList_readProcessName(opl->kd, kproc, &proc->basenameOffset); } else { if (settings->updateProcessNames) { free(proc->comm); - proc->comm = OpenBSDProcessList_readProcessName(fpl->kd, kproc, &proc->basenameOffset); + proc->comm = OpenBSDProcessList_readProcessName(opl->kd, kproc, &proc->basenameOffset); } } From 4c23a81d725da49e3142e6de3655ef2cfb47da41 Mon Sep 17 00:00:00 2001 From: Christian Hesse Date: Tue, 5 Jan 2016 10:23:08 +0100 Subject: [PATCH 6/7] use AC_HELP_STRING for proc dir --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 709b69b7..424e80ce 100644 --- a/configure.ac +++ b/configure.ac @@ -103,7 +103,7 @@ if test "x$enable_proc" = xyes; then AC_DEFINE(HAVE_PROC, 1, [Define if using a Linux-compatible proc filesystem.]) fi -AC_ARG_WITH(proc, [ --with-proc=DIR Location of a Linux-compatible proc filesystem (default=/proc).], +AC_ARG_WITH(proc, [AC_HELP_STRING([--with-proc=DIR], [Location of a Linux-compatible proc filesystem (default=/proc).])], if test -n "$withval"; then AC_DEFINE_UNQUOTED(PROCDIR, "$withval", [Path of proc filesystem]) From 4b29e8485fcdd9a838173d13b1d348c8ac2b0218 Mon Sep 17 00:00:00 2001 From: Benjamin Porter Date: Wed, 6 Jan 2016 13:49:49 -0900 Subject: [PATCH 7/7] Fix typo: prority => priority --- Action.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Action.c b/Action.c index 6c387def..5e0e109c 100644 --- a/Action.c +++ b/Action.c @@ -404,7 +404,7 @@ static struct { const char* key; const char* info; } helpRight[] = { #if (HAVE_LIBHWLOC || HAVE_NATIVE_AFFINITY) { .key = " a: ", .info = "set CPU affinity" }, #endif - { .key = " i: ", .info = "set IO prority" }, + { .key = " i: ", .info = "set IO priority" }, { .key = " l: ", .info = "list open files with lsof" }, { .key = " s: ", .info = "trace syscalls with strace" }, { .key = " ", .info = "" },