From 6d5c32f38a4ba1e58118d315306f56448f4f0207 Mon Sep 17 00:00:00 2001 From: PatR Date: Mon, 13 Jan 2020 19:09:57 -0800 Subject: [PATCH 1/5] groundwork Fix one warning and reformat some of the recently added code. --- src/options.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/src/options.c b/src/options.c index 1a91d6a51..b77cda435 100644 --- a/src/options.c +++ b/src/options.c @@ -1,4 +1,4 @@ -/* NetHack 3.6 options.c $NHDT-Date: 1575245078 2019/12/02 00:04:38 $ $NHDT-Branch: NetHack-3.6 $:$NHDT-Revision: 1.391 $ */ +/* NetHack 3.6 options.c $NHDT-Date: 1578971391 2020/01/14 03:09:51 $ $NHDT-Branch: NetHack-3.6 $:$NHDT-Revision: 1.394 $ */ /* Copyright (c) Stichting Mathematisch Centrum, Amsterdam, 1985. */ /*-Copyright (c) Michael Allison, 2008. */ /* NetHack may be freely redistributed. See license for details. */ @@ -42,7 +42,7 @@ enum window_option_types { #define PILE_LIMIT_DFLT 5 -static char empty_optstr[] = {0}; +static char empty_optstr[] = { '\0' }; /* * NOTE: If you add (or delete) an option, please update the short @@ -2514,7 +2514,8 @@ boolean tinitial, tfrom_file; config_error_add("Unknown %s parameter '%s'", fullname, opts); return FALSE; } - if (opttype > 0 && (op = string_for_opt(opts, FALSE)) != empty_optstr) { + if (opttype > 0 + && (op = string_for_opt(opts, FALSE)) != empty_optstr) { wc_set_font_name(opttype, op); #ifdef MAC set_font_name(opttype, op); @@ -2614,7 +2615,6 @@ boolean tinitial, tfrom_file; if (match_optname(opts, "fruit", 2, TRUE)) { struct fruit *forig = 0; - char empty_str = '\0'; if (duplicate) complain_about_duplicate(opts, 1); @@ -2629,7 +2629,7 @@ boolean tinitial, tfrom_file; } if (op == empty_optstr) return FALSE; - /* stripped leading and trailing spaces, condensed internal ones in 3.6.2 */ + /* strip leading/trailing spaces, condense internal ones (3.6.2) */ mungspaces(op); if (!initial) { struct fruit *f; @@ -3088,7 +3088,8 @@ boolean tinitial, tfrom_file; if (duplicate) complain_about_duplicate(opts, 1); op = string_for_opt(opts, negated); - if ((negated && op == empty_optstr) || (!negated && op != empty_optstr)) + if ((negated && op == empty_optstr) + || (!negated && op != empty_optstr)) flags.pile_limit = negated ? 0 : atoi(op); else if (negated) { bad_negation(fullname, TRUE); @@ -3187,7 +3188,8 @@ boolean tinitial, tfrom_file; } /* "disclose" without a value means "all with prompting" and negated means "none without prompting" */ - if (op == empty_optstr || !strcmpi(op, "all") || !strcmpi(op, "none")) { + if (op == empty_optstr + || !strcmpi(op, "all") || !strcmpi(op, "none")) { if (op != empty_optstr && !strcmpi(op, "none")) negated = TRUE; for (num = 0; num < NUM_DISCLOSURE_OPTIONS; num++) @@ -3473,7 +3475,8 @@ boolean tinitial, tfrom_file; if (duplicate) complain_about_duplicate(opts, 1); op = string_for_opt(opts, negated); - if ((negated && op == empty_optstr) || (!negated && op != empty_optstr)) { + if ((negated && op == empty_optstr) + || (!negated && op != empty_optstr)) { iflags.wc_scroll_amount = negated ? 1 : atoi(op); } else if (negated) { bad_negation(fullname, TRUE); @@ -3489,7 +3492,8 @@ boolean tinitial, tfrom_file; if (duplicate) complain_about_duplicate(opts, 1); op = string_for_opt(opts, negated); - if ((negated && op == empty_optstr) || (!negated && op != empty_optstr)) { + if ((negated && op == empty_optstr) + || (!negated && op != empty_optstr)) { iflags.wc_scroll_margin = negated ? 5 : atoi(op); } else if (negated) { bad_negation(fullname, TRUE); @@ -3524,7 +3528,8 @@ boolean tinitial, tfrom_file; if (duplicate) complain_about_duplicate(opts, 1); op = string_for_opt(opts, negated); - if ((negated && op == empty_optstr) || (!negated && op != empty_optstr)) { + if ((negated && op == empty_optstr) + || (!negated && op != empty_optstr)) { iflags.wc_tile_width = negated ? 0 : atoi(op); } else if (negated) { bad_negation(fullname, TRUE); @@ -3553,7 +3558,8 @@ boolean tinitial, tfrom_file; if (duplicate) complain_about_duplicate(opts, 1); op = string_for_opt(opts, negated); - if ((negated && op == empty_optstr) || (!negated && op != empty_optstr)) { + if ((negated && op == empty_optstr) + || (!negated && op != empty_optstr)) { iflags.wc_tile_height = negated ? 0 : atoi(op); } else if (negated) { bad_negation(fullname, TRUE); @@ -3569,7 +3575,8 @@ boolean tinitial, tfrom_file; if (duplicate) complain_about_duplicate(opts, 1); op = string_for_opt(opts, negated); - if ((negated && op == empty_optstr) || (!negated && op != empty_optstr)) { + if ((negated && op == empty_optstr) + || (!negated && op != empty_optstr)) { iflags.wc_vary_msgcount = negated ? 0 : atoi(op); } else if (negated) { bad_negation(fullname, TRUE); @@ -3670,7 +3677,7 @@ boolean tinitial, tfrom_file; bad_negation(fullname, FALSE); retval = FALSE; - /* this just checks atol() sanity, not logical window size sanity */ + /* just checks atol() sanity, not logical window size sanity */ } else if (ltmp <= 0L || ltmp >= (long) LARGEST_INT) { config_error_add("Invalid %s: %ld", fullname, ltmp); retval = FALSE; From c35139e9cebdfb63e2378c4514a92dc8f06f94b5 Mon Sep 17 00:00:00 2001 From: PatR Date: Mon, 13 Jan 2020 19:17:35 -0800 Subject: [PATCH 2/5] fixes36.5 --- doc/fixes36.5 | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/doc/fixes36.5 b/doc/fixes36.5 index e4bfd7949..7ce10f270 100644 --- a/doc/fixes36.5 +++ b/doc/fixes36.5 @@ -1,14 +1,13 @@ -$NHDT-Branch: NetHack-3.6 $:$NHDT-Revision: 1.0 $ $NHDT-Date: 1576705788 2019/12/18 21:49:48 $ +$NHDT-Branch: NetHack-3.6 $:$NHDT-Revision: 1.1 $ $NHDT-Date: 1578971847 2020/01/14 03:17:27 $ + +fixes36.5 contains a terse summary of changes made to 3.6.4 in order to +produce 3.6.5 as well as any post-release fixes in binaries. -This fixes36.5 file is here to capture information about updates in the 3.6.x -lineage following the release of 3.6.4 in December 2019. Please note, however, -that another 3.6.x release is not anticipated, and most developer -focus will shift to the next major release. General Fixes and Modified Features ----------------------------------- have string_for_opt() return empty_optstr on failure -ensure existing callers of string_for_opt() check the return value before using it +ensure existing callers of string_for_opt() check return value before using it Fixes to Post-3.6.4 Problems that Were Exposed Via git Repository @@ -22,12 +21,5 @@ Windows OPTIONS=map_mode:fit_to_screen could cause a game start failure General New Features -------------------- - - -NetHack Community Patches (or Variation) Included -------------------------------------------------- - - -Code Cleanup and Reorganization -------------------------------- +none From a842fda44c612cd9a126c3039a575da92c6ee0f4 Mon Sep 17 00:00:00 2001 From: PatR Date: Mon, 13 Jan 2020 19:26:53 -0800 Subject: [PATCH 3/5] fix add_menu_coloring() buffer overrun Fix 'Bug 2' where too long MENUCOLOR=string in run-time config file could overflow a local buffer and clobber the stack. Theoretically a menu coloring regular expression could require a bigger buffer but I don't think we need to try to support that. 255 characters minus the amount needed to specify color and/or attributes should be ample. --- doc/fixes36.5 | 3 ++- src/options.c | 9 +++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/doc/fixes36.5 b/doc/fixes36.5 index 7ce10f270..c3cad8fe0 100644 --- a/doc/fixes36.5 +++ b/doc/fixes36.5 @@ -1,4 +1,4 @@ -$NHDT-Branch: NetHack-3.6 $:$NHDT-Revision: 1.1 $ $NHDT-Date: 1578971847 2020/01/14 03:17:27 $ +$NHDT-Branch: NetHack-3.6 $:$NHDT-Revision: 1.2 $ $NHDT-Date: 1578972411 2020/01/14 03:26:51 $ fixes36.5 contains a terse summary of changes made to 3.6.4 in order to produce 3.6.5 as well as any post-release fixes in binaries. @@ -8,6 +8,7 @@ General Fixes and Modified Features ----------------------------------- have string_for_opt() return empty_optstr on failure ensure existing callers of string_for_opt() check return value before using it +fix potential buffer overflow in add_menu_coloring() Fixes to Post-3.6.4 Problems that Were Exposed Via git Repository diff --git a/src/options.c b/src/options.c index b77cda435..3e2582a4e 100644 --- a/src/options.c +++ b/src/options.c @@ -1,4 +1,4 @@ -/* NetHack 3.6 options.c $NHDT-Date: 1578971391 2020/01/14 03:09:51 $ $NHDT-Branch: NetHack-3.6 $:$NHDT-Revision: 1.394 $ */ +/* NetHack 3.6 options.c $NHDT-Date: 1578972408 2020/01/14 03:26:48 $ $NHDT-Branch: NetHack-3.6 $:$NHDT-Revision: 1.395 $ */ /* Copyright (c) Stichting Mathematisch Centrum, Amsterdam, 1985. */ /*-Copyright (c) Michael Allison, 2008. */ /* NetHack may be freely redistributed. See license for details. */ @@ -1833,15 +1833,16 @@ int c, a; /* parse '"regex_string"=color&attr' and add it to menucoloring */ boolean add_menu_coloring(tmpstr) -char *tmpstr; +char *tmpstr; /* never Null but could be empty */ { int c = NO_COLOR, a = ATR_NONE; char *tmps, *cs, *amp; char str[BUFSZ]; - Sprintf(str, "%s", tmpstr); + (void) strncpy(str, tmpstr, sizeof str - 1); + str[sizeof str - 1] = '\0'; - if (!tmpstr || (cs = index(str, '=')) == 0) { + if ((cs = index(str, '=')) == 0) { config_error_add("Malformed MENUCOLOR"); return FALSE; } From 74de7d31e0a6b3e2ebd852e333fe66d212fd6a90 Mon Sep 17 00:00:00 2001 From: PatR Date: Tue, 14 Jan 2020 02:05:14 -0800 Subject: [PATCH 4/5] fix sym_val() buffer overrun Fix 'Bug 3' where too long SYMBOL=string in run-time config file could overflow a local buffer and clobber the stack. Valid value is only one character long after processing an 'escaped' encoded character which can be at most 6 characters (plus terminator): backslash M backslash and up three digits. If/when UTF8 gets added the number of digits will increase. Use a truncated copy of the input (substantially bigger than 6+1); ignore any excess. --- doc/fixes36.5 | 3 ++- src/options.c | 19 +++++++++++-------- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/doc/fixes36.5 b/doc/fixes36.5 index c3cad8fe0..a44081a06 100644 --- a/doc/fixes36.5 +++ b/doc/fixes36.5 @@ -1,4 +1,4 @@ -$NHDT-Branch: NetHack-3.6 $:$NHDT-Revision: 1.2 $ $NHDT-Date: 1578972411 2020/01/14 03:26:51 $ +$NHDT-Branch: NetHack-3.6 $:$NHDT-Revision: 1.3 $ $NHDT-Date: 1578996303 2020/01/14 10:05:03 $ fixes36.5 contains a terse summary of changes made to 3.6.4 in order to produce 3.6.5 as well as any post-release fixes in binaries. @@ -9,6 +9,7 @@ General Fixes and Modified Features have string_for_opt() return empty_optstr on failure 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() Fixes to Post-3.6.4 Problems that Were Exposed Via git Repository diff --git a/src/options.c b/src/options.c index 3e2582a4e..1f8477193 100644 --- a/src/options.c +++ b/src/options.c @@ -1,4 +1,4 @@ -/* NetHack 3.6 options.c $NHDT-Date: 1578972408 2020/01/14 03:26:48 $ $NHDT-Branch: NetHack-3.6 $:$NHDT-Revision: 1.395 $ */ +/* NetHack 3.6 options.c $NHDT-Date: 1578996303 2020/01/14 10:05:03 $ $NHDT-Branch: NetHack-3.6 $:$NHDT-Revision: 1.396 $ */ /* Copyright (c) Stichting Mathematisch Centrum, Amsterdam, 1985. */ /*-Copyright (c) Michael Allison, 2008. */ /* NetHack may be freely redistributed. See license for details. */ @@ -952,8 +952,8 @@ int maxlen; */ STATIC_OVL void escapes(cp, tp) -const char *cp; -char *tp; +const char *cp; /* might be 'tp', updating in place */ +char *tp; /* result is never longer than 'cp' */ { static NEARDATA const char oct[] = "01234567", dec[] = "0123456789", hex[] = "00112233445566778899aAbBcCdDeEfF"; @@ -6177,9 +6177,9 @@ char *buf; int sym_val(strval) -const char *strval; +const char *strval; /* up to 4*BUFSZ-1 long; only first few chars matter */ { - char buf[QBUFSZ]; + char buf[QBUFSZ], tmp[QBUFSZ]; /* to hold trucated copy of 'strval' */ buf[0] = '\0'; if (!strval[0] || !strval[1]) { /* empty, or single character */ @@ -6200,7 +6200,7 @@ const char *strval; /* not simple quote or basic backslash; strip closing quote and let escapes() deal with it */ } else { - char *p, tmp[QBUFSZ]; + char *p; (void) strncpy(tmp, strval + 1, sizeof tmp - 1); tmp[sizeof tmp - 1] = '\0'; @@ -6209,8 +6209,11 @@ const char *strval; escapes(tmp, buf); } /* else buf[0] stays '\0' */ } - } else /* not lone char nor single quote */ - escapes(strval, buf); + } else { /* not lone char nor single quote */ + (void) strncpy(tmp, strval + 1, sizeof tmp - 1); + tmp[sizeof tmp - 1] = '\0'; + escapes(tmp, buf); + } return (int) *buf; } From cdc598e8bdf725eb79265ff9e0e16ae779046b09 Mon Sep 17 00:00:00 2001 From: PatR Date: Tue, 14 Jan 2020 02:52:34 -0800 Subject: [PATCH 5/5] fix pline.c potential buffer overruns Fix 'Bugs 4, 5, and 6' which all use a similar fix but would have conflicts over '#define BIGBUFSZ' if committed separately. Format ("short explanation %s", string_argument), where the explanation always has modest length but the string is potentially up to 4*BUFSZ in length, into a 5*BUFSZ buffer. Then truncate the result to at most BUFSZ-1 characters so that it can be safely passed to interface-specific putstr() or raw_print(). Applies to pline(), raw_printf(), and config_error_add(). Also done for impossible() although there's no evidence that its buffer could be overflowed in a controlled manner. --- doc/fixes36.5 | 1 + src/pline.c | 15 +++++++++------ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/doc/fixes36.5 b/doc/fixes36.5 index a44081a06..9085747ab 100644 --- a/doc/fixes36.5 +++ b/doc/fixes36.5 @@ -10,6 +10,7 @@ have string_for_opt() return empty_optstr on failure 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() Fixes to Post-3.6.4 Problems that Were Exposed Via git Repository diff --git a/src/pline.c b/src/pline.c index b1ac1dffc..9b5fc31de 100644 --- a/src/pline.c +++ b/src/pline.c @@ -6,6 +6,10 @@ #define NEED_VARARGS /* Uses ... */ /* comment line for pre-compiled headers */ #include "hack.h" +#define BIGBUFSZ (5 * BUFSZ) /* big enough to format a 4*BUFSZ string (from + * config file parsing) with modest decoration; + * result will then be truncated to BUFSZ-1 */ + static unsigned pline_flags = 0; static char prevmsg[BUFSZ]; @@ -118,7 +122,7 @@ VA_DECL(const char *, line) #endif /* USE_STDARG | USE_VARARG */ { /* start of vpline() or of nested block in USE_OLDARG's pline() */ static int in_pline = 0; - char pbuf[3 * BUFSZ]; + char pbuf[BIGBUFSZ]; /* will get chopped down to BUFSZ-1 if longer */ int ln; int msgtyp; boolean no_repeat; @@ -443,15 +447,14 @@ void raw_printf VA_DECL(const char *, line) #endif { - char pbuf[3 * BUFSZ]; - int ln; + char pbuf[BIGBUFSZ]; /* will be chopped down to BUFSZ-1 if longer */ /* Do NOT use VA_START and VA_END in here... see above */ if (index(line, '%')) { Vsprintf(pbuf, line, VA_ARGS); line = pbuf; } - if ((ln = (int) strlen(line)) > BUFSZ - 1) { + if ((int) strlen(line) > BUFSZ - 1) { if (line != pbuf) line = strncpy(pbuf, line, BUFSZ - 1); /* unlike pline, we don't futz around to keep last few chars */ @@ -470,7 +473,7 @@ VA_DECL(const char *, line) void impossible VA_DECL(const char *, s) { - char pbuf[2 * BUFSZ]; + char pbuf[BIGBUFSZ]; /* will be chopped down to BUFSZ-1 if longer */ VA_START(s); VA_INIT(s, const char *); @@ -571,7 +574,7 @@ config_error_add VA_DECL(const char *, str) #endif /* ?(USE_STDARG || USE_VARARG) */ { /* start of vconf...() or of nested block in USE_OLDARG's conf...() */ - char buf[2 * BUFSZ]; + char buf[BIGBUFSZ]; /* will be chopped down to BUFSZ-1 if longer */ Vsprintf(buf, str, VA_ARGS); buf[BUFSZ - 1] = '\0';