Bug 187392

Yes the proper way would be to use the filehandle created the first thing in
main and rewind(f) that each use. And also make sure that it doesn't get closed
anywhere. Michael, shall I wip up a quick fix?

I thought about cc'ing you on this bug, and wondered if you'd stumble across it
if I didn't.  :)

I'm already working on a patch, but you're more than welcome to double check my
work before I check it in.

Sure feel free to attach it here and I'll take a look.

Created attachment 127086 [details]
better setgid handling

Attached patch to drop setgid immediately after opening scoreboard file.  This
also fixes the seg fault caused by file permission failures trying to re-open a
file descriptor after it's been closed.

Yes the attached patch is better but still not good enough:
-main immediatly calls open_score, sofar so good
-open_score starts by calling md_getroguedir()
-md_getroguedir checks for an environment variable $ROGUEHOME and strncpy's
 this to a static buffer of 1024 chars, the strncpy however is limited to
 PATH_MAX - 20, why not use PATH_MAX in the declaration of the decleration of
 the buffer too? also if an attacker sets this environment variable to a string
 which is PATH_MAX - 20 or longer, we will have a problem because strncpy does 
 not guarantee a terminating 0, if the src string is longer then the dest buf
 the result will not be 0 terminated (and next we pass it to a strlen).

IOW md_getroguedir is broken and needs fixing, let me know if you need any help.

Created attachment 127166 [details]
Updated patch fixes md_getroguedir

Good catch.  Here's an updated patch to fix the md_getroguedir buffer overflow.
 I saw no reason for the PATH_MAX-20 limitation, changed the size of the path
static buffer to match, and ensured that path is always null-terminated.

Erm,

The PATH_MAX-20 was there for a good reason, open_score appends stuff to the
buffer returned by md_getroguedir, ugly, but it does!

So the "correct" fix would be to make the buffer in md_getroguedir PATH_MAX size
and correctly limit the string put in there to PATH_MAX-20, so either use
strncpy correctly, or preferably use snprintf(buf, sizeof(buf), "%s", src)
instead of strncpy.

p.s. I didn't look at the patch but from your decription it is obviously wrong.

Created attachment 127215 [details]
sanitize ROGUEHOME before using

Even the use of strncpy() doesn't seem entirely correct, as it may truncate the
path which will cause rogue to try to write the scoreboard file in a different
directory than specified.

A better approach would be to limit ROGUEHOME to PATH_MAX-20 (as you said
earlier), but also verify that ROGUEHOME doesn't exceed this length.  If it
does, then print a warning, throw it away, and fall back to the default
locations.

Patch added to FC4, FC5, and devel branches.  Look for the new build (-4) to hit
the mirrors shortly.

Sorry for not reviewing your latest patch sooner, still here are some remarks:
-in open_score, home_dir gets set to "" if its NULL, note though that it will
 never be NULL because md_getroguedir() never returns NULL, if it can't
 find a roguedir it will return "" itself. This is however a good check to make
 non the less.
-I was wrong about open_score appending to the buffer, it first copies the
 result to a buffer and then strcat to this the MAX_PATH-20 as limit is still 
 nescesarry though since the buffer which is copied to is MAX_PATH size and
 after the copy will have stuff strcatted to it.
-Unfortunatly the code in md_getroguedir is still not bullet proof, you first 
 check that ROGUEHOME is not longer then MAX_PATH-20, then you strncpy upto
 MAX_PATH-20 from ROGUEHOME to the buffer. Now wat happens if ROGUEHOME is
 exact MAX_PATH-20 long? The result isn't 0 terminated, you try to fix this 
 with:
 path[PATH_MAX-1] = (char)NULL;
 this however will leave 19 bytes of garbage in the buffer, what you want is:
 path[PATH_MAX-20] = '\0';
 Notice though that this will make you use MAXPATH-19 bytes in the buffer, this 
 is probably not what you want so to ROGUEHOME size check:
 if (strlen(home) > PATH_MAX-20)
 should be changed to:
 if (strlen(home) >= PATH_MAX-20)
 after which a regular strcpy can be used, or my snprintf construction above,
 hopefully you now also understand why I advocate the snprintf way over strncpy.
-The biggest problem however is the use of md_getroguedir() in open_score in the
 first place this allows the user to try to open a rogue54.scr and a rogue54.lck
 file with group games rights in dirs where we don't want to try this, for 
 example the user could set ROGUEHOME to his homedir and put a symlink file in 
 its homedir to another games owned file and have that overwritten, this 
 probably won;t do any harm beyond DOS for some other game but still is not 
 pretty.

Conclusion:
-Sorry for the late review and for not thinking about the badness of using 
 ROGUEHOME in open_score at all untill now, but the use of md_getroguedir
 in open_score should be removed and instead /var/games should always be used.
-Besides that it might not hurt to properly fix md_getroguedir while your at it.

All of these are good points.  I apologize, I should have pinged you before
committing this fix.

You are right that the biggest problem is allowing the user to set the
scoreboard location.  I verified that this can be used to overwrite the
scoreboard file of another game.  While it didn't DOS this particular other game
(Ularn), it did fill the scoreboard file with junk.

Per your suggestion, I'll disable the use of ROGUEHOME.

Created attachment 127644 [details]
Disable use of ROGUEHOME

Updated patch that disables the use of ROGUEHOME for specifying the save/lock
file directory.

Sorry about not responding earlier, for some reason I didn't get any bugzilla
mail for a couple of days I know of atleast one other bug wich needed my
attention but didn't get it because of this. It looks like the same has happened
here.

Looks fine, although the mach_dep.c and mdport.c parts look a bit overly
complicated now, you could just do a hardcoded snprintf
"/var/games/roguelike/rogue54.xxx" and drop completly drop / remove 
 md_getroguedir(). But as said it looks fine.

Fixed in latest update.