738000 - enable CSRF protection in existing balrog code

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P3)

Description

[[e-mail] (:bhearsum)]

The security team says that we need to have CSRF protection for Balrog, even though it's behind a VPN. There's already some code landed that doesn't have it, and bug 672918 will add a bunch more. We should add csrf protection for existing code as soon as we can, and make sure all new code uses it, too.

Comment 1

[[e-mail] (:bhearsum)]

http://packages.python.org/Flask-WTF/ seems to say that we only need to put it on our forms, and then make sure it's passed with AJAX requests.

Comment 3

[Erick Dransch [:edransch]]

We will need to update the csrf token on the admin-ui pages when the form is submitted.
Currently we can only do one 'submit' per reload of the page. If we make a mistake on a form, or want to do multiple operations, we need to reload.

We shouldn't need to reload for each form submission.

Comment 4

[[e-mail] (:bhearsum)]

Catlee suggested that we could return a new csrf token in the HTTP headers of PUT/POST/DELETE responses. Eg, X-CSRF-TOKEN or something. That seems a bit nicer than mixing it in with the real data.

Comment 5

[[e-mail] (:bhearsum)]

I looked into this further today and found that the latest versions of WTForms and Flask-WTF don't require csrf token regeneration with every request - they use an hmac digest that's based on the secret key to generate the token, and set a relatively short expiry time. This seems like a much easier way to do things...I'm going to try upgrading us.

Comment 6

[[e-mail] (:bhearsum)]

Attachment: client csrf support

I think this patch is mostly straightforward. I did a little bit of refactoring, to make API a proper base class, and taught it how to get CSRF tokens and data versions from headers. I also had to use a session because the WTForms CSRF protection puts the hmac key in it.

Comment 7

[[e-mail] (:bhearsum)]

Attachment: upgrade flask, wtforms, flask-wtf; fix up server side csrf implementation

I'm not sure I'm completely happy with this, as it muddies are usage of csrf_token and data_version a bit, so I'm curious what you think.

For templates/UI, we're still putting hidden csrf_token and data_version fields onto the pages. For API endpoints (eg, /releases/:release/builds/:platform/:locale), we return them in X-CSRF-Token and X-Data-Version headers. I was going to put the data version in the response body instead, but most of those methods don't have a nice place to put it, because they return parts or all of a blob that aren't wrapped with a variable name. Eg, returning {'complete': {...}, 'data_version': 1} seems a bit awkward. However, I'm not sure it makes sense to put data version in a header either...

What do you think?

Comment 8

[Rail Aliiev [:rail]]

Comment on attachment 628901 [details] [diff] [review]
client csrf support

Review of attachment 628901 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/python/balrog/client/api.py
@@ +13,5 @@
>  
> +def is_csrf_token_expired(token):
> +    from datetime import datetime
> +    expiry = token.split('##')[0]
> +    if expiry <= datetime.now().strftime('%Y%m%d%H%M%S'):

* This function isn't used anywhere. Do you need it?
* split and strftime may raise exceptions, be careful :)

Comment 9

[[e-mail] (:bhearsum)]

(In reply to Rail Aliiev [:rail] from comment #8)
> Comment on attachment 628901 [details] [diff] [review]
> client csrf support
> 
> Review of attachment 628901 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: lib/python/balrog/client/api.py
> @@ +13,5 @@
> >  
> > +def is_csrf_token_expired(token):
> > +    from datetime import datetime
> > +    expiry = token.split('##')[0]
> > +    if expiry <= datetime.now().strftime('%Y%m%d%H%M%S'):
> 
> * This function isn't used anywhere. Do you need it?
> * split and strftime may raise exceptions, be careful :)

Whoops, I forgot to re-add expiration checking after I refactored!

Comment 10

[[e-mail] (:bhearsum)]

Attachment: check expiration of csrf token

Comment 11

[Chris AtLee [:catlee]]

Comment on attachment 628903 [details] [diff] [review]
upgrade flask, wtforms, flask-wtf; fix up server side csrf implementation

Review of attachment 628903 [details] [diff] [review]:
-----------------------------------------------------------------

looks good to me!

::: auslib/web/views/__init__.py
@@ +3,3 @@
>  from auslib.web.views.permissions import *
>  from auslib.web.views.releases import *
>  from auslib.web.views.rules import *

import *'s are bad...how hard would it be to import views explicitly?

::: requirements/prod.txt
@@ +1,4 @@
> +flask==0.8
> +Werkzeug==0.8.3
> +wtforms==1.0.1
> +flask-wtf==0.6

are these required even though they're available in the vendor lib?

Comment 12

[[e-mail] (:bhearsum)]

Attachment: move routing to avoid import *

