From owner-freebsd-audit  Sun Jan 16 18:11:35 2000
Delivered-To: freebsd-audit@freebsd.org
Received: from alcanet.com.au (border.alcanet.com.au [203.62.196.10])
	by hub.freebsd.org (Postfix) with ESMTP id BC81C15120
	for <audit@FreeBSD.ORG>; Sun, 16 Jan 2000 18:11:29 -0800 (PST)
	(envelope-from jeremyp@gsmx07.alcatel.com.au)
Received: by border.alcanet.com.au id <40324>; Mon, 17 Jan 2000 13:03:25 +1100
Content-return: prohibited
From: Peter Jeremy <peter.jeremy@alcatel.com.au>
Subject: Re: awk tempfile handling
In-reply-to: <Pine.BSF.4.21.0001161707440.18027-100000@hub.freebsd.org>; from
 kris@hub.freebsd.org on Mon, Jan 17, 2000 at 12:01:00PM +1100
To: Kris Kennaway <kris@hub.freebsd.org>
Cc: audit@FreeBSD.ORG
Message-Id: <00Jan17.130325est.40324@border.alcanet.com.au>
MIME-version: 1.0
X-Mailer: Mutt 1.0i
Content-type: text/plain; charset=us-ascii
References: <Pine.BSF.4.21.0001161707440.18027-100000@hub.freebsd.org>
Date: Mon, 17 Jan 2000 13:03:25 +1100
Sender: owner-freebsd-audit@FreeBSD.ORG
Precedence: bulk
X-Loop: FreeBSD.ORG

On 2000-Jan-17 12:01:00 +1100, Kris Kennaway <kris@hub.freebsd.org> wrote:
>+	char *name = "/tmp/pipXXXXXXXXXX";

No good.  mkstemp() updates the passed string in place (see the notes
in mktemp(3).  The above defines a read-only string.  This leads to
segmentation violations at runtime.

> 	static char cmdbuf[256];
Into which you're sprintf()ing an unbounded string.  Not a good idea.

>-	if ((name = tempnam(".", "pip")) == NULL)
>+	if ((current = mkstemp(name)) == INVALID_HANDLE)
> 		return NULL;
tempnam() returns a malloc'd buffer, mkstemp() reuses the passed name.
This will cause problems if `name' is later free()d.

> 	sprintf(cmdbuf, "%s > %s", cmd, name);
> 	system(cmdbuf);

system(3) is one of those system calls which is virtually impossible to
use safely.  There's also a race condition here - there's nothing stopping
another process doing something nasty between the mkstemp() and the
I/O redirection done by the shell within system().

The following is an outline of what needs to be done:

{
	static char	name[] = "/tmp/pipXXXXXXXXXX";
	int	current;	/* FD for stdout from command */
	pid_t	pid;
	int	status;

	if ((current = mkstemps(name, 10)) == INVALID_HANDLE)
		return NULL;

	if ((pid = fork()) < 0)
		return NULL;
	else if (pid == 0) {
		if (current != 1) {
			if (dup2(current, 1) < 0)
				_exit(1);
			close(current);
		}
		/*XXX close other random FD's and/or set close-on-exec flags */

		/*XXX If security is an issue, don't call sh at all */
		execl("/bin/sh", "sh", "-c", cmd, NULL);
		_exit(1);
	} else {
		while (waitpid(pid, &status, 0) < 0 && errno == EINTR)
			;
	}

	pipes[current].name = strdup(name);
	pipes[current].command = strdup(cmd);
	rp->iop = iop_alloc(current, name, NULL);
...


To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-audit" in the body of the message