427974 - Form submission on AMO is vulnerable to CSRF

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Developer Pages, defect)

Description

[Wladimir Palant]

Attachment: Testcase

AMO already has CSRF protection in place. From what I can tell, it kicks in for POST requests only. Whenever somebody sends a POST request, the Referer header is checked - if it is there and not pointing to AMO, the user will be logged out. This will supposedly prevent any web sites from submitting hidden forms to AMO.

Unfortunately, this protection can be worked around. See attached testcase: it will switch to a javascript: URL first and that javascript: URL will submit the POST form then. As far as I can tell, no browser will send a Referer header in this case, and AMO will accept the request without logging out the user.

The testcase will make the statistics dashboard for Adblock Plus public on preview.addons.mozilla.org - if whoever opens that page is logged in on preview.addons.mozilla.org and has access to this statistics dashboard. Submitting other forms should work similarly, from what I can tell none of them are protected by tokens.

The easiest solution would be adding a hidden field with user's session ID to all forms - and rejecting to accept the request if this session ID doesn't match the one in the cookie. Unless breaking AMO for all users who set network.http.sendRefererHeader to 0 is acceptable of course.

Comment 1

[Reed Loden [:reed]]

Could also use a random token per form that specifies that the form is valid. This is what Bugzilla does.

Comment 2

[Wil Clouser [:clouserw]]

