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!