From d18e9a4895599a479df264a6c7380b8805abb434 Mon Sep 17 00:00:00 2001 From: Michael Klein Date: Wed, 2 Dec 2015 22:15:46 +0100 Subject: [PATCH 1/4] 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/4] 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/4] 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/4] 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;