Help me understand the CSRF protection - Update: it doesn't work properly!

[ThrawnCA]

I've taken a look at the CSRF protection in middleware.py, and I can't see how it actually protects against CSRF. It doesn't seem to be matching the cookie token against the contents of the request, so how would it stop an attack?

I'm going to see if I can get an instance set up and properly test it, but if someone can explain, that would be good.

[ThrawnCA]

OK, I have now been able to test the CSRF filter, and it's very problematic. Despite all the fussing about cookies, it is in fact nothing more than a referrer check.

• If the end user blocks the Referer header, then the site becomes unusable;
• If the end user spoofs the Referer header to be the value of the target page (which could be done deliberately for privacy reasons, or could be because an attacker found an exploit that lets him/her add headers), then the filter is ineffective;
• The token cookie achieves nothing, since the whole basis of CSRF attacks is that the browser will automatically attach cookies to cross-site requests, including malicious requests.

[camfindlay]

@thrawnca thanks for seeing this code inspection through. We’re keen to work through with you on a fix for this. Ideally we’d like to see CSRF implemented in CKAN core but we’ll do what we can here in the meantime.

[camfindlay]

Have you got a preferred implementation approach if we are to patch this issue @thrawnca?

[ThrawnCA]

Well, the key issue is that the token cookie needs to be compared to some part of the request. Otherwise, the token's presence doesn't prove that the request is legitimate.

I'm one of the maintainers of the Queensland Government CKAN extension, which uses a CSRF filter based on the OWASP Double Submit Cookie pattern. That has its own drawbacks, but we chose it because it's stateless and could be implemented with a very small footprint. The filter code is visible at https://github.com/qld-gov-au/ckan-ex-qgov/blob/master/ckanext/qgov/common/anti_csrf.py

Essentially:

• When a page is loaded, in addition to setting the token cookie in the response, the filter automatically injects the token as a hidden field in all POST forms
• When receiving a POST, the filter checks that the HTTP parameter token matches the cookie token. Since the attacker cannot read the cookie, s/he cannot provide a valid HTTP parameter token.

Ideally, the limitations of the Double Submit Cookie approach could be worked around by using aspects of the Encryption based Token pattern (but continuing to use cookies instead of relying on AJAX). If tokens were generated by encrypting user ID, timestamp, and nonce using the beaker session secret or other server-side key, then it would no longer be possible, for example, for an attacker to set legitimate-seeming cookies from alternative subdomains. (They still need to be compared to some aspect of the request, to prove that they originated from a HTML form issued by the server.)

[camfindlay]

@ThrawnCA thanks very much for the code example also. We're keen in the short term to look at adopting the Double Submit Cookie pattern into this module... raises the question as to whether we can co-maintain that CSRF solution here as a more generic Security/CSRF extension for CKAN? Happy to collab on this if you are?

