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

diff --git a/usr.bin/patch/common.h b/usr.bin/patch/common.h
index c2402d8f2d737acd297d7ed1765fb2956b9565f3..95a7bca336f61776a1e7d23b3af02ab1db16fcff 100644 (file)
--- 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 6bf1b662a76a438a52b124af13e66737371500a7..707f420af4fb797fb518ec3d8306df777167630d 100644 (file)
--- 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 <sys/cdefs.h>
-__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 <sys/types.h>
 #include <sys/file.h>
 #include <sys/stat.h>
 #include <sys/mman.h>
+#include <sys/wait.h>
 
 #include <ctype.h>
+#include <errno.h>
 #include <fcntl.h>
 #include <libgen.h>
 #include <limits.h>
@@ -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);