(In reply to Chris AtLee [:catlee] from comment #11)
> Comment on attachment 628903 [details] [diff] [review]
> upgrade flask, wtforms, flask-wtf; fix up server side csrf implementation
> 
> Review of attachment 628903 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks good to me!
> 
> ::: auslib/web/views/__init__.py
> @@ +3,3 @@
> >  from auslib.web.views.permissions import *
> >  from auslib.web.views.releases import *
> >  from auslib.web.views.rules import *
> 
> import *'s are bad...how hard would it be to import views explicitly?

Easy enough to fix.

> ::: requirements/prod.txt
> @@ +1,4 @@
> > +flask==0.8
> > +Werkzeug==0.8.3
> > +wtforms==1.0.1
> > +flask-wtf==0.6
> 
> are these required even though they're available in the vendor lib?

I'm using the model described here: https://github.com/mozilla/zamboni-lib. I believe the idea is that prod.txt contains the list of libraries in your vendor lib. You use this file when you build it, eg:
pip install -I --install-option="--home=`pwd`/vendor" --src='vendor/src' -r requirements/prod.txt

Comment 13

[[e-mail] (:bhearsum)]

Comment on attachment 630192 [details] [diff] [review]
move routing to avoid import *

Jenkins run is here: https://jenkins.mozilla.org/job/Balrog/90/
I restarted the apache daemons on the web heads, to work around bug 745890.

Comment 14

[[e-mail] (:bhearsum)]

Attachment: add csrf/data_version to /releases/:name endpoint

I missed an edge case in my original patch. It didn't work in the case that the release existed but the platform didn't. It would receive a 404 from the initial HEAD request, and then send a HEAD to /csrf_token. However, because it never received a data version it eventually got an HTTP 400 when trying to PUT because it was missing.

This patch adds those headers to /releases/:name, so we can retrieve them in that case.

Comment 15

[[e-mail] (:bhearsum)]

Attachment: use a different url to retrieve data_version/csrf_token

This patch makes it possible to retrieve the csrf token and data version from a different URL than we're submitting to. We need this to support the scenario described in the previous comment.

Comment 16

[Chris AtLee [:catlee]]

Comment on attachment 630285 [details] [diff] [review]
add csrf/data_version to /releases/:name endpoint

Review of attachment 630285 [details] [diff] [review]:
-----------------------------------------------------------------

::: admin.py
@@ +17,5 @@
>      )
>  
>      parser.add_option("-d", "--db", dest="db", help="database to use, relative to inputdir")
>      parser.add_option("-p", "--port", dest="port", type="int", help="port for server")
> +    parser.add_option("--host", dest="host", default='127.0.0.1', help="host for server")

this is the address the server will be listening on, right? e.g. you could have a public ip or 0.0.0.0 to listen on all interfaces?

if so, can you adjust the help string to indicate that more clearly?

Comment 17

[[e-mail] (:bhearsum)]

Comment on attachment 630286 [details] [diff] [review]
use a different url to retrieve data_version/csrf_token

Once the Balrog patch gets reviewed/landed we should have updates on Linux/Mac every day again.

Comment 18

[[e-mail] (:bhearsum)]

Comment on attachment 630285 [details] [diff] [review]
add csrf/data_version to /releases/:name endpoint

I fixed up the help string and landed this: https://jenkins.mozilla.org/job/Balrog/91/

After the webheads pick it up I'll restart them to pick up the changes, and then resubmit any failed mozilla-central submissions from this morning.

Comment 19

[[e-mail] (:bhearsum)]

Okay, things seem to be working now. Sample update URLs:
https://aus4-dev.allizom.org/update/3/Firefox/15.0a1/20120505030510/WINNT_x86-msvc/en-US/nightly/Linux%203.2.0-24-generic%20%28GTK%202.24.10%29/default/default/update.xml?force=1
https://aus4-dev.allizom.org/update/3/Firefox/15.0a1/20120505030510/Linux_x86_64-gcc3/en-US/nightly/Linux%203.2.0-24-generic%20%28GTK%202.24.10%29/default/default/update.xml?force=1
https://aus4-dev.allizom.org/update/3/Firefox/15.0a1/20120505030510/Linux_x86-gcc3/en-US/nightly/Linux%203.2.0-24-generic%20%28GTK%202.24.10%29/default/default/update.xml?force=1
https://aus4-dev.allizom.org/update/3/Firefox/15.0a1/20120505030510/Darwin_x86-gcc3-u-i386-x86_64/en-US/nightly/Linux%203.2.0-24-generic%20%28GTK%202.24.10%29/default/default/update.xml?force=1

I had to rerun the balrog client by hand for a few of them because my patches didn't land in time to be picked up. Subsequent nightlies should work from the get-go, though. We even have working Windows updates now, because they moved to 64-bit machines. (Previously they were broken because the 32-bit windows build machines weren't using a new enough Python).