Michael,

If these games are only sgid because of a shared scoreboard file you should be
able to easily skip the audit by just doing it as done with Ularn and others,
first thing in main open the score file "r+" and immediatly drop any special rights.

The license file is a bigger problem though.

Whats dm?

Yes, these are setgid for shared scoreboard files.  Most of the games aren't
setgid, so only a few need to be checked.  From the cursory glance that I made
it seems that ~10 of them will require a little work.

dm is a tool to let a system administrator control access to the various games.
 You can move a game binary to %[_libdir} and make a link from dm to it's old
name in %{_bindir}.  Once that's done then you can use the dm configuration
files to control the times during the day that the program can be run, who is
allowed to run it, or disable access to it altogether.

It turns out there is a license file that describes the license text that is
included with each game.  That should work well enough.

I've fixed up the setgid issues and played a few of the games to make sure they
work.  I changed all references to setregid() to setresgid() and rearranged
where this happens in a few of the games.  The only one that really needs a
second look is 'hack'; the scoreboard location can be specified on the command
line or in environment settings, so there's a bit more processing that takes
place before running setresgid().  Here is the new package:

http://www.kobold.org/~wart/fedora/bsd-games.spec
http://www.kobold.org/~wart/fedora/bsd-games-2.17-2.src.rpm

I'll do  a review for this. But first please remove the ability to specify the
scoreboard location from the cmdline and/or environment from hack, see comment
10 in bug 187392 (the rogue scoreboard story) for rationale.

The scoreboard for hack is now fixed to /var/games/hack/record.  Now the ability
to save games is broken in hack because it tries to write to
/var/games/hack/save/ after it's dropped setgid.  IMO, it shouldn't be storing
save games there anyway.  They should go in $HOME/.hack/.  I'm working on
another patch for that problem.

http://www.kobold.org/~wart/fedora/bsd-games-2.17-3.src.rpm
http://www.kobold.org/~wart/fedora/bsd-games.spec

I've fixed the 'hack' savefile problem and updated the -3 src rpm to contain the
new patch.

Ugh.  I overlooked a couple of games in this package that are still setgid but
don't drop setgid correctly:  bsd-tetris, phantasia, and sail.  Time to dig
around in the code again...

I don't like the putting the highscore file in a dir under /var/games which hack
currently does with:
/var/games/hack/record

Since the save games will no longer be in the /var/games/hack dir, I see no use
for a directory, could you change the highscore file to /var/games/hack.hs or
hack.record please?

Buffer overflow in sail, CVE-2006-1744:
http://www.cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2006-1744

I have a problem...
file /usr/bin/monop from install of bsd-games-2.17-3 conflicts with file from 
package mono-devel-1.1.13.4-2

Yeah, the whole "put all executables in /usr/bin" thing turns out to not be such
a keen idea after all.

I don't have a problem with renaming it to m-poly or monop-game to remove the
conflict.

I have solved this problem and any other potential problems with renaming all 
binaries to bsd_* style, e.g. bsd_banner, bsd_monop etc.

So, now is problem with bsd_rot13...

$bsd_rot13
/usr/bin/bsd_rot13: line 39: /usr/bin/caesar: No such file or directory
/usr/bin/bsd_rot13: line 39: exec: /usr/bin/caesar: cannot execute: No such file 
or directory

Well, you can patch that. But "bsd_rot13" is a lot less nice than just "rot13",
which is a vaguely handy little utility.

You can't globally rename all of the applications because of these
interdependencies, and I believe that some of the apps look at argv[0] to
determine how to function.

It's better to rename them on an as-needed basis only.

It seems to me that only bsd_rot13 has a dependency with another bsd-games app. 
I think that patch only this app should fix all problems.

I really don't like the precedent of renaming all the binaries in a classic
package just because some new thing comes up with a binary with the same name.

Changes in this iteration:
- Cleaned up tetris-bsd so that it minimizes the amount of code that runs setgid.
- Added patch set from Debian's package that, among other things, fixes
CVE-2006-1744
- Renamed monop to mpoly to remove conflict with mono-devel

Hack has more variable game data than just the scoreboard.  There are also bones
files from dead players.  As such, I'm leaving the scoreboard in /var/games/hack/.

http://www.kobold.org/~wart/fedora/bsd-games.spec
http://www.kobold.org/~wart/fedora/bsd-games-2.17-4.src.rpm

There are still a few remaining issues to resolve:
* modify bones-file handling in hack so that we can drop setgid earlier
* Take a closer look at sail and phantasia to see if the amount of setgid code
can be minimized.

This iteration uses a custom group for each of hack, sail, and phantasia to
alleviate some of the potential security issues arising from running setgid. 
This is in lieu of having them drop setgid early.  The custom groups are created
with fedora-groupadd and have been registered on the wiki: 
http://fedoraproject.org/wiki/Packaging/UserRegistry

