Several potential security problems found and fixed by Stefan Esser &

Sebastian Krahmer.

ok millert@

7 files changed, 245 insertions, 27 deletions

diff --git a/gnu/usr.bin/cvs/lib/xsize.h b/gnu/usr.bin/cvs/lib/xsize.h
new file mode 100644
index 00000000000..7634c6d4d5a
--- /dev/null
+++ b/gnu/usr.bin/cvs/lib/xsize.h
@@ -0,0 +1,108 @@
+/* xsize.h -- Checked size_t computations.
+
+   Copyright (C) 2003 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 2, or (at your option)
+   any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software Foundation,
+   Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.  */
+
+#ifndef _XSIZE_H
+#define _XSIZE_H
+
+/* Get size_t.  */
+#include <stddef.h>
+
+/* Get SIZE_MAX.  */
+#include <limits.h>
+#if HAVE_STDINT_H
+# include <stdint.h>
+#endif
+
+/* The size of memory objects is often computed through expressions of
+   type size_t. Example:
+      void* p = malloc (header_size + n * element_size).
+   These computations can lead to overflow.  When this happens, malloc()
+   returns a piece of memory that is way too small, and the program then
+   crashes while attempting to fill the memory.
+   To avoid this, the functions and macros in this file check for overflow.
+   The convention is that SIZE_MAX represents overflow.
+   malloc (SIZE_MAX) is not guaranteed to fail -- think of a malloc
+   implementation that uses mmap --, it's recommended to use size_overflow_p()
+   or size_in_bounds_p() before invoking malloc().
+   The example thus becomes:
+      size_t size = xsum (header_size, xtimes (n, element_size));
+      void *p = (size_in_bounds_p (size) ? malloc (size) : NULL);
+*/
+
+/* Convert an arbitrary value >= 0 to type size_t.  */
+#define xcast_size_t(N) \
+  ((N) <= SIZE_MAX ? (size_t) (N) : SIZE_MAX)
+
+/* Sum of two sizes, with overflow check.  */
+static inline size_t
+#if __GNUC__ >= 3
+__attribute__ ((__pure__))
+#endif
+xsum (size_t size1, size_t size2)
+{
+  size_t sum = size1 + size2;
+  return (sum >= size1 ? sum : SIZE_MAX);
+}
+
+/* Sum of three sizes, with overflow check.  */
+static inline size_t
+#if __GNUC__ >= 3
+__attribute__ ((__pure__))
+#endif
+xsum3 (size_t size1, size_t size2, size_t size3)
+{
+  return xsum (xsum (size1, size2), size3);
+}
+
+/* Sum of four sizes, with overflow check.  */
+static inline size_t
+#if __GNUC__ >= 3
+__attribute__ ((__pure__))
+#endif
+xsum4 (size_t size1, size_t size2, size_t size3, size_t size4)
+{
+  return xsum (xsum (xsum (size1, size2), size3), size4);
+}
+
+/* Maximum of two sizes, with overflow check.  */
+static inline size_t
+#if __GNUC__ >= 3
+__attribute__ ((__pure__))
+#endif
+xmax (size_t size1, size_t size2)
+{
+  /* No explicit check is needed here, because for any n:
+     max (SIZE_MAX, n) == SIZE_MAX and max (n, SIZE_MAX) == SIZE_MAX.  */
+  return (size1 >= size2 ? size1 : size2);
+}
+
+/* Multiplication of a count with an element size, with overflow check.
+   The count must be >= 0 and the element size must be > 0.
+   This is a macro, not an inline function, so that it works correctly even
+   when N is of a wider tupe and N > SIZE_MAX.  */
+#define xtimes(N, ELSIZE) \
+  ((N) <= SIZE_MAX / (ELSIZE) ? (size_t) (N) * (ELSIZE) : SIZE_MAX)
+
+/* Check for overflow.  */
+#define size_overflow_p(SIZE) \
+  ((SIZE) == SIZE_MAX)
+/* Check against overflow.  */
+#define size_in_bounds_p(SIZE) \
+  ((SIZE) != SIZE_MAX)
+
+#endif /* _XSIZE_H */
diff --git a/gnu/usr.bin/cvs/src/commit.c b/gnu/usr.bin/cvs/src/commit.c
index 149da7cb74d..0fe9f9425d7 100644
--- a/gnu/usr.bin/cvs/src/commit.c
+++ b/gnu/usr.bin/cvs/src/commit.c
@@ -472,7 +472,12 @@ commit (argc, argv)
 	   operate on, and only work with those files in the future.
 	   This saves time--we don't want to search the file system
 	   of the working directory twice.  */
