* Re: posix_spawn() can expose the error pipe to the spawned process
2019-07-08 15:39 posix_spawn() can expose the error pipe to the spawned process Tavian Barnes
@ 2019-07-08 17:09 ` Rich Felker
2019-07-09 2:17 ` Tavian Barnes
1 sibling, 0 replies; 3+ messages in thread
From: Rich Felker @ 2019-07-08 17:09 UTC (permalink / raw)
To: musl
[-- Attachment #1: Type: text/plain, Size: 2145 bytes --]
On Mon, Jul 08, 2019 at 11:39:49AM -0400, Tavian Barnes wrote:
> posix_spawn[p]() is implemented with a pipe that sends any error codes
> encountered back to the parent process. It attempts to move the pipe
> out of the way with dup() whenever that fd is used by the file_actions
> as an output, but not as an input. So something like this:
>
> $ cat spawn_pipe.c
> #include <spawn.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <unistd.h>
>
> extern char **environ;
>
> int main() {
> posix_spawn_file_actions_t fa;
> posix_spawn_file_actions_init(&fa);
> posix_spawn_file_actions_adddup2(&fa, 4, 1);
>
> char *argv[] = { "printf", "\\5\\0\\0\\0", NULL };
>
> pid_t pid;
> int ret = posix_spawnp(&pid, "printf", &fa, NULL, argv, environ);
> fprintf(stderr, "posix_spawnp(): %s\n", strerror(ret));
> return ret;
> }
> $ musl-gcc -Wall spawn_pipe.c -o spawn_pipe && ./spawn_pipe
> posix_spawnp(): I/O error
>
> ends up writing to that pipe and causing posix_spawn() to report
> arbitrary errors. Presumably it should fail before exec()ing with
> EBADF instead.
Thanks! To clarify, for anyone reading, the issue here is that you're
able to use a dup2 action in the spawn file actions to copy, and
thereby obtain the ability to send junk to, the pipe file descriptor
used internally. It's expected that the implementation can use file
descriptors internally, and that if you use/copy fds you don't own,
you could end up accessing one of them (this is the rationale for why
POSIX has no closeall operation). However it seems preferable to avoid
getting into an internally inconsistent state if this happens, and
that should be easy to do.
Does the attached fix look ok to you?
Note that there are still plenty of other ways you can do evil things
by copying internal fds, e.g. racing with another thread also calling
posix_spawn to copy its pipe fd, or anywhere else fds are used
(locale, message catalog, timezone, etc. loading, hosts/dns lookups,
...). These are pretty much fundamental issues in using dup2 with a fd
you don't own.
Rich
[-- Attachment #2: spawn_pipe.diff --]
[-- Type: text/plain, Size: 418 bytes --]
diff --git a/src/process/posix_spawn.c b/src/process/posix_spawn.c
index 5aaf829d..306faa05 100644
--- a/src/process/posix_spawn.c
+++ b/src/process/posix_spawn.c
@@ -101,6 +101,10 @@ static int child(void *args_vp)
break;
case FDOP_DUP2:
fd = op->srcfd;
+ if (fd == p) {
+ ret = -EBADF;
+ goto fail;
+ }
if (fd != op->fd) {
if ((ret=__sys_dup2(fd, op->fd))<0)
goto fail;
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: posix_spawn() can expose the error pipe to the spawned process
2019-07-08 15:39 posix_spawn() can expose the error pipe to the spawned process Tavian Barnes
2019-07-08 17:09 ` Rich Felker
@ 2019-07-09 2:17 ` Tavian Barnes
1 sibling, 0 replies; 3+ messages in thread
From: Tavian Barnes @ 2019-07-09 2:17 UTC (permalink / raw)
To: musl
[Sorry for messing up the threading, I'm not subscribed so I grabbed
this from the archive]
On Mon, 8 Jul 2019 at 13:09:55 -0400, Rich Felker wrote:
> On Mon, Jul 08, 2019 at 11:39:49AM -0400, Tavian Barnes wrote:
> > posix_spawn[p]() is implemented with a pipe that sends any error codes
> > encountered back to the parent process. It attempts to move the pipe
> > out of the way with dup() whenever that fd is used by the file_actions
> > as an output, but not as an input. So something like this:
> >
> > $ cat spawn_pipe.c
> > #include <spawn.h>
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <string.h>
> > #include <unistd.h>
> >
> > extern char **environ;
> >
> > int main() {
> > posix_spawn_file_actions_t fa;
> > posix_spawn_file_actions_init(&fa);
> > posix_spawn_file_actions_adddup2(&fa, 4, 1);
> >
> > char *argv[] = { "printf", "\\5\\0\\0\\0", NULL };
> >
> > pid_t pid;
> > int ret = posix_spawnp(&pid, "printf", &fa, NULL, argv, environ);
> > fprintf(stderr, "posix_spawnp(): %s\n", strerror(ret));
> > return ret;
> > }
> > $ musl-gcc -Wall spawn_pipe.c -o spawn_pipe && ./spawn_pipe
> > posix_spawnp(): I/O error
> >
> > ends up writing to that pipe and causing posix_spawn() to report
> > arbitrary errors. Presumably it should fail before exec()ing with
> > EBADF instead.
>
> Thanks! To clarify, for anyone reading, the issue here is that you're
> able to use a dup2 action in the spawn file actions to copy, and
> thereby obtain the ability to send junk to, the pipe file descriptor
> used internally. It's expected that the implementation can use file
> descriptors internally, and that if you use/copy fds you don't own,
> you could end up accessing one of them (this is the rationale for why
> POSIX has no closeall operation). However it seems preferable to avoid
> getting into an internally inconsistent state if this happens, and
> that should be easy to do.
Agreed.
> Does the attached fix look ok to you?
Yep, looks right. I was going to suggest moving the check next to the
if (op->fd == p) check above, but it looks like op->srcfd isn't always
initialized.
> Note that there are still plenty of other ways you can do evil things
> by copying internal fds, e.g. racing with another thread also calling
> posix_spawn to copy its pipe fd, or anywhere else fds are used
> (locale, message catalog, timezone, etc. loading, hosts/dns lookups,
> ...). These are pretty much fundamental issues in using dup2 with a fd
> you don't own.
Yep, agreed. I don't think the current behaviour is a bug or
non-standard-compliant or anything. Just seemed like an easy QoI
improvement.
> Rich
^ permalink raw reply [flat|nested] 3+ messages in thread