Longer term we are keen to see how much security enhancements we can get baked into CKAN core (at which time we might opt to tackle elements of the Encryption based Token pattern you suggest.

So seems like we have a bit of a roadmap forward here 👍

[ThrawnCA]

We're currently migrating our CKAN instances to new hosting, but after that, there is some interest at this end in making use of your extension, so there's probably room for collaboration at that point.

There are a few aspects of our different filter approaches that are worth comparing and contrasting:

• Caching the token in Redis provides protection against an attacker writing cookies via an alternative subdomain, at the cost of having to maintain server-side state.

I suppose that if the Redis interaction could be adjusted to more easily coexist with CKAN core (and the Harvest extension if applicable), that might not be a significant overhead. Especially if the extension could default to reusing the CKAN core Redis URL instead of needing it to be repeated in the config file. Note that our Redis instance does not support multiple databases, so we'd rather use a unique prefix on the same database.

On the other hand, encrypted tokens would resolve this without needing server-side state.

• ckanext-security generates a new token on every request and immediately invalidates the old one, while ckanext-qgov generates a new token only if logged in (which I think is a low-hanging fruit for ckanext-security) and only if the existing one is over ten minutes old, plus it still accepts older tokens. The token-per-request model theoretically makes it harder to use a stolen token, but not impossible. On the other hand, it means that if two forms are opened in two tabs, the second one will invalidate the first.

Using encrypted tokens containing a nonce and timestamp, instead of random ones, would give better options here. Suppose that we expect people to ordinarily take no more than 10 minutes to fill in a form. We could set a hard limit, eg allow tokens up to 30 minutes old, but at the same time, attempt to set a new token if the current token has less than ten minutes left. That way, we ensure that people have at least ten minutes to use their token, but we don't churn out tokens unnecessarily before that point. There would also be no need to invalidate old tokens on page load, so multiple tabs would work normally.

[anotheredward]

Hi @ThrawnCA , thanks for raising this issue.

On using the same redis instance:

• The reason we use a seperate redis instance is that the CSRF token redis instance is configured to delete the Least-recently-used token out of a fixed space sized of memory. It's Redis's official recommendation to run a seperate instance in the case that the configuration is this different. If we run them in the same instance, then we'd be concerned about harvested items disappearing for example. This is important as this approach creates a token for every single public user as well, so it fills up very quickly. Previously we had to run an independent Memcached instance (which is fixed-sized, and LRU by default), but running two instances of Redis seemed simpler.

Now there's no question that your implementation is an improvement on the one we've inherited, but we'd be keen on working together to understand it a bit better and potentially improve it. Thanks for giving us the link to the OWASP cheatsheet, that gives us a lot more context on your approach.

Questions about the current implementation:

• It looks as though you're using auth_tkt instead of beaker, I was under the impression that auth_tkt doesn't invalidate sessions, we had some worked queued up to make Beaker the default here: https://github.com/ckan/ckan/issues/4002 Would you be keen to move across to Beaker?

• We previously looked at methods that would require having a hidden field on each page, but were concerned that we couldn't cover the potential range of pages with form actions that plugins might introduce. How happy are you that the method you've used to inject that hidden field doesn't cause other issues?

• One weakness that the OWASP documented about the double-cookie solution was lack of Login protection. Is that something you'd look to address in potentially changing approaches? Do you think that's a valid concern?

• I did note that the Double-cookie approach wasn't one of the "Primary Defence Techniques" but a "Defence In Depth Technique", is that another reason why you're potentially looking to change approaches?

Notes:

• Multiple tabs currently don't work for form submission, while we'd prefer they did, we're ok with having a single valid token per user at the current point in time.

• I'm not confident enough in my understanding of encryption to attempt encrypted tokens, but happy to work with you if you are :)

After reading through and thinking on this issue a bit, I feel like using an implementation that caches the token on the server-side might be a valid final solution, but adopting your current, and most importantly working, solution would be a good intermediate step. Thoughts?

[ThrawnCA]

Hi @anotheredward

This is important as this approach creates a token for every single public user as well, so it fills up very quickly.

Why do you need to create tokens for all public users? Do you allow them to take actions with side effects without registration? On our instances we haven't, so we only generate or require tokens when logged in.

Edit: Ah, is that to prevent login CSRF? It should be sufficient to generate a token when an unauthenticated user actually requests the login form, and then requiring a token to log in. Thanks for bringing our attention to that; it's something we could improve.

The reason we use a seperate redis instance is that the CSRF token redis instance is configured to delete the Least-recently-used token out of a fixed space sized of memory.

Your explanation does make sense. I'd rather ditch server-side state altogether, if possible.

Would you be keen to move across to Beaker?

Actually I hadn't realised that that move was a thing. If it's just a different login cookie name, it should be easy enough to support both, yes? Our only interaction with auth_tkt is to check whether someone is logged in or not.

How happy are you that the method you've used to inject that hidden field doesn't cause other issues?

Ah. Actually I know for a fact that it does, in at least one remaining case that hasn't become enough of a priority to fix yet: deleting a member from an organisation. The reason for this is a JavaScript confirmation dialog that intercepts the link and assembles an empty POST aimed at the link target. Weird behaviour, if you ask me, but that's what it does.

You might notice the special CONFIRM_LINK handling; that is a workaround for cases where the link has no query string. We haven't yet developed a workaround for the one existing case where it does have a query string, although it shouldn't be especially difficult.

But in general, the HTML rewrite has been quite reliable. If you have suggestions to refine the POST_FORM regex, I'd be happy to consider them.

One weakness that the OWASP documented about the double-cookie solution was lack of Login protection.

Addressed above; I'm pretty sure we could defend against this simply by adjusting the conditions for when to set and expect tokens.

I did note that the Double-cookie approach wasn't one of the "Primary Defence Techniques" but a "Defence In Depth Technique", is that another reason why your potentially looking to change approaches?

Actually I came across your extension by accident :). I was looking up information about CSRF on the CKAN API (which, btw, is a serious problem), and my search terms found ckanext-security, which looked interesting because of its similarity to our own work.

