option parsing buffer overflow vulnerability

The report that parse_config_line() could overflow its buffer
when passed a long value from parse_config_file() was already
fixed for 3.6.1, but longer config lines mean that later stages
of option parsing need to be aware of the possibility of lines
longer than BUFSZ.  This fixes the three special options which
deal with regular expressions.  Autopickup exceptions, message
types, and menu coloring all could exceed BUFSZ when formatting
a value to put in the menu for 'list' or 'remove' of existing
entries.  Menu colors could overflow by an arbitrary amount,
message types by up to 13 bytes, and autopickup exceptions by
just 1 byte.

Both tty and X11 seem to cope with long menu entries (wider
than can be displayed but not exceeding BUFSZ) by truncating.
For menu colorings, that hides the color and highlight attribute
since those are placed after the regexp.

Menu line truncation for tty was straightforward, just chopped
a big chunk off the end of the long string.  For X11, it did
that too, taking off a lot less but ending up with much of the
menu hidden off the left edge of the screen.  Dragging it into
view showed that the width fit the whole screen (or possibly
fit the width of the map, which was also as wide as the screen).
So the initial position is being miscalculated.

diff --git a/doc/fixes36.1 b/doc/fixes36.1
index b364ad2eb9d8782ded1335072cc5122b42924c30..a3aeb01a58cde2f8d7e70fbfba00695b049db3e4 100644 (file)
--- a/doc/fixes36.1
+++ b/doc/fixes36.1
@@ -243,6 +243,9 @@ in baalz_fixup, move any monster away from the two fake pool spots
 switching farlook from xname to doname was giving away information for items
        located via object detection (quantity of detected gold)
 catch up win/Qt/qt_win.cpp on 18-Dec-2015 change to formatkiller()
+fix for long lines in config file (28-Jan-2016) made 'O' command's 'list' and
+       'remove' menu choices in interactive handling for menu colorings,
+       message types, and autopickup exceptions subject to buffer overflow
 
 
 Platform- and/or Interface-Specific Fixes

diff --git a/src/options.c b/src/options.c
index 397c34a3ae00e9477594b997af0ef27ed53b577c..7abc3dfc66895295eb832aa6530391c2953eeba1 100644 (file)
--- a/src/options.c
+++ b/src/options.c
@@ -3879,7 +3879,7 @@ int numtotal;
 {
     winid tmpwin;
     anything any;
-    int i, pick_cnt, pick_idx, opt_idx;
+    int i, pick_cnt, opt_idx;
     menu_item *pick_list = (menu_item *) 0;
     static const struct action {
         char letr;
@@ -3897,6 +3897,7 @@ int numtotal;
     any = zeroany;
     for (i = 0; i < SIZE(action_titles); i++) {
         char tmpbuf[BUFSZ];
+
         any.a_int++;
         /* omit list and remove if there aren't any yet */
         if (!numtotal && (i == 1 || i == 2))
@@ -3904,24 +3905,17 @@ int numtotal;
         Sprintf(tmpbuf, action_titles[i].desc,
                 (i == 1) ? makeplural(optname) : optname);
         add_menu(tmpwin, NO_GLYPH, &any, action_titles[i].letr, 0, ATR_NONE,
-                 tmpbuf,
-#if 0 /* this ought to work but doesn't... */
-                 (action_titles[i].letr == 'x') ? MENU_SELECTED :
-#endif
-                 MENU_UNSELECTED);
+                 tmpbuf, (i == 3) ? MENU_SELECTED : MENU_UNSELECTED);
     }
     end_menu(tmpwin, "Do what?");
     if ((pick_cnt = select_menu(tmpwin, PICK_ONE, &pick_list)) > 0) {
-        for (pick_idx = 0; pick_idx < pick_cnt; ++pick_idx) {
-            opt_idx = pick_list[pick_idx].item.a_int - 1;
-        }
+        opt_idx = pick_list[0].item.a_int - 1;
+        if (pick_cnt > 1 && opt_idx == 3)
+            opt_idx = pick_list[1].item.a_int - 1;
         free((genericptr_t) pick_list);
-        pick_list = (menu_item *) 0;
-    }
-    destroy_nhwindow(tmpwin);
-
-    if (pick_cnt < 1)
+    } else
         opt_idx = 3; /* none selected, exit menu */
+    destroy_nhwindow(tmpwin);
     return opt_idx;
 }
 
