NetHack 3.6.5

The NetHack DevTeam released NetHack 3.6.5 on January 27, 2020, primarily to address six buffer overflows I reported to them on January 12 and January 15. All of these bugs were found with AFL as I described in Fuzzing NetHack.
  • .nethackrc (1/12)
    • CVE-2020-5214 Error recovery after syntax error in configuration file is subject to a buffer overflow
    • CVE-2020-5213 SYMBOL configuration file option is subject to a buffer overflow
    • CVE-2020-5212 MENUCOLOR configuration file option is subject to a buffer overflow
    • CVE-2020-5211 AUTOCOMPLETE configuration file option is subject to a buffer overflow
  • argv (1/15)
    • CVE-2020-5210 NetHack command line -w option parsing is subject to a buffer overflow
    • CVE-2020-5209 Command line parsing of options starting with -de and -i is subject to a buffer overflow

.nethackrc

I actually reported these as five bugs, not four, and the DevTeam consolidated them. I also included a non-security bug in my report. Test cases and proposed patches are here.

Bug 1 (not a security bug)

parseoptions does not check for a NULL return value from string_for_opt when parsing
term_cols, term_columns, or term_rows options.

In this code, parseoptions calls string_for_opt with val_optional=FALSE and then
passes the result unconditionally to atol:
    fullname = "term_cols";
    if (match_optname(opts, fullname, 8, TRUE)
        /* alternate spelling */
        || match_optname(opts, "term_columns", 9, TRUE)
        /* different option but identical handlng */
        || (fullname = "term_rows", match_optname(opts, fullname, 8, TRUE))) {
        long ltmp;

        op = string_for_opt(opts, negated);
        ltmp = atol(op);
However, string_for_opt can return NULL:
    if (!colon || !*++colon) {
        if (!val_optional)
            config_error_add("Missing parameter for '%s'", opts);
        return (char *) 0;
This leads to a crash when there is no colon following the term_cols, term_columns, or term_rows token. Despite the crash, this is not a security bug.

Bug 2

parse_config_line passes a buffer of size 4 * BUFSZ to parseoptions, which in turn passes it to add_menu_coloring. The buffer is then sprintfed into a buffer of size BUFSZ, leading to a buffer overflow.
boolean
add_menu_coloring(tmpstr)
char *tmpstr;
{
    int c = NO_COLOR, a = ATR_NONE;
    char *tmps, *cs, *amp;
    char str[BUFSZ];

    Sprintf(str, "%s", tmpstr);

Bug 3

parse_config_line passes a buffer of size 4 * BUFSZ to parsesymbols, which in turn passes it to sym_val. sym_val then allocates a buffer of size QBUFSZ and calls escapes to escape the 4 * BUFSZ buffer into the QBUFSZ buffer, leading to a buffer overflow.
int
sym_val(strval)
const char *strval;
{
    char buf[QBUFSZ];
...
    } else /* not lone char nor single quote */
        escapes(strval, buf);

Bug 4

When a line in .nethackrc does not begin with a known key, parse_config_line invokes
config_error_add with the message "Unknown config statement". config_error_add calls vconfig_error_add which calls config_erradd. The case for if (!config_error_data->origline_shown && !config_error_data->secure) is tripped, invoking pline with config_error_data->origline.

origline is declared in struct _config_error_frame as size 4 * BUFSIZ. Because config_erradd sends a format string of "\n%s", the case for if (index(line, '%')) is
tripped, leading to a Vsprintf into a buffer of size 3 * BUFSZ, causing a buffer overflow.
void
pline
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];
    int ln;
    int msgtyp;
    boolean no_repeat;
    /* Do NOT use VA_START and VA_END in here... see above */

    if (!line || !*line)
        return;
#ifdef HANGUPHANDLING
    if (program_state.done_hup)
        return;
#endif
    if (program_state.wizkit_wishing)
        return;

    if (index(line, '%')) {
        Vsprintf(pbuf, line, VA_ARGS);

Bug 5

After fixing bug 2 and bug 4, it is still possible to trigger a buffer overflow using a MENUCOLOR line in .nethackrc. An invalid color in match_str2clr will lead to a call to config_error_add with the format string "Unknown color '%s'", where the %s can be of size 4 * BUFSZ from parse_config_line. Because bug 4 is now fixed, the call to config_erradd will not cause a buffer overflow; however, the buf declared in config_error_add itself can still overflow as it is only size 2 * BUFSZ. Indeed, this buffer was overflowing even prior to fixing bug 4, but the overflow was moot because bug 4 would cause a crash before bug 5 mattered.
void
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];

    Vsprintf(buf, str, VA_ARGS);

Bug 6

parse_config_line passes a buffer of size 4 * BUFSZ to parseautocomplete, which in turn passes it to raw_printf/vraw_printf on an invalid command. Because parseautocomplete sends a format string of string "Bad autocomplete: invalid extended command '%s'.", the case for if (index(line, '%')) is tripped, leading to a Vsprintf into a buffer of size 3 * BUFSZ, causing a buffer overflow.
void raw_printf
VA_DECL(const char *, line)
#endif
{
    char pbuf[3 * BUFSZ];
    int ln;
    /* Do NOT use VA_START and VA_END in here... see above */

    if (index(line, '%')) {
        Vsprintf(pbuf, line, VA_ARGS);

Suggestions for .nethackrc parsing hardening

Bugs 2, 4, 5, 6, and CVE-2019-19905, could have been prevented with systematic usage of safer libc functions:
  • strncpy or strlcpy (preferred, as NUL termination is guaranteed) instead of strcpy
  • snprintf instead of sprintf
  • vsnprintf instead of vsprintf
Even when a use of strcpy, sprintf, or vsprintf can be proven to be safe, this may not be true after future code modifications. It would also be easier to regularly audit a codebase which does not use them. I understand NetHack was designed to be highly portable, including to systems which do not include these newer functions in their standard library. They could still be used on newer systems, though, or NetHack could provide its own implementations.

As part of NetHack 3.6.5, the DevTeam did change from vsprintf to vsnprintf in pline.c. See commit 92deddd6a3.

Another approach, and one which would also address bug 3, is to move away from fixed size stack buffers. Instead of this:
char buf[BUFSZ];
strncpy(buf, str, sizeof buf);
buf[sizeof buf - 1] = 0;
the DevTeam could code:
int len = strlen(str);
char *buf = alloca(len + 1);
strncpy(buf, str, len);
Some systems may not provide alloca, but it may be possible for NetHack to provide its own implementations by manipulating the stack pointer directly (with inline assembly).

argv

As I previously said, I did not originally consider command line arguments as a likely exploitation path, so I ignored them completely until after submitting my January 12 report. However, AFL found these argv buffer overflows so quickly that I scrambled to get another report to the DevTeam as fast as possible. I didn't want them to publish another release with the .nethackrc fixes and then have to immediately publish again with the argv fixes. I ended up not even doing a full analysis, but instead just sending three test cases with the gdb seg fault backtraces, and a proposed patch. I was luckily able to get the second report to them in time and they combined all the fixes into 3.6.5.

Bug 7

-w00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
(gdb) where
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007ffff7035801 in __GI_abort () at abort.c:79
#2  0x00007ffff707e897 in __libc_message (action=action@entry=do_abort, fmt=fmt@entry=0x7ffff71ab988 "*** %s ***: %s terminated\n") at ../sysdeps/posix/libc_fatal.c:181
#3  0x00007ffff7129cd1 in __GI___fortify_fail_abort (need_backtrace=need_backtrace@entry=false, msg=msg@entry=0x7ffff71ab966 "stack smashing detected") at fortify_fail.c:33
#4  0x00007ffff7129c92 in __stack_chk_fail () at stack_chk_fail.c:29
#5  0x000055555572862b in vconfig_error_add (str=0x5555558984d0 "Window type %s not recognized.  Choices are:  %s", the_args=0x7fffffffdc10) at pline.c:579
#6  0x000055555572859e in config_error_add (str=0x5555558984d0 "Window type %s not recognized.  Choices are:  %s") at pline.c:548
#7  0x00005555557db657 in choose_windows (s=0x7fffffffe202 '0' ...) at windows.c:291
#8  0x00005555557f9127 in process_options (argc=1, argv=0x7fffffffdfa0) at ../sys/unix/unixmain.c:448
#9  0x00005555557f8db8 in main (argc=2, argv=0x7fffffffdf98) at ../sys/unix/unixmain.c:192

Bug 8

-de00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
(gdb) where
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007ffff7035801 in __GI_abort () at abort.c:79
#2  0x00007ffff707e897 in __libc_message (action=action@entry=do_abort, fmt=fmt@entry=0x7ffff71ab988 "*** %s ***: %s terminated\n") at ../sysdeps/posix/libc_fatal.c:181
#3  0x00007ffff7129cd1 in __GI___fortify_fail_abort (need_backtrace=need_backtrace@entry=false, msg=msg@entry=0x7ffff71ab966 "stack smashing detected") at fortify_fail.c:33
#4  0x00007ffff7129c92 in __stack_chk_fail () at stack_chk_fail.c:29
#5  0x0000555555728272 in vraw_printf (line=0x7fffffffd8a0 "Unknown option: -de", '0' ..., the_args=0x7fffffffdd50) at pline.c:459
#6  0x0000555555728164 in raw_printf (line=0x55555589bd18 "Unknown option: %s") at pline.c:418
#7  0x00005555557f8eee in process_options (argc=1, argv=0x7fffffffdf80) at ../sys/unix/unixmain.c:388
#8  0x00005555557f8db8 in main (argc=2, argv=0x7fffffffdf78) at ../sys/unix/unixmain.c:192

Bug 9

-i000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
(gdb) where
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007ffff7035801 in __GI_abort () at abort.c:79
#2  0x00007ffff707e897 in __libc_message (action=action@entry=do_abort, fmt=fmt@entry=0x7ffff71ab988 "*** %s ***: %s terminated\n") at ../sysdeps/posix/libc_fatal.c:181
#3  0x00007ffff7129cd1 in __GI___fortify_fail_abort (need_backtrace=need_backtrace@entry=false, msg=msg@entry=0x7ffff71ab966 "stack smashing detected") at fortify_fail.c:33
#4  0x00007ffff7129c92 in __stack_chk_fail () at stack_chk_fail.c:29
#5  0x0000555555728272 in vraw_printf (line=0x7fffffffd8a0 "Unknown option: -i", '0' ..., the_args=0x7fffffffdd50) at pline.c:459
#6  0x0000555555728164 in raw_printf (line=0x55555589bd18 "Unknown option: %s") at pline.c:418
#7  0x00005555557f8ff5 in process_options (argc=1, argv=0x7fffffffdf80) at ../sys/unix/unixmain.c:418
#8  0x00005555557f8db8 in main (argc=2, argv=0x7fffffffdf78) at ../sys/unix/unixmain.c:192

Patch

You can see from this output that the same functions as the .nethackrc report are implicated: raw_printf and config_error_add. However, the size of the command line arguments is unconstrained, so no matter how large the output buffers are, it will always be possible to overflow them. Instead, the arguments need to be truncated either on input or on output. I suggested truncating on input in my proposed patch, but the DevTeam chose to truncate on output. 
diff --git a/include/extern.h b/include/extern.h
index a21b76988..9301d63ff 100644
--- a/include/extern.h
+++ b/include/extern.h
@@ -2958,7 +2958,7 @@ E boolean FDECL(mwelded, (struct obj *));
 
 /* ### windows.c ### */
 
-E void FDECL(choose_windows, (const char *));
+E void FDECL(choose_windows, (char *));
 #ifdef WINCHAIN
 void FDECL(addto_windowchain, (const char *s));
 void NDECL(commit_windowchain);
diff --git a/src/windows.c b/src/windows.c
index 5b0555189..44ad522cf 100644
--- a/src/windows.c
+++ b/src/windows.c
@@ -239,7 +239,7 @@ const char *s;
 
 void
 choose_windows(s)
-const char *s;
+char *s;
 {
     register int i;
 
@@ -271,6 +271,8 @@ const char *s;
         nh_terminate(EXIT_FAILURE);
     }
     if (!winchoices[1].procs) {
+        if (strlen(s) > QBUFSZ)
+            s[QBUFSZ] = 0; /* truncate long window type name */
         config_error_add(
                      "Window type %s not recognized.  The only choice is: %s",
                          s, winchoices[0].procs->name);
@@ -288,6 +290,8 @@ const char *s;
                     first ? "" : ", ", winchoices[i].procs->name);
             first = FALSE;
         }
+        if (strlen(s) > QBUFSZ)
+            s[QBUFSZ] = 0; /* truncate long window type name */
         config_error_add("Window type %s not recognized.  Choices are:  %s",
                          s, buf);
     }
diff --git a/sys/unix/unixmain.c b/sys/unix/unixmain.c
index 372895c70..863d99122 100644
--- a/sys/unix/unixmain.c
+++ b/sys/unix/unixmain.c
@@ -383,6 +383,8 @@ char *argv[];
                 load_symset("DECGraphics", PRIMARY);
                 switch_symbols(TRUE);
             } else {
+                if (strlen(argv[0]) > QBUFSZ)
+                    argv[0][QBUFSZ] = 0; /* truncate long option */
                 raw_printf("Unknown option: %s", *argv);
             }
             break;
@@ -413,6 +415,8 @@ char *argv[];
                 load_symset("RogueIBM", ROGUESET);
                 switch_symbols(TRUE);
             } else {
+                if (strlen(argv[0]) > QBUFSZ)
+                    argv[0][QBUFSZ] = 0; /* truncate long option */
                 raw_printf("Unknown option: %s", *argv);
             }
             break;