We do, in fact, use Strict Transport Security, although not with subdomains, and it should be difficult for an attacker to register a qld.gov.au subdomain, so Double Submit Cookie is relatively safe; however, it's true that it has weaknesses. Encrypted cookies should cover most of them.

We actually have a Referer check, too, though implemented in Apache:

RewriteCond %{REQUEST_METHOD} !(GET|HEAD|OPTIONS)
RewriteCond %{REQUEST_URI} !/api\b [NC]
RewriteCond %{HTTP_REFERER} !https://data.qld.gov.au/ [NC]
RewriteCond %{HTTP:Origin} !https://data.qld.gov.au$ [NC]
RewriteRule / - [F]

I'm not confident enough in my understanding of encryption to attempt encrypted tokens, but happy to work with you if you are :)

Actually, we wouldn't be storing sensitive data, we just need to stop any kind of tampering or forgery. So a signature should be sufficient. The cookie value would end up something like:

admin!1554344057471!9999999!hmac-value-goes-here

Hmm...OWASP seems to think that injecting such a token into the form might be enough without having a token cookie at all, although they would then add an extra field representing the operation to be performed. There may be something in that.

Anyway, there's information about Python encryption at https://www.blog.pythonlibrary.org/2016/05/18/python-3-an-intro-to-encryption/ if you want to read up and get some ideas.

[ThrawnCA]

The documentation of the Python hmac module is at https://docs.python.org/3/library/hmac.html

I think we could get a lot of benefit from something like:

import time, random, hmac, hashlib

timestamp = int(time.time())
nonce = random.randint(1, 999999)
message = timestamp + '!' + nonce + '!' + username

token = hmac.HMAC(beaker_secret, message, hashlib.sha512).hexdigest() + '!' + message

It doesn't matter that the nonce is a regular random value, rather than being cryptographically strong randomness, since the only point of it is to ensure that successive calls in the same second don't produce the same hash.

This could be set and used much like the existing random tokens, but it would be impossible to forge and has expiry information built in.

[anotheredward]

Hi @ThrawnCA , I've now got some time to work on this issue, I'm just taking a look at your implementation, but I'm not sure exactly how to get it up and running.

I've added the intercept_csrf call to a new init method in the plugin file of this project https://github.com/data-govt-nz/ckanext-security/blob/master/ckanext/security/plugin.py

But that doesn't appear to be working.

Do I potentially need to add the additional interfaces that are used by the Queensland plugin for your implementation to work correctly? https://github.com/qld-gov-au/ckan-ex-qgov/blob/739081d6e6a453e52676b385ec4f3a2e3f96f6db/ckanext/qgov/common/plugin.py#L382

[ThrawnCA]

How are you testing whether it works? Bear in mind that it won't take any action if you aren't logged in. If you are, then you should be able to see a token hidden field in any POST form.

