Bug 187392 - seg fault when users write to scoreboard file
seg fault when users write to scoreboard file
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: rogue (Show other bugs)
5
All Linux
medium Severity medium
: ---
: ---
Assigned To: Wart
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2006-03-30 12:56 EST by Wart
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-05-19 12:35:26 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
better setgid handling (6.99 KB, patch)
2006-03-30 18:28 EST, Wart
no flags Details | Diff
Updated patch fixes md_getroguedir (7.67 KB, patch)
2006-03-31 19:11 EST, Wart
no flags Details | Diff
sanitize ROGUEHOME before using (8.66 KB, patch)
2006-04-02 18:27 EDT, Wart
no flags Details | Diff
Disable use of ROGUEHOME (8.76 KB, patch)
2006-04-11 22:26 EDT, Wart
no flags Details | Diff

  None (edit)
Description Wart 2006-03-30 12:56:42 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):
rogue-5.4.2-3.x86_64

How reproducible:
Always

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.
  
Actual results:

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.

Expected results:
No seg faults and both users are listed in the scoreboard file.

Additional info:

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.
Comment 1 Hans de Goede 2006-03-30 15:31:55 EST
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?
Comment 2 Wart 2006-03-30 17:27:41 EST
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.
Comment 3 Hans de Goede 2006-03-30 17:43:11 EST
Sure feel free to attach it here and I'll take a look.
Comment 4 Wart 2006-03-30 18:28:18 EST
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.
Comment 5 Hans de Goede 2006-03-31 16:04:40 EST
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.
Comment 6 Wart 2006-03-31 19:11:40 EST
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.
Comment 7 Hans de Goede 2006-04-01 02:05:44 EST
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.
Comment 8 Wart 2006-04-02 18:27:02 EDT
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.
Comment 9 Wart 2006-04-10 22:12:36 EDT
Patch added to FC4, FC5, and devel branches.  Look for the new build (-4) to hit
the mirrors shortly.
Comment 10 Hans de Goede 2006-04-11 03:19:01 EDT
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.
Comment 11 Wart 2006-04-11 20:47:33 EDT
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.
Comment 12 Wart 2006-04-11 22:26:45 EDT
Created attachment 127644 [details]
Disable use of ROGUEHOME

Updated patch that disables the use of ROGUEHOME for specifying the save/lock
file directory.
Comment 13 Hans de Goede 2006-05-15 15:31:44 EDT
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.
Comment 14 Wart 2006-05-19 12:35:26 EDT
Fixed in latest update.

Note You need to log in before you can comment on or make changes to this bug.