CSRF not on some POST forms.

[boykoc]

@ThrawnCA Continuation of previous issue. I originally tried to implement CSRF protection from the ckanext-security extension but with various issues (likely my "user error") ended up jumping to this implementation so I'm moving the conversation here.

Potential Issue:
Token is not added to all post forms. Specifically, I've found that the token doesn't get added to the user edit form (user profile). Can you confirm this on your installation by chance? If it works there, it's something I've done.

So far I've found intercept_csrf() is successfully called when loading the user edit form but it appears that line 61 (anti_csrf_render_jinja2) is not called. But when loading another form (e.g. the dataset edit form) it is called as expected. This happens before apply_token() is called, which is where I suspected the issue to be at first.

update:

• organization pages seem fine (e.g. adding, editing, adding users, editing users)
• dataset pages seem fine
• user login, user register, user profile editing, admin config, admin purge is "skipped"

[boykoc]

I may be going down the wrong rabbit hole but it seems that render_jinja2() is only called when _pylons_prepare_renderer() is called here

This led me down to seeing why it was trying to treat it like a pylons request vs. flask as some of these have been migrated.

I think is_flask_request() is giving a false positive.

when you make a call to the dataset edit page, render() does a check to see if it's a flask request or not (e.g. if not is_flask_request():). This evaluates to True for some pages such as the dataset page, then triggers the render_jinja2() call. But, in this case, when you look at the return value of is_flask_request():, instead of False, you actually get

Traceback (most recent call last):
   File "/usr/lib/python2.7/logging/__init__.py", line 868, in emit
     msg = self.format(record)
   File "/usr/lib/python2.7/logging/__init__.py", line 741, in format
     return fmt.format(record)
   File "/usr/lib/python2.7/logging/__init__.py", line 465, in format
     record.message = record.getMessage()
   File "/usr/lib/python2.7/logging/__init__.py", line 325, in getMessage
     msg = str(self.msg)
   File "/usr/lib/ckan/default/lib/python2.7/site-packages/werkzeug/local.py", line 366, in <lambda>
     __str__ = lambda x: str(x._get_current_object())
   File "/usr/lib/ckan/default/lib/python2.7/site-packages/werkzeug/local.py", line 306, in _get_current_object
     return self.__local()
   File "/usr/lib/ckan/default/lib/python2.7/site-packages/flask/globals.py", line 37, in _lookup_req_object
     raise RuntimeError(_request_ctx_err_msg)
 RuntimeError: Working outside of request context.

 This typically means that you attempted to use functionality that needed
 an active HTTP request.  Consult the documentation on testing for
 information about how to avoid this problem.
 Logged from file base.py, line 124

so the if statement evaluates to True but the actual value is an error.

In the user profile edit page, the if statement evaluates to False because the return value is True and then render_jinja2() is never called and that means the CSRF token is never inserted.

But, this may be the wrong rabbit-hole I've went down.

[boykoc]

This diff will get the token added to all POST forms by going up to render() instead of render_jinja2().

But, base.BaseController.__before__ doesn't kick in so the token isn't validated for these requests. I think because new views don't rely on BaseController class and instead of the views. As part of the Flask migration alot of this logic is moved to ckan/views/__init__.py and using before_request in the views. I'll try to get to this tomorrow.

Current Pylons controllers share common logic that is executed before and after each request. This is done on the before, call and after methods of the ckan.lib.base.BaseController class, from which all controllers are extended. This includes identifying the user, handling i18n, setting CORS headers, etc. All this logic will be moved to ckan/views/init.py and reused by the Pylons BaseController and Flask's before_request and after_request handlers. source

This also does not fix the is_flask_request() issue mentioned above.

--- a/anti_csrf.py
+++ b/anti_csrf.py
@@ -20,6 +20,7 @@ LOG = getLogger(__name__)

 RAW_RENDER_JINJA = base.render_jinja2
 RAW_BEFORE = base.BaseController.__before__
+RAW_RENDER = base.render

 """ Used as the cookie name and input field name.
 """
