diff --git a/doc/fixes36.5 b/doc/fixes36.5 index 54bb65f72..514e59c01 100644 --- a/doc/fixes36.5 +++ b/doc/fixes36.5 @@ -13,6 +13,9 @@ 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() +use vsnprintf instead of vsprintf in pline.c where possible Fixes to Post-3.6.4 Problems that Were Exposed Via git Repository diff --git a/include/patchlevel.h b/include/patchlevel.h index 5f59f323b..f5130c60f 100644 --- a/include/patchlevel.h +++ b/include/patchlevel.h @@ -46,20 +46,15 @@ /* Patch 5, January ??, 2020 * - * FIXME: update entries depending on whether the cherry-picks are included - * -- start cherry-pick entries * fix accessing mons[-1] when trying to gate in a non-valid demon * fix accessing mons[-1] when monster figures out if a tin cures stoning - * walking out of tethered-to-buried-object trap condition was supposed to - * reinstate punishment but wasn't finding the buried iron ball because - * the trap condition was cleared first to indicate escape; result was - * attached chain that got dragged around but had no ball attached - * -- end cherry-pick entries * 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()0 * fix potential buffer overflow in sym_val() * fix potential buffer overflow in pline(), raw_printf(), and config_error_add() + * fix potential buffer overflow in choose_windows() + * use vsnprintf instead of vsprintf in pline.c where possible * Windows: incldues a fix from a 3.6.4 post-release update where * OPTIONS=map_mode:fit_to_screen could cause a game start failure */ diff --git a/include/vmsconf.h b/include/vmsconf.h index 9f061563b..4fc219c2e 100644 --- a/include/vmsconf.h +++ b/include/vmsconf.h @@ -168,6 +168,11 @@ PANICTRACE_GDB=2 #at conclusion of panic, show a call traceback and then #define FCMASK 0660 /* file creation mask */ +/* + * + */ +#define NO_VSNPRINTF /* Avoid vsnprintf, use less-safe vsprintf instead. */ + /* * The remainder of the file should not need to be changed. */ diff --git a/src/end.c b/src/end.c index 74d1bdf53..0f601cc99 100644 --- a/src/end.c +++ b/src/end.c @@ -610,7 +610,11 @@ VA_DECL(const char *, str) { char buf[BUFSZ]; +#if !defined(NO_VSNPRINTF) + (void) vsnprintf(buf, sizeof buf, str, VA_ARGS); +#else Vsprintf(buf, str, VA_ARGS); +#endif raw_print(buf); paniclog("panic", buf); } diff --git a/src/pline.c b/src/pline.c index 4bf141950..28d5dd4be 100644 --- a/src/pline.c +++ b/src/pline.c @@ -121,6 +121,9 @@ VA_DECL(const char *, line) char pbuf[BIGBUFSZ]; /* will get chopped down to BUFSZ-1 if longer */ int ln; int msgtyp; +#if !defined(NO_VSNPRINTF) + int vlen = 0; +#endif boolean no_repeat; /* Do NOT use VA_START and VA_END in here... see above */ @@ -134,7 +137,16 @@ VA_DECL(const char *, line) return; if (index(line, '%')) { +#if !defined(NO_VSNPRINTF) + vlen = vsnprintf(pbuf, sizeof pbuf, line, VA_ARGS); +#if (NH_DEVEL_STATUS != NH_STATUS_RELEASED) && defined(DEBUG) + if (vlen >= (int) sizeof pbuf) + panic("%s: truncation of buffer at %zu of %d bytes", + "pline", sizeof pbuf, vlen); +#endif +#else Vsprintf(pbuf, line, VA_ARGS); +#endif line = pbuf; } if ((ln = (int) strlen(line)) > BUFSZ - 1) { @@ -443,7 +455,11 @@ VA_DECL(const char *, line) /* Do NOT use VA_START and VA_END in here... see above */ if (index(line, '%')) { +#if !defined(NO_VSNPRINTF) + (void) vsnprintf(pbuf, sizeof pbuf, line, VA_ARGS); +#else Vsprintf(pbuf, line, VA_ARGS); +#endif line = pbuf; } if ((int) strlen(line) > BUFSZ - 1) { @@ -473,7 +489,11 @@ VA_DECL(const char *, s) panic("impossible called impossible"); g.program_state.in_impossible = 1; +#if !defined(NO_VSNPRINTF) + (void) vsnprintf(pbuf, sizeof pbuf, s, VA_ARGS); +#else Vsprintf(pbuf, s, VA_ARGS); +#endif pbuf[BUFSZ - 1] = '\0'; /* sanity */ paniclog("impossible", pbuf); if (iflags.debug_fuzzer) @@ -570,9 +590,21 @@ 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...() */ +#if !defined(NO_VSNPRINTF) + int vlen = 0; +#endif char buf[BIGBUFSZ]; /* will be chopped down to BUFSZ-1 if longer */ +#if !defined(NO_VSNPRINTF) + vlen = vsnprintf(buf, sizeof buf, str, VA_ARGS); +#if (NH_DEVEL_STATUS != NH_STATUS_RELEASED) && defined(DEBUG) + if (vlen >= (int) sizeof buf) + panic("%s: truncation of buffer at %zu of %d bytes", + "config_error_add", sizeof buf, vlen); +#endif +#else Vsprintf(buf, str, VA_ARGS); +#endif buf[BUFSZ - 1] = '\0'; config_erradd(buf); diff --git a/src/topten.c b/src/topten.c index 34c43e536..4ca879a98 100644 --- a/src/topten.c +++ b/src/topten.c @@ -997,6 +997,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 839f20b12..476c8bbf8 100644 --- a/src/windows.c +++ b/src/windows.c @@ -241,7 +241,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]) @@ -267,9 +268,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", @@ -291,6 +305,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 372895c70..f1e5dcb74 100644 --- 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); */ } } diff --git a/sys/winnt/nttty.c b/sys/winnt/nttty.c index 43c106e85..f9e223cf0 100644 --- a/sys/winnt/nttty.c +++ b/sys/winnt/nttty.c @@ -1162,7 +1162,7 @@ VA_DECL(const char *, s) if (iflags.window_inited) end_screen(); buf[0] = '\n'; - (void) vsprintf(&buf[1], s, VA_ARGS); + (void) vsnprintf(&buf[1], sizeof buf - 1, s, VA_ARGS); msmsg(buf); really_move_cursor(); VA_END(); @@ -1980,7 +1980,7 @@ VA_DECL(const char *, fmt) char buf[ROWNO * COLNO]; /* worst case scenario */ VA_START(fmt); VA_INIT(fmt, const char *); - Vsprintf(buf, fmt, VA_ARGS); + (void) vsnprintf(buf, sizeof buf, fmt, VA_ARGS); if (redirect_stdout) fprintf(stdout, "%s", buf); else { diff --git a/sys/winnt/winnt.c b/sys/winnt/winnt.c index af1944678..bd0d2cf48 100644 --- a/sys/winnt/winnt.c +++ b/sys/winnt/winnt.c @@ -241,11 +241,11 @@ VA_DECL(const char *, s) end_screen(); if (WINDOWPORT("tty")) { buf[0] = '\n'; - (void) vsprintf(&buf[1], s, VA_ARGS); + (void) vsnprintf(&buf[1], sizeof buf - (1 + sizeof "\n"), s, VA_ARGS); Strcat(buf, "\n"); msmsg(buf); } else { - (void) vsprintf(buf, s, VA_ARGS); + (void) vsnprintf(buf, sizeof buf - sizeof "\n", s, VA_ARGS); Strcat(buf, "\n"); raw_printf(buf); }