Re: posix_spawn() can expose the error pipe to the spawned process

[Tavian Barnes <tavianator@tavianator.com>]

[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