@@ -54,10 +55,8 @@ def is_logged_in():
     """
     return _get_user()

-def anti_csrf_render_jinja2(template_name, extra_vars=None):
-    """ Wrap the core page-rendering function and inject tokens into HTML where appropriate.
-    """
-    html = apply_token(RAW_RENDER_JINJA(template_name, extra_vars))
+def anti_csrf_render(template_name, extra_vars=None, *pargs, **kwargs):
+    html = apply_token(RAW_RENDER(template_name, extra_vars, *pargs, **kwargs))
     return html

 def apply_token(html):
@@ -279,5 +278,5 @@ def _get_post_token():
 def intercept_csrf():
     """ Monkey-patch the core rendering methods to apply our CSRF tokens.
     """
-    base.render_jinja2 = anti_csrf_render_jinja2
+    base.render = anti_csrf_render
     base.BaseController.__before__ = anti_csrf_before

[ThrawnCA]

Thanks for looking into this. I may not be able to do much on it over the next few days, but I'll keep it on our radar. There should be time to work on it before Christmas.

[boykoc]

The is_flask_request() is not an issue by setting streaming=False as per issue #5118

[boykoc]

FYI, I've got a hacked together version that kind of works now. I'll try to clean it up and share what I've done as a work in progress over the next few days.

[boykoc]

After digging into it and trying numerous different things I ended up creating a Flask based CSRF and using this CSRF for Pylons based requests. Then in the future I can just remove the Pylons based on once CKAN has fully migrated to Flask.

I modified the ckanext-security CSRF implementation to handle flask, and using this CSRF implementation for Pylons (this one didn't require anything else and was a little simpler to setup I found).

Some issues I ran into and general info:

• it was pretty easy to get the token to be added to all responses using the above snippet, that way anything rendered was getting the token added instead of just the Pylons based responses.
• Getting the validating and getting the response object was much more difficult while staying within the current implementation, basically, Flask doesn't have a global response object like Pylons does, so there are only certain ways to get and modify it.
• I tried the middleware approach from ckanext-security, I was able to get it working to add the token to all requests by adding it as typical middleware using IMiddleware (much nicer than fiddling with pylons_app.py). But validating the token failed. Pylons requests were getting overlooked and failing, and the code within was written for Pylons/webob requests, not flask. So I had to start trying to hack it apart and check what type of request it was which wasn't clean.
• I ended up realizing both are just very different (flask and pylons) and since CKAN is migrating to Flask maybe keeping it separate would work better anyway.

[boykoc]

hopefully tomorrow I can share what i have in a easily reproducible way. It's now been tested and held up which is a bonus.

[boykoc]

Alright, I wasn't sure how best to capture the changes and explain my approach so I put it in a post over here. I didn't want to try and do a PR against this repo as it may not be a fix you'd like to implement, or you may want to go a different direction.

Basics:

1. Used this CSRF setup for Pylons requests
   1. changed TOKEN_FIELD_NAME value
2. Modified the ckanext-security CSRF setup for Flask requests
   1. work with flask,
   2. moved to before_app_request and after_app_request instead of middleware

[ThrawnCA]

Thanks, @boykoc !

We're definitely interested in this, and I'll assemble a pull request soon. ckanext-security might take a bit longer; they haven't as yet implemented our move to HMAC-based tokens, so their filter code is a bit different.

Can you advise the simplest way to reproduce the problem, so that our testing team can be sure when it's resolved?

[boykoc]

Curious your timeline on upgrading to python 3 and ckan 2.9? Looks like pylons will finally be removed in PR #5712 and CSRF can be done just on Flask side.

Super late reply on your question for simplest way to reproduce:

1. Authenticate to CKAN UI
2. Create a CSRF http post request on users behalf editing some profile content (i.e. About text)
   One way is to create a local html page that has a basic form with relevant inputs and the action set to https://domain.com/user/edit/user_name and submit the form/make the request.
3. Go to users profile to verify content was changed

From my review and testing it seemed pylons controlled pages were protected by your or ckanext-security implementation. But Anything that had been migrated to flask was not; request "hooks" are different in these frameworks.

My implementation described in the above link to the post has lots of extra stuff I know. I didn't want to hack apart what you all had done but that meant some unused pieces were left lying around.

[ThrawnCA]

@boykoc I think I have a better handle on this now. You're right; the current implementation wouldn't protect anything implemented as a Flask blueprint, and I can see that the user edit page has no form token.

I'm looking through the code in your blog post now, with a view to updating this. I'll offer the changes to ckanext-security when I'm done.