From 6c06b69dc518e26f37ce866b47342e2cd52fe051 Mon Sep 17 00:00:00 2001 From: Martin Zimmermann Date: Wed, 4 Dec 2013 23:19:51 +0100 Subject: [PATCH] another approach to fix #40 (return 403 on false Content-Type) When an attacker uses a
to downvote a comment, the browser *should* add a `Content-Type: ...` header with three possible values: * application/x-www-form-urlencoded * multipart/form-data * text/plain If the header is not sent or requests `application/json`, the request is not forged (XHR is restricted by CORS separately). --- isso/js/app/api.js | 1 + isso/views/comments.py | 29 +++++++++++++++++++++++++++++ specs/fixtures.py | 8 ++++++++ specs/test_comments.py | 42 +++++++++++++++++++++++++++++------------- specs/test_vote.py | 5 ++--- 5 files changed, 69 insertions(+), 16 deletions(-) diff --git a/isso/js/app/api.js b/isso/js/app/api.js index 144415e..23e3102 100644 --- a/isso/js/app/api.js +++ b/isso/js/app/api.js @@ -93,6 +93,7 @@ define(["q"], function(Q) { try { xhr.open(method, url, true); xhr.withCredentials = true; + xhr.setRequestHeader("Content-Type", "application/json"); if (method === "GET") { xhr.setRequestHeader("X-Origin", window.location.origin); diff --git a/isso/views/comments.py b/isso/views/comments.py index 0fe1a0a..83a6238 100644 --- a/isso/views/comments.py +++ b/isso/views/comments.py @@ -31,6 +31,30 @@ class JSON(Response): return super(JSON, self).__init__(*args, content_type='application/json') +def xhr(func): + """A decorator to check for CSRF on POST/PUT/DELETE using a + element and JS to execute automatically (see #40 for a proof-of-concept). + + When an attacker uses a to downvote a comment, the browser *should* + add a `Content-Type: ...` header with three possible values: + + * application/x-www-form-urlencoded + * multipart/form-data + * text/plain + + If the header is not sent or requests `application/json`, the request is + not forged (XHR is restricted by CORS separately). + """ + + def dec(self, env, req, *args, **kwargs): + + if req.content_type and not req.content_type.startswith("application/json"): + raise Forbidden("CSRF") + return func(self, env, req, *args, **kwargs) + + return dec + + class API(object): FIELDS = set(['id', 'parent', 'text', 'author', 'website', 'email', @@ -91,6 +115,7 @@ class API(object): return True, "" + @xhr @requires(str, 'uri') def new(self, environ, request, uri): @@ -174,6 +199,7 @@ class API(object): return Response(json.dumps(rv), 200, content_type='application/json') + @xhr def edit(self, environ, request, id): try: @@ -217,6 +243,7 @@ class API(object): resp.headers.add("X-Set-Cookie", cookie("isso-%i" % rv["id"])) return resp + @xhr def delete(self, environ, request, id, key=None): try: @@ -294,11 +321,13 @@ class API(object): return JSON(json.dumps(rv), 200) + @xhr def like(self, environ, request, id): nv = self.comments.vote(True, id, utils.anonymize(str(request.remote_addr))) return Response(json.dumps(nv), 200) + @xhr def dislike(self, environ, request, id): nv = self.comments.vote(False, id, utils.anonymize(str(request.remote_addr))) diff --git a/specs/fixtures.py b/specs/fixtures.py index 91f8349..3941937 100644 --- a/specs/fixtures.py +++ b/specs/fixtures.py @@ -2,6 +2,7 @@ import json +from werkzeug.test import Client class FakeIP(object): @@ -14,6 +15,13 @@ class FakeIP(object): return self.app(environ, start_response) +class JSONClient(Client): + + def open(self, *args, **kwargs): + kwargs.setdefault('content_type', 'application/json') + return super(JSONClient, self).open(*args, **kwargs) + + class Dummy: status = 200 diff --git a/specs/test_comments.py b/specs/test_comments.py index 81d90a8..27f9f9b 100644 --- a/specs/test_comments.py +++ b/specs/test_comments.py @@ -12,14 +12,13 @@ try: except ImportError: from urllib import urlencode -from werkzeug.test import Client from werkzeug.wrappers import Response from isso import Isso, core from isso.utils import http from isso.views import comments -from fixtures import curl, loads, FakeIP +from fixtures import curl, loads, FakeIP, JSONClient http.curl = curl @@ -37,7 +36,7 @@ class TestComments(unittest.TestCase): self.app = App(conf) self.app.wsgi_app = FakeIP(self.app.wsgi_app, "192.168.1.1") - self.client = Client(self.app, Response) + self.client = JSONClient(self.app, Response) self.get = self.client.get self.put = self.client.put self.post = self.client.post @@ -133,7 +132,7 @@ class TestComments(unittest.TestCase): def testDeleteWithReference(self): - client = Client(self.app, Response) + client = JSONClient(self.app, Response) client.post('/new?uri=%2Fpath%2F', data=json.dumps({'text': 'First'})) client.post('/new?uri=%2Fpath%2F', data=json.dumps({'text': 'First', 'parent': 1})) @@ -160,7 +159,7 @@ class TestComments(unittest.TestCase): --- [ comment 4, ref 2 ] [ comment 5 ] """ - client = Client(self.app, Response) + client = JSONClient(self.app, Response) client.post('/new?uri=%2Fpath%2F', data=json.dumps({'text': 'First'})) client.post('/new?uri=%2Fpath%2F', data=json.dumps({'text': 'Second', 'parent': 1})) @@ -193,11 +192,11 @@ class TestComments(unittest.TestCase): def testDeleteAndCreateByDifferentUsersButSamePostId(self): - mallory = Client(self.app, Response) + mallory = JSONClient(self.app, Response) mallory.post('/new?uri=%2Fpath%2F', data=json.dumps({'text': 'Foo'})) mallory.delete('/id/1') - bob = Client(self.app, Response) + bob = JSONClient(self.app, Response) bob.post('/new?uri=%2Fpath%2F', data=json.dumps({'text': 'Bar'})) assert mallory.delete('/id/1').status_code == 403 @@ -263,10 +262,27 @@ class TestComments(unittest.TestCase): def testDeleteCommentRemovesThread(self): - rv = self.client.post('/new?uri=%2F', data=json.dumps({"text": "..."})) - assert '/' in self.app.db.threads - self.client.delete('/id/1') - assert '/' not in self.app.db.threads + rv = self.client.post('/new?uri=%2F', data=json.dumps({"text": "..."})) + assert '/' in self.app.db.threads + self.client.delete('/id/1') + assert '/' not in self.app.db.threads + + def testCSRF(self): + + js = "application/json" + form = "application/x-www-form-urlencoded" + + self.post('/new?uri=%2F', data=json.dumps({"text": "..."})) + + # no header is fine (default for XHR) + assert self.post('/id/1/dislike', content_type="").status_code == 200 + + # x-www-form-urlencoded is definitely not RESTful + assert self.post('/id/1/dislike', content_type=form).status_code == 403 + assert self.post('/new?uri=%2F', data=json.dumps({"text": "..."}), + content_type=form).status_code == 403 + # just for the record + assert self.post('/id/1/dislike', content_type=js).status_code == 200 class TestModeratedComments(unittest.TestCase): @@ -283,7 +299,7 @@ class TestModeratedComments(unittest.TestCase): self.app = App(conf) self.app.wsgi_app = FakeIP(self.app.wsgi_app, "192.168.1.1") - self.client = Client(self.app, Response) + self.client = JSONClient(self.app, Response) def tearDown(self): os.unlink(self.path) @@ -314,7 +330,7 @@ class TestPurgeComments(unittest.TestCase): self.app = App(conf) self.app.wsgi_app = FakeIP(self.app.wsgi_app, "192.168.1.1") - self.client = Client(self.app, Response) + self.client = JSONClient(self.app, Response) def testPurgeDoesNoHarm(self): self.client.post('/new?uri=test', data=json.dumps({"text": "..."})) diff --git a/specs/test_vote.py b/specs/test_vote.py index 89e37ed..464754d 100644 --- a/specs/test_vote.py +++ b/specs/test_vote.py @@ -6,13 +6,12 @@ import json import tempfile import unittest -from werkzeug.test import Client from werkzeug.wrappers import Response from isso import Isso, core from isso.utils import http -from fixtures import curl, loads, FakeIP +from fixtures import curl, loads, FakeIP, JSONClient http.curl = curl @@ -33,7 +32,7 @@ class TestVote(unittest.TestCase): app = App(conf) app.wsgi_app = FakeIP(app.wsgi_app, ip) - return Client(app, Response) + return JSONClient(app, Response) def testZeroLikes(self):