Red Hat Bugzilla – Bug 524992
Review Request: toppler - platform game
Last modified: 2011-05-25 14:52:52 EDT
Spec URL: http://www.bachelot.org/fedora/SPECS/toppler.spec
SRPM URL: http://www.bachelot.org/fedora/SRPMS/toppler-1.1.3-2.fc10.src.rpm
Help a cute little green animal switch off some kind of "evil" mechanism. The
"power off switch" is hidden somewhere in high towers. On your way to the
target you need to avoid a lot of strange robots that guard the tower.
toppler.i586: E: non-standard-executable-perm /usr/bin/toppler 02755
toppler.i586: E: non-standard-dir-perm /var/games/toppler 0775
3 packages and 0 specfiles checked; 2 errors, 0 warnings
The binary is setgid games to write to the highscores file. The highscores dir needs to be group writable to allow file creation from the binary.
Have you done any security review of this package to determine whether it properly handles its setgid privileges?
All the setegid magic is handled in highscore.cc and it seems to be fine to me. games privileges are dropped very early and are only used to write to the highscores file after. I'm no expert though, and another pair of eyes couldn't hurt.
New version :
- Fix License.
- Fix buffer overflow in level editor
Spec URL: http://www.bachelot.org/fedora/SPECS/toppler.spec
SRPM URL: http://www.bachelot.org/fedora/SRPMS/toppler-1.1.3-3.fc10.src.rpm
Toppler is a reimplementation of an '80s game called Nebulus in Europe and Tower Toppler in the US.
I don't think it is acceptable for Fedora. Blocking FE-LEGAL.
There are no current US trademarks (live or dead) for "Toppler" or "Tower Toppler". This isn't terribly surprising since the original company died off in the early 1990s. Gameplay is not copyrightable/patentable, so there is no issue there. There is also no indication that the original source code was ever available, or used in the creation of this clone.
Looks fine to me, lifting FE-Legal.
Full review (md5sum, license, spec file readability, etc.) done, the package looks good. I have only one remark. I'm not completely happy with how the highscore file is handled.
My problem is that toppler does not drop its sgid rights, it changes its egid, but it keeps the rights. So if someone is able to take control of the toppler
process, he can then use the sgid games rights to get access to highscore files
of other games, which in turn could be used to inject data into other people's processes with the purpose of taking over control of said process.
I would like to see toppler patched to open the highscore file at startup
(in rw mode) as the first thing in main, and then drop the sgid rights completely.
This means the lock file will have to go, this lack of highscore file locking is a problem with many games in general, but one which is usually just ignored as in practice it never gets triggered.
Thanks for the review, Hans. I'll follow up on the highscore file issue with upstream. I have already some issues going on with the 1.1.4 version, which includes some of the patches I added to 1.1.3, but also introduces some regressions.
(In reply to comment #7)
> Full review (md5sum, license, spec file readability, etc.) done, the package
> looks good. I have only one remark. I'm not completely happy with how the
> highscore file is handled.
Just wanted to run a possible solution past you guys:
- each high score gets added to individual gamename-highscores-username.file
- game loads and merges all gamename-highscores-usernames.file
- sorts by high score, throws away any record below the h.s.table count.
- even if you haven't made the top table, it would be nice for your best score to date to be displayed ;-)
I'm still not sure about where the game, run as a user, would store such a file, possible a local .config/games/highscore file.
It would make sense that while joeb is playing, only the -joeb.file needs to be writable by joeb (which works out, since it is in his directory).
But when jenc is playing, and the game is playing as jenc, to display the hs table, the game needs to read all -username.file from all available home dirs; that would require all users on the machine to be able to read other user's high scores.
That might also trouble selinux.
(In reply to comment #9)
> Just wanted to run a possible solution past you guys:
> - each high score gets added to individual gamename-highscores-username.file
> - game loads and merges all gamename-highscores-usernames.file
> - sorts by high score, throws away any record below the h.s.table count.
> - even if you haven't made the top table, it would be nice for your best score
> to date to be displayed ;-)
user's home dirs are by default not readable by other users, so this wont work.
Xavier, putting this on needinfo until you've got something for the highscore issue (just some bookkeeping so that this drops of my bugs needing attention list).
Sure, no pb. I didn't heard back from upstream on this issue (nor on the others 1.1.4 issues either) and I hadn't had time to hack anything myself. I'll ping them again.
What is the status on this? Are you still waiting for a response from upstream wrt to highscore issue? Have you tried pinging upstream?
If the highscore issue is the only blocker and you're still interested in getting toppler into Fedora, I can write a patch to fix the highscore issue I've done so for many other games already.
The way I plan on solving the highscore issue is using 1 file shared by all users under /var/games owned by group games, make the toppler binary sgid games, and make it open the file rw as fisr thing on startup, immediately followed by dropping the sgid rights, see:
Thanks for the head up, Hans.
The new upstream maintainer quickly disappeared after he released a new version against which I reported a number of issues as well as the highscore issue. I never packaged the new version given the issues I had with it and I'm rather happy with the old one. This review then fall off my radar, but I would still like to get toppler into Fedora. I'll be glad if you can provide a patch fixing the outstanding highscore file issue.
(In reply to comment #14)
> This review then fall off my radar, but I would still
> like to get toppler into Fedora.
> I'll be glad if you can provide a patch fixing
> the outstanding highscore file issue.
I'll put this on my todo and get back to you when I get around to it.
Created attachment 495638 [details]
Patch for proper shared highscore file rights handling
First of all apologies for taking for ever to get around to this. If you take the attached patch, and drop the following 2 (which it obsoletes):
Then the dropping of sgid games handling is handled properly, further you should also create (just a "touch" is enough) /var/games/toppler/toppler.hsc in the spec file and include it with the proper rights / owner. Otherwise it will get created with the first user starting toppler as owner which is not what we want.
While working on this I also noticed the use of strncpy in highscore.cc strncpy is evil as it does not guarantee 0 termination. So I've also done a patch to replace it with snprintf, I'll attach that next.
If you can do a new version with these patches integrated, then I'll aprove this package.
Created attachment 495639 [details]
PATCH: no strncpy please
Thanks Hans, this is much appreciated :-)
toppler 1.1.5 has been released some time ago, I need to check if the issue I had with 1.1.4 have been fixed. Hopefully, your patches will still apply to 1.1.5. It could take me a bit of time to get that done as my spare time is sparse at the moment, but I'm still interested in getting this game into Fedora, so it will happen eventually. Ah, the good old C64 days :-)
Created attachment 496449 [details]
Patch for proper shared highscore file rights handling for v 1.1.5
I've rebased my patches to 1.1.5, I've also made some slight changes to the highscores patch, to keep the filenames 100% identical to what they were, so it should be acceptable upstream. I'll send them upstream and put you in the CC.
I'll also attached a rebased version of my strncpy patch. All other patches can be dropped, except for the toppler-1.1.3-move_hiscores one. Talking about that one since the toppler.hsc file must pre-exist so that it has the right owner, there is no need for a separate dir, so the proper value for pkglocalstatedir would be:
pkglocalstatedir = $(localstatedir)/games
Created attachment 496450 [details]
PATCH: no strncpy please for v 1.1.5
I test played a bit (completed the first tower) and I see no problems with v 1.1.5
Here are the updated spec and SRPM :
Looks good to me now, approved!
New Package SCM Request
Package Name: toppler
Short Description: platform game
Branches: f14 f15 el5 el6
Thanks again for the review and patches, Hans :-)
Git done (by process-git-requests).
toppler-1.1.5-1.el5.1 has been submitted as an update for Fedora EPEL 5.
toppler-1.1.5-1.fc15 has been submitted as an update for Fedora 15.
toppler-1.1.5-1.fc14 has been submitted as an update for Fedora 14.
toppler-1.1.5-1.el6 has been submitted as an update for Fedora EPEL 6.
toppler-1.1.5-1.el6 has been pushed to the Fedora EPEL 6 testing repository.
toppler-1.1.5-1.fc15 has been pushed to the Fedora 15 stable repository.
toppler-1.1.5-1.el5.1 has been pushed to the Fedora EPEL 5 stable repository.
toppler-1.1.5-1.el6 has been pushed to the Fedora EPEL 6 stable repository.
toppler-1.1.5-1.fc14 has been pushed to the Fedora 14 stable repository.