Skip to content

Commit

Permalink
Do not close FDs 0, 1, or 2
Browse files Browse the repository at this point in the history
If they are closed, another file descriptor could be created with these
numbers, and so standard library functions that use them might write to
an unwanted place.  dup2() a file descriptor to /dev/null over them
instead.

Also statically initialize trigger_fd to -1, which is the conventional
value for an invalid file descriptor.

This requires care to avoid closing the file descriptor to /dev/null in
fix_fds(), which took over an hour to debug.
  • Loading branch information
DemiMarie committed Feb 4, 2025
1 parent d116d40 commit 24ef8b1
Showing 1 changed file with 24 additions and 7 deletions.
31 changes: 24 additions & 7 deletions agent/qrexec-agent.c
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ static libvchan_t *ctrl_vchan;

static pid_t wait_for_session_pid = -1;

static int trigger_fd;
static int trigger_fd = -1;

static int terminate_requested;

Expand Down Expand Up @@ -155,13 +155,18 @@ _Noreturn static void really_exec(const char *prog, const struct passwd *pw,
_exit(QREXEC_EXIT_PROBLEM);
}

static void close_std(void)
static void close_std(int null_fd)
{
/* close std*, so when child process closes them, qrexec-agent will receive EOF */
/* this is the main purpose of this reimplementation of /bin/su... */
close(0);
close(1);
close(2);
for (int i = 0; i < 3; ++i) {
int j;
do {
j = dup2(null_fd, i);
} while (j == -1 && (errno == EINTR || errno == EBUSY));
if (j != i)
abort();
}
}

static int really_wait(pid_t child)
Expand Down Expand Up @@ -231,6 +236,12 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user)
exit(QREXEC_EXIT_PROBLEM);
}

int null_fd = open("/dev/null", O_RDWR|O_CLOEXEC);
if (null_fd == -1) {
LOG(ERROR, "cannot open /dev/null");
exit(QREXEC_EXIT_PROBLEM);
}

/* FORK HERE */
child = fork();

Expand All @@ -241,7 +252,7 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user)
really_exec(prog, pw, environ, cmd);
default:
/* parent */
close_std();
close_std(null_fd);
exit(really_wait(child));
}
}
Expand Down Expand Up @@ -329,6 +340,12 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user)
if (retval != PAM_SUCCESS)
goto error;

int null_fd = open("/dev/null", O_RDWR|O_CLOEXEC);
if (null_fd == -1) {
LOG(ERROR, "cannot open /dev/null");
goto error;
}

/* FORK HERE */
child = fork();

Expand All @@ -348,7 +365,7 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user)
really_exec(prog, pw, env, cmd);
default:
/* parent */
close_std();
close_std(null_fd);
}

/* reachable only in parent */
Expand Down

0 comments on commit 24ef8b1

Please sign in to comment.