Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 124 additions & 0 deletions Action.c
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,15 @@ in the source distribution for its full text.
#include <ctype.h>
#include <math.h>
#include <pwd.h>
#include <signal.h>
#include <stdlib.h>
#include <stdbool.h>
#include <sys/param.h>
#include <sys/time.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <termios.h>
#include <unistd.h>

Object* Action_pickFromVector(State* st, Panel* list, int x, bool followProcess) {
Panel* panel = st->panel;
Expand Down Expand Up @@ -378,6 +383,123 @@ static Htop_Reaction actionStrace(State* st) {
return HTOP_REFRESH | HTOP_REDRAW_BAR;
}

/**
* Creates a subprocess with the given argv.
*
* argv must be a null-terminated array of strings,
* and will be passed to execvp.
*
* The subprocess is launched as a child process,
* in its own foreground process group;
* this decouples signals like SIGINT from htop.
*/
bool launchSubProcess(char *const argv[]) {
endwin();

// backup the current terminal settings
struct termios old_attrs;
if (tcgetattr(STDIN_FILENO, &old_attrs) < 0)
{
perror("tcgetattr()");
return false;
}

// backup the current foreground process group
pid_t old_pgrp = tcgetpgrp(STDIN_FILENO);
if (old_pgrp < 0) {
perror("tcgetpgrp()");
return false;
}

pid_t child_pid = fork();
if (child_pid == -1) {
perror("fork()");
return false;
}

if (child_pid == 0) {
// this is the child process. launch the subprocess
printf("htop: launching");
for (size_t i = 0; argv[i] != NULL; i++) {
printf(" %s", argv[i]);
}
putchar('\n');
execvp(argv[0], argv);
// we should never reach this line.
// most likely cause of error: argv[0] doesn't exist
perror("excecvp()");
exit(1);
}

// setup a process group for the child
setpgid(child_pid, child_pid);
tcsetpgrp(STDIN_FILENO, child_pid);
// if the child prints to stdout between setpgid and tcsetpgrp,
// it is stopped; we need to send SIGCONT to prevent this.
kill(child_pid, SIGCONT);

// wait for the child to terminate.
int child_status;
while (waitpid(child_pid, &child_status, 0) == child_pid) {}

// restore the foreground process group
// we must ignore SIGTTOU while we do this
void (*old_handler)(int) = signal(SIGTTOU, SIG_IGN);
tcsetpgrp(STDIN_FILENO, old_pgrp);
if (old_handler != SIG_ERR) {
signal(SIGTTOU, old_handler);
}

// restore the terminal settings
if (tcsetattr(STDIN_FILENO, TCSADRAIN, &old_attrs) < 0) {
perror("tcsetattr()");
return false;
}

// check the child exit status
if (WIFEXITED(child_status)) {
if (WEXITSTATUS(child_status) == 0) {
// all good!
return true;
}
printf("child exited with %d\n", WEXITSTATUS(child_status));
return false;
} else if (WIFSIGNALED(child_status)) {
printf("child killed (signal %d)\n", WTERMSIG(child_status));
return false;
} else {
printf("unexpected wait() status: 0x%x\n", child_status);
return false;
}
}

static Htop_Reaction actionDebugger(State* st) {
Settings* settings = st->settings;
Process* p = (Process*) Panel_getSelected(st->panel);
if (!p) return HTOP_OK;

// prepare the argv for the debugger subprocess
char *arg0 = settings->debuggerTool;
char arg1[] = "-p";
char arg2[64];
xSnprintf(arg2, sizeof(arg2), "%d", (int) p->pid);
char *const argv[] = { arg0, arg1, arg2, NULL };

if (!launchSubProcess(argv)) {
// something went wrong
// pause to allow the user to read the error message(s)
printf("press ENTER to continue");
int c = 0;
while (c != EOF && c != '\n') {
c = getchar();
}
}

refresh();
CRT_enableDelay();
return HTOP_REFRESH | HTOP_REDRAW_BAR | HTOP_UPDATE_PANELHDR;
}

static Htop_Reaction actionTag(State* st) {
Process* p = (Process*) Panel_getSelected(st->panel);
if (!p) return HTOP_OK;
Expand Down Expand Up @@ -423,6 +545,7 @@ static const struct { const char* key; const char* info; } helpRight[] = {
{ .key = " i: ", .info = "set IO priority" },
{ .key = " l: ", .info = "list open files with lsof" },
{ .key = " s: ", .info = "trace syscalls with strace" },
{ .key = " D: ", .info = "attach debugger" },
{ .key = " ", .info = "" },
{ .key = " F2 C S: ", .info = "setup" },
{ .key = " F1 h: ", .info = "show this help screen" },
Expand Down Expand Up @@ -576,6 +699,7 @@ void Action_setBindings(Htop_Action* keys) {
keys[KEY_F(2)] = actionSetup;
keys['l'] = actionLsof;
keys['s'] = actionStrace;
keys['D'] = actionDebugger;
keys[' '] = actionTag;
keys['\014'] = actionRedraw; // Ctrl+L
keys[KEY_F(1)] = actionHelp;
Expand Down
8 changes: 7 additions & 1 deletion Settings.c
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ void Settings_delete(Settings* this) {
String_freeArray(this->columns[i].names);
free(this->columns[i].modes);
}
free(this->debuggerTool);
free(this);
}

Expand Down Expand Up @@ -181,7 +182,10 @@ static bool Settings_read(Settings* this, const char* fileName) {
} else if (String_eq(option[0], "color_scheme")) {
this->colorScheme = atoi(option[1]);
if (this->colorScheme < 0 || this->colorScheme >= LAST_COLORSCHEME) this->colorScheme = 0;
} else if (String_eq(option[0], "enable_mouse")) {
} else if (String_eq(option[0], "debugger_tool")) {
this->debuggerTool = String_trim(option[1]);
if (strlen(this->debuggerTool) == 0) this->debuggerTool = xStrdup("gdb");
} else if (String_eq(option[0], "enable_mouse")) {
this->enableMouse = atoi(option[1]);
} else if (String_eq(option[0], "left_meters")) {
Settings_readMeters(this, option[1], 0);
Expand Down Expand Up @@ -272,6 +276,7 @@ bool Settings_write(Settings* this) {
fprintf(fd, "update_process_names=%d\n", (int) this->updateProcessNames);
fprintf(fd, "account_guest_in_cpu_meter=%d\n", (int) this->accountGuestInCPUMeter);
fprintf(fd, "color_scheme=%d\n", (int) this->colorScheme);
fprintf(fd, "debugger_tool=%s\n", this->debuggerTool);
fprintf(fd, "enable_mouse=%d\n", (int) this->enableMouse);
fprintf(fd, "delay=%d\n", (int) this->delay);
fprintf(fd, "left_meters="); writeMeters(this, fd, 0);
Expand Down Expand Up @@ -353,6 +358,7 @@ Settings* Settings_new(int cpuCount) {
CRT_restorePrivileges();
}
this->colorScheme = 0;
this->debuggerTool = xStrdup("gdb");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe htop should check the ownership of the configuration file it's about to read, so that no un-privileged user can by accident modify a privileged configuration file and inject commands.

(Another unrelated personal thought is to set the value to NULL, i.e. disable the feature by default, so if someone fat-fingers the key d nothing happens.)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I add an ownership check in general and refuse to read the file if (g+w || o+w) is set, or just for this specific setting?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re your unrelated personal thought: I don't think that's necessary; if people don't have gdb installed, nothing will happen when they fat-finger d. If they do have gdb installed, well, it's likely that they know what they're doing, and if they pressed the key by accident they know how to leave gdb.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re again: I've changed it to D which should be less likely to be hit by accident.

Copy link
Copy Markdown
Author

@mic-e mic-e Sep 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding your request to check the ownership:

I think that this could be accomplished using the following code to Settings_read():

   bool trusted = true;
   struct stat64 file_stat;
   fstat64(fileno(fd), &file_stat);
   if (file_stat.st_uid != getuid32()) {
      printf("warning: htop settings file is not owned by uid %d\n", getuid32());
      trusted = false;
   } else if (file_stat.st_gid != getgid32()) {
      printf("warning: htop settings file is not owned by gid %d\n", getgid32());
      trusted = false;
   } else if (file_stat.st_mode & 0002) {
      printf("warning: htop settings file is writable by others\n");
      trusted = false;
   }

And then ignoring certain values if trusted == false.

However, having implemented this and thought about it for a bit, I now strongly oppose the idea because:

  • It can be easily circumvented by an attacker who has write permissions to any folder in the hierarchy
  • It prevents the config from working when I launch htop with sudo
  • If somebody manages to manipulate the htop config in my home folder, they could just as well inject stuff into my .bashrc. Other tools like bash don't check their config files for permissions either.
  • ssh used to check its config files for permission bits but this was removed because it caused too much pain and actually decreased security by e.g. preventing users from putting their secret keys onto flash drives
  • I think ACLs would be ignored

(unless I misunderstood your request?)

this->enableMouse = true;
this->changed = false;
this->delay = DEFAULT_DELAY;
Expand Down
1 change: 1 addition & 0 deletions Settings.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ typedef struct Settings_ {
ProcessField* fields;
int flags;
int colorScheme;
char *debuggerTool;
int delay;

int cpuCount;
Expand Down
4 changes: 3 additions & 1 deletion TESTPLAN
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,9 @@ Main screen:

<F2>, <C>, <S> - enter Setup screen.

<D>, <E> - do nothing.
<D> - attach debugger (must be allowed by ptrace_scope)

<E> - do nothing.

<F> - follow process.

Expand Down
4 changes: 4 additions & 0 deletions htop.1.in
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,10 @@ update of system calls issued by the process.
.B l
Display open files for a process: if lsof(1) is installed, pressing this key
will display the list of file descriptors opened by the process.
.B D
Attach debugger to process: if the configured debugger tool (default: gdb(1))
is installed, pressing this key will launch and attach it to the currently
selected process.
.TP
.B F1, h, ?
Go to the help screen
Expand Down