Secure version of popen() needed

[Eric S. Raymond]

OK, I give up. Can someone *else* spot the bug in this code? The idea is to
craft a secure replacement for popen() that does its own parsing of I/O
redirections on the command line, then forks a bare process rather than a
shell. The problem arises when it actually tries to do the I/O redirection.
Please see the comment under 'PROBLEM SECTION STARTS HERE'.

Please reply by email, a summary of results will be posted.

------------------------------------------------------------------------------
#include <stdio.h>

#ifndef _NFILE		/* this will be defined correctly on USG systems */
#define _NFILE	64
#endif

#define MAXARGS	64	/* max # of arguments accepted for secure command */

#define	RDR	0
#define	WTR	1
static	int	peopen_pid[_NFILE];

FILE *peopen(cmd, mode)
/*
 * This is a modified version of popen, made more secure.  Rather than
 * forking off a shell, you get a bare process.
 */
char *cmd, *mode;
{
    int	pipes[2];
    register int *poptr;
    register int myside, yourside, pid, wmode = (strcmp(mode, "w") == 0);

    char    *cp, line[BUFSIZ], *largv[MAXARGS], *redirect = (char *)NULL;
    int	    largc;

    /* crack the argument list into a dope vector */
    (void) strcpy(line, cmd);
    for (cp = line; *cp; cp++)
    {
	if (isspace(*cp))
	    *cp = '\0';
	else if (cp == line || cp[-1] == 0)
	{
 	    /* I/O redirection tokens aren't arguments */
	    if (*cp == '<' || *cp == '>')
	    {
		if ((wmode && *cp == '>') || (!wmode && *cp == '<'))
		    redirect = cp + 1;
		else
		    return((FILE *)NULL);
	    }
	    else if (largc >= MAXARGS - 1)
		return((FILE *)NULL);
	    else
		largv[largc++] = cp;
	}
    }
    largv[largc] = (char *) NULL;

    if (pipe(pipes) < 0)
	return((FILE *)NULL);
    myside = (wmode ? pipes[WTR] : pipes[RDR]);
    yourside = (wmode ? pipes[RDR] : pipes[WTR]);
    if ((pid = fork()) == 0)	/* myside/yourside reverse roles in child */
    {
	/* close all pipes from other popen's */
	for (poptr = peopen_pid; poptr < peopen_pid+20; poptr++) {
	    if(*poptr)
		(void) close(poptr - peopen_pid);
	}
	(void) close(myside);
	(void) close(wmode ? 0 : 1);
	(void) dup(yourside);
	(void) close(yourside);

	/* do redirections */
	if (redirect != (char *)NULL)
	{
/*
 * PROBLEM SECTION BEGINS HERE
 *
 * In theory, what we're doing is a) opening the file we want to redirect to,
 * b) closing the fd corresponding to either stdin or stdout (depending on
 * mode), c) dup()ing the redirect file descriptor into the slot we just
 * closed, and d) closing the original redirect fd. Instrumenting the code
 * confirms that this is in fact happening as we'd expect
 *
 * The net result should be that the redirect file is attached to stdin
 * (if mode was "r") or stdout (if mode was "w") of the child process,
 * which is what we want.
 *
 * What's actually happening is that the calling process is dying with
 * signal 13 (broken pipe). Help?
 *
 */
	    FILE *fp;
	    int fd;

	    fp = fopen(redirect, mode);
	    if (fp == (FILE *)NULL || (fd = fileno(fp)) == FAIL)
		_exit(2);

	    (void) close(wmode);
	    (void) dup(fd);
	    (void) close(fd);
	}

	(void) execv(largv[0], largv);
	_exit(1);
    }
    if(pid == -1)
	return((FILE *)NULL);
    peopen_pid[myside] = pid;
    (void) close(yourside);
    return(fdopen(myside, mode));
}

/* peopen.c ends here */
------------------------------------------------------------------------------
-- 
      Eric S. Raymond                     (the mad mastermind of TMN-Netnews)
      UUCP: {{uunet,rutgers,ihnp4}!cbmvax,rutgers!vu-vlsi,att}!snark!eric
      Post: 22 South Warren Avenue, Malvern, PA 19355   Phone: (215)-296-5718