command line triggered buffer overruns

Prevent extremely long command line arguments from overflowing local
buffers in raw_printf or config_error_add.  The increased buffer
sizes they recently got to deal with long configuration file values
aren't sufficient to handle command line induced overflows.

choose_windows(core): copy and truncate the window_type argument in
case it gets passed to config_error_add().

process_options(unix): report bad values with "%.60s" so that vsprintf
will implicitly truncate when formatted by raw_printf().

diff --git a/doc/fixes36.5 b/doc/fixes36.5
index 54bb65f729a1f401140600d28c65168047b0b94a..18b38439a5552f89773b02eec68e84f23277a346 100644 (file)
--- a/doc/fixes36.5
+++ b/doc/fixes36.5
@@ -13,6 +13,8 @@ ensure existing callers of string_for_opt() check return value before using it
 fix potential buffer overflow in add_menu_coloring()
 fix potential buffer overflow in sym_val()
 fix potential buffer overflow in pline(), raw_printf(), and config_error_add()
+       via bad config file values or command line arguments
+fix potential buffer overflow in choose_windows()
 
 
 Fixes to Post-3.6.4 Problems that Were Exposed Via git Repository

diff --git a/src/topten.c b/src/topten.c
index 6a226df3f728b96b7ccb9d7f1447817c3cb7841d..7515c35e3f0c8878ca981c9ae7e91517b8dc02c9 100644 (file)
--- a/src/topten.c
+++ b/src/topten.c
@@ -1000,6 +1000,7 @@ int uid;
  * print selected parts of score list.
  * argc >= 2, with argv[0] untrustworthy (directory names, et al.),
  * and argv[1] starting with "-s".
+ * caveat: some shells might allow argv elements to be arbitrarily long.
  */
 void
 prscore(argc, argv)

diff --git a/src/windows.c b/src/windows.c
index 5fae44db383bff5afb2949fa63af49fb6443d9fb..01ef4ccb95d2ba319d4aec7140433bfbcb6f437b 100644 (file)
--- a/src/windows.c
+++ b/src/windows.c
@@ -243,7 +243,8 @@ void
 choose_windows(s)
 const char *s;
 {
-    register int i;
+    int i;
+    char *tmps = 0;
 
     for (i = 0; winchoices[i].procs; i++) {
         if ('+' == winchoices[i].procs->name[0])
@@ -269,9 +270,22 @@ const char *s;
         windowprocs.win_wait_synch = def_wait_synch;
 
     if (!winchoices[0].procs) {
-        raw_printf("No window types?");
+        raw_printf("No window types supported?");
         nh_terminate(EXIT_FAILURE);
     }
+    /* 50: arbitrary, no real window_type names are anywhere near that long;
+       used to prevent potential raw_printf() overflow if user supplies a
+       very long string (on the order of 1200 chars) on the command line
+       (config file options can't get that big; they're truncated at 1023) */
+#define WINDOW_TYPE_MAXLEN 50
+    if (strlen(s) >= WINDOW_TYPE_MAXLEN) {
+        tmps = (char *) alloc(WINDOW_TYPE_MAXLEN);
+        (void) strncpy(tmps, s, WINDOW_TYPE_MAXLEN - 1);
+        tmps[WINDOW_TYPE_MAXLEN - 1] = '\0';
+        s = tmps;
+    }
+#undef WINDOW_TYPE_MAXLEN
+
     if (!winchoices[1].procs) {
         config_error_add(
                      "Window type %s not recognized.  The only choice is: %s",
@@ -293,6 +307,8 @@ const char *s;
         config_error_add("Window type %s not recognized.  Choices are:  %s",
                          s, buf);
     }
+    if (tmps)
+        free((genericptr_t) tmps) /*, tmps = 0*/ ;
 
     if (windowprocs.win_raw_print == def_raw_print
             || WINDOWPORT("safe-startup"))

diff --git a/sys/unix/unixmain.c b/sys/unix/unixmain.c
index c2690ce4a758a52e961430e253c6bc137d04b178..1e7880165d58b9ffdf43f733dbc55350e7fcf883 100644 (file)
--- a/sys/unix/unixmain.c
+++ b/sys/unix/unixmain.c
@@ -355,6 +355,7 @@ char *argv[];
     return 0;
 }
 
+/* caveat: argv elements might be arbitrary long */
 static void
 process_options(argc, argv)
 int argc;
@@ -383,11 +384,10 @@ char *argv[];
                 load_symset("DECGraphics", PRIMARY);
                 switch_symbols(TRUE);
             } else {
-                raw_printf("Unknown option: %s", *argv);
+                raw_printf("Unknown option: %.60s", *argv);
             }
             break;
         case 'X':
-
             discover = TRUE, wizard = FALSE;
             break;
 #ifdef NEWS
@@ -413,7 +413,7 @@ char *argv[];
                 load_symset("RogueIBM", ROGUESET);
                 switch_symbols(TRUE);
             } else {
-                raw_printf("Unknown option: %s", *argv);
+                raw_printf("Unknown option: %.60s", *argv);
             }
             break;
         case 'p': /* profession (role) */
@@ -451,7 +451,7 @@ char *argv[];
                 flags.initrole = i;
                 break;
             }
-            /* else raw_printf("Unknown option: %s", *argv); */
+            /* else raw_printf("Unknown option: %.60s", *argv); */
         }
     }