From b1d76a772f89d249d18a09bdd8e52ea6793b1cd0 Mon Sep 17 00:00:00 2001 From: PatR Date: Thu, 30 Sep 2021 13:08:27 -0700 Subject: [PATCH] fix github issue #596 - wishing exploit for helm of opposite alignment. Discovered and described by vultur-cadens. The #adjust command can be used to split an object stack and if the shop price of the two halves are different, the new stack will have its obj->o_id modified to make the prices the same. That could be used to tip off the player as to what the low bits of the next o_id will be. Since no time passes, no intervening activity such as random creation of a new monster can take place, so the player could wish for something that depends on o_id with some degree of control. Matters mainly for helms of opposite alignment intended to be used by neutral characters since the player isn't supposed to be able to control that. (Other items like T-shirt slogan text and candy bar wrapper text had a similar issue but controlling those wouldn't have had any tangible difference on play.) The issue writeup suggested allowing the player to specify a helm's alignment during a wish. That would defeat the purpose of having o_id affect the helm's behavior in an arbitrary but repeatable way so is rejected. I implemented this fix before seeing a followup comment that suggests using a more sophisticated decision than 'obj->o_id % N' for the arbitrary effect. This just increments context.ident for the next obj->o_id or mon->m_id by 1 or 2 instead of always by 1 and should be adequate. It also has the side-effect that two consecutive wishes for helm of opposite alignment won't necessary give one for each of the two possible 'polarities', even with no intervening activity by monsters, reinforcing the lack of player control. Minor bonus fix: it moves the incrementing check for wrap-to-0 into a single place instead of replicating that half a dozen times. Ones that should have been there for shop billing and for objects loaded from bones files were missing. Fixes #596 --- doc/fixes37.0 | 6 ++++++ include/extern.h | 1 + src/makemon.c | 8 ++------ src/mkobj.c | 41 +++++++++++++++++++++++++++++++++-------- src/restore.c | 6 ++++-- src/shk.c | 2 +- 6 files changed, 47 insertions(+), 17 deletions(-) diff --git a/doc/fixes37.0 b/doc/fixes37.0 index c884f2cd5..b2d17af47 100644 --- a/doc/fixes37.0 +++ b/doc/fixes37.0 @@ -624,6 +624,12 @@ data tracking for #overview was mis-using u.urooms[] and after being in a levels might flag unvisisted rooms as having been visited special damage attacks by the Riders and by fatal-illness inflictors such as Demogorgon did no damage against other monsters, only against the hero +using obj->o_id to control 'random' behavior of a helm of opposite alignment + could potentially be controlled by player when wishing for such +obj->o_id might be set to invalid value 0 when a partly used up stack had a + dummy copy added to a shop's bill or when a bones file was loaded + (in theory that could happen on any system but in practice it could + only happen on a configuration that uses 16-bit ints) Fixes to 3.7.0-x Problems that Were Exposed Via git Repository diff --git a/include/extern.h b/include/extern.h index 98b08e1d9..42ab9dacf 100644 --- a/include/extern.h +++ b/include/extern.h @@ -1329,6 +1329,7 @@ extern struct obj *mkobj(int, boolean); extern int rndmonnum(void); extern boolean bogon_is_pname(char); extern struct obj *splitobj(struct obj *, long); +extern unsigned next_ident(void); extern struct obj *unsplitobj(struct obj *); extern void clear_splitobjs(void); extern void replace_object(struct obj *, struct obj *); diff --git a/src/makemon.c b/src/makemon.c index 067ef4381..28576d41e 100644 --- a/src/makemon.c +++ b/src/makemon.c @@ -837,9 +837,7 @@ clone_mon(struct monst *mon, m2->mextra = (struct mextra *) 0; m2->nmon = fmon; fmon = m2; - m2->m_id = g.context.ident++; - if (!m2->m_id) - m2->m_id = g.context.ident++; /* ident overflowed */ + m2->m_id = next_ident(); m2->mx = mm.x; m2->my = mm.y; @@ -1205,9 +1203,7 @@ makemon(register struct permonst *ptr, mtmp->msleeping = 1; mtmp->nmon = fmon; fmon = mtmp; - mtmp->m_id = g.context.ident++; - if (!mtmp->m_id) - mtmp->m_id = g.context.ident++; /* ident overflowed */ + mtmp->m_id = next_ident(); set_mon_data(mtmp, ptr); /* mtmp->data = ptr; */ if (ptr->msound == MS_LEADER && quest_info(MS_LEADER) == mndx) g.quest_status.leader_m_id = mtmp->m_id; diff --git a/src/mkobj.c b/src/mkobj.c index 1ee4d00e9..490fa37e3 100644 --- a/src/mkobj.c +++ b/src/mkobj.c @@ -358,9 +358,7 @@ copy_oextra(struct obj *obj2, struct obj *obj1) OMONST(obj2)->mextra = (struct mextra *) 0; OMONST(obj2)->nmon = (struct monst *) 0; #if 0 - OMONST(obj2)->m_id = g.context.ident++; - if (OMONST(obj2)->m_id) /* ident overflowed */ - OMONST(obj2)->m_id = g.context.ident++; + OMONST(obj2)->m_id = next_ident(); #endif if (OMONST(obj1)->mextra) copy_mextra(OMONST(obj2), OMONST(obj1)); @@ -421,6 +419,34 @@ splitobj(struct obj *obj, long num) return otmp; } +/* return the value of context.ident and then increment it to be ready for + its next use; used to be simple += 1 so that every value from 1 to N got + used but now has a random increase that skips half of potential values */ +unsigned +next_ident(void) +{ + unsigned res = g.context.ident; + + /* +rnd(2): originally just +1; changed to rnd() to avoid potential + exploit of player using #adjust to split an object stack in a manner + that makes most recent ident%2 known; since #adjust takes no time, + no intervening activity like random creation of a new monster will + take place before next user command; with former +1, o_id%2 of the + next object to be created was knowable and player could make a wish + under controlled circumstances for an item that is affected by the + low bits of its obj->o_id [particularly helm of opposite alignment] */ + g.context.ident += rnd(2); /* ready for next new object or monster */ + + /* if ident has wrapped to 0, force it to be non-zero; if/when it + ever wraps past 0 (unlikely, but possible on a configuration which + uses 16-bit 'int'), just live with that and hope no o_id conflicts + between objects or m_id conflicts between monsters arise */ + if (!g.context.ident) + g.context.ident = rnd(2); + + return res; +} + /* when splitting a stack that has o_id-based shop prices, pick an o_id value for the new stack that will maintain the same price */ static unsigned @@ -436,8 +462,9 @@ nextoid(struct obj *oldobj, struct obj *newobj) ++oid; newdif = oid_price_adjustment(newobj, oid); } while (newdif != olddif && --trylimit >= 0); - g.context.ident = oid + 1; /* ready for next new object */ - return oid; + g.context.ident = oid; /* update 'last ident used' */ + (void) next_ident(); /* increment context.ident for next use */ + return oid; /* caller will use this ident */ } /* try to find the stack obj was split from, then merge them back together; @@ -756,9 +783,7 @@ mksobj(int otyp, boolean init, boolean artif) otmp = newobj(); *otmp = cg.zeroobj; otmp->age = g.moves; - otmp->o_id = g.context.ident++; - if (!otmp->o_id) - otmp->o_id = g.context.ident++; /* ident overflowed */ + otmp->o_id = next_ident(); otmp->quan = 1L; otmp->oclass = let; otmp->otyp = otyp; diff --git a/src/restore.c b/src/restore.c index f82abf688..18bab2d08 100644 --- a/src/restore.c +++ b/src/restore.c @@ -263,7 +263,8 @@ restobjchn(NHFILE* nhfp, boolean frozen) otmp2->nobj = otmp; if (ghostly) { - unsigned nid = g.context.ident++; + unsigned nid = next_ident(); + add_id_mapping(otmp->o_id, nid); otmp->o_id = nid; } @@ -404,7 +405,8 @@ restmonchn(NHFILE* nhfp) mtmp2->nmon = mtmp; if (ghostly) { - unsigned nid = g.context.ident++; + unsigned nid = next_ident(); + add_id_mapping(mtmp->m_id, nid); mtmp->m_id = nid; } diff --git a/src/shk.c b/src/shk.c index f10378130..d54b9b7a2 100644 --- a/src/shk.c +++ b/src/shk.c @@ -2786,7 +2786,7 @@ sub_one_frombill(register struct obj* obj, register struct monst* shkp) otmp = newobj(); *otmp = *obj; otmp->oextra = (struct oextra *) 0; - bp->bo_id = otmp->o_id = g.context.ident++; + bp->bo_id = otmp->o_id = next_ident(); /* g.context.ident++ */ otmp->where = OBJ_FREE; otmp->quan = (bp->bquan -= obj->quan); otmp->owt = 0; /* superfluous */