empire/empserver
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

xundump: Refuse to undump strings too long for terminating null

Browse source 
We're dealing with three kinds of string storage: char * pointing to a
null-terminated string, char[] holding a null-terminated string, and
char holding a string of length 0 or 1.

Unfortunately, xdump meta data doesn't distinguish the latter two:
both are NSC_STRINGY.  Because of that, xundump happily fills char[]
to the limit, producing strings that aren't null-terminated, resulting
in read beyond buffer and possibly worse.

Affects struct shpstr members shp_path, shp_name, shp_rpath, struct
lndstr member lnd_rpath, and struct natstr members nat_cnam, nat_pnam,
nat_hostaddr, nat_hostname, nat_userid.  Since these are all in game
state, only the empdump utility program is affected, not the
configuration table reader.

We clearly need to require null-termination for char[] values.  Since
using char[1] for null-terminated strings makes no sense, we can still
make NSC_STRINGY with length 1 serve char values as before, by
permitting non-null-terminated strings only when length is 1.  Ugly
wart, but it fixes the bug without a possibly awkward change xdump
meta-data.

Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
This commit is contained in:
Markus Armbruster 2014-01-27 17:52:32 +01:00
parent 47ccc40e38
commit 002a3a3f1e
2 changed files with 14 additions and 9 deletions
Download patch file Download diff file Expand all files Collapse all files

13
include/nsc.h
View file

@ -28,7 +28,7 @@
* *
* Known contributors to this file: * Known contributors to this file:
* Dave Pare, 1989 * Dave Pare, 1989
* Markus Armbruster, 2004-2010 * Markus Armbruster, 2004-2014
*/ */
#ifndef NSC_H #ifndef NSC_H
@ -92,9 +92,10 @@ enum nsc_cat {
* promoted type. * promoted type.
* If category is NSC_OFF, the value is in a context object, and * If category is NSC_OFF, the value is in a context object, and
* val_as.sym specifies how to get it, as follows. * val_as.sym specifies how to get it, as follows.
* If sym.get is null, and type is NSC_STRINGY, the value is an array * If sym.get is null, and type is NSC_STRINGY, the value is a string
* of sym.len characters starting at sym.offs in the context object. * stored in sym.len characters starting at sym.offs in the context
* sym.idx must be zero. * object. sym.idx must be zero. Ugly wart: if sym.len is one, the
* terminating null character may be omitted.
* Else if sym.get is null, and sym.len is zero, the value is in the * Else if sym.get is null, and sym.len is zero, the value is in the
* context object at offset sym.off. sym.idx must be zero. * context object at offset sym.off. sym.idx must be zero.
* Else if sym.get is null, sym.len is non-zero, and the value has * Else if sym.get is null, sym.len is non-zero, and the value has
@ -202,7 +203,9 @@ enum {
* A selector with ca_type NSC_NOTYPE is invalid. * A selector with ca_type NSC_NOTYPE is invalid.
* If ca_get is null, the selector describes a datum of type ca_type * If ca_get is null, the selector describes a datum of type ca_type
* at offset ca_offs in the context object. * at offset ca_offs in the context object.
* A datum of type NSC_STRINGY is an array of ca_len characters. * A datum of type NSC_STRINGY is a string stored in an array of
* ca_len characters. Ugly wart: if ca_len is one, the terminating
* null character may be omitted.
* A datum of any other type is either a scalar of that type (if * A datum of any other type is either a scalar of that type (if
* ca_len is zero), or an array of ca_len elements of that type. * ca_len is zero), or an array of ca_len elements of that type.
* If ca_get is not null, the selector is virtual. Values can be * If ca_get is not null, the selector is virtual. Values can be

10
src/lib/common/xundump.c
View file

@ -28,7 +28,7 @@
* *
* Known contributors to this file: * Known contributors to this file:
* Ron Koenderink, 2005 * Ron Koenderink, 2005
* Markus Armbruster, 2005-2011 * Markus Armbruster, 2005-2014
*/ */
/* /*
@ -775,7 +775,7 @@ setstr(int fldno, char *str)
{ {
struct castr *ca; struct castr *ca;
int must_match, idx; int must_match, idx;
size_t len; size_t sz, len;
char *memb_ptr, *old; char *memb_ptr, *old;
ca = getfld(fldno, &idx); ca = getfld(fldno, &idx);
@ -803,13 +803,15 @@ setstr(int fldno, char *str)
return -1; return -1;
if (!str) if (!str)
return gripe("Field %d doesn't take nil", fldno + 1); return gripe("Field %d doesn't take nil", fldno + 1);
len = ca->ca_len; /* Wart: if ca_len <= 1, the terminating null may be omitted */
sz = ca->ca_len;
len = sz > 1 ? sz - 1 : sz;
if (strlen(str) > len) if (strlen(str) > len)
return gripe("Field %d takes at most %d characters", return gripe("Field %d takes at most %d characters",
fldno + 1, (int)len); fldno + 1, (int)len);
old = memb_ptr; old = memb_ptr;
if (!must_match) if (!must_match)
strncpy(memb_ptr, str, len); strncpy(memb_ptr, str, sz);
break; break;
default: default:
return gripe("Field %d doesn't take strings", fldno + 1); return gripe("Field %d doesn't take strings", fldno + 1);