Red Hat Bugzilla – Bug 187392
seg fault when users write to scoreboard file
Last modified: 2007-11-30 17:11:28 EST
Description of problem:
Only the first rogue user can write to the scoreboard file. Subsequent users
get a seg fault when they exit their game.
Version-Release number of selected component (if applicable):
Steps to Reproduce:
1. yum install rogue
2. Play a game as any user not belonging to the 'games' group
3. Play a second game as a different user also not belonging to the 'games' group
4. Run 'rogue -s' as the second user to view the scoreboard file.
When the second game ends, the scoreboard is printed along with a segmentation
fault. Running 'rogue -s' as either user does not show the results from the
second user's game.
No seg faults and both users are listed in the scoreboard file.
The seg fault is a result of poor error checking on filehandles when trying to
write to the scoreboard file. The code tries to write to filehandle '-1', which
is invalid and triggers the seg fault.
The invalid filehandle occurs because rogue reads/writes to the scoreboard file
multiple times during a game, but drops setuid/setgid privileges after each
time. As a result, only the first read attempt has setgid games, and subsequent
writes fail because it no longer has permission to write to the file.
This might be fixable by using FILE * and rewind() instead of passing around
file descriptors, and dropping setgid after the FILE * has been opened. This
will prevent the need to re-open the file using the file descriptor.
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.
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
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
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
-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
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
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.