Bug 524992 - Review Request: toppler - platform game
Summary: Review Request: toppler - platform game
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Hans de Goede
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-09-22 22:00 UTC by Xavier Bachelot
Modified: 2011-05-25 18:52 UTC (History)
6 users (show)

Fixed In Version: toppler-1.1.5-1.fc14
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-05-19 04:47:45 UTC
Type: ---
Embargoed:
hdegoede: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)
Patch for proper shared highscore file rights handling (9.33 KB, patch)
2011-04-28 19:01 UTC, Hans de Goede
no flags Details | Diff
PATCH: no strncpy please (1.04 KB, patch)
2011-04-28 19:02 UTC, Hans de Goede
no flags Details | Diff
Patch for proper shared highscore file rights handling for v 1.1.5 (11.68 KB, patch)
2011-05-03 08:22 UTC, Hans de Goede
no flags Details | Diff
PATCH: no strncpy please for v 1.1.5 (1.08 KB, patch)
2011-05-03 08:22 UTC, Hans de Goede
no flags Details | Diff

Description Xavier Bachelot 2009-09-22 22:00:01 UTC
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
Description: 
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.

Comment 1 Xavier Bachelot 2009-09-22 22:06:56 UTC
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.

Comment 2 Jason Tibbitts 2009-09-23 07:03:35 UTC
Have you done any security review of this package to determine whether it properly handles its setgid privileges?

Comment 3 Xavier Bachelot 2009-09-23 07:56:00 UTC
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.

Comment 4 Xavier Bachelot 2009-10-01 22:12:28 UTC
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

Comment 5 Andrea Musuruane 2009-10-05 11:06:46 UTC
Toppler is a reimplementation of an '80s game called Nebulus in Europe and Tower Toppler in the US. 
http://en.wikipedia.org/wiki/Nebulus_%28computer_game%29

I don't think it is acceptable for Fedora. Blocking FE-LEGAL.

Comment 6 Tom "spot" Callaway 2009-10-06 14:40:22 UTC
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.

Comment 7 Hans de Goede 2009-11-21 14:09:31 UTC
Hi,

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.

Regards,

Hans

Comment 8 Xavier Bachelot 2009-11-21 22:25:19 UTC
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.

Comment 9 David Timms 2009-11-22 10:07:39 UTC
(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.

Comment 10 Hans de Goede 2009-11-22 13:23:07 UTC
(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.

Comment 11 Hans de Goede 2010-01-04 11:24:44 UTC
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).

Comment 12 Xavier Bachelot 2010-01-04 11:31:01 UTC
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.

Comment 13 Hans de Goede 2010-11-17 08:36:35 UTC
Hi Xavier,

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.

Regards,

Hans

p.s.

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:
http://fedoraproject.org/wiki/SIGs/Games/Packaging

Comment 14 Xavier Bachelot 2010-11-18 16:50:56 UTC
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.

Regards,
Xavier

Comment 15 Hans de Goede 2010-11-19 08:23:40 UTC
Hi,

(In reply to comment #14)
> This review then fall off my radar, but I would still
> like to get toppler into Fedora.

Great!

> 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.

Regards,

Hans

Comment 16 Hans de Goede 2011-04-28 19:01:35 UTC
Created attachment 495638 [details]
Patch for proper shared highscore file rights handling

Hi,

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):
toppler-1.1.3-create_hiscores_file.patch
toppler-1.1.3-fix_highscore_lockfile_creation.patch

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.

Regards,

Hans

Comment 17 Hans de Goede 2011-04-28 19:02:43 UTC
Created attachment 495639 [details]
PATCH: no strncpy please

Comment 18 Xavier Bachelot 2011-05-02 23:06:31 UTC
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 :-)

Comment 19 Hans de Goede 2011-05-03 08:22:11 UTC
Created attachment 496449 [details]
Patch for proper shared highscore file rights handling for v 1.1.5

Hi,

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

Regards,

Hans

Comment 20 Hans de Goede 2011-05-03 08:22:56 UTC
Created attachment 496450 [details]
PATCH: no strncpy please for v 1.1.5

p.s.

I test played a bit (completed the first tower) and I see no problems with v 1.1.5

Comment 22 Hans de Goede 2011-05-04 08:54:47 UTC
Looks good to me now, approved!

Comment 23 Xavier Bachelot 2011-05-04 09:09:03 UTC
New Package SCM Request
=======================
Package Name: toppler
Short Description: platform game
Owners: xavierb
Branches: f14 f15 el5 el6
InitialCC: 


Thanks again for the review and patches, Hans :-)

Comment 24 Jason Tibbitts 2011-05-05 15:29:10 UTC
Git done (by process-git-requests).

Comment 25 Fedora Update System 2011-05-06 06:57:04 UTC
toppler-1.1.5-1.el5.1 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/toppler-1.1.5-1.el5.1

Comment 26 Fedora Update System 2011-05-06 06:57:15 UTC
toppler-1.1.5-1.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/toppler-1.1.5-1.fc15

Comment 27 Fedora Update System 2011-05-06 06:57:22 UTC
toppler-1.1.5-1.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/toppler-1.1.5-1.fc14

Comment 28 Fedora Update System 2011-05-06 06:57:30 UTC
toppler-1.1.5-1.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/toppler-1.1.5-1.el6

Comment 29 Fedora Update System 2011-05-07 01:31:39 UTC
toppler-1.1.5-1.el6 has been pushed to the Fedora EPEL 6 testing repository.

Comment 30 Fedora Update System 2011-05-19 04:47:37 UTC
toppler-1.1.5-1.fc15 has been pushed to the Fedora 15 stable repository.

Comment 31 Fedora Update System 2011-05-25 16:04:11 UTC
toppler-1.1.5-1.el5.1 has been pushed to the Fedora EPEL 5 stable repository.

Comment 32 Fedora Update System 2011-05-25 16:07:53 UTC
toppler-1.1.5-1.el6 has been pushed to the Fedora EPEL 6 stable repository.

Comment 33 Fedora Update System 2011-05-25 18:52:47 UTC
toppler-1.1.5-1.fc14 has been pushed to the Fedora 14 stable repository.


Note You need to log in before you can comment on or make changes to this bug.