From a0d61db1e11863102a79406059dc1ad526133950 Mon Sep 17 00:00:00 2001 From: anotheredward Date: Wed, 24 Apr 2019 16:46:26 +1200 Subject: [PATCH 1/6] replace csrf plugin with QLD Government solution --- README.md | 8 +- ckanext/security/anti_csrf.py | 169 +++++++++++++++++++++++++++++++++ ckanext/security/middleware.py | 95 ------------------ ckanext/security/plugin.py | 4 + 4 files changed, 175 insertions(+), 101 deletions(-) create mode 100644 ckanext/security/anti_csrf.py delete mode 100644 ckanext/security/middleware.py diff --git a/README.md b/README.md index d412662..4ca7081 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ A CKAN extension to hold various security improvements for CKAN, including: * Stronger password reset tokens * Brute force protection -* Cookie-based CSRF protection for requests +* Double-submit CSRF protection for requests (Courtesy of the Queensland Government) * Removed ability to change usernames after signup * Server-side session storage * Session invalidation on logout @@ -30,11 +30,7 @@ A notification email will be sent to locked out users. ## Requirements -* Session- and CSRFMiddleware need to be placed at the bottom of the middleware -stack. This requires to patch `ckan.config.middleware.pylons_app`. The patch is -currently available in the data.govt.nz [CKAN repository](https://github.com/data-govt-nz/ckan/) on the `dia` branch, -or [commit `74f78865`](https://github.com/data-govt-nz/ckan/commit/74f78865b8825c91d1dfe6b189228f4b975610a3) for cherry-pick. -* A running Redis instance to store CSRF tokens configured with a maxmemory and maxmemory-policy=lru so it overwrites the least recently used item rather than running out of space. This instance should be a different instance from the one used for Harvest items to avoid data loss. [Redis LRU-Cache documentation](https://redis.io/topics/lru-cache). +* A running Redis instance to store CSRF tokens configured with a maxmemory and maxmemory-policy=lru so it overwrites the least recently used item rather than running out of space. This instance should be a different instance from the one used for Harvest items to avoid data loss. [Redis LRU-Cache documentation](https://redis.io/topics/lru-cache). ### Changes to `who.ini` You will need at least the following setting ins your `who.ini` diff --git a/ckanext/security/anti_csrf.py b/ckanext/security/anti_csrf.py new file mode 100644 index 0000000..70407b0 --- /dev/null +++ b/ckanext/security/anti_csrf.py @@ -0,0 +1,169 @@ +"""Provides a self-contained filter to prevent Cross-Site Request Forgery, +based on the Double Submit Cookie pattern, +www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF)_Prevention_Cheat_Sheet#Double_Submit_Cookie + +The filter can be enabled simply by invoking 'intercept_csrf()'. +""" +import ckan.lib.base as base +import re +from re import DOTALL, IGNORECASE, MULTILINE +from logging import getLogger +from ckan.common import request, response + +LOG = getLogger(__name__) + +RAW_RENDER = base.render +RAW_RENDER_JINJA = base.render_jinja2 +RAW_BEFORE = base.BaseController.__before__ + +""" Used as the cookie name and input field name. +""" +TOKEN_FIELD_NAME = 'token' +""" Used to rotate the token cookie periodically. +If the freshness cookie doesn't appear, the token cookie is still OK, +but we'll set a new one for next time. +""" +TOKEN_FRESHNESS_COOKIE_NAME = 'token-fresh' + +# We need to edit confirm-action links, which get intercepted by JavaScript, +#regardless of which order their 'data-module' and 'href' attributes appear. +CONFIRM_LINK = re.compile(r'(]*data-module=["\']confirm-action["\'][^>]*href=["\'][^"\'?]+)(["\'])', IGNORECASE | MULTILINE) +CONFIRM_LINK_REVERSED = re.compile(r'(]*href=["\'][^"\'?]+)(["\'][^>]*data-module=["\']confirm-action["\'])', IGNORECASE | MULTILINE) + +""" +This will match a POST form that has whitespace after the opening tag (which all existing forms do). +Once we have injected a token immediately after the opening tag, +it won't match any more, which avoids redundant injection. +""" +POST_FORM = re.compile(r'(
]*method=["\']post["\'][^>]*>)([^<]*\s<)', IGNORECASE | MULTILINE) + +"""The format of the token HTML field. +""" +HEX_PATTERN=re.compile(r'^[0-9a-z]+$') +TOKEN_PATTERN = r'' +TOKEN_SEARCH_PATTERN = re.compile(TOKEN_PATTERN.format(token=r'([0-9a-f]+)')) +API_URL = re.compile(r'^/api\b.*') + +def is_logged_in(): + return True + +""" Rewrite HTML to insert tokens if applicable. +""" + +def anti_csrf_render_jinja2(template_name, extra_vars=None): + html = apply_token(RAW_RENDER_JINJA(template_name, extra_vars)) + return html + +def apply_token(html): + if not is_logged_in() or not POST_FORM.search(html): + return html + + token_match = TOKEN_SEARCH_PATTERN.search(html) + if token_match: + token = token_match.group(1) + else: + token = get_response_token() + + def insert_form_token(form_match): + return form_match.group(1) + TOKEN_PATTERN.format(token=token) + form_match.group(2) + + def insert_link_token(link_match): + return link_match.group(1) + '?' + TOKEN_FIELD_NAME + '=' + token + link_match.group(2) + + return CONFIRM_LINK_REVERSED.sub(insert_link_token, CONFIRM_LINK.sub(insert_link_token, POST_FORM.sub(insert_form_token, html))) + +def get_cookie_token(): + """Retrieve the token expected by the server. + + This will be retrieved from the 'token' cookie, if it exists. + If not, an error will occur. + """ + if request.cookies.has_key(TOKEN_FIELD_NAME): + LOG.debug("Obtaining token from cookie") + token = request.cookies.get(TOKEN_FIELD_NAME) + if token is None or token.strip() == "": + csrf_fail("CSRF token is blank") + + return token + +def get_response_token(): + """Retrieve the token to be injected into pages. + + This will be retrieved from the 'token' cookie, if it exists and is fresh. + If not, a new token will be generated and a new cookie set. + """ + # ensure that the same token is used when a page is assembled from pieces + if request.environ['webob.adhoc_attrs'].has_key('response_token'): + LOG.debug("Reusing response token from request attributes") + token = request.response_token + elif request.cookies.has_key(TOKEN_FIELD_NAME) and request.cookies.has_key(TOKEN_FRESHNESS_COOKIE_NAME): + LOG.debug("Obtaining token from cookie") + token = request.cookies.get(TOKEN_FIELD_NAME) + if not HEX_PATTERN.match(token): + LOG.debug("Invalid cookie token; making new token cookie") + token = create_response_token() + request.response_token = token + else: + LOG.debug("No fresh token found; making new token cookie") + token = create_response_token() + request.response_token = token + + return token + +def create_response_token(): + import binascii, os + token = binascii.hexlify(os.urandom(32)) + response.set_cookie(TOKEN_FIELD_NAME, token, secure=True, httponly=True) + response.set_cookie(TOKEN_FRESHNESS_COOKIE_NAME, '1', max_age=600, secure=True, httponly=True) + return token + +# Check token on applicable requests + +def is_request_exempt(): + return not is_logged_in() or API_URL.match(request.path) or request.method in {'GET', 'HEAD', 'OPTIONS'} + +def anti_csrf_before(obj, action, **params): + if not is_request_exempt() and get_cookie_token() != get_post_token(): + csrf_fail("Could not match session token with form token") + + RAW_BEFORE(obj, action) + +def csrf_fail(message): + from flask import abort + LOG.error(message) + abort(403, "Your form submission could not be validated") + +def get_post_token(): + """Retrieve the token provided by the client. + + This is normally a single 'token' parameter in the POST body. + However, for compatibility with 'confirm-action' links, + it is also acceptable to provide the token as a query string parameter, + if it is the *only* parameter of either kind. + """ + if request.environ['webob.adhoc_attrs'].has_key(TOKEN_FIELD_NAME): + return request.token + + # handle query string token if there are no other parameters + # this is needed for the 'confirm-action' JavaScript module + if not request.POST and len(request.GET) == 1 and len(request.GET.getall(TOKEN_FIELD_NAME)) == 1: + request.token = request.GET.getone(TOKEN_FIELD_NAME) + del request.GET[TOKEN_FIELD_NAME] + return request.token + + postTokens = request.POST.getall(TOKEN_FIELD_NAME) + if not postTokens: + csrf_fail("Missing CSRF token in form submission") + elif len(postTokens) > 1: + csrf_fail("More than one CSRF token in form submission") + else: + request.token = postTokens[0] + + # drop token from request so it doesn't populate resource extras + del request.POST[TOKEN_FIELD_NAME] + + return request.token + +def intercept_csrf(): + base.render_jinja2 = anti_csrf_render_jinja2 + base.BaseController.__before__ = anti_csrf_before diff --git a/ckanext/security/middleware.py b/ckanext/security/middleware.py deleted file mode 100644 index 7925344..0000000 --- a/ckanext/security/middleware.py +++ /dev/null @@ -1,95 +0,0 @@ -import os -import codecs - -import webob -from webob.exc import HTTPForbidden - -from ckanext.security.cache.clients import CSRFClient - -import logging -log = logging.getLogger(__name__) - -try: - from hmac import compare_digest -except ImportError: - def compare_digest(a, b): - return a == b - - -CSRF_ERR = 'CSRF authentication failed. Token missing or invalid.' - - -class Request(webob.Request): - def is_secure(self): - # allow requests which have the x-forwarded-proto of https (inserted by nginx) - if self.headers.get('X-Forwarded-Proto') == 'https': - return True - return self.scheme == 'https' - - def is_safe(self): - "Check if the request is 'safe', if the request is safe it will not be checked for csrf" - # api requests are exempt from csrf checks - if self.path.startswith("/api"): - return True - - # get/head/options/trace are exempt from csrf checks - return self.method in ('GET', 'HEAD', 'OPTIONS', 'TRACE') - - def good_referer(self): - "Returns true if the referrer is https and matching the host" - if not self.referer: - return False - else: - match = "https://{}/".format(self.host) - return self.referer.startswith(match) - - -class CSRFMiddleware(object): - COOKIE_NAME = 'csrftoken' - - def __init__(self, app, config): - self.app = app - self.cache = CSRFClient() - self.domain = config['ckanext.security.domain'] - - def __call__(self, environ, start_response): - request = Request(environ) - self.session = environ['beaker.session'] - self.session.save() - - if self.is_valid(request): - resp = request.get_response(self.app) - else: - resp = HTTPForbidden(CSRF_ERR) - - if 'text/html' in resp.headers.get('Content-type', ''): - resp = self.add_new_token(resp) - - return resp(environ, start_response) - - def is_valid(self, request): - return request.is_safe() or self.unsafe_request_is_valid(request) - - def unsafe_request_is_valid(self, request): - return request.is_secure() and request.good_referer() and self.check_cookie(request) - - def check_cookie(self, request): - token = request.cookies.get(self.COOKIE_NAME, None) - - if token is None: - # Just in case this is set by an AJAX request - token = request.cookies.get('X-CSRFToken', None) - - csrf_token = self.cache.get(self.session.id) - - if csrf_token == None: - log.warning('Could not find a csrf token for session id: {}\n{}'.format(self.session.id, request)) - return False - - return compare_digest(str(token), str(csrf_token)) - - def add_new_token(self, response): - token = codecs.encode(os.urandom(32), 'hex') - self.cache.set(self.session.id, token) - response.set_cookie(self.COOKIE_NAME, token, httponly=True, overwrite=True, secure=True, domain=self.domain) - return response diff --git a/ckanext/security/plugin.py b/ckanext/security/plugin.py index 2eadfc5..7fc3d20 100644 --- a/ckanext/security/plugin.py +++ b/ckanext/security/plugin.py @@ -1,6 +1,7 @@ import ckan.plugins as plugins import ckan.plugins.toolkit as toolkit import ckan.logic.schema +import anti_csrf from ckanext.security import schema @@ -9,6 +10,9 @@ class CkanSecurityPlugin(plugins.SingletonPlugin): plugins.implements(plugins.IConfigurer) plugins.implements(plugins.IRoutes) + def __init__(self, **kwargs): + anti_csrf.intercept_csrf() + def update_config(self, config): # Monkeypatching all user schemas in order to enforce a stronger password # policy. I tried mokeypatching `ckan.logic.validators.user_password_validator` From 38466a2210b7fb6e713bd5cc171e345417bf75b4 Mon Sep 17 00:00:00 2001 From: anotheredward Date: Thu, 2 May 2019 11:16:08 +1200 Subject: [PATCH 2/6] fix delete user from organisation This work was done by the ThrawnCA of the Queensland Government. https://github.com/ThrawnCA/ckanext-qgov/blob/196bc328a57cfcfe0e75b52cbb223603f94ae014/ckanext/qgov/common/anti_csrf.py --- ckanext/security/anti_csrf.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/ckanext/security/anti_csrf.py b/ckanext/security/anti_csrf.py index 70407b0..8c39d23 100644 --- a/ckanext/security/anti_csrf.py +++ b/ckanext/security/anti_csrf.py @@ -27,8 +27,8 @@ # We need to edit confirm-action links, which get intercepted by JavaScript, #regardless of which order their 'data-module' and 'href' attributes appear. -CONFIRM_LINK = re.compile(r'(]*data-module=["\']confirm-action["\'][^>]*href=["\'][^"\'?]+)(["\'])', IGNORECASE | MULTILINE) -CONFIRM_LINK_REVERSED = re.compile(r'(]*href=["\'][^"\'?]+)(["\'][^>]*data-module=["\']confirm-action["\'])', IGNORECASE | MULTILINE) +CONFIRM_LINK = re.compile(r'(]*data-module=["\']confirm-action["\'][^>]*href=["\']([^"\']+))(["\'])', IGNORECASE | MULTILINE) +CONFIRM_LINK_REVERSED = re.compile(r'(]*href=["\']([^"\']+))(["\'][^>]*data-module=["\']confirm-action["\'])', IGNORECASE | MULTILINE) """ This will match a POST form that has whitespace after the opening tag (which all existing forms do). @@ -45,7 +45,7 @@ API_URL = re.compile(r'^/api\b.*') def is_logged_in(): - return True + return request.cookies.get("auth_tkt") """ Rewrite HTML to insert tokens if applicable. """ @@ -68,7 +68,11 @@ def insert_form_token(form_match): return form_match.group(1) + TOKEN_PATTERN.format(token=token) + form_match.group(2) def insert_link_token(link_match): - return link_match.group(1) + '?' + TOKEN_FIELD_NAME + '=' + token + link_match.group(2) + if '?' in link_match.group(2): + separator = '&' + else: + separator = '?' + return link_match.group(1) + separator + TOKEN_FIELD_NAME + '=' + token + link_match.group(3) return CONFIRM_LINK_REVERSED.sub(insert_link_token, CONFIRM_LINK.sub(insert_link_token, POST_FORM.sub(insert_form_token, html))) @@ -139,14 +143,14 @@ def get_post_token(): This is normally a single 'token' parameter in the POST body. However, for compatibility with 'confirm-action' links, it is also acceptable to provide the token as a query string parameter, - if it is the *only* parameter of either kind. + if there is no POST body. """ if request.environ['webob.adhoc_attrs'].has_key(TOKEN_FIELD_NAME): return request.token - # handle query string token if there are no other parameters + # handle query string token if there are no POST parameters # this is needed for the 'confirm-action' JavaScript module - if not request.POST and len(request.GET) == 1 and len(request.GET.getall(TOKEN_FIELD_NAME)) == 1: + if not request.POST and len(request.GET.getall(TOKEN_FIELD_NAME)) == 1: request.token = request.GET.getone(TOKEN_FIELD_NAME) del request.GET[TOKEN_FIELD_NAME] return request.token From 2185128466bc2284cdb4d9ccd36e66c4671844c1 Mon Sep 17 00:00:00 2001 From: Ersin Date: Wed, 8 May 2019 08:21:48 +1200 Subject: [PATCH 3/6] implement the caching behaviour in the middleware --- ckanext/security/anti_csrf.py | 53 +++++------- ckanext/security/cache/clients.py | 3 - ckanext/security/middleware.py | 130 ++++++++++++++++++++++++++++++ ckanext/security/plugin.py | 4 - 4 files changed, 150 insertions(+), 40 deletions(-) create mode 100644 ckanext/security/middleware.py diff --git a/ckanext/security/anti_csrf.py b/ckanext/security/anti_csrf.py index 8c39d23..4bab835 100644 --- a/ckanext/security/anti_csrf.py +++ b/ckanext/security/anti_csrf.py @@ -1,14 +1,14 @@ -"""Provides a self-contained filter to prevent Cross-Site Request Forgery, +"""Provides a filter to prevent Cross-Site Request Forgery, based on the Double Submit Cookie pattern, -www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF)_Prevention_Cheat_Sheet#Double_Submit_Cookie +https://github.com/OWASP/CheatSheetSeries/blob/master/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.md#double-submit-cookie -The filter can be enabled simply by invoking 'intercept_csrf()'. +This is integrated in the CSRFMiddleware """ import ckan.lib.base as base import re -from re import DOTALL, IGNORECASE, MULTILINE +from re import IGNORECASE, MULTILINE from logging import getLogger -from ckan.common import request, response +from ckan.common import request LOG = getLogger(__name__) @@ -45,24 +45,19 @@ API_URL = re.compile(r'^/api\b.*') def is_logged_in(): - return request.cookies.get("auth_tkt") + # auth_tkt does not exist in context.. + # return request.cookies.get("auth_tkt") + return True -""" Rewrite HTML to insert tokens if applicable. -""" - -def anti_csrf_render_jinja2(template_name, extra_vars=None): - html = apply_token(RAW_RENDER_JINJA(template_name, extra_vars)) - return html - -def apply_token(html): +def apply_token(html, token): + """ Rewrite HTML to insert tokens if applicable. + """ if not is_logged_in() or not POST_FORM.search(html): return html token_match = TOKEN_SEARCH_PATTERN.search(html) if token_match: token = token_match.group(1) - else: - token = get_response_token() def insert_form_token(form_match): return form_match.group(1) + TOKEN_PATTERN.format(token=token) + form_match.group(2) @@ -76,45 +71,40 @@ def insert_link_token(link_match): return CONFIRM_LINK_REVERSED.sub(insert_link_token, CONFIRM_LINK.sub(insert_link_token, POST_FORM.sub(insert_form_token, html))) -def get_cookie_token(): + +def get_cookie_token(request): """Retrieve the token expected by the server. This will be retrieved from the 'token' cookie, if it exists. If not, an error will occur. """ + token = None if request.cookies.has_key(TOKEN_FIELD_NAME): LOG.debug("Obtaining token from cookie") token = request.cookies.get(TOKEN_FIELD_NAME) if token is None or token.strip() == "": csrf_fail("CSRF token is blank") - return token -def get_response_token(): +def get_response_token(request, response): """Retrieve the token to be injected into pages. This will be retrieved from the 'token' cookie, if it exists and is fresh. If not, a new token will be generated and a new cookie set. """ # ensure that the same token is used when a page is assembled from pieces - if request.environ['webob.adhoc_attrs'].has_key('response_token'): - LOG.debug("Reusing response token from request attributes") - token = request.response_token - elif request.cookies.has_key(TOKEN_FIELD_NAME) and request.cookies.has_key(TOKEN_FRESHNESS_COOKIE_NAME): + if TOKEN_FIELD_NAME in request.cookies and TOKEN_FRESHNESS_COOKIE_NAME in request.cookies: LOG.debug("Obtaining token from cookie") token = request.cookies.get(TOKEN_FIELD_NAME) if not HEX_PATTERN.match(token): LOG.debug("Invalid cookie token; making new token cookie") - token = create_response_token() - request.response_token = token + token = create_response_token(response) else: LOG.debug("No fresh token found; making new token cookie") - token = create_response_token() - request.response_token = token - + token = create_response_token(response) return token -def create_response_token(): +def create_response_token(response): import binascii, os token = binascii.hexlify(os.urandom(32)) response.set_cookie(TOKEN_FIELD_NAME, token, secure=True, httponly=True) @@ -137,7 +127,7 @@ def csrf_fail(message): LOG.error(message) abort(403, "Your form submission could not be validated") -def get_post_token(): +def get_post_token(request): """Retrieve the token provided by the client. This is normally a single 'token' parameter in the POST body. @@ -168,6 +158,3 @@ def get_post_token(): return request.token -def intercept_csrf(): - base.render_jinja2 = anti_csrf_render_jinja2 - base.BaseController.__before__ = anti_csrf_before diff --git a/ckanext/security/cache/clients.py b/ckanext/security/cache/clients.py index 7f592b1..5350315 100644 --- a/ckanext/security/cache/clients.py +++ b/ckanext/security/cache/clients.py @@ -19,8 +19,5 @@ def set(self, key, value): def delete(self, key): return self.client.delete(self.prefix + key) -class CSRFClient(RedisClient): - prefix = 'security_csrf_' - class ThrottleClient(RedisClient): prefix = 'security_throttle_' diff --git a/ckanext/security/middleware.py b/ckanext/security/middleware.py new file mode 100644 index 0000000..0276837 --- /dev/null +++ b/ckanext/security/middleware.py @@ -0,0 +1,130 @@ +import anti_csrf +import webob +from webob.exc import HTTPForbidden + +import logging + +log = logging.getLogger(__name__) + + +CSRF_ERR = 'CSRF authentication failed. Token missing or invalid.' + + +class Request(webob.Request): + def __init__(self, environ): + super(Request, self).__init__(environ) + self.token = self._get_post_token() + + def is_secure(self): + # allow requests which have the x-forwarded-proto of https (inserted by nginx) + if self.headers.get('X-Forwarded-Proto') == 'https': + return True + return self.scheme == 'https' + + def is_safe(self): + "Check if the request is 'safe', if the request is safe it will not be checked for csrf" + # api requests are exempt from csrf checks + if self.path.startswith("/api"): + return True + + # get/head/options/trace are exempt from csrf checks + return self.method in ('GET', 'HEAD', 'OPTIONS', 'TRACE') + + def good_referer(self, domain): + "Returns true if the referrer is https and matching the host" + if not self.referer: + return False + else: + match = "https://{}".format(domain) + return self.referer.startswith(match) + + def good_origin(self, domain): + """ + checks if the origin header is present and matches the header" + :param domain: string: the expected origin domain + :return: boolean: true if the origin header is present and matches the expected domain + """ + origin = self.headers.get('origin', None) + if not origin: + log.warning("Potentially unsafe CSRF request is missing the origin header") + return True + else: + match = "https://{}".format(domain) + return origin.startswith(match) + + def _get_post_token(self): + """Retrieve the token provided by the client. Or return None if not present + + This is normally a single 'token' parameter in the POST body. + However, for compatibility with 'confirm-action' links, + it is also acceptable to provide the token as a query string parameter, + if there is no POST body. + """ + # handle query string token if there are no POST parameters + # this is needed for the 'confirm-action' JavaScript module + if not self.POST and len(self.GET.getall(anti_csrf.TOKEN_FIELD_NAME)) == 1: + token = self.GET.getone(anti_csrf.TOKEN_FIELD_NAME) + del self.GET[anti_csrf.TOKEN_FIELD_NAME] + return token + post_tokens = self.POST.getall(anti_csrf.TOKEN_FIELD_NAME) + if not post_tokens or len(post_tokens) != 1: + return None + token = post_tokens[0] + log.error("GOT TOKEN {}".format(token)) + # drop token from request so it doesn't populate resource extras + del self.POST[anti_csrf.TOKEN_FIELD_NAME] + return token + + def get_cookie_token(self): + """Retrieve the token expected by the server. + + This will be retrieved from the 'token' cookie + """ + if anti_csrf.TOKEN_FIELD_NAME in self.cookies: + log.debug("Obtaining token from cookie") + return self.cookies.get(anti_csrf.TOKEN_FIELD_NAME) + else: + return None + + def check_token(self): + log.error(" Token {}, cookie_token: {}".format(self.token, self.get_cookie_token())) + return self.token is not None and self.token == self.get_cookie_token() + +class CSRFMiddleware(object): + def __init__(self, app, config): + self.app = app + self.domain = config['ckanext.security.domain'] + + def __call__(self, environ, start_response): + request = Request(environ) + self.session = environ['beaker.session'] + self.session.save() + if self.is_valid(request): + resp = request.get_response(self.app) + else: + resp = HTTPForbidden(CSRF_ERR) + if 'text/html' in resp.headers.get('Content-type', ''): + token = anti_csrf.get_response_token(request, resp) + log.error("Generated a token for the response {}".format(token)) + + response_value = resp(environ, start_response) + return [anti_csrf.apply_token(response_value[0], token)] + else: + response_value = resp(environ, start_response) + return response_value + + def is_valid(self, request): + log.error("IS _SAFE ? {}".format(request.is_safe())) + if not request.is_safe(): + log.error("Unsafe req is valid ? {}".format(self.unsafe_request_is_valid(request))) + log.error('===================================') + return request.is_safe() or self.unsafe_request_is_valid(request) + + def unsafe_request_is_valid(self, request): + log.error("is_secure: {}".format(request.is_secure())) + log.error("good_referer: {}".format(request.good_referer(self.domain))) + log.error("good_origin: {}".format(request.good_origin(self.domain))) + log.error("check_token: {}".format(request.check_token())) + log.error('===================================') + return request.is_secure() and request.good_referer(self.domain) and \ + request.good_origin(self.domain) and request.check_token() diff --git a/ckanext/security/plugin.py b/ckanext/security/plugin.py index 7fc3d20..2eadfc5 100644 --- a/ckanext/security/plugin.py +++ b/ckanext/security/plugin.py @@ -1,7 +1,6 @@ import ckan.plugins as plugins import ckan.plugins.toolkit as toolkit import ckan.logic.schema -import anti_csrf from ckanext.security import schema @@ -10,9 +9,6 @@ class CkanSecurityPlugin(plugins.SingletonPlugin): plugins.implements(plugins.IConfigurer) plugins.implements(plugins.IRoutes) - def __init__(self, **kwargs): - anti_csrf.intercept_csrf() - def update_config(self, config): # Monkeypatching all user schemas in order to enforce a stronger password # policy. I tried mokeypatching `ckan.logic.validators.user_password_validator` From 9651436109a9b2e93674ca340759567733fe05a9 Mon Sep 17 00:00:00 2001 From: Ersin Date: Thu, 23 May 2019 14:36:30 +1200 Subject: [PATCH 4/6] implement middleware, fix logical issue with csrf token insertion --- ckanext/security/anti_csrf.py | 2 +- ckanext/security/middleware.py | 20 ++++++-------------- 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/ckanext/security/anti_csrf.py b/ckanext/security/anti_csrf.py index 4bab835..12ecb8e 100644 --- a/ckanext/security/anti_csrf.py +++ b/ckanext/security/anti_csrf.py @@ -52,7 +52,7 @@ def is_logged_in(): def apply_token(html, token): """ Rewrite HTML to insert tokens if applicable. """ - if not is_logged_in() or not POST_FORM.search(html): + if not is_logged_in(): return html token_match = TOKEN_SEARCH_PATTERN.search(html) diff --git a/ckanext/security/middleware.py b/ckanext/security/middleware.py index 0276837..5d970a7 100644 --- a/ckanext/security/middleware.py +++ b/ckanext/security/middleware.py @@ -70,7 +70,6 @@ def _get_post_token(self): if not post_tokens or len(post_tokens) != 1: return None token = post_tokens[0] - log.error("GOT TOKEN {}".format(token)) # drop token from request so it doesn't populate resource extras del self.POST[anti_csrf.TOKEN_FIELD_NAME] return token @@ -87,7 +86,7 @@ def get_cookie_token(self): return None def check_token(self): - log.error(" Token {}, cookie_token: {}".format(self.token, self.get_cookie_token())) + log.debug("Checking token matches Token {}, cookie_token: {}".format(self.token, self.get_cookie_token())) return self.token is not None and self.token == self.get_cookie_token() class CSRFMiddleware(object): @@ -103,28 +102,21 @@ def __call__(self, environ, start_response): resp = request.get_response(self.app) else: resp = HTTPForbidden(CSRF_ERR) + return resp(environ, start_response) if 'text/html' in resp.headers.get('Content-type', ''): token = anti_csrf.get_response_token(request, resp) - log.error("Generated a token for the response {}".format(token)) - response_value = resp(environ, start_response) - return [anti_csrf.apply_token(response_value[0], token)] + new_response = anti_csrf.apply_token(resp.unicode_body, token) + resp.unicode_body = new_response + return resp(environ, start_response) + else: response_value = resp(environ, start_response) return response_value def is_valid(self, request): - log.error("IS _SAFE ? {}".format(request.is_safe())) - if not request.is_safe(): - log.error("Unsafe req is valid ? {}".format(self.unsafe_request_is_valid(request))) - log.error('===================================') return request.is_safe() or self.unsafe_request_is_valid(request) def unsafe_request_is_valid(self, request): - log.error("is_secure: {}".format(request.is_secure())) - log.error("good_referer: {}".format(request.good_referer(self.domain))) - log.error("good_origin: {}".format(request.good_origin(self.domain))) - log.error("check_token: {}".format(request.check_token())) - log.error('===================================') return request.is_secure() and request.good_referer(self.domain) and \ request.good_origin(self.domain) and request.check_token() From 8577644b7056a6b9831a452aab071f2960e634e8 Mon Sep 17 00:00:00 2001 From: Ersin Date: Mon, 27 May 2019 15:43:24 +1200 Subject: [PATCH 5/6] refactor and clean up unused methods. Update readme --- README.md | 2 +- ckanext/security/anti_csrf.py | 19 +++---------------- 2 files changed, 4 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index 4ca7081..21fb4d2 100644 --- a/README.md +++ b/README.md @@ -30,7 +30,7 @@ A notification email will be sent to locked out users. ## Requirements -* A running Redis instance to store CSRF tokens configured with a maxmemory and maxmemory-policy=lru so it overwrites the least recently used item rather than running out of space. This instance should be a different instance from the one used for Harvest items to avoid data loss. [Redis LRU-Cache documentation](https://redis.io/topics/lru-cache). +* A running Redis instance to store brute force protection tokens configured with a maxmemory and maxmemory-policy=lru so it overwrites the least recently used item rather than running out of space. This instance should be a different instance from the one used for Harvest items to avoid data loss. [Redis LRU-Cache documentation](https://redis.io/topics/lru-cache). ### Changes to `who.ini` You will need at least the following setting ins your `who.ini` diff --git a/ckanext/security/anti_csrf.py b/ckanext/security/anti_csrf.py index 12ecb8e..154561e 100644 --- a/ckanext/security/anti_csrf.py +++ b/ckanext/security/anti_csrf.py @@ -44,16 +44,10 @@ TOKEN_SEARCH_PATTERN = re.compile(TOKEN_PATTERN.format(token=r'([0-9a-f]+)')) API_URL = re.compile(r'^/api\b.*') -def is_logged_in(): - # auth_tkt does not exist in context.. - # return request.cookies.get("auth_tkt") - return True def apply_token(html, token): """ Rewrite HTML to insert tokens if applicable. """ - if not is_logged_in(): - return html token_match = TOKEN_SEARCH_PATTERN.search(html) if token_match: @@ -86,6 +80,7 @@ def get_cookie_token(request): csrf_fail("CSRF token is blank") return token + def get_response_token(request, response): """Retrieve the token to be injected into pages. @@ -104,6 +99,7 @@ def get_response_token(request, response): token = create_response_token(response) return token + def create_response_token(response): import binascii, os token = binascii.hexlify(os.urandom(32)) @@ -111,22 +107,13 @@ def create_response_token(response): response.set_cookie(TOKEN_FRESHNESS_COOKIE_NAME, '1', max_age=600, secure=True, httponly=True) return token -# Check token on applicable requests - -def is_request_exempt(): - return not is_logged_in() or API_URL.match(request.path) or request.method in {'GET', 'HEAD', 'OPTIONS'} - -def anti_csrf_before(obj, action, **params): - if not is_request_exempt() and get_cookie_token() != get_post_token(): - csrf_fail("Could not match session token with form token") - - RAW_BEFORE(obj, action) def csrf_fail(message): from flask import abort LOG.error(message) abort(403, "Your form submission could not be validated") + def get_post_token(request): """Retrieve the token provided by the client. From 0c4da82bcfd8dc9ec81018c3085e73e50984df92 Mon Sep 17 00:00:00 2001 From: Ersin Date: Mon, 22 Jul 2019 11:44:51 +1200 Subject: [PATCH 6/6] update readme with middleware instructions --- README.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 21fb4d2..5673452 100644 --- a/README.md +++ b/README.md @@ -29,7 +29,10 @@ A notification email will be sent to locked out users. ## Requirements - +* The CSRFMiddleware needs to be placed at the bottom of the middleware +stack. This requires to patch `ckan.config.middleware.pylons_app`. The patch is +currently available in the data.govt.nz [CKAN repository](https://github.com/data-govt-nz/ckan/) on the `dia` branch, +or [commit `74f78865`](https://github.com/data-govt-nz/ckan/commit/74f78865b8825c91d1dfe6b189228f4b975610a3) for cherry-pick. * A running Redis instance to store brute force protection tokens configured with a maxmemory and maxmemory-policy=lru so it overwrites the least recently used item rather than running out of space. This instance should be a different instance from the one used for Harvest items to avoid data loss. [Redis LRU-Cache documentation](https://redis.io/topics/lru-cache). ### Changes to `who.ini`