[pLog-svn] r7015 - plog/branches/lifetype-1.2/js/ui

[Jon Daley]

On Sun, 1 Aug 2010, Reto Hugi wrote:
> I didn't actually test the code, but just read your svn commits, so
> maybe I've overlooked or misinterpreted stuff...
 	No, I think you've gotten it.

> At least it was one quick way to ad the protection through the plugin
> event. But parsing the the rendered page is expensive and thus it's
> probably not an option for high traffic sites.
 	It is only on the admin side, so the high traffic has to be for 
blog owners, so that limits the load.  I don't have access to any high 
traffic sites.  The only one I've worked on for someone is 
christianblogsites.com.  I just checked his version, and he is still 
running the 1.2.9-dev version I installed for him a while ago, and he has 
customized the admin-side a lot, which is why he doesn't upgrade often.
 	If I changed the event from LOGIN_SUCCESS to "USER_LOADED 
from=>Login" (something like that), I think I could work on any 1.2.x 
version.  I noticed that event being fired, but I didn't play with it.

> Nevertheless I think if we implement CSRF protection like that, it's a 
> must that the plugin comes pre-installed.
 	Yes, I agree.

> ...and excluding actions that don't need protection would of course be
> safer, but you knew that.
 	Right, I believe I have done that.  I suppose there are some 
searches that are done via POST, but I don't think we should look to 
exclude some POST events.  And I believe I've filtered out the GETs that 
don't need to be protected.

>> 1. Plugins.  Not sure how to fix this unless we protect all plugin input,
>> using the configuration key array.  Or if I switch to be a whitelist,
>> rather than a blacklist, but that makes the URL parsing harder
>
> Good point. That's probably the limitation of adding the protection on
> the rendered HTML. Else you could have provided an API for plugin authors.
 	Yes, although getting people to change their plugins is about the 
hardest thing to do...  :)

>> 2. Remote URIs shouldn't ever have a CSRF token added.  I've thought about
>> this a little, and I think it can be done with regexps, all fully
>> qualified URIs need to be checked to see if they are really local or not.
>> And then all relative URLs should be protected.  I don't think we need to
>> worry about forms that post externally.  That doesn't exist, right?
>
> Correct, I think we don't have to worry about external URLs.
 	Right, but I need to "worry" about them in terms of checking to 
make sure that the links don't get parsed.  I wonder how hard it would be 
to edit the links in the templates (though some links are built in the 
action/view code) and add a "class='csrf'" tag to the appropriate links.
 	My other thought is to fix this in 2.0 and make the appropriate 
links POSTs - a lot of the things are ajax anyway, so they are already 
changed to POST, or easily could be changed.

> Please have a look at the /class/data/nonce.class.php (not the
> randomizer - yours is better)
 	Thanks.  I did look at your code when you first mentioned it, but 
then after my reading of CSRF protection stuff, and all the various ways 
people do CSRF protection incorrectly, made up my own randomizer.  One 
thing that lots of security people don't take into account is that lots of 
their methods only work on closed-source apps.  I thought about adding a 
configuration option with a secret salt, but then we'd need a 
configuration page.  I think in an upcoming version, I am going to add a 
salt like WP does, where it is generated at install time, and then used in 
various places to hash items.

> and at the class/action/admin/adminaction.class.php (Line 174). The 
> implementation of the csrf-branche defers to yours in that the nonce 
> changes with every request which effectively protects from 
> replay-attacks. Of course this would stop support for multiple tabs in 
> one browser.
 	One thing I didn't like about your code was that you modified each 
action.  If the code could go in adminaction or something, then I'd be 
happier.  I guess the adminaction could check my table of nonce-required 
actions, and decide whether to validate or not there.

> Overall I think your approach makes a good trade off between effort and
> protection. I'll definitely upgrade but probably not the next 2 months...
 	Why not - it's easy to upgrade this.  You just need two files 
(that you aren't likely to have modified), plus the plugin - should take 4 
minutes to upgrade.  :)


-- 
Jon Daley
http://jon.limedaley.com
~~
The first duty of love is to listen.
-- Paul Tillich