http://www.kobold.org/~wart/fedora/bsd-games.spec
http://www.kobold.org/~wart/fedora/bsd-games-2.17-5.src.rpm

If the groups are seen as a good idea and are going to stay: why do these games
need statically assigned GIDs, and plain groupadd without -g is not enough?

So it's consistant across systems?

Same question rephrased: what's the real world benefit of having the GIDs
consistent across systems *in this particular case* for these three game
executables?

Consistent GIDs are useful if the scoreboard files are to be shared across a
cluster.  This is how nethack and the like were configured at my university's
computing cluster.

Ok, I give up.  But if you intend to keep the current implementation, a
dependency on fedora-usermgmt needs to be added.

As I think about it more, I'm changing my mind that fedora-usermgmt is needed
here.  consistent gids in a cluster is better solved by nis or some other
network login system.  These aren't system groups that need to be installed, so
plain groupadd should be enough.

(In reply to comment #24)
> Consistent GIDs are useful if the scoreboard files are to be shared across a
> cluster.  This is how nethack and the like were configured at my university's
> computing cluster.
So what? What guarantees these uids/gids are consistent with that you rsp.
fedora-usermgnt chose?

Answer: Your system administrator. Neither you nor fedora-usermgnt will be able
to do so.

Okay,

So were not going to use fedora-usermgnt and we're going to use seperate groups
for: hack, sail and phantasia. Although this doesn't completly remove all
security concerns (see fedora-games-list discussion) it does remove 99.9 % of
them. Besides that these games are shipped by others (freebsd?) using sgid too,
and lately a security alert was send out for hack. So it seems that others have
taken a look at the sgid stuff too and found it acceptable to include and it
seems that atleast one person has done some kinda auditing for this. So I say
that putting hack, sail and phantasia in their own group is safe enough (notting
is really 100% safe).

Mike,

Can you post a new version which drops the fedora-usermgt and has any other
fixes you still want todo? IOW can you post a new version which is ready for a
full review? Then I'll review it asap (which might not be so soon though).

Updated packages without fedora-usermgmt, ready for a full review:

http://www.kobold.org/~wart/fedora/bsd-games-2.17-6.src.rpm
http://www.kobold.org/~wart/fedora/bsd-games.spec

I'm slowly descending into the bsd-games source, some initial remarks:
1) in bsd-games-2.17-hackgid.patch (line 81) you've seem to have left in /
 added a debug printf.
2) in hack you allow the user to specify the save game location and then the 
 savegame gets opened with full gamehack gid rights. This is a problem.
 Please modify hack so that the first thing it does in main is:
 gamehack_gid = getegid();
 setgid(getgid());
 Where gamehack_gid is a global available gid_t and then when it needs the extra
 rights it should do:
 setgid(gamehack_gid);
 // do stuff
 setgid(getgid());
3) The same (2) probably also goes for the other games which don't permanently
 drop their setgid rights with setresgid.

The spec looks good, but it will probably take me some time to properly review
all the setresgid using games for any problems. In the mean time a new version
addressing the above would be nice.

Okay, I've reviewed all the "sgid games"-games except for bsd-tetris which seems
to be a special case.

Remarks:
1) cfscores is sgid games, but canfield/cfscores/cfscores.c  drops rights
 immediatly even before reading the scores file because of this and since this
 program only reads hence I believe the sgid bit is not needed.
2) Whats the use of having dm if we install the games directly instead of in a 
 "hide" path and creating gamename symlinks to the game?

I just also checked tetris, thats ok too.

Mike could you, post a new version which takes my comments from comment 30 and
comment 31 into account, then I can hopefully use that for a full review.

