dynamic format strings vulnerable to user input

This adds new utility routine strNsubst(), a more versatile version
of the existing strsubst(), that can replace the Nth occurrence of
a substring rather than just the first, and replaces all occurrences
if N is 0.

When working on vampire shape-shifting messages a few days ago I
noticed that a constructed pline/sprintf format was vulnerable to
the player giving the vampire a name with '%' in it and included
a fix for that.  This fixes two other instances of the same
vulnerability:  a monster with reflection triggering a floating
eye's gaze and the hero using a silver weapon against a silver-
hating monster.

I didn't do a lot of experimenting with the failure, just assigned
the name "foo%s" to the floating eye or the weapon.  The resulting
feedback for the relevant messages was garbled due to parameters
being substituted in the wrong place.  When that caused there to be
too few arguments to satisfy the format, the final message included
"null" for the missing one rather than triggering a crash while
trying to format something arbitrary from the stack.

I don't think these bugs provided sufficient user control to be
vulnerable to stack manipulation that does something naughty.

I found the dynamic format strings by searching for "%%".  There
may be others scattered around the code which don't have that as
an indicator....

diff --git a/doc/fixes36.1 b/doc/fixes36.1
index d010c25419fc8ed047179cef2106456559c5cdfc..4a43125d109ec3698923fbb125aa892c78807b82 100644 (file)
--- a/doc/fixes36.1
+++ b/doc/fixes36.1
@@ -393,6 +393,9 @@ poor message when named vampire shifts shape within view:
        You observe a Dracula where a Dracula was.
 vampire shifting into fog cloud to pass under door "oozed" rather than "flowed"
 adult green dragons and the Chromatic Dragon were blinded by gas clouds
+named floating eye (when hit by another monster with reflection) or named
+       silver weapon (when hero hits silver-hating monster) could disrupt
+       message formatting and conceivably trigger crash if name had '%' in it
 
 
 Fixes to Post-3.6.0 Problems that Were Exposed Via git Repository

diff --git a/include/extern.h b/include/extern.h
index 98a258cef0183b8da5dfec8be0c6dcdaa024eaf9..58a451c67aefe4fa1f25d3cbba41f635f680eaa9 100644 (file)
--- a/include/extern.h
+++ b/include/extern.h
@@ -1,4 +1,4 @@
-/* NetHack 3.6 extern.h        $NHDT-Date: 1496531111 2017/06/03 23:05:11 $  $NHDT-Branch: NetHack-3.6.0 $:$NHDT-Revision: 1.589 $ */
+/* NetHack 3.6 extern.h        $NHDT-Date: 1496860756 2017/06/07 18:39:16 $  $NHDT-Branch: NetHack-3.6.0 $:$NHDT-Revision: 1.590 $ */
 /* Copyright (c) Steve Creps, 1988.                              */
 /* NetHack may be freely redistributed.  See license for details. */
 
@@ -876,6 +876,7 @@ E boolean FDECL(onlyspace, (const char *));
 E char *FDECL(tabexpand, (char *));
 E char *FDECL(visctrl, (CHAR_P));
 E char *FDECL(strsubst, (char *, const char *, const char *));
+E int FDECL(strNsubst, (char *, const char *, const char *, int));
 E const char *FDECL(ordin, (int));
 E char *FDECL(sitoa, (int));
 E int FDECL(sgn, (int));

diff --git a/src/hacklib.c b/src/hacklib.c
index 8cb76021b001a158f188881990a1ca7118a4f67a..1c4fe63d651b1b2d856f56a58c1ca44c7c3e98d2 100644 (file)
--- a/src/hacklib.c
+++ b/src/hacklib.c
@@ -1,4 +1,4 @@
-/* NetHack 3.6 hacklib.c       $NHDT-Date: 1472006251 2016/08/24 02:37:31 $  $NHDT-Branch: NetHack-3.6.0 $:$NHDT-Revision: 1.48 $ */
+/* NetHack 3.6 hacklib.c       $NHDT-Date: 1496860756 2017/06/07 18:39:16 $  $NHDT-Branch: NetHack-3.6.0 $:$NHDT-Revision: 1.50 $ */
 /* Copyright (c) Stichting Mathematisch Centrum, Amsterdam, 1985. */
 /* Copyright (c) Robert Patrick Rankin, 1991                      */
 /* NetHack may be freely redistributed.  See license for details. */
@@ -33,6 +33,7 @@
         char *          tabexpand       (char *)
         char *          visctrl         (char)
         char *          strsubst        (char *, const char *, const char *)
+        int             strNsubst       (char *,const char *,const char *,int)
         const char *    ordin           (int)
         char *          sitoa           (int)
         int             sgn             (int)
@@ -45,7 +46,7 @@
         boolean         pmatchz         (const char *, const char *)
         int             strncmpi        (const char *, const char *, int)
         char *          strstri         (const char *, const char *)
