Spec Name or Url: http://www.kobold.org/~wart/fedora/bsd-games.spec SRPM Name or Url: http://www.kobold.org/~wart/fedora/bsd-games-2.17-1.src.rpm Description: Bsd-games includes adventure, arithmetic, atc, backgammon, battlestar, bcd, caesar, canfield, cfscores, cribbage, dm, fish, gomoku, hunt, mille, monop, morse, number, phantasia, pig, pom, ppt, primes, quiz, rain, random, robots, rot13, sail, snake, snscore, teachgammon, tetris-bsd, trek, wargames, worm, worms and wump. Version 2.9 of this packages was originally included with RedHat 7 and subsequently dropped. This is an update to 2.17 with some cleanup for Fedora Extras. This package still needs work; I just wanted to throw it out here while it was still fresh in my mind. Feel free to ignore this package review request until I've been able to clean up some of these known issues: * No documentation other than manpages is included, not even a license file. * None of the setgid games have been audited. I suspect that most, if not all, will need to be modified to be made more secure. * I have not tested the use of dm for limiting access to games. * Most of the games have not been tested yet. * rpmlint has lots of warnings about empty scoreboard files and strange setgid file permissions. This can be ignored.
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!