From d8127f841f370c290f11352f513f005cb8c59e40 Mon Sep 17 00:00:00 2001 From: David van Moolenbroek Date: Sun, 26 Jul 2015 15:47:05 +0000 Subject: [PATCH] patch(1): fix arbitrary code execution bug This is the combination of two NetBSD patches committed by Christos Zoulas, based on the findings and Bitrig patch by Martin Natano. The NetBSD log messages read: From Martin Natano @bitrig: Use execve(2) instead of system to apply patches that require rcs command execution instead system(3) to avoid malicious filenames in patches causing bad things to happen. In the process, lose SCCS support. It is not like we are shipping sccs commands for that to work. And: Use absolute paths for RCS commands (Martin Natano) Change-Id: Id44bd59a5a6bc6cd95d1e1fae468bd718cfff2db --- usr.bin/patch/common.h | 4 +- usr.bin/patch/inp.c | 109 +++++++++++++++++++++++++---------------- 2 files changed, 69 insertions(+), 44 deletions(-) diff --git a/usr.bin/patch/common.h b/usr.bin/patch/common.h index c2402d8f2..95a7bca33 100644 --- a/usr.bin/patch/common.h +++ b/usr.bin/patch/common.h @@ -49,8 +49,8 @@ #define SCCSDIFF "get -p %s | diff - %s >/dev/null" #define RCSSUFFIX ",v" -#define CHECKOUT "co -l %s" -#define RCSDIFF "rcsdiff %s > /dev/null" +#define CHECKOUT "/usr/bin/co" +#define RCSDIFF "/usr/bin/rcsdiff" #define ORIGEXT ".orig" #define REJEXT ".rej" diff --git a/usr.bin/patch/inp.c b/usr.bin/patch/inp.c index 6bf1b662a..707f420af 100644 --- a/usr.bin/patch/inp.c +++ b/usr.bin/patch/inp.c @@ -1,7 +1,7 @@ /* * $OpenBSD: inp.c,v 1.34 2006/03/11 19:41:30 otto Exp $ * $DragonFly: src/usr.bin/patch/inp.c,v 1.6 2007/09/29 23:11:10 swildner Exp $ - * $NetBSD: inp.c,v 1.23 2009/10/21 17:16:11 joerg Exp $ + * $NetBSD: inp.c,v 1.24 2015/07/24 18:56:00 christos Exp $ */ /* @@ -31,14 +31,16 @@ */ #include -__RCSID("$NetBSD: inp.c,v 1.23 2009/10/21 17:16:11 joerg Exp $"); +__RCSID("$NetBSD: inp.c,v 1.24 2015/07/24 18:56:00 christos Exp $"); #include #include #include #include +#include #include +#include #include #include #include @@ -139,12 +141,14 @@ reallocate_lines(size_t *lines_allocated) static bool plan_a(const char *filename) { - int ifd, statfailed; + int ifd, statfailed, devnull, pstat; char *p, *s, lbuf[MAXLINELEN]; struct stat filestat; off_t i; ptrdiff_t sz; size_t iline, lines_allocated; + pid_t pid; + char *argp[4] = {NULL}; #ifdef DEBUGGING if (debug & 8) @@ -172,13 +176,13 @@ plan_a(const char *filename) } if (statfailed && check_only) fatal("%s not found, -C mode, can't probe further\n", filename); - /* For nonexistent or read-only files, look for RCS or SCCS versions. */ + /* For nonexistent or read-only files, look for RCS versions. */ if (statfailed || /* No one can write to it. */ (filestat.st_mode & 0222) == 0 || /* I can't write to it. */ ((filestat.st_mode & 0022) == 0 && filestat.st_uid != getuid())) { - const char *cs = NULL, *filebase, *filedir; + char *filebase, *filedir; struct stat cstat; char *tmp_filename1, *tmp_filename2; @@ -186,43 +190,26 @@ plan_a(const char *filename) tmp_filename2 = strdup(filename); if (tmp_filename1 == NULL || tmp_filename2 == NULL) fatal("strdupping filename"); - filebase = basename(tmp_filename1); - filedir = dirname(tmp_filename2); - - /* Leave room in lbuf for the diff command. */ - s = lbuf + 20; - + + filebase = basename(tmp_filename1); + filedir = dirname(tmp_filename2); + #define try(f, a1, a2, a3) \ - (snprintf(s, sizeof lbuf - 20, f, a1, a2, a3), stat(s, &cstat) == 0) - - if (try("%s/RCS/%s%s", filedir, filebase, RCSSUFFIX) || - try("%s/RCS/%s%s", filedir, filebase, "") || - try("%s/%s%s", filedir, filebase, RCSSUFFIX)) { - snprintf(buf, buf_len, CHECKOUT, filename); - snprintf(lbuf, sizeof lbuf, RCSDIFF, filename); - cs = "RCS"; - } else if (try("%s/SCCS/%s%s", filedir, SCCSPREFIX, filebase) || - try("%s/%s%s", filedir, SCCSPREFIX, filebase)) { - snprintf(buf, buf_len, GET, s); - snprintf(lbuf, sizeof lbuf, SCCSDIFF, s, filename); - cs = "SCCS"; - } else if (statfailed) - fatal("can't find %s\n", filename); - - free(tmp_filename1); - free(tmp_filename2); + (snprintf(lbuf, sizeof lbuf, f, a1, a2, a3), stat(lbuf, &cstat) == 0) /* * else we can't write to it but it's not under a version * control system, so just proceed. */ - if (cs) { + if (try("%s/RCS/%s%s", filedir, filebase, RCSSUFFIX) || + try("%s/RCS/%s%s", filedir, filebase, "") || + try("%s/%s%s", filedir, filebase, RCSSUFFIX)) { if (!statfailed) { if ((filestat.st_mode & 0222) != 0) /* The owner can write to it. */ fatal("file %s seems to be locked " - "by somebody else under %s\n", - filename, cs); + "by somebody else under RCS\n", + filename); /* * It might be checked out unlocked. See if * it's safe to check out the default version @@ -230,21 +217,59 @@ plan_a(const char *filename) */ if (verbose) say("Comparing file %s to default " - "%s version...\n", - filename, cs); - if (system(lbuf)) + "RCS version...\n", filename); + + switch (pid = fork()) { + case -1: + fatal("can't fork: %s\n", + strerror(errno)); + case 0: + devnull = open("/dev/null", O_RDONLY); + if (devnull == -1) { + fatal("can't open /dev/null: %s", + strerror(errno)); + } + (void)dup2(devnull, STDOUT_FILENO); + argp[0] = __UNCONST(RCSDIFF); + argp[1] = __UNCONST(filename); + execv(RCSDIFF, argp); + exit(127); + } + pid = waitpid(pid, &pstat, 0); + if (pid == -1 || WEXITSTATUS(pstat) != 0) { fatal("can't check out file %s: " - "differs from default %s version\n", - filename, cs); + "differs from default RCS version\n", + filename); + } } + if (verbose) - say("Checking out file %s from %s...\n", - filename, cs); - if (system(buf) || stat(filename, &filestat)) - fatal("can't check out file %s from %s\n", - filename, cs); + say("Checking out file %s from RCS...\n", + filename); + + switch (pid = fork()) { + case -1: + fatal("can't fork: %s\n", strerror(errno)); + case 0: + argp[0] = __UNCONST(CHECKOUT); + argp[1] = __UNCONST("-l"); + argp[2] = __UNCONST(filename); + execv(CHECKOUT, argp); + exit(127); + } + pid = waitpid(pid, &pstat, 0); + if (pid == -1 || WEXITSTATUS(pstat) != 0 || + stat(filename, &filestat)) { + fatal("can't check out file %s from RCS\n", + filename); + } + } else if (statfailed) { + fatal("can't find %s\n", filename); } + free(tmp_filename1); + free(tmp_filename2); } + filemode = filestat.st_mode; if (!S_ISREG(filemode)) fatal("%s is not a normal file--can't patch\n", filename);