fix #Q101 - score file bullet proofing

     Guard against buffer overflows when reading in score entries, in case
`record' has become corrupted or been maliciously modified.  This addresses
the part of "#Q101: Security bug in nethack 3.4.3" that we have control
over.  A Gentoo bug tracking discussion pointed out to us by <email deleted>, describes how that particular Linux
distribution makes users be members of the games group, allowing them to
modify files in nethack's playground directory when it has been set up in
the usual ``setgid games'' manner, thus making score processing in that
environment be vulnerable to buffer overrun exploits.

diff --git a/doc/fixes34.4 b/doc/fixes34.4
index ce9bea4d0ad93641a346646eddf596f269960355..04799918417232a0b2ecd23d61b5c4f1cfeaa7fc 100644 (file)
--- a/doc/fixes34.4
+++ b/doc/fixes34.4
@@ -334,6 +334,7 @@ don't give a speed change message when an immobile monster is seen to be hit
 when shopkeeper "gratefully inherits possessions" of hero who dies in shop
        doorway without owing the shop, move those items inside shop for bones
 dying in a shop while wielding two weapons could cause "Setworn: mask" warning
+make score file processing more bullet proof to avoid potential security issue
 
 
 Platform- and/or Interface-Specific Fixes

diff --git a/src/topten.c b/src/topten.c
index 562d5910f82e679092ea2115ca21119cf041568d..cba0d613251189b8c32fadc2b5917c5790a5b805 100644 (file)
--- a/src/topten.c
+++ b/src/topten.c
@@ -1,4 +1,4 @@
-/*     SCCS Id: @(#)topten.c   3.5     2006/05/09      */
+/*     SCCS Id: @(#)topten.c   3.5     2007/03/05      */
 /* Copyright (c) Stichting Mathematisch Centrum, Amsterdam, 1985. */
 /* NetHack may be freely redistributed.  See license for details. */
 
@@ -64,6 +64,7 @@ STATIC_DCL void FDECL(topten_print_bold, (const char *));
 STATIC_DCL xchar FDECL(observable_depth, (d_level *));
 STATIC_DCL void NDECL(outheader);
 STATIC_DCL void FDECL(outentry, (int,struct toptenentry *,BOOLEAN_P));
+STATIC_DCL void FDECL(discardexcess, (FILE *));
 STATIC_DCL void FDECL(readentry, (FILE *,struct toptenentry *));
 STATIC_DCL void FDECL(writeentry, (FILE *,struct toptenentry *));
 STATIC_DCL void FDECL(free_ttlist, (struct toptenentry *));
@@ -122,11 +123,26 @@ d_level *lev;
            return depth(lev);
 }
 
+/* throw away characters until current record has been entirely consumed */
+STATIC_OVL void
+discardexcess(rfile)
+FILE *rfile;
+{
+       int c;
+
+       do {
+           c = fgetc(rfile);
+       } while (c != '\n' && c != EOF);
+}
+
 STATIC_OVL void
 readentry(rfile,tt)
 FILE *rfile;
 struct toptenentry *tt;
 {
+       char inbuf[BUFSZ],
+            s1[BUFSZ], s2[BUFSZ], s3[BUFSZ], s4[BUFSZ], s5[BUFSZ], s6[BUFSZ];
+
 #ifdef NO_SCAN_BRACK /* Version_ Pts DgnLevs_ Hp___ Died__Born id */
        static const char fmt[] = "%d %d %d %ld %d %d %d %d %d %d %ld %ld %d%*c";
        static const char fmt32[] = "%c%c %s %s%*c";
@@ -138,7 +154,7 @@ struct toptenentry *tt;
 #endif
 
 #ifdef UPDATE_RECORD_IN_PLACE
-       /* note: fscanf() below must read the record's terminating newline */
+       /* note: input below must read the record's terminating newline */
        final_fpos = tt->fpos = ftell(rfile);
 #endif
 #define TTFIELDS 13
@@ -147,18 +163,32 @@ struct toptenentry *tt;
                        &tt->points, &tt->deathdnum, &tt->deathlev,
                        &tt->maxlvl, &tt->hp, &tt->maxhp, &tt->deaths,
                        &tt->deathdate, &tt->birthdate,
-                       &tt->uid) != TTFIELDS)
+                       &tt->uid) != TTFIELDS) {
 #undef TTFIELDS
                tt->points = 0;
-       else {
+               discardexcess(rfile);
+       } else {
+               /* load remainder of record into a local buffer;
+                  this imposes an implicit length limit of BUFSZ
+                  on every string field extracted from the buffer */
+               if (!fgets(inbuf, sizeof inbuf, rfile)) {
+                   /* sscanf will fail and tt->points will be set to 0 */
+                   *inbuf = '\0';
+               } else if (!index(inbuf, '\n')) {
+                   Strcpy(&inbuf[sizeof inbuf - 2], "\n");
+                   discardexcess(rfile);
+               }
                /* Check for backwards compatibility */
                if (tt->ver_major < 3 ||
                                (tt->ver_major == 3 && tt->ver_minor < 3)) {
-                       int i;
-
-                   if (fscanf(rfile, fmt32,
-                               tt->plrole, tt->plgend,
-                               tt->name, tt->death) != 4)
+                   int i;
+
+                   if (sscanf(inbuf, fmt32,
+                              tt->plrole, tt->plgend, s1, s2) == 4) {
+                       tt->plrole[1] = tt->plgend[1] = '\0'; /* read via %c */
+                       copynchars(tt->name, s1, (int)(sizeof tt->name) - 1);
+                       copynchars(tt->death, s2, (int)(sizeof tt->death) - 1);
+                   } else
                        tt->points = 0;
                    tt->plrole[1] = '\0';
                    if ((i = str2role(tt->plrole)) >= 0)
@@ -166,9 +196,14 @@ struct toptenentry *tt;
                    Strcpy(tt->plrace, "?");
                    Strcpy(tt->plgend, (tt->plgend[0] == 'M') ? "Mal" : "Fem");
                    Strcpy(tt->plalign, "?");
-               } else if (fscanf(rfile, fmt33,
-                               tt->plrole, tt->plrace, tt->plgend,
-                               tt->plalign, tt->name, tt->death) != 6)
+               } else if (sscanf(inbuf, fmt33, s1, s2, s3, s4, s5, s6) == 6) {
+                   copynchars(tt->plrole, s1, (int)(sizeof tt->plrole) - 1);
+                   copynchars(tt->plrace, s2, (int)(sizeof tt->plrace) - 1);
+                   copynchars(tt->plgend, s3, (int)(sizeof tt->plgend) - 1);
+                   copynchars(tt->plalign, s4, (int)(sizeof tt->plalign) - 1);
+                   copynchars(tt->name, s5, (int)(sizeof tt->name) - 1);
+                   copynchars(tt->death, s6, (int)(sizeof tt->death) - 1);
+               } else
                        tt->points = 0;
 #ifdef NO_SCAN_BRACK
                if(tt->points > 0) {
@@ -304,16 +339,11 @@ int how;
        t0->maxhp = u.uhpmax;
        t0->deaths = u.umortality;
        t0->uid = uid;
-       (void) strncpy(t0->plrole, urole.filecode, ROLESZ);
-       t0->plrole[ROLESZ] = '\0';
-       (void) strncpy(t0->plrace, urace.filecode, ROLESZ);
-       t0->plrace[ROLESZ] = '\0';
-       (void) strncpy(t0->plgend, genders[flags.female].filecode, ROLESZ);
-       t0->plgend[ROLESZ] = '\0';
-       (void) strncpy(t0->plalign, aligns[1-u.ualign.type].filecode, ROLESZ);
-       t0->plalign[ROLESZ] = '\0';
-       (void) strncpy(t0->name, plname, NAMSZ);
-       t0->name[NAMSZ] = '\0';
+       copynchars(t0->plrole, urole.filecode, ROLESZ);
+       copynchars(t0->plrace, urace.filecode, ROLESZ);
+       copynchars(t0->plgend, genders[flags.female].filecode, ROLESZ);
+       copynchars(t0->plalign, aligns[1 - u.ualign.type].filecode, ROLESZ);
+       copynchars(t0->name, plname, NAMSZ);
        t0->death[0] = '\0';
        switch (killer.format) {
                default: impossible("bad killer format?");