From: PatR Date: Mon, 9 May 2016 00:56:52 +0000 (-0700) Subject: option parsing buffer overflow vulnerability X-Git-Tag: NetHack-3.6.1_RC01~786 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=119b86bf09b36a35cbe120b5ac5a4c3206d8f6c8;p=nethack 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 b364ad2eb..a3aeb01a5 100644 --- 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 397c34a3a..7abc3dfc6 100644 --- 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; }