Bug 183089
Summary: | Review Request: ularn - a text-based roguelike game | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Wart <wart> |
Component: | Package Review | Assignee: | Hans de Goede <hdegoede> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | ||
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-03-17 18:38:32 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Wart
2006-02-26 04:27:36 UTC
Added an icon to the .desktop file and changed file ownership on most files to root.root: http://www.kobold.org/~wart/fedora/ularn-1.5p4-2.src.rpm http://www.kobold.org/~wart/fedora/ularn.spec Since I'm about to request yet another game SIG related review from you I thought I would return the favor and see if there were still any open review requests done by you, so here I'am :) I haven't taken a look, but the sgid games stuff does not make me very happy. I'll probably write up a patch which as the first thing of main opens the highscore file, leaves an fd around for the highscore functions to use and then drops the games group rights. The only issue which then remains is do we use the games user / group for stuff like this, or do we create a user per game. I must say I like the games user idea for simple on file cases, for more complex stuff like daemons we should use a new user. What do you think? Maybe we should take this to the list? Anyways I think that whatever we decide we should put this on the games-SIG wiki page as games-SIG policy. I'll start a real review tomorrow, as usual with the kinda comments from me its bedtime over here (I have a daytime job too :) (In reply to comment #2) > Since I'm about to request yet another game SIG related review from you I > thought I would return the favor and see if there were still any open review > requests done by you, so here I'am :) :) > I haven't taken a look, but the sgid games stuff does not make me very happy. > I'll probably write up a patch which as the first thing of main opens the > highscore file, leaves an fd around for the highscore functions to use and then > drops the games group rights. Patches are always welcome. > The only issue which then remains is do we use > the games user / group for stuff like this, or do we create a user per game. I > must say I like the games user idea for simple on file cases, for more complex > stuff like daemons we should use a new user. What do you think? I think it's overkill to create users for a single setgid scoreboard file. Definitely we want to create them for game servers that run as daemon processes. I feel that this is the perfect use for the 'games' user/group. > Maybe we should > take this to the list? Anyways I think that whatever we decide we should put > this on the games-SIG wiki page as games-SIG policy. I posed the question to the -extras and -devel lists a couple of weeks ago, but nobody seemed to have an opinion on the setgid bits. I interpreted that to mean that there were no objections. https://www.redhat.com/archives/fedora-extras-list/2006-March/msg00002.html https://www.redhat.com/archives/fedora-devel-list/2006-March/msg00000.html This is also in the SIG wiki, but could probably be clarified. Looking at the specfile some more comments: -the config.sh stuff is messy, very messy. But if it works it works. -why games games as default group/owner. This should be root root -why games games for the binary, this should be root games. This way if someone manages to get games uid rights he still can't modify (trojan) the binary -why the fortune help and maps in /var/games can these be modified? -why 775 for the dir can't you precreate the highscore file and make it 664 and leave the directory as default (755). Or even better move maps help and fortunes to /use/share and put the highscore file directly /var/games (with a name indicating its owner package like ularn-highscores.bin) And judging from the ularn-build.patch will all the varg stuff its a security nigthmare, it doesnot do any networking does it? Otherwise it will first need a full audit. (In reply to comment #4) > Looking at the specfile some more comments: > -the config.sh stuff is messy, very messy. But if it works it works. Yes, it is messy. Unfortunately, the included Configure script is interactive and thus, unsuitable for being run in a rpm spec file. sed + a pregenerated configure output file seemed like the next best solution. > -why games games as default group/owner. This should be root root I thought I had fixed that to %defattr(-root,root,-) in the -2 package. See comment #1. > -why games games for the binary, this should be root games. This way if someone > manages to get games uid rights he still can't modify (trojan) the binary Good point. The scoreboard should also be made root.games. > -why the fortune help and maps in /var/games can these be modified? The fortune file contains messages for fountains. The help is displayed in-game and may be used to provide specific messages to players when they run the game. The maps file contains maps for the final volcano levels. While all of these are modifiable, it is more likely that the help and fortunes file will change and the maps will remain static. Unfortunately, the game searches for all 4 (including the scoreboard) of these files in the same directory. I could patch the game to place maps in %{_datadir} and fortune and help in %{_sysconfdir}, but it seemed simplest to leave them all in /var/games/ularn. If the package placed only one file in /var/games, then there wouldn't be a need for the <gamename> subdir. But since there's 4 files, the subdir helps reduce the clutter. > -why 775 for the dir can't you precreate the highscore file and make it 664 and > leave the directory as default (755). Or even better move maps help and fortunes > to /use/share and put the highscore file directly /var/games (with a name > indicating its owner package like ularn-highscores.bin) > > And judging from the ularn-build.patch will all the varg stuff The vararg stuff is a nightmare. Many of these early roguelike games seemed to feel that they had to rewrite sprintf, which introduced all of this mess. I haven't tried using a precreated highscore file. That's a good idea and should let us tighten up some of the file permissions, assuming it works. It seems that the setgid trick isn't actually letting me write to the scoreboard file, however. I'll have to dig around to see what's wrong with that. > its a security > nigthmare, it doesnot do any networking does it? Otherwise it will first need a > full audit. Networking? Oh my, no. ularn predates network-aware games. The only way you can use it in a networked environment is with 'ssh -t hostname ularn'. :) Ok, todo before starting a full review: -make highscore file with sgid work, or forget all about sgid and store it under $HOME/.ularn (per user highscores, as most other games do). -move fortune, maps and help to /usr/share/ularn IMHO they are all static, other wise one could put pwads (== maps) under /etc too. -if using sgid highscores make it a single file under /var/games If you run into trouble doing this feel free to ask for help (as always). (In reply to comment #6) > Ok, todo before starting a full review: > -make highscore file with sgid work, or forget all about sgid and store it under > $HOME/.ularn (per user highscores, as most other games do). I like the idea of a shared scoreboard file so that you can see how you rank with other players, so I'll continue to work on the sgid bits. > -move fortune, maps and help to /usr/share/ularn IMHO they are all static, other > wise one could put pwads (== maps) under /etc too. > -if using sgid highscores make it a single file under /var/games Will do. The scoreboard is now located in /var/games/Ularn-scoreboard, and the other data files have been moved to /usr/share/ularn. Additionally, ularn drops its setgid privileges almost immediately, just after opening the scoreboard file. http://kobold.org/~wart/fedora/ularn-1.5p4-3.src.rpm http://kobold.org/~wart/fedora/ularn.spec The failure to write to the scoreboard turned out to be a misunderstanding. ularn won't write a scoreboard file entry if you have a zero score, which is what was happening during my testing. :O Attempts to debug the problem with strace and gdb failed because they use ptrace(), which disables any set[ug]id bits. MUST ==== * rpmlint output: E: ularn zero-length /var/games/Ularn-scoreboard E: ularn non-standard-executable-perm /usr/bin/Ularn 02755 Both are due to the already discussed scoreboard stuff and can be ignored. * Package named correctly * GPL license OK. * spec file legible, in Am. English * Source matches upstream * Successfully compiles and builds on at least one platform (FC-5 x86_64) (lots of warnings though!) * no locale data, shared libraries, or static libraries * No excessive Requires: or BR: * Summary and description ok * macro use consistent * Game content permissible * Not relocatable * %doc does not affect runtime MUSTFIX ======= * Package should own /usr/share/ularn, just use %{_datadir}/%{name} instead of the 3 seperate lines for the 3 files under this dir. * You currently use setegid to drop the games group, that however wont affect the saved gid and thus an attacker can regain these rights by a simpel setgid(games-gid). I've been reading a lot if setxxxgid man pages, and this is the solution: #define _GNU_SOURCE /* this must be done before the first include of unistd.h */ #include <unistd.h> .... gid_t realgid = getgid(); if (setresgid(-1, realgid, realgid) != 0) { perror("Could not drop setgid privileges. Aborting."); exit(1); } Also notice the perror instead of the "fprintf(stderr, " this will tell the user why it failed (or atleast give a clue). (In reply to comment #9) > MUST > ==== [...] > * Successfully compiles and builds on at least one platform (FC-5 x86_64) > (lots of warnings though!) Odd. I don't get many warnings on FC-4, mostly a few harmless "ignoring return value" warnings. Must be from the new gcc. I'll look at fixing these up after I import the package and upgrade my desktop to FC-5. [...] > MUSTFIX > ======= > * Package should own /usr/share/ularn, just use %{_datadir}/%{name} instead > of the 3 seperate lines for the 3 files under this dir. Done. > * You currently use setegid to drop the games group, that however wont affect > the saved gid and thus an attacker can regain these rights by a simpel > setgid(games-gid). I've been reading a lot if setxxxgid man pages, and this is > the solution: [...] Fixed. New package, contains an updated -drop-setgid patch and 0wns %{_datadir}/ularn: http://www.kobold.org/~wart/fedora/ularn-1.5p4-4.src.rpm http://www.kobold.org/~wart/fedora/ularn.spec Approved, About the warnings I get lots of non void function lacking return warnings and lots of /* within comment warnings. Imported and built on -devel. Many, many thanks for the thorough review! |