-	find_args.argv = (char **) xmalloc (find_args.argc * sizeof (char **));
+	if (size_overflow_p (xtimes (find_args.argc, sizeof (char **))))
+	{
+	    find_args.argc = 0;
+	    return 0;
+	}
+	find_args.argv = xmalloc (xtimes (find_args.argc, sizeof (char **)));
 	find_args.argc = 0;
 	walklist (find_args.ulist, copy_ulist, &find_args);
 
diff --git a/gnu/usr.bin/cvs/src/cvs.h b/gnu/usr.bin/cvs/src/cvs.h
index 961e02c5240..188df983469 100644
--- a/gnu/usr.bin/cvs/src/cvs.h
+++ b/gnu/usr.bin/cvs/src/cvs.h
@@ -41,6 +41,10 @@
 #include "popen.h"
 #endif
 
+/* Begin GNULIB headers.  */
+#include "xsize.h"
+/* End GNULIB headers.  */
+
 #ifdef STDC_HEADERS
 #include <stdlib.h>
 #else
diff --git a/gnu/usr.bin/cvs/src/filesubr.c b/gnu/usr.bin/cvs/src/filesubr.c
index 6f7dfd08a82..e41f3a553cc 100644
--- a/gnu/usr.bin/cvs/src/filesubr.c
+++ b/gnu/usr.bin/cvs/src/filesubr.c
@@ -965,8 +965,14 @@ expand_wild (argc, argv, pargc, pargv)
     char ***pargv;
 {
     int i;
+    if (size_overflow_p (xtimes (argc, sizeof (char *)))) {
+	*pargc = 0;
+	*pargv = NULL;
+	error (0, 0, "expand_wild: too many arguments");
+	return;
+    }
     *pargc = argc;
-    *pargv = (char **) xmalloc (argc * sizeof (char *));
+    *pargv = xmalloc (xtimes (argc, sizeof (char *)));
     for (i = 0; i < argc; ++i)
 	(*pargv)[i] = xstrdup (argv[i]);
 }
diff --git a/gnu/usr.bin/cvs/src/history.c b/gnu/usr.bin/cvs/src/history.c
index 4979398c19c..945b1e116a9 100644
--- a/gnu/usr.bin/cvs/src/history.c
+++ b/gnu/usr.bin/cvs/src/history.c
@@ -414,8 +414,11 @@ history (argc, argv)
 		working = 1;
 		break;
 	    case 'X':			/* Undocumented debugging flag */
+#ifdef DEBUG
 		histfile = optarg;
+#endif
 		break;