(In reply to comment #1)
> Could also use a random token per form that specifies that the form is valid.
> This is what Bugzilla does.
> 

Drupal as well.  Fixing this bug would fix the oft-complained about bug 408984.

Comment 3

[Chris Pollett]

Attachment: patch to fix vulnerability

This patch involves a few different components: it modify addons_html helper so that the formTag method now output a hidden input tag. formTag is deprecated in Cake; however, AMO does not use its replacement yet. A hiddenSession method was also added to addons_html to just output the hidden input tag. This is useful as there are several post'd forms in AMO that do not use the formTag method, but just use raw html <form ...>. A second component of this patch makes sure to call the hiddenSession method for each of these. The third component of the patch is to check that the variable is as expected after it is posted to the server. This is done in the AppController method checkCSRF. This method is called because it is listed in the $beforeFilter callback array. A last component of the patch is that some of the subclasses of AppController redeclare $beforeFilter, and for each of these one needs to add to its list checkCSRF otherwise it won't be called.

Comment 4

[Michael Morgan [:morgamic]]

why didnt you use beforeFilter in app_controller?

Comment 5

[Chris Pollett]

app_controller currently has no overriden beforeFilter (relies on parent). addon_controller's beforeFilter, as an example, and others override beforeFilter without calling the parent method, so each would need to be modified. This would probably take as many changes as the $beforeFilter array approach above. 

Comment 6

[Chris Pollett]

Attachment: now uses session_id rather then CAKE_SESSION_STRING, also beforeFilter to UsersController modified

In the previous patch, users_controllers $beforeFilter wasn't used so people could log-in. This left some of the actions for the controller vulnerable. This is rectified in this patch by overriding beforeFilter and calling checkCSRF in all cases except where the action is login. Since CAKE_SESSION_STRING is private, session_id() is used instead in this version.

Comment 7

[Wil Clouser [:clouserw]]

Comment on attachment 318288 [details] [diff] [review]
now uses session_id rather then CAKE_SESSION_STRING, also beforeFilter to UsersController modified

Looks good.  Fligtar had a valid thought about removing the <div> though - not sure what to do there.  At a minimum we should add a class to it so we can make it display:none.

Comment 8

[Chris Pollett]

okay I've checked it in and as requested added a class to the div tag.

Comment 9

[Fred Wenzel [:wenzel]]

The code relies on a user session, and when you're not logged in, there notice on every page that contains a form (such as the login page and the addons details page):

Notice: Undefined variable: _SESSION in /Users/fred/dev/addons/site/app/views/helpers/addons_html.php on line 73

Please handle not being logged in gracefully, for example by checking for the existance of a user session (using Cake's session component: http://manual.cakephp.org/view/173/sessions instead of directly accessing $_SESSION).

Also, please develop with PHP notices enabled so that you see these problems when you write code :-/

Comment 10

[Fred Wenzel [:wenzel]]

Also, would you be so kind and mention the SVN revision in the bug when you check something in? It makes finding possible problems much easier, because bugzilla automatically links to the changeset when you say something like "I checked it in (r1234)". Thanks!

Comment 11

[Chris Pollett]

Hey Wenzel, 

The line when I checked it in was:

fixes Bug427974 checking into 12623

which says the revision. I can put it in the format (r12623) next time though. The reason why _SESSION was being used rather than cake (cake session is used elsewhere) is that the helper won't necessarily have a path to the cake stuff.
I agree it would be better to use cake if possible. I will put up a patch shortly addressing the notices issue.

Comment 12

[Fred Wenzel [:wenzel]]

(In reply to comment #11)
> The line when I checked it in was:
> 
> fixes Bug427974 checking into 12623

We might have a misunderstanding: I would like to see this as a bug comment on the bug that it applies to (which in this case is this bug right here), but comment 8 did not contain this information?

> The reason why _SESSION was being used rather than cake (cake session is used
> elsewhere) is that the helper won't necessarily have a path to the cake stuff.
> I agree it would be better to use cake if possible. I will put up a patch
> shortly addressing the notices issue.

Okay I understand. I *think* you can use $this->controller->Session->...() in the helper, considering "Session" is one of Cake's default components and should thus be available in every controller (though I haven't triple-checked so I am not 100% sure).

But I am not strongly opposed to the use of $_SESSION, as long as it doesn't throw notices.

Thanks!

Comment 13

[Chris Pollett]

Attachment: Patch to prevent random notices of undefined _SESSION

this patch fixes the issue about notices. I tested the $this->controller->Session it is not always defined. Added a comment to the function hiddenSession warning about this.

Comment 14

[Fred Wenzel [:wenzel]]

Comment on attachment 318433 [details] [diff] [review]
Patch to prevent random notices of undefined _SESSION

Looks good.

Just fix the typos in the line...
+     * For example, wthis happens when editng settings on the statistics dashboard.

before you commit :)

Comment 15

[Chris Pollett]

Cool, thanks wenzel

fixed typo, checked into (r12645)

Comment 16

[Reed Loden [:reed]]

Shouldn't this be push-needed?

Comment 17

[Fred Wenzel [:wenzel]]

(In reply to comment #16)
> Shouldn't this be push-needed?

Yea.

Comment 18

[Stephen Donner [:stephend] Not actively reading bugmail]

I logged in to preview (where I have access to the stats dashboard), loaded the testcase in comment 0, and got the following output: "There are errors in this form. Please correct them and resubmit."

Verified FIXED

Comment 19

[Wladimir Palant]

Thanks for fixing this, the solution looks good. Still, I have some minor comments:

> +        //Check for cross-site request forgery if we have already logged-in
> +        if ($this->action != 'login') {
> +            $this->checkCSRF();		    
> +        }

Why have an exception for the login action? User should always log in via AMO's form, I don't think login forms on third-party sites are a valid use case. However:

> +        if(isset($_SESSION['User']['id'])) { // check so code doesn't generate a lot of undefined notices.
> +            $id = htmlentities($_SESSION['User']['id']);
> +        } else {
> +            $id = 0; // in this case, on the server we will give an error.
> +        }

That's pretty pointless. If the user is not logged in, then the sessionCheck field is easily predicable (actually, the "attacker" can just get its value from AMO himself). However, there is also nothing to be protected then - so the "right" thing to do would be immediately returning an empty string here. checkCSRF also has to be modified to return immediately if there is no session.

> +        return sprintf('<div><input type="hidden" name="sessionCheck" value="%s" /></div>', md5(session_id().$id.$current_epoch));

So that <div> is only there to make the validator happy? Argh...

Comment 20

[Chris Pollett]

Attachment: patch to implement Wladimir Palant's suggestions

I think both of Wlad's suggestions are reasonable, so I made a patch that implements them. The second change involves removing the special casing that existed for users_controller and just putting checkCSRF into $beforeFilter. The original reason for the special casing was that login was a posted form that might occur before a session has started. So it was thought if we didn't special case, then user's couldn't log-in. However, there is code that checks for this already in checkCSRF so the special casing is redundant. I have tested that under my patch people can still log-in and register. So applying it should cause the problems of Bug 431855. I think with or without the patch the CSRF problem has been solved, however, with the patch the code is probably tighter and less likely to have some loophole.

Comment 21

[Chris Pollett]

>>So applying it should cause the problems of Bug 431855.
So applying it should not cause the problems of Bug 431855.

Comment 22

[Wil Clouser [:clouserw]]

Comment on attachment 319582 [details] [diff] [review]
patch to implement Wladimir Palant's suggestions

Works for me.  As mentioned, the login form and register form are still special cased.

Comment 23

[Wladimir Palant]

(In reply to comment #22)
> As mentioned, the login form and register form are still special cased.

I would rather have a special case for "user sending POST data but not logged in" - current solution is fine with me as well however. Attachment 319582 [details] [diff] looks good.

Comment 24

[Chris Pollett]

checked into r12645