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 Jan 24, 2025
1 parent 350fca9 commit 92cb1ef
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 @@ -152,13 +152,18 @@ _Noreturn static void really_exec(const struct passwd *pw, char **env,
_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();

Check warning on line 165 in agent/qrexec-agent.c

View check run for this annotation

Codecov / codecov/patch

agent/qrexec-agent.c#L165

Added line #L165 was not covered by tests
}
}

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

Check warning on line 233 in agent/qrexec-agent.c

View check run for this annotation

Codecov / codecov/patch

agent/qrexec-agent.c#L233

Added line #L233 was not covered by tests
}

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

Check warning on line 239 in agent/qrexec-agent.c

View check run for this annotation

Codecov / codecov/patch

agent/qrexec-agent.c#L238-L239

Added lines #L238 - L239 were not covered by tests
}

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

Expand All @@ -238,7 +249,7 @@ _Noreturn void do_exec(const char *cmd, const char *user)
really_exec(pw, environ, cmd);
default:
/* parent */
close_std();
close_std(null_fd);
exit(really_wait(child));
}
}
Expand Down Expand Up @@ -326,6 +337,12 @@ _Noreturn void do_exec(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;

Check warning on line 343 in agent/qrexec-agent.c

View check run for this annotation

Codecov / codecov/patch

agent/qrexec-agent.c#L340-L343

Added lines #L340 - L343 were not covered by tests
}

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

Expand All @@ -345,7 +362,7 @@ _Noreturn void do_exec(const char *cmd, const char *user)
really_exec(pw, env, cmd);

Check warning on line 362 in agent/qrexec-agent.c

View check run for this annotation

Codecov / codecov/patch

agent/qrexec-agent.c#L362

Added line #L362 was not covered by tests
default:
/* parent */
close_std();
close_std(null_fd);

Check warning on line 365 in agent/qrexec-agent.c

View check run for this annotation

Codecov / codecov/patch

agent/qrexec-agent.c#L365

Added line #L365 was not covered by tests
}

/* reachable only in parent */
Expand Down

0 comments on commit 92cb1ef

Please sign in to comment.