@@ -4338,6 +4332,7 @@ boolean setinitial, setfromfile;
         } else { /* list (1) or remove (2) */
             int pick_idx, pick_cnt;
             int mt_idx;
+            unsigned ln;
             const char *mtype;
             menu_item *pick_list = (menu_item *) 0;
             struct plinemsg_type *tmp = plinemsg_types;
@@ -4349,7 +4344,12 @@ boolean setinitial, setfromfile;
             while (tmp) {
                 mtype = msgtype2name(tmp->msgtype);
                 any.a_int = ++mt_idx;
-                Sprintf(mtbuf, "%-5s \"%s\"", mtype, tmp->pattern);
+                Sprintf(mtbuf, "%-5s \"", mtype);
+                ln = sizeof mtbuf - strlen(mtbuf) - sizeof "\"";
+                if (strlen(tmp->pattern) > ln)
+                    Strcat(strncat(mtbuf, tmp->pattern, ln - 3), "...\"");
+                else
+                    Strcat(mtbuf, "\"");
                 add_menu(tmpwin, NO_GLYPH, &any, 0, 0, ATR_NONE, mtbuf,
                          MENU_UNSELECTED);
                 tmp = tmp->next;
@@ -4394,6 +4394,7 @@ boolean setinitial, setfromfile;
         } else { /* list (1) or remove (2) */
             int pick_idx, pick_cnt;
             int mc_idx;
+            unsigned ln;
             const char *sattr, *sclr;
             menu_item *pick_list = (menu_item *) 0;
             struct menucoloring *tmp = menu_colorings;
@@ -4406,9 +4407,19 @@ boolean setinitial, setfromfile;
                 sattr = attr2attrname(tmp->attr);
                 sclr = clr2colorname(tmp->color);
                 any.a_int = ++mc_idx;
-                Sprintf(mcbuf, "\"%s\"=%s%s%s", tmp->origstr, sclr,
+                /* construct suffix */
+                Sprintf(buf, "\"\"=%s%s%s", sclr,
                         (tmp->attr != ATR_NONE) ? " & " : "",
                         (tmp->attr != ATR_NONE) ? sattr : "");
+                /* now main string */
+                ln = sizeof buf - strlen(buf) - 1; /* length available */
+                Strcpy(mcbuf, "\"");
+                if (strlen(tmp->origstr) > ln)
+                    Strcat(strncat(mcbuf, tmp->origstr, ln - 3), "...");
+                else
+                    Strcat(mcbuf, tmp->origstr);
+                /* combine main string and suffix */
+                Strcat(mcbuf, &buf[1]); /* skip buf[]'s initial quote */
                 add_menu(tmpwin, NO_GLYPH, &any, 0, 0, ATR_NONE, mcbuf,
                          MENU_UNSELECTED);
                 tmp = tmp->next;
@@ -4470,6 +4481,7 @@ boolean setinitial, setfromfile;
                          MENU_UNSELECTED);
                 for (i = 0; i < numapes[pass] && ape; i++) {
                     any.a_void = (opt_idx == 1) ? 0 : ape;
+                    /* length of pattern plus quotes is less than BUFSZ */
                     Sprintf(apebuf, "\"%s\"", ape->pattern);
                     add_menu(tmpwin, NO_GLYPH, &any, 0, 0, ATR_NONE, apebuf,
                              MENU_UNSELECTED);
@@ -5055,39 +5067,54 @@ int
 add_autopickup_exception(mapping)
 const char *mapping;
 {
+    static const char
+        APE_regex_error[] = "regex error in AUTOPICKUP_EXCEPTION",
+        APE_syntax_error[] = "syntax error in AUTOPICKUP_EXCEPTION";
+
     struct autopickup_exception *ape, **apehead;
-    char text[256], *text2;
+    char text[256], end;
+    int n;
     boolean grab = FALSE;
 
-    if (sscanf(mapping, "\"%255[^\"]\"", text) == 1) {
-        text2 = &text[0];
-        if (*text2 == '<') { /* force autopickup */
-            grab = TRUE;
-            ++text2;
-        } else if (*text2 == '>') { /* default - Do not pickup */
-            grab = FALSE;
-            ++text2;
-        }
-        apehead = (grab) ? &iflags.autopickup_exceptions[AP_GRAB]
-                         : &iflags.autopickup_exceptions[AP_LEAVE];
-        ape = (struct autopickup_exception *) alloc(
-                                        sizeof (struct autopickup_exception));
-        ape->regex = regex_init();
-        if (!regex_compile(text2, ape->regex)) {
-            raw_print("regex error in AUTOPICKUP_EXCEPTION");
-            regex_free(ape->regex);
-            free((genericptr_t) ape);
-            return 0;
-        }
-        ape->pattern = (char *) alloc(strlen(text2) + 1);
-        strcpy(ape->pattern, text2);
-        ape->grab = grab;
-        ape->next = *apehead;
-        *apehead = ape;
+    /* scan length limit used to be 255, but smaller size allows the
+       quoted value to fit within BUFSZ, simplifying formatting elsewhere;
+       this used to ignore the possibility of trailing junk but now checks
+       for it, accepting whitespace but rejecting anything else unless it
+       starts with '#" for a comment */
+    end = '\0';
+    if ((n = sscanf(mapping, "\"<%253[^\"]\" %c", text, &end)) == 1
+        || (n == 2 && end == '#')) {
+        grab = TRUE;
+    } else if ((n = sscanf(mapping, "\">%253[^\"]\" %c", text, &end)) == 1
+               || (n = sscanf(mapping, "\"%253[^\"]\" %c", text, &end)) == 1
+               || (n == 2 && end == '#')) {
+        grab = FALSE;
     } else {
-        raw_print("syntax error in AUTOPICKUP_EXCEPTION");
+        if (!iflags.window_inited)
+            raw_print(APE_syntax_error); /* from options file */
+        else
+            pline("%s", APE_syntax_error); /* via 'O' command */
         return 0;
     }
+
+    ape = (struct autopickup_exception *) alloc(sizeof *ape);
+    ape->regex = regex_init();
+    if (!regex_compile(text, ape->regex)) {
+        if (!iflags.window_inited)
+            raw_print(APE_regex_error);
+        else
+            pline("%s", APE_regex_error);
+        regex_free(ape->regex);
+        free((genericptr_t) ape);
+        return 0;
+    }
+    apehead = (grab) ? &iflags.autopickup_exceptions[AP_GRAB]
+                     : &iflags.autopickup_exceptions[AP_LEAVE];
+
+    ape->pattern = dupstr(text);
+    ape->grab = grab;
+    ape->next = *apehead;
+    *apehead = ape;
     return 1;
 }