(In reply to comment #32)
> I just also checked tetris, thats ok too.
> 
> Mike could you, post a new version which takes my comments from comment 30 and
> comment 31 into account, then I can hopefully use that for a full review.

I'm working on that now in between meetings.  :)  I'll have something by evening
PDT.

I think I'm going to drop dm altogether since it takes a significant amount of
admin work to use, and I don't see too much benefit from using it.  Besides, the
links to dm will get replaced with the actual binaries once the package gets an
update.

I'm all for dropping dm, its a strange beast, especially since there is no
similar functionality for any of the other games we package.

I've addressed the issues from comment 30 and comment 31.  The hack gid patch
has a lot of changes to limit the setgid portions of code.  Fortunately,
phantasia was easier to deal with, and sail was already done upstream.

dm has also been dropped.

http://www.kobold.org/~wart/fedora/bsd-games-2.17-7.src.rpm
http://www.kobold.org/~wart/fedora/bsd-games.spec

Looking good, almost there!

MUST:
-----
* rpmlint output is:
E: bsd-games configure-without-libdir-spec
E: bsd-games zero-length /var/games/robots_roll
E: bsd-games non-standard-executable-perm /usr/bin/canfield 02755
E: bsd-games non-standard-gid /var/games/phantasia/characs gamephant
E: bsd-games zero-length /var/games/phantasia/characs
E: bsd-games non-standard-executable-perm /usr/bin/snake 02755
E: bsd-games non-standard-gid /var/games/phantasia gamephant
E: bsd-games non-standard-dir-perm /var/games/phantasia 0775
E: bsd-games non-standard-gid /usr/bin/phantasia gamephant
E: bsd-games setgid-binary /usr/bin/phantasia gamephant 02755
E: bsd-games non-standard-executable-perm /usr/bin/phantasia 02755
E: bsd-games non-standard-executable-perm /usr/bin/robots 02755
E: bsd-games non-standard-gid /var/games/hack gamehack
E: bsd-games non-standard-dir-perm /var/games/hack 0775
E: bsd-games non-standard-gid /var/games/phantasia/motd gamephant
E: bsd-games zero-length /var/games/phantasia/motd
E: bsd-games non-standard-executable-perm /usr/bin/atc 02755
E: bsd-games zero-length /var/games/cfscores
E: bsd-games zero-length /var/games/atc_score
E: bsd-games non-standard-gid /var/games/hack/record gamehack
E: bsd-games zero-length /var/games/hack/record
E: bsd-games non-standard-executable-perm /usr/bin/battlestar 02755
E: bsd-games non-standard-gid /var/games/saillog gamesail
E: bsd-games zero-length /var/games/saillog
E: bsd-games non-standard-executable-perm /usr/bin/tetris-bsd 02755
E: bsd-games non-standard-gid /usr/bin/sail gamesail
E: bsd-games setgid-binary /usr/bin/sail gamesail 02755
E: bsd-games non-standard-executable-perm /usr/bin/sail 02755
E: bsd-games zero-length /var/games/tetris-bsd.scores
E: bsd-games non-standard-gid /var/games/phantasia/monsters gamephant
E: bsd-games non-standard-executable-perm /usr/bin/cribbage 02755
E: bsd-games zero-length /var/games/criblog
E: bsd-games non-standard-gid /var/games/phantasia/scoreboard gamephant
E: bsd-games zero-length /var/games/phantasia/scoreboard
E: bsd-games non-standard-gid /var/games/phantasia/mess gamephant
E: bsd-games zero-length /var/games/phantasia/mess
E: bsd-games non-standard-gid /var/games/hack/perm gamehack
E: bsd-games zero-length /var/games/hack/perm
E: bsd-games non-standard-gid /var/games/sail gamesail
E: bsd-games non-standard-dir-perm /var/games/sail 0775
E: bsd-games zero-length /var/games/battlestar.log
E: bsd-games non-standard-gid /var/games/phantasia/void gamephant
E: bsd-games zero-length /var/games/phantasia/void
E: bsd-games zero-length /var/games/snakerawscores
E: bsd-games non-standard-gid /var/games/phantasia/lastdead gamephant
E: bsd-games zero-length /var/games/phantasia/lastdead
E: bsd-games non-standard-gid /var/games/phantasia/gold gamephant
E: bsd-games zero-length /var/games/phantasia/gold
E: bsd-games zero-length /var/games/snake.log
E: bsd-games non-standard-gid /usr/bin/hack gamehack
E: bsd-games setgid-binary /usr/bin/hack gamehack 02755
E: bsd-games non-standard-executable-perm /usr/bin/hack 02755
These are all not a problem, see discussion above.
* Package and spec file named appropriately
* Packaged according to packaging guidelines
* License ok, license file included
* spec file is legible and in Am. English.
* Source matches upstream
* Compiles and builds on devel-x86_64
* BR: ok (see below)
* No locales
* No shared libraries
* Not relocatable
* Package owns all dirs it installs, with one exception see below.
* No duplicate files & Permissions ok
* %clean & macro usage OK
* Contains code and permissable content
* %doc does not affect runtime, and isn't large enough to warrent a sub package
* no -devel package needed, no libs / .la files.
* no gui -> no .desktop file required


MUST Fix:
---------
* Add missing "Requires(Pre): /usr/sbin/groupadd"
* Package must own /usr/share/bsd-games


Should Fix:
-----------
* "# It looks like textutils became coreutils at some point.  I'm not sure
   # what is needed from it, so I'm commenting it out until I can find out.
   #Requires: textutils"
* Maybe use "BSD" as license, as all games eem to be under one or the other
  variant of the BSD license?
* Shouldn't the highscore files be marked %config(noreplace) ?
* Suspicious compiler warnings:
 "canfield/cfscores/cfscores.c:130: warning: comparison of unsigned expression <
0 is always false"
 "hunt/hunt/playit.c:117: warning: comparison is always true due to limited
range of data type"
 "hunt/hunt/playit.c:652: warning: comparison is always true due to limited
range of data type
 "phantasia/setup.c:71: warning: 'path' may be used uninitialized in this function"

 "

(In reply to comment #36)

> MUST Fix:
> ---------
> * Add missing "Requires(Pre): /usr/sbin/groupadd"

Added.

> * Package must own /usr/share/bsd-games

Good catch.  It's owned now.

> Should Fix:
> -----------
> * "# It looks like textutils became coreutils at some point.  I'm not sure
>    # what is needed from it, so I'm commenting it out until I can find out.
>    #Requires: textutils"

I never did figure out what needs it.  I didn't notice any obvious exec() or
system() calls to external applications.

> * Maybe use "BSD" as license, as all games eem to be under one or the other
>   variant of the BSD license?

I won't object to that.  I used Distributable as that's what RH7 used for the
license before it was dropped.

> * Shouldn't the highscore files be marked %config(noreplace) ?

I say no.  If the high score file formats changes at any time then we want to
make sure that the old ones get removed.  We could mark them as %config only,
but that seems pointless because the only reason to preserve the old scoreboard
files is if a migration tool is also provided to move it to the new format, and
I don't see that happening for any of these games.

> * Suspicious compiler warnings:
>  "canfield/cfscores/cfscores.c:130: warning: comparison of unsigned expression <
> 0 is always false"
>  "hunt/hunt/playit.c:117: warning: comparison is always true due to limited
> range of data type"
>  "hunt/hunt/playit.c:652: warning: comparison is always true due to limited
> range of data type

These are part of some paranoid error checking.  All are harmless.

>  "phantasia/setup.c:71: warning: 'path' may be used uninitialized in this
function"

This is a bogus warning.  Besides, this file is only used during the %build
phase to create the initial shared game files, not as part of any shipped
executable.

New package with MUSTFIX fixes:

http://www.kobold.org/~wart/fedora/bsd-games-2.17-8.src.rpm
http://www.kobold.org/~wart/fedora/bsd-games.spec

FWIW, I discovered that the change from setregid() to setresgid() is superfluous
because it turns out, on linux at least, that setregid() will also change the
saved gid if the new real gid is not -1, or if the new egid != the old egid.

However, the setresgid patch should remain because it adds error checking to the
setresgid() call and aborts if the privileges could not be dropped.

(In reply to comment #37)
> (In reply to comment #36)
> 
> > MUST Fix:
> > ---------
> > * Package must own /usr/share/bsd-games
> 
> Good catch.  It's owned now.
> 

Wouldn't it be easier to just write:
%{_datadir}/bsd-games

Instead of all those seperate entreis for files and dirs under it in the %files
section?  Anyways all MUST Fix items fixed: _approved_!

> > Should Fix:
> > -----------
> > * "# It looks like textutils became coreutils at some point.  I'm not sure
> >    # what is needed from it, so I'm commenting it out until I can find out.
> >    #Requires: textutils"
> 

In that case concider dropping these 3 lines from the spec?

> > * Shouldn't the highscore files be marked %config(noreplace) ?
> 
> I say no.  If the high score file formats changes at any time then we want to
> make sure that the old ones get removed.  We could mark them as %config only,
> but that seems pointless because the only reason to preserve the old scoreboard
> files is if a migration tool is also provided to move it to the new format, and
> I don't see that happening for any of these games.
> 

Do you concider the changing of these files format likely? If you don't makr
them %config(noreplace) the highscores will get reset on each package update, I
don't think you / we want that.

(In reply to comment #39)
> (In reply to comment #37)
> > (In reply to comment #36)
> Wouldn't it be easier to just write:
> %{_datadir}/bsd-games

Why yes, it would.


> 
> In that case concider dropping these 3 lines from the spec?

Done.

> > > * Shouldn't the highscore files be marked %config(noreplace) ?
> > 
> > I say no.  If the high score file formats changes at any time then we want to
> > make sure that the old ones get removed.  We could mark them as %config only,
> > but that seems pointless because the only reason to preserve the old scoreboard
> > files is if a migration tool is also provided to move it to the new format, and
> > I don't see that happening for any of these games.
> > 
> 
> Do you concider the changing of these files format likely? If you don't makr
> them %config(noreplace) the highscores will get reset on each package update, I
> don't think you / we want that.

I was under the mistaken impression that rpm would not touch files during an
upgrade that hadn't changed from one release to the next, but it seems that's
not the case for these scoreboard files.

I'll fix these three minor issues before building.

Thanks a bunch for the review.  I know it was time consuming since this is like
40 packages all rolled into one.  :)

Imported and built.

Thanks!