From f847518f4a6e48988a02f965dc4709a4240d1625 Mon Sep 17 00:00:00 2001 From: "nethack.rankin" Date: Fri, 29 Jun 2007 01:18:51 +0000 Subject: [PATCH] fix #H620 - dangerous/disruptive strings in bones data It's possible for the player to put escape sequences into strings via dogname/catname/fruit options (or probably interactively by using "\233" instead of "\033["--the two character 7-bit version wouldn't work because its leading ESC gets treated as player's request to abort current input, but the 8-bit version probably works, I just can't test it because I don't know how to type such things with this terminal emulator). Such sequences can do funny things like clear the screen and say "game over" (or worse with creative abuse of some terminals' "answer back" capability--when reproducing the reported situation, I kept things simple and had my dog's name underlined and fruit name blinking; they displayed correctly but nethack was confused about how long they were since it doesn't expect to be given characters which don't advance the cursor). This fix still lets users experiment with such stuff during their own games, but it replaces suspect characters while loading bones data, so if one player creates a bones file with suspect strings in it, another can--I hope--be able to use that file safely. Monster and object names, engravings, and named fruits are handled. For the last, if uncensored string matches one already present then it leaves that alone, so bones data created with same OPTIONS=fruit:whatever as being used in the current game will continue to keep the same value. --- doc/fixes35.0 | 2 ++ include/extern.h | 4 +++- src/bones.c | 31 +++++++++++++++++++++++++++++++ src/engrave.c | 12 ++++++++++++ src/options.c | 12 +++++++++--- 5 files changed, 57 insertions(+), 4 deletions(-) diff --git a/doc/fixes35.0 b/doc/fixes35.0 index b99fba365..217b62c6d 100644 --- a/doc/fixes35.0 +++ b/doc/fixes35.0 @@ -252,6 +252,8 @@ monster could attack with a polearm even after attempt to wield that failed sometimes got "you trip over it" after intervening messages following the one which described "it" wizard mode: WIZKIT wishes could overflow inventory's 52 slots +when loading bones files, censor suspect characters from player-supplied + strings such as pet and fruit names Platform- and/or Interface-Specific Fixes diff --git a/include/extern.h b/include/extern.h index aa2e50f84..db0add319 100644 --- a/include/extern.h +++ b/include/extern.h @@ -1,4 +1,4 @@ -/* SCCS Id: @(#)extern.h 3.5 2007/01/05 */ +/* SCCS Id: @(#)extern.h 3.5 2007/06/27 */ /* Copyright (c) Steve Creps, 1988. */ /* NetHack may be freely redistributed. See license for details. */ @@ -127,6 +127,7 @@ E void NDECL(drag_down); /* ### bones.c ### */ +E void FDECL(sanitize_name, (char *)); E void FDECL(drop_upon_death, (struct monst *,struct obj *,int,int)); E boolean NDECL(can_make_bones); E void FDECL(savebones, (struct obj *)); @@ -656,6 +657,7 @@ E void FDECL(make_engr_at, (int,int,const char *,long,XCHAR_P)); E void FDECL(del_engr_at, (int,int)); E int NDECL(freehand); E int NDECL(doengrave); +E void NDECL(sanitize_engravings); E void FDECL(save_engravings, (int,int)); E void FDECL(rest_engravings, (int)); E void FDECL(del_engr, (struct engr *)); diff --git a/src/bones.c b/src/bones.c index 619343ffb..9edc42539 100644 --- a/src/bones.c +++ b/src/bones.c @@ -82,6 +82,8 @@ boolean restore; } else { artifact_exists(otmp, safe_oname(otmp), TRUE); } + } else if (has_oname(otmp)) { + sanitize_name(ONAME(otmp)); } } else { /* saving */ /* do not zero out o_ids for ghost levels anymore */ @@ -169,6 +171,33 @@ boolean restore; } } +/* while loading bones, strip out text possibly supplied by old player + that might accidentally or maliciously disrupt new player's display */ +void +sanitize_name(namebuf) +char *namebuf; +{ + int c; + boolean strip_8th_bit = !strcmp(windowprocs.name, "tty") && + !iflags.wc_eight_bit_input; + + /* it's tempting to skip this for single-user platforms, since + only the current player could have left these bones--except + things like "hearse" and other bones exchange schemes make + that assumption false */ + while (*namebuf) { + c = *namebuf & 0177; + if (c < ' ' || c == '\177') { + /* non-printable or undesireable */ + *namebuf = '.'; + } else if (c != *namebuf) { + /* expected to be printable if user wants such things */ + if (strip_8th_bit) *namebuf = '_'; + } + ++namebuf; + } +} + /* called by savebones(); also by finish_paybill(shk.c) */ void drop_upon_death(mtmp, cont, x, y) @@ -498,6 +527,7 @@ getbones() * set to the magic DEFUNCT_MONSTER cookie value. */ for(mtmp = fmon; mtmp; mtmp = mtmp->nmon) { + if (has_mname(mtmp)) sanitize_name(MNAME(mtmp)); if (mtmp->mhpmax == DEFUNCT_MONSTER) { #if defined(DEBUG) && defined(WIZARD) if (wizard) @@ -514,6 +544,7 @@ getbones() } } (void) close(fd); + sanitize_engravings(); #ifdef WIZARD if(wizard) { diff --git a/src/engrave.c b/src/engrave.c index cbfd5280a..7e8915377 100644 --- a/src/engrave.c +++ b/src/engrave.c @@ -1133,6 +1133,18 @@ doengrave() return(1); } +/* while loading bones, clean up text which might accidentally + or maliciously disrupt player's terminal when displayed */ +void +sanitize_engravings() +{ + struct engr *ep; + + for (ep = head_engr; ep; ep = ep->nxt_engr) { + sanitize_name(ep->engr_txt); + } +} + void save_engravings(fd, mode) int fd, mode; diff --git a/src/options.c b/src/options.c index 06d71ff08..a42b5d089 100644 --- a/src/options.c +++ b/src/options.c @@ -4030,7 +4030,7 @@ char *str; register struct fruit *f; struct fruit *lastf = 0; int highest_fruit_id = 0; - char buf[PL_FSIZ]; + char buf[PL_FSIZ], altname[PL_FSIZ]; boolean user_specified = (str == pl_fruit); /* if not user-specified, then it's a fruit name for a fruit on * a bones level... @@ -4079,11 +4079,17 @@ char *str; Strcpy(pl_fruit, "candied "); nmcpy(pl_fruit+8, buf, PL_FSIZ-8); } + *altname = '\0'; + } else { + /* not user_supplied, so assumed to be from bones */ + copynchars(altname, str, PL_FSIZ-1); + sanitize_name(altname); } for(f=ffruit; f; f = f->nextf) { lastf = f; if(f->fid > highest_fruit_id) highest_fruit_id = f->fid; - if(!strncmp(str, f->fname, PL_FSIZ)) + if (!strncmp(str, f->fname, PL_FSIZ-1) || + (*altname && !strcmp(altname, f->fname))) goto nonew; } /* if adding another fruit would overflow spe, use a random @@ -4093,7 +4099,7 @@ char *str; f = newfruit(); if (ffruit) lastf->nextf = f; else ffruit = f; - Strcpy(f->fname, str); + copynchars(f->fname, *altname ? altname : str, PL_FSIZ-1); f->fid = highest_fruit_id; f->nextf = 0; nonew: