fix exploitable security bug in options processing

     From a bug report, the function escapes(),
which is used during options parsing for various options that accept
string values, is given user-controlled input that could end with a
backslash or caret (or two character "\M").  Such a malformed escape
sequence would make it consume the input's end-of-string character and
then keep processing whatever followed.  That meant that it could
generate more data than its output buffer was prepared to hold, making
nethack be vulnerable to stack overflow issues.

     His example that was supposed to clobber the stack didn't trigger
any trouble for me, and I didn't bother trying the second one that can
allegedly cause the Win32 binary to run another program.  But the bug
itself is clearly real.

diff --git a/doc/fixes34.4 b/doc/fixes34.4
index eea2e2d95d1548f26a4996de51289a28486c01ff..b259db9e3e14656158a12a87287d29f193023326 100644 (file)
--- a/doc/fixes34.4
+++ b/doc/fixes34.4
@@ -410,6 +410,9 @@ unlit candelabrum would become unlightable if its candles had exactly 1 turn
 temporary loss of Dex from wounded legs will become permanent if it occurs
        while mounted and hero dismounts before steed's legs have healed
 jaberwocks don't have hands
+character escape sequence handling during options processing was vulernable
+       to malformed escapes and could potentially be abused to clobber the
+       stack and launch a buffer overrun attack
 
 
 Platform- and/or Interface-Specific Fixes

diff --git a/src/options.c b/src/options.c
index e36f91ca73fccfbb1548a337c9fe90f4229c8687..c4a696aa0827f58e886651803c49b427c0ed4c02 100644 (file)
--- a/src/options.c
+++ b/src/options.c
@@ -747,45 +747,47 @@ nmcpy(dest, src, maxlen)
 }
 
 /*
- * escapes: escape expansion for showsyms. C-style escapes understood include
- * \n, \b, \t, \r, \xnnn (hex), \onnn (octal), \nnn (decimal). The ^-prefix
- * for control characters is also understood, and \[mM] followed by any of the
- * previous forms or by a character has the effect of 'meta'-ing the value (so
- * that the alternate character set will be enabled).
+ * escapes(): escape expansion for showsyms.  C-style escapes understood
+ * include \n, \b, \t, \r, \xnnn (hex), \onnn (octal), \nnn (decimal).
+ * The ^-prefix for control characters is also understood, and \[mM]
+ * has the effect of 'meta'-ing the value which follows (so that the
+ * alternate character set will be enabled).
+ *
+ * For 3.4.3 and earlier, ending with "\M", backslash, or caret prior
+ * to terminating '\0' would pull that '\0' into the output and then
+ * keep processing past it.  Now, a trailing escape will be handled
+ * as if it was preceded with its own backslash and be kept as is,
+ * with end of string terminator always honored as end of input.
  */
 STATIC_OVL void
 escapes(cp, tp)
 const char     *cp;
 char *tp;
 {
-    while (*cp)
-    {
+    while (*cp) {
        int     cval = 0, meta = 0;
 
-       if (*cp == '\\' && index("mM", cp[1])) {
+       if (*cp == '\\' && cp[1] && index("mM", cp[1]) && cp[2]) {
                meta = 1;
                cp += 2;
        }
-       if (*cp == '\\' && index("0123456789xXoO", cp[1]))
-       {
-           const char *dp, *hex = "00112233445566778899aAbBcCdDeEfF";
+       if (*cp == '\\' && cp[1] && index("0123456789xXoO", cp[1]) && cp[2]) {
+           NEARDATA const char hex[] = "00112233445566778899aAbBcCdDeEfF";
+           const char *dp;
            int dcount = 0;
 
            cp++;
            if (*cp == 'x' || *cp == 'X')
                for (++cp; *cp && (dp = index(hex, *cp)) && (dcount++ < 2); cp++)
-                   cval = (int)((cval * 16) + (dp - hex) / 2);
+                   cval = (cval * 16) + ((int)(dp - hex) / 2);
            else if (*cp == 'o' || *cp == 'O')
                for (++cp; *cp && (index("01234567",*cp)) && (dcount++ < 3); cp++)
                    cval = (cval * 8) + (*cp - '0');
            else
                for (; *cp && (index("0123456789",*cp)) && (dcount++ < 3); cp++)
                    cval = (cval * 10) + (*cp - '0');
-       }
-       else if (*cp == '\\')           /* C-style character escapes */
-       {
-           switch (*++cp)
-           {
+       } else if (*cp == '\\' && cp[1]) {      /* C-style character escapes */
+           switch (*++cp) {
            case '\\': cval = '\\'; break;
            case 'n': cval = '\n'; break;
            case 't': cval = '\t'; break;
@@ -794,14 +796,12 @@ char *tp;
            default: cval = *cp;
            }
            cp++;
-       }
-       else if (*cp == '^')            /* expand control-character syntax */
-       {
+       } else if (*cp == '^' && cp[1]) { /* expand control-character syntax */
            cval = (*++cp & 0x1f);
            cp++;
-       }
-       else
+       } else
            cval = *cp++;
+
        if (meta)
            cval |= 0x80;
        *tp++ = cval;