Comments

Post a Comment

Popular posts from this blog

Dungeon Crawl Stone Soup

Dungeon Crawl Stone Soup , aka DCSS, aka “crawl”, is an open source Rogue-like game, available at https://github.com/crawl/crawl . Public DCSS servers ( crawl.kelbi.org , crawl.develz.org , crawl.akrasiac.org , underhound.eu , crawl.beRotato.org , lazy-life.ddo.jp , webzook.net , crawl.xtahua.com , crawl.project357.org ) allow users to play DCSS without compiling it or installing it on their own system. On these servers, before starting a game players can upload their own .crawlrc configuration file. These files can include Lua 5.1 scripts, and the global environment for these scripts includes the load and loadstring functions. load and loadstring can be used to load not just other Lua scripts, but also valid or invalid Lua bytecode. Although Lua 5.1 includes a perfunctory bytecode verifier, there are known weaknesses in that verifier which have been well documented in the security community.
Read more

DoSing a Public NetHack Server

What AFL found During fuzzing (as described here ), AFL reported a hang on this .nethackrc line: MENUCOLOR=0+++++++++++++++++++++++++++++++++++++++++=blue This line is saying that if a menu item matches the regular expression (in this case, one or more ‘0’ characters), then that line should be colored blue in the UI. Only one ‘+’ character is needed to express this, but if you string many ‘+’ characters together, NetHack will hang. I was also able to reproduce the hang with MSGTYPE and AUTOPICKUP_EXCEPTION lines. So why does it hang? NetHack 3.6/3.7 has a pluggable API for regular expression engines in sys/share . Three implementations of this API are included in the source: posixregex.c – Uses the POSIX API from /usr/include/regex.h , which is implemented via the platform’s libc. This is the default on Unix. cppregex.cpp – Uses the C++ standard library available in <regex> , which is implemented via the platform’s libc++. This the default on Windows. Obviously, ...
Read more

NetHack 3.6.6, or, How to Glitch NetHack

The NetHack DevTeam released NetHack 3.6.6 on March 8, 2020, primarily to address CVE-2020-5254 , which  I reported to them on March 3. Although most of my report was focused on security--and indeed I was able to get full remote code execution (RCE) on pre-release NetHack 3.7 (but not 3.6.5)--I actually thought the more interesting finding was how to exploit the bug as a glitch.
Read more