+
 	    case 'D':			/* Since specified date */
 		if (*since_rev || *since_tag || *backto)
 		{
@@ -891,9 +894,13 @@ save_user (name)
 {
     if (user_count == user_max)
     {
-	user_max += USER_INCREMENT;
-	user_list = (char **) xrealloc ((char *) user_list,
-					(int) user_max * sizeof (char *));
+	user_max = xsum (user_max, USER_INCREMENT);
+	if (size_overflow_p (xtimes (user_max, sizeof (char *))))
+	{
+	    error (0, 0, "save_user: too many users");
+	    return;
+	}
+	user_list = xrealloc (user_list, xtimes (user_max, sizeof (char *)));
     }
     user_list[user_count++] = xstrdup (name);
 }
@@ -921,9 +928,13 @@ save_file (dir, name, module)
 
     if (file_count == file_max)
     {
-	file_max += FILE_INCREMENT;
-	file_list = (struct file_list_str *) xrealloc ((char *) file_list,
-						   file_max * sizeof (*fl));
+	file_max = xsum (file_max, FILE_INCREMENT);
+	if (size_overflow_p (xtimes (file_max, sizeof (*fl))))
+	{
+	    error (0, 0, "save_file: too many files");
+	    return;
+	}
+	file_list = xrealloc (file_list, xtimes (file_max, sizeof (*fl)));
     }
     fl = &file_list[file_count++];
     fl->l_file = cp = xmalloc (strlen (dir) + strlen (name) + 2);
@@ -962,9 +973,13 @@ save_module (module)
 {
     if (mod_count == mod_max)
     {
-	mod_max += MODULE_INCREMENT;
-	mod_list = (char **) xrealloc ((char *) mod_list,
-				       mod_max * sizeof (char *));
+	mod_max = xsum (mod_max, MODULE_INCREMENT);
+	if (size_overflow_p (xtimes (mod_max, sizeof (char *))))
+	{
+	    error (0, 0, "save_module: too many modules");
+	    return;
+	}
+	mod_list = xrealloc (mod_list, xtimes (mod_max, sizeof (char *)));
     }
     mod_list[mod_count++] = xstrdup (module);
 }
diff --git a/gnu/usr.bin/cvs/src/server.c b/gnu/usr.bin/cvs/src/server.c
index 6b9e1f1678c..694dfb056e1 100644
--- a/gnu/usr.bin/cvs/src/server.c
+++ b/gnu/usr.bin/cvs/src/server.c
@@ -930,7 +930,7 @@ serve_max_dotdot (arg)
     int i;
     char *p;
 
-    if (lim < 0)
+    if (lim < 0 || lim > 10000)
 	return;
     p = malloc (strlen (server_temp_dir) + 2 * lim + 10);
     if (p == NULL)
@@ -1635,8 +1635,7 @@ serve_unchanged (arg)
     char *cp;
     char *timefield;
 
-    if (error_pending ())
-	return;
+    if (error_pending ()) return;
 
     if (outside_dir (arg))
 	return;
@@ -1650,7 +1649,16 @@ serve_unchanged (arg)
 	    && strlen (arg) == cp - name
 	    && strncmp (arg, name, cp - name) == 0)
 	{
-	    timefield = strchr (cp + 1, '/') + 1;
+	    if (!(timefield = strchr (cp + 1, '/')) || *++timefield == '\0')
+	    {
+		/* We didn't find the record separator or it is followed by
+		 * the end of the string, so just exit.
+		 */
+		if (alloc_pending (80))
+		    sprintf (pending_error_text,
+		             "E Malformed Entry encountered.");
+		return;
+	    }
 	    /* If the time field is not currently empty, then one of
 	     * serve_modified, serve_is_modified, & serve_unchanged were
 	     * already called for this file.  We would like to ignore the
@@ -1697,8 +1705,7 @@ serve_is_modified (arg)
     /* Have we found this file in "entries" yet.  */
     int found;
 
-    if (error_pending ())
-	return;
+    if (error_pending ()) return;
 
     if (outside_dir (arg))
 	return;
@@ -1713,7 +1720,16 @@ serve_is_modified (arg)
 	    && strlen (arg) == cp - name
 	    && strncmp (arg, name, cp - name) == 0)
 	{
-	    timefield = strchr (cp + 1, '/') + 1;
+	    if (!(timefield = strchr (cp + 1, '/')) || *++timefield == '\0')
+	    {
+		/* We didn't find the record separator or it is followed by
+		 * the end of the string, so just exit.
+		 */
+		if (alloc_pending (80))
+		    sprintf (pending_error_text,
+		             "E Malformed Entry encountered.");
+		return;
+	    }
 	    /* If the time field is not currently empty, then one of
 	     * serve_modified, serve_is_modified, & serve_unchanged were
 	     * already called for this file.  We would like to ignore the
@@ -1798,8 +1814,29 @@ serve_entry (arg)
 {
     struct an_entry *p;
     char *cp;
+    int i = 0;
     if (error_pending()) return;
-    p = (struct an_entry *) malloc (sizeof (struct an_entry));
+
+    /* Verify that the entry is well-formed.  This can avoid problems later.
+     * At the moment we only check that the Entry contains five slashes in
+     * approximately the correct locations since some of the code makes
+     * assumptions about this.
+     */
+    cp = arg;
+    if (*cp == 'D') cp++;
+    while (i++ < 5)
+    {
+      if (!cp || *cp != '/')
+      {
+          if (alloc_pending (80))
+              sprintf (pending_error_text,
+                       "E protocol error: Malformed Entry");
+           return;
+      }
+    cp = strchr (cp + 1, '/');
+    }
+
+    p = xmalloc (sizeof (struct an_entry));
     if (p == NULL)
     {
 	pending_error = ENOMEM;
@@ -2031,6 +2068,9 @@ serve_notify (arg)
     {
 	char *cp;
 
+	if (!data[0])
+	    goto error;
+
 	if (strchr (data, '+'))
 	    goto error;
 
@@ -2162,6 +2202,15 @@ serve_argument (arg)
     char *p;
     
     if (error_pending()) return;
+
+    if (argument_count >= 10000)
+    {
+       if (alloc_pending (80))
+           sprintf (pending_error_text,
+                    "E Protocol error: too many arguments");
+       return;
+    }
+
     
     if (argument_vector_size <= argument_count)
     {
@@ -2192,6 +2241,15 @@ serve_argumentx (arg)
     char *p;
     
     if (error_pending()) return;
+
+    if (argument_count <= 1)
+    {
+        if (alloc_pending (80))
+            sprintf (pending_error_text,
+                     "E Protocol error: called argumentx without prior call to argument");
+        return;
+    }
+
     
     p = argument_vector[argument_count - 1];
     p = realloc (p, strlen (p) + 1 + strlen (arg) + 1);
@@ -2549,7 +2607,7 @@ check_command_legal_p (cmd_name)
                     save some code here...  -kff */
 
                  /* Chop newline by hand, for strcmp()'s sake. */
-                 if (linebuf[num_red - 1] == '\n')
+                 if (num_red > 0 && linebuf[num_red - 1] == '\n')
                      linebuf[num_red - 1] = '\0';
 
                  if (strcmp (linebuf, CVS_Username) == 0)
@@ -2604,7 +2662,7 @@ check_command_legal_p (cmd_name)
          while ((num_red = getline (&linebuf, &linebuf_len, fp)) >= 0)
          {
              /* Chop newline by hand, for strcmp()'s sake. */
-             if (linebuf[num_red - 1] == '\n')
+             if (num_red > 0 && linebuf[num_red - 1] == '\n')
                  linebuf[num_red - 1] = '\0';
            
              if (strcmp (linebuf, CVS_Username) == 0)
diff --git a/gnu/usr.bin/cvs/src/wrapper.c b/gnu/usr.bin/cvs/src/wrapper.c
index 039590ad269..7dbaa5538a4 100644
--- a/gnu/usr.bin/cvs/src/wrapper.c
+++ b/gnu/usr.bin/cvs/src/wrapper.c
@@ -246,6 +246,30 @@ wrap_unparse_rcs_options (line, first_call_p)
 #endif /* SERVER_SUPPORT || CLIENT_SUPPORT */
 
 /*
+ * Remove fmt str specifier other than %% or %s. And allow
+ * only max_s %s specifiers
+ */
+wrap_clean_fmt_str(char *fmt, int max_s)
+{
+    while (*fmt) {
+	if (fmt[0] == '%' && fmt[1])
+	{
+	    if (fmt[1] == '%') 
+		fmt++;
+	    else
+		if (fmt[1] == 's' && max_s > 0)
+		{
+		    max_s--;
+		    fmt++;
+		} else 
+		    *fmt = ' ';
+	}
+	fmt++;
+    }
+    return;
+}
+
+/*
  * Open a file and read lines, feeding each line to a line parser. Arrange
  * for keeping a temporary list of wrappers at the end, if the "temp"
  * argument is set.
@@ -569,9 +593,8 @@ wrap_tocvs_process_file(fileName)
     args = xmalloc (strlen (e->tocvsFilter)
 		    + strlen (fileName)
 		    + strlen (buf));
-    /* FIXME: sprintf will blow up if the format string contains items other
-       than %s, or contains too many %s's.  We should instead be parsing
-       e->tocvsFilter ourselves and giving a real error.  */
+
+    wrap_clean_fmt_str(e->tocvsFilter, 2);
     sprintf (args, e->tocvsFilter, fileName, buf);
     run_setup (args);
     run_exec(RUN_TTY, RUN_TTY, RUN_TTY, RUN_NORMAL|RUN_REALLY );
@@ -603,9 +626,8 @@ wrap_fromcvs_process_file(fileName)
 
     args = xmalloc (strlen (e->fromcvsFilter)
 		    + strlen (fileName));
-    /* FIXME: sprintf will blow up if the format string contains items other
-       than %s, or contains too many %s's.  We should instead be parsing
-       e->fromcvsFilter ourselves and giving a real error.  */
+
+    wrap_clean_fmt_str(e->fromcvsFilter, 1);
     sprintf (args, e->fromcvsFilter, fileName);
     run_setup (args);
     run_exec(RUN_TTY, RUN_TTY, RUN_TTY, RUN_NORMAL );