-        boolean         fuzzymatch      (const char *,const char *,
+        boolean         fuzzymatch      (const char *, const char *,
                                          const char *, boolean)
         void            setrandom       (void)
         time_t          getnow          (void)
@@ -442,6 +443,7 @@ const char *orig, *replacement;
     char *found, buf[BUFSZ];
 
     if (bp) {
+        /* [this could be replaced by strNsubst(bp, orig, replacement, 1)] */
         found = strstr(bp, orig);
         if (found) {
             Strcpy(buf, found + strlen(orig));
@@ -452,6 +454,52 @@ const char *orig, *replacement;
     return bp;
 }
 
+/* substitute the Nth occurrence of a substring within a string (in place);
+   if N is 0, substitute all occurrences; returns the number of subsitutions;
+   maximum output length is BUFSZ (BUFSZ-1 chars + terminating '\0') */
+int
+strNsubst(inoutbuf, orig, replacement, n)
+char *inoutbuf; /* current string, and result buffer */
+const char *orig, /* old substring; if "" then insert in front of Nth char */
+           *replacement; /* new substring; if "" then delete old substring */
+int n; /* which occurrence to replace; 0 => all */
+{
+    char *bp, *op, workbuf[BUFSZ];
+    const char *rp;
+    unsigned len = (unsigned) strlen(orig);
+    int ocount = 0, /* number of times 'orig' has been matched */
+        rcount = 0; /* number of subsitutions made */
+
+    for (bp = inoutbuf, op = workbuf; *bp && op < &workbuf[BUFSZ - 1]; ) {
+        if ((!len || !strncmp(bp, orig, len)) && (++ocount == n || n == 0)) {
+            /* Nth match found */
+            for (rp = replacement; *rp && op < &workbuf[BUFSZ - 1]; )
+                *op++ = *rp++;
+            ++rcount;
+            if (len) {
+                bp += len; /* skip 'orig' */
+                continue;
+            }
+        }
+        /* no match (or len==0) so retain current character */
+        *op++ = *bp++;
+    }
+    if (!len && n == ocount + 1) {
+        /* special case: orig=="" (!len) and n==strlen(inoutbuf)+1,
+           insert in front of terminator (in other words, append);
+           [when orig=="", ocount will have been incremented once for
+           each input char] */
+        for (rp = replacement; *rp && op < &workbuf[BUFSZ - 1]; )
+            *op++ = *rp++;
+        ++rcount;
+    }
+    if (rcount) {
+        *op = '\0';
+        Strcpy(inoutbuf, workbuf);
+    }
+    return rcount;
+}
+
 /* return the ordinal suffix of a number */
 const char *
 ordin(n)

diff --git a/src/mhitm.c b/src/mhitm.c
index 38a56404b4f5ead31550a511cc137fcf7d96d71c..00ac76cb156c0fdcadecceae1cd9ce146d56761e 100644 (file)
--- a/src/mhitm.c
+++ b/src/mhitm.c
@@ -1,4 +1,4 @@
-/* NetHack 3.6 mhitm.c $NHDT-Date: 1496619132 2017/06/04 23:32:12 $  $NHDT-Branch: NetHack-3.6.0 $:$NHDT-Revision: 1.96 $ */
+/* NetHack 3.6 mhitm.c $NHDT-Date: 1496860757 2017/06/07 18:39:17 $  $NHDT-Branch: NetHack-3.6.0 $:$NHDT-Revision: 1.97 $ */
 /* Copyright (c) Stichting Mathematisch Centrum, Amsterdam, 1985. */
 /* NetHack may be freely redistributed.  See license for details. */
 
@@ -1558,8 +1558,10 @@ int mdead;
                     tmp = 127;
                 if (magr->mcansee && haseyes(madat) && mdef->mcansee
                     && (perceives(madat) || !mdef->minvis)) {
-                    Sprintf(buf, "%s gaze is reflected by %%s %%s.",
-                            s_suffix(Monnam(mdef)));
+                    /* construct format string; guard against '%' in Monnam */
+                    Strcpy(buf, s_suffix(Monnam(mdef)));
+                    (void) strNsubst(buf, "%", "%%", 0);
+                    Strcat(buf, " gaze is reflected by %s %s.");
                     if (mon_reflects(magr,
                                      canseemon(magr) ? buf : (char *) 0))
                         return (mdead | mhit);

diff --git a/src/uhitm.c b/src/uhitm.c
index a4a31cde0c90b23588e7a43c7296c6c8a10b5b1b..b11cb0fd8059be7fbe4d139417a89eb104e6ed52 100644 (file)
--- a/src/uhitm.c
+++ b/src/uhitm.c
@@ -1,4 +1,4 @@
-/* NetHack 3.6 uhitm.c $NHDT-Date: 1470819843 2016/08/10 09:04:03 $  $NHDT-Branch: NetHack-3.6.0 $:$NHDT-Revision: 1.164 $ */
+/* NetHack 3.6 uhitm.c $NHDT-Date: 1496860757 2017/06/07 18:39:17 $  $NHDT-Branch: NetHack-3.6.0 $:$NHDT-Revision: 1.166 $ */
 /* Copyright (c) Stichting Mathematisch Centrum, Amsterdam, 1985. */
 /* NetHack may be freely redistributed.  See license for details. */
 
@@ -1123,9 +1123,13 @@ int thrown; /* HMON_xxx (0 => hand-to-hand, other => ranged) */
             else if (barehand_silver_rings == 2)
                 fmt = "Your silver rings sear %s!";
             else if (silverobj && saved_oname[0]) {
-                Sprintf(silverobjbuf, "Your %s%s %s %%s!",
+                /* guard constructed format string against '%' in
+                   saved_oname[] from xname(via cxname()) */
+                Sprintf(silverobjbuf, "Your %s%s %s",
                         strstri(saved_oname, "silver") ? "" : "silver ",
                         saved_oname, vtense(saved_oname, "sear"));
+                (void) strNsubst(silverobjbuf, "%", "%%", 0);
+                Strcat(silverobjbuf, " %s!");
                 fmt = silverobjbuf;
             } else
                 fmt = "The silver sears %s!";