Bugzilla – Bug 5193
CSRF security flaws return
Last modified: 2008-12-18 11:12:53 UTC
As discussed in the Developers forum http://forums.slimdevices.com/showthread.php?t=35317 cross-site request forgery (CSRF) flaws have returned to SlimServer. I believe there are a number of factors at play here: - new and modified commands since CSRF protection was put in place in 5.x that aren't protected - SlimServer now accepts POST requests and the CSRF protection in 6.5.x and 7.x only works with GET - the CSRF countermeasures appear unable to protect against attacks that use simple HTTP URL encoding, e.g. http://localhost:9000/status.html?p0=resc%61n as a way of bypassing 6.5.3's forbidding "p0=rescan" pattern and triggering a rescan. These flaws are bad already, but would be especially critical in SlimServer 7.x if SlimServer 7.x includes the often discussed web interface for installing and upgrading plugins. I've developed improved protection mechanisms and have notes and patches for SlimServer 6.5.x and 7.x here: http://www.tux.org/~peterw/slim/csrf/ I believe the basic protection *mechanism* is fairly sound (and the APIs easy for developers to use), but I have not taken the time to test all the "interesting" actions a user could undertake in the web UI, nor to verify that my 6.5 patch's use of my new APIs is sufficient. Here are notes on usage for the new APIs: To protect a command you've registered with addDispatch, use Slim::Web::HTTP::protectCommand, e.g. addDispatch(['power', '_newvalue'], [1, 0, 0, \&Slim::Control::Commands::powerCommand]); Slim::Web::HTTP::protectCommand('power'); Once you've done that, any user with CSRF Protection at Medium or High will need a "cauth" token or Referer header to call your command via direct HTTP. Web pages: To protect a web page, register it with a routine that uses protectURI, e.g. sub page { Slim::Web::HTTP::protectURI('settings/server/basic.html'); } When registering a web page by name, use protectName, e.g. sub name { return Slim::Web::HTTP::protectName('BASIC_SERVER_SETTINGS'); } Be sure your <form> has code like the following to insert an anti-forgery token. Note that the standard settings page template includes this, so you probably don't need to add this code: <input type="hidden" name="pageAntiCSRFToken" value="[% pageAntiCSRFToken %]"> Once you've done that, any user with CSRF Protection at Medium or High will need that anti-CSRF token, "cauth" token, or Referer header to submit your form.
Peter do you know if these changes work with Slimserver 7? If so it seems to me that we should consider checking them in for that release at least, if not 6.5.x.
Hi, Christopher. Sorry for the delay... I've been really busy over here. I do have patches for version 7.x on http://www.tux.org/~peterw/slim/csrf/ but they're not well-tested. I believe some of the changes I made in the "v7b" patches defintely broke some standard plugins, but aside from seeing some startup errors, I have not investigated further, nor do I expect to have time to. I'm using my "v8" patches on my Slimserver 6.5.2 host internally. Can't say I've stress-tested it a whole lot, but it seems to be working well. Thanks.
Just checking in on the status of this ticket... I just updated my 7.x SVN snapshot to revision 12952 and it's still got the old (broken) code. Thanks, Peter
I did some more work this morning on the train, and have posted a "v9" patch for Slimserver 7.x against SVN 13138. http://www.tux.org/~peterw/slim/csrf/ This new patch fixes the plugin startup error messages mentioned in Comment #2. I've tested it very lightly (what do you expect for rush hour commuter train work?) with the "new Default" skin and have not encountered problems. Also, Andy G had voiced concerns about overhead, so I briefly added some code to time the impact -- it's about 0.001 seconds per HTTP request when CSRF protections are enabled (there's no other runtime impact), and that's on a PC running at a lowly 600 MHz. This appears to be about the same impact as the faulty countermeasures that are currently in the SVN repo -- I timed my new patch at an average of 0.000969 seconds per hit, and the current 7.x SVN code at 0.001004 per hit, so this new, better code might actually be a little bit faster. With CSRF Protection Level at None, the impact is an rder of magnitude less, about 0.0001 seconds per request at 600 MHz.
Change 13149 - please test...
Thanks, Michael. As I mentioned in the forums, I found a bug with v9 and playlistXtracksCommand, which needs to know not to use the anti-CSRF token in a SQL query. Relevant v9 -> v10 diff: Index: Slim/Control/Commands.pm =================================================================== --- Slim/Control/Commands.pm (revision 13138) +++ Slim/Control/Commands.pm (working copy) @@ -2459,6 +2459,11 @@ while (my ($key, $value) = each %{$terms}) { + # ignore anti-CSRF token + if ($key eq 'pageAntiCSRFToken') { + next; + } + # Bug: 4063 - don't enforce contributor.role when coming from # the web UI's search. if ($key eq 'contributor.role') {
Peter did you get a chance to check Michael's checkin? Does your last comment indicate that there is an additional problem? Thanks.
Hi, Chris. I think #6 is OK now, but in the last couple days I've found more core modules that seem to need protection for their page() and name() subs, as outlined in the initial ticket text (i.e., protecting them should be simple). All the following settings modules are not currently using protectName() and protectURI(), and I believe they probably should be. Slim/Plugin/MusicMagic/Settings.pm Slim/Plugin/RadioIO/Settings.pm Slim/Plugin/DateTime/Settings.pm Slim/Plugin/Podcast/Settings.pm Slim/Plugin/Favorites/Settings.pm Slim/Plugin/xPL/Settings.pm Slim/Plugin/RSSNews/Settings.pm Slim/Plugin/CLI/Settings.pm Slim/Plugin/iTunes/Settings.pm Slim/Plugin/RS232/Settings.pm Slim/Plugin/Rescan/Settings.pm Slim/Plugin/RadioTime/Settings.pm Slim/Plugin/InfoBrowser/Settings.pm
Created attachment 2333 [details] plugin settings all plugin settings, including the new audioscrobbler plugin. radioIO no longer has a settings page, so that's not in here.
Thanks kdf - change 14284
since we have the change, lets mark fixed. It can be reopened later if there are any issues from anyone testing the fix.
This bug is being closed since it was resolved for a version which is now released! Please download the new version of SqueezeCenter (formerly SlimServer) at http://www.slimdevices.com/su_downloads.html If you are still seeing this bug, please re-open it and we will consider it for a future release.