From f3def5c0b999478da2d0a8f0b6a7c370a2065f77 Mon Sep 17 00:00:00 2001 From: PatR Date: Thu, 16 Jan 2020 05:22:18 -0800 Subject: [PATCH 1/6] 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(). --- doc/fixes36.5 | 2 ++ src/topten.c | 1 + src/windows.c | 20 ++++++++++++++++++-- sys/unix/unixmain.c | 8 ++++---- 4 files changed, 25 insertions(+), 6 deletions(-) diff --git a/doc/fixes36.5 b/doc/fixes36.5 index 54bb65f72..18b38439a 100644 --- 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 6a226df3f..7515c35e3 100644 --- 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 5fae44db3..01ef4ccb9 100644 --- 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 c2690ce4a..1e7880165 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); */ } } From 92deddd6a336a8c76f90d50b502c238780fdccb5 Mon Sep 17 00:00:00 2001 From: nhmall Date: Mon, 20 Jan 2020 16:08:11 -0500 Subject: [PATCH 2/6] use vsnprintf instead of vsprintf in pline.c --- doc/fixes36.5 | 1 + include/vmsconf.h | 5 +++++ src/pline.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+) diff --git a/doc/fixes36.5 b/doc/fixes36.5 index 18b38439a..514e59c01 100644 --- a/doc/fixes36.5 +++ b/doc/fixes36.5 @@ -15,6 +15,7 @@ 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/vmsconf.h b/include/vmsconf.h index a815e704a..dbfbff948 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/pline.c b/src/pline.c index 9b5fc31de..19cafd372 100644 --- a/src/pline.c +++ b/src/pline.c @@ -125,6 +125,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 */ @@ -138,7 +141,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("pline", "truncation of buffer at %zu of %d bytes", + sizeof pbuf, vlen); +#endif +#else Vsprintf(pbuf, line, VA_ARGS); +#endif line = pbuf; } if ((ln = (int) strlen(line)) > BUFSZ - 1) { @@ -447,11 +459,23 @@ void raw_printf VA_DECL(const char *, line) #endif { +#if !defined(NO_VSNPRINTF) + int vlen = 0; +#endif 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, '%')) { +#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("raw_printf", "truncation of buffer at %zu of %d bytes", + sizeof pbuf, vlen); +#endif +#else Vsprintf(pbuf, line, VA_ARGS); +#endif line = pbuf; } if ((int) strlen(line) > BUFSZ - 1) { @@ -473,6 +497,9 @@ VA_DECL(const char *, line) void impossible VA_DECL(const char *, s) { +#if !defined(NO_VSNPRINTF) + int vlen = 0; +#endif char pbuf[BIGBUFSZ]; /* will be chopped down to BUFSZ-1 if longer */ VA_START(s); @@ -481,7 +508,16 @@ VA_DECL(const char *, s) panic("impossible called impossible"); program_state.in_impossible = 1; +#if !defined(NO_VSNPRINTF) + vlen = vsnprintf(pbuf, sizeof pbuf, s, VA_ARGS); +#if (NH_DEVEL_STATUS != NH_STATUS_RELEASED) && defined(DEBUG) + if (vlen >= (int) sizeof pbuf) + panic("impossible", "truncation of buffer at %zu of %d bytes", + sizeof pbuf, vlen); +#endif +#else Vsprintf(pbuf, s, VA_ARGS); +#endif pbuf[BUFSZ - 1] = '\0'; /* sanity */ paniclog("impossible", pbuf); if (iflags.debug_fuzzer) @@ -574,9 +610,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("config_error_add", "truncation of buffer at %zu of %d bytes", + sizeof buf, vlen); +#endif +#else Vsprintf(buf, str, VA_ARGS); +#endif buf[BUFSZ - 1] = '\0'; config_erradd(buf); From b6efb765dc90be6a3cfb22e71da724d32c73f0fb Mon Sep 17 00:00:00 2001 From: nhmall Date: Mon, 20 Jan 2020 16:58:12 -0500 Subject: [PATCH 3/6] update README and patchlevel.h --- README | 9 ++------- include/patchlevel.h | 9 ++------- 2 files changed, 4 insertions(+), 14 deletions(-) diff --git a/README b/README index 36c2368a7..6ccd32e92 100644 --- a/README +++ b/README @@ -18,20 +18,15 @@ section. Below you will find some other general notes that were not considered spoilers: -FIXME: Update depending on whether cherry-picks are included or not --- 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/patchlevel.h b/include/patchlevel.h index 46e42f800..1f208d380 100644 --- a/include/patchlevel.h +++ b/include/patchlevel.h @@ -38,20 +38,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 */ From d61bfc98b402a5da0dd6bbba4c6e12b0f5a02ca6 Mon Sep 17 00:00:00 2001 From: nhmall Date: Mon, 20 Jan 2020 18:14:19 -0500 Subject: [PATCH 4/6] more vsnprintf --- src/end.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/end.c b/src/end.c index fd00aa2ec..4df88cecb 100644 --- a/src/end.c +++ b/src/end.c @@ -631,7 +631,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); } From 775519f851f830fe4e6131a8190c1c7a896baff4 Mon Sep 17 00:00:00 2001 From: nhmall Date: Mon, 20 Jan 2020 19:22:18 -0500 Subject: [PATCH 5/6] more vsnprintf --- src/pline.c | 28 ++++++---------------------- sys/winnt/nttty.c | 4 ++-- 2 files changed, 8 insertions(+), 24 deletions(-) diff --git a/src/pline.c b/src/pline.c index 19cafd372..24bdea211 100644 --- a/src/pline.c +++ b/src/pline.c @@ -145,8 +145,8 @@ VA_DECL(const char *, line) vlen = vsnprintf(pbuf, sizeof pbuf, line, VA_ARGS); #if (NH_DEVEL_STATUS != NH_STATUS_RELEASED) && defined(DEBUG) if (vlen >= (int) sizeof pbuf) - panic("pline", "truncation of buffer at %zu of %d bytes", - sizeof pbuf, vlen); + panic("%s: truncation of buffer at %zu of %d bytes", + "pline", sizeof pbuf, vlen); #endif #else Vsprintf(pbuf, line, VA_ARGS); @@ -459,20 +459,12 @@ void raw_printf VA_DECL(const char *, line) #endif { -#if !defined(NO_VSNPRINTF) - int vlen = 0; -#endif 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, '%')) { #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("raw_printf", "truncation of buffer at %zu of %d bytes", - sizeof pbuf, vlen); -#endif + (void) vsnprintf(pbuf, sizeof pbuf, line, VA_ARGS); #else Vsprintf(pbuf, line, VA_ARGS); #endif @@ -497,9 +489,6 @@ VA_DECL(const char *, line) void impossible VA_DECL(const char *, s) { -#if !defined(NO_VSNPRINTF) - int vlen = 0; -#endif char pbuf[BIGBUFSZ]; /* will be chopped down to BUFSZ-1 if longer */ VA_START(s); @@ -509,12 +498,7 @@ VA_DECL(const char *, s) program_state.in_impossible = 1; #if !defined(NO_VSNPRINTF) - vlen = vsnprintf(pbuf, sizeof pbuf, s, VA_ARGS); -#if (NH_DEVEL_STATUS != NH_STATUS_RELEASED) && defined(DEBUG) - if (vlen >= (int) sizeof pbuf) - panic("impossible", "truncation of buffer at %zu of %d bytes", - sizeof pbuf, vlen); -#endif + (void) vsnprintf(pbuf, sizeof pbuf, s, VA_ARGS); #else Vsprintf(pbuf, s, VA_ARGS); #endif @@ -619,8 +603,8 @@ VA_DECL(const char *, str) vlen = vsnprintf(buf, sizeof buf, str, VA_ARGS); #if (NH_DEVEL_STATUS != NH_STATUS_RELEASED) && defined(DEBUG) if (vlen >= (int) sizeof buf) - panic("config_error_add", "truncation of buffer at %zu of %d bytes", - sizeof buf, vlen); + panic("%s: truncation of buffer at %zu of %d bytes", + "config_error_add", sizeof buf, vlen); #endif #else Vsprintf(buf, str, VA_ARGS); diff --git a/sys/winnt/nttty.c b/sys/winnt/nttty.c index 2760b5493..c0c94fb24 100644 --- a/sys/winnt/nttty.c +++ b/sys/winnt/nttty.c @@ -1155,7 +1155,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(); @@ -1973,7 +1973,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 { From 30fc4b9e0562f9d6b18912f5d622d8bf508c798a Mon Sep 17 00:00:00 2001 From: nhmall Date: Mon, 20 Jan 2020 20:58:40 -0500 Subject: [PATCH 6/6] windows vsprintf bit --- sys/winnt/winnt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sys/winnt/winnt.c b/sys/winnt/winnt.c index 967fa6e4e..e7a2040a6 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); }