Skip to content

Commit db5faa9

Browse files
committed
Do not close FDs 0, 1, or 2
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.
1 parent 2dfe1d3 commit db5faa9

File tree

1 file changed

+24
-7
lines changed

1 file changed

+24
-7
lines changed

agent/qrexec-agent.c

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ static libvchan_t *ctrl_vchan;
7171

7272
static pid_t wait_for_session_pid = -1;
7373

74-
static int trigger_fd;
74+
static int trigger_fd = -1;
7575

7676
static int terminate_requested;
7777

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

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

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

239+
int null_fd = open("/dev/null", O_RDWR|O_CLOEXEC);
240+
if (null_fd == -1) {
241+
LOG(ERROR, "cannot open /dev/null");
242+
exit(QREXEC_EXIT_PROBLEM);
243+
}
244+
234245
/* FORK HERE */
235246
child = fork();
236247

@@ -241,7 +252,7 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user)
241252
really_exec(prog, pw, environ, cmd);
242253
default:
243254
/* parent */
244-
close_std();
255+
close_std(null_fd);
245256
exit(really_wait(child));
246257
}
247258
}
@@ -329,6 +340,12 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user)
329340
if (retval != PAM_SUCCESS)
330341
goto error;
331342

343+
int null_fd = open("/dev/null", O_RDWR|O_CLOEXEC);
344+
if (null_fd == -1) {
345+
LOG(ERROR, "cannot open /dev/null");
346+
goto error;
347+
}
348+
332349
/* FORK HERE */
333350
child = fork();
334351

@@ -348,7 +365,7 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user)
348365
really_exec(prog, pw, env, cmd);
349366
default:
350367
/* parent */
351-
close_std();
368+
close_std(null_fd);
352369
}
353370

354371
/* reachable only in parent */

0 commit comments

Comments
 (0)