The plugin interfaces shouldn't be needed for this; they're for other features. So long as something is calling the __init__ method of your plugin, that should be enough. Can you paste your plugin code?

[anotheredward]

@ThrawnCA I'm not explicitly calling the init function, my assumption was that the CKAN framework would do that when using the plugin, that could be my problem.

I've put my work so far in a branch here: https://github.com/data-govt-nz/ckanext-security/blob/fix/csrf_implementation/ckanext/security/plugin.py#L13

[ThrawnCA]

Hmm...your function name looks right, and you're correct that it should be called automatically when the plugin is created by CKAN. What tests have you performed?

[anotheredward]

I rebuilt the application then inspected the pages to see if I could find any hidden fields, or the appropriate cookie was being attached.

As our current implementation gives all users sessions, I've hard-coded the value of the is_logged_in method to True (For the time-being, this is probably not the approach we'd like to take moving forwards).

Potentially there was an issue with the rebuild, I'll continue to investigate.

Thanks for confirming that the implementation looks about right, that gives me one less thing to investigate :)

[anotheredward]

Ok, thanks for the help @ThrawnCA , I've managed to get it up and working, the following potential tasks remain:

• Fix deleting a member from an organisation
• Referrer check will need to be moved to Nginx, and both added to the documentation, I think it'll be something like this:

  location / {
  if ($request_method !~ "(GET|HEAD|OPTIONS)"
  && $request_uri !~ "/api"
  && $http_referer !~ https://catalogue.data.govt.nz/
  && $http_origin !~ https://catalogue.data.govt.nz/){
    return 403;
  }
  }

Our implementation of brute-force protection requires us to continue to have sessions for all users, this also fixes not having login-protection, so I think I'll comment the hack and leave it in place for now.

[ThrawnCA]

Good to hear it's working :).

Re brute-force protection: Is that just to prevent brute-force logins? If so, then adjusting the is_logged_in function to include the case where the requested URL is the login page would cover that plus Login CSRF.

[anotheredward]

I would like to shrink sessions down to just users who are visiting the login page/are already logged in if at all possible, considering how few admins we have compared to the number of views.

I'll take a look at the beaker configuration while I'm here and see if we can work that out.

Thanks for the suggestion @ThrawnCA .

(Head's up, will probably be a bit unresponsive until Monday next week, ANZAC day tomorrow, Friday off, but will get on it first thing Monday :))

[ThrawnCA]

@anotheredward I've pushed a fix for the organisation member bug: https://github.com/ThrawnCA/ckanext-qgov/commit/196bc328a57cfcfe0e75b52cbb223603f94ae014

[anotheredward]

@ThrawnCA Thanks for that patch! I'm integrating it now and should have an update PR out soon :)

[ThrawnCA]

@anotheredward I've done some work on our extension to convert the tokens from random strings to HMACs. This makes it essentially impossible to forge a valid token without access to our Beaker session secret, thus eliminating the main weakness of the Double Submit cookies technique, and it also means that they can contain useful data such as a timestamp (which simplifies token rotation).

The new token format is <SHA-512 hash>!<timestamp in seconds>/<random nonce>/<username>. After the cookie token and form token are matched, the filter calculates the hash of the three fields after the exclamation point, combined with the Beaker session secret, and checks that it matches the provided hash; otherwise, the token is rejected. It further checks that the timestamp is current, and that the username matches the current user (so an attacker cannot obtain their own token and then force a victim to use it).

For reference: https://github.com/qld-gov-au/ckan-ex-qgov/pull/6

[camfindlay]

Thanks @ThrawnCA - @anotheredward has moved to a new role and @ebuckley is on the case. We'll look at making a tagged release of this module soon with the changes in #24 and then perhaps follow that up with some token enhancements as you've proposed :+1:

[ebuckley]

Thanks for your contributions on this @ThrawnCA

I have merged the PR and cut a release with our fix https://github.com/data-govt-nz/ckanext-security/releases/tag/1.1.0