From cd829c704246e727af13eb4c26c43d3f0543b87f Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Tue, 6 Mar 2018 15:46:51 +0000 Subject: [PATCH 1/8] HTTP: CSRF protection for RPC endpoint. By now enforcing the Content-Type header is set to 'application/json', we force browsers attempting a cross-domain request to first perform a CORS preflight OPTIONS request. This request always includes an Origin header which we check against our whitelist. The whitelist contains the current Host as well as anything specified in the new optional allowed_origins config value. Any non-browser tools must also now set the Context-type header. --- mopidy/http/__init__.py | 1 + mopidy/http/ext.conf | 1 + mopidy/http/handlers.py | 35 +++++++++++++++++++++++++++++++++-- 3 files changed, 35 insertions(+), 2 deletions(-) diff --git a/mopidy/http/__init__.py b/mopidy/http/__init__.py index 3fa4bcd6..4f6239b9 100644 --- a/mopidy/http/__init__.py +++ b/mopidy/http/__init__.py @@ -25,6 +25,7 @@ class Extension(ext.Extension): schema['port'] = config_lib.Port() schema['static_dir'] = config_lib.Path(optional=True) schema['zeroconf'] = config_lib.String(optional=True) + schema['allowed_origins'] = config_lib.List(optional=True) return schema def validate_environment(self): diff --git a/mopidy/http/ext.conf b/mopidy/http/ext.conf index d35229bc..5750e855 100644 --- a/mopidy/http/ext.conf +++ b/mopidy/http/ext.conf @@ -4,3 +4,4 @@ hostname = 127.0.0.1 port = 6680 static_dir = zeroconf = Mopidy HTTP server on $hostname +allowed_origins = diff --git a/mopidy/http/handlers.py b/mopidy/http/handlers.py index 6250163c..2bc037a6 100644 --- a/mopidy/http/handlers.py +++ b/mopidy/http/handlers.py @@ -11,20 +11,24 @@ import tornado.websocket import mopidy from mopidy import core, models +from mopidy.compat import urllib from mopidy.internal import encoding, jsonrpc - logger = logging.getLogger(__name__) def make_mopidy_app_factory(apps, statics): def mopidy_app_factory(config, core): + origin_whitelist = { + x.lower() for x in config['http']['allowed_origins'] if x + } return [ (r'/ws/?', WebSocketHandler, { 'core': core, }), (r'/rpc', JsonRpcHandler, { 'core': core, + 'origin_whitelist': origin_whitelist, }), (r'/(.+)', StaticFileHandler, { 'path': os.path.join(os.path.dirname(__file__), 'data'), @@ -143,16 +147,31 @@ def set_mopidy_headers(request_handler): 'X-Mopidy-Version', mopidy.__version__.encode('utf-8')) +def check_origin(origin, request_headers, origin_whitelist): + if origin is None: + logger.debug('Origin was not set') + return False + origin_whitelist.add(request_headers.get('Host', None)) + parsed_origin = urllib.parse.urlparse(origin).netloc.lower() + return parsed_origin and parsed_origin in origin_whitelist + + class JsonRpcHandler(tornado.web.RequestHandler): - def initialize(self, core): + def initialize(self, core, origin_whitelist): self.jsonrpc = make_jsonrpc_wrapper(core) + self.origin_whitelist = origin_whitelist def head(self): self.set_extra_headers() self.finish() def post(self): + content_type = self.request.headers.get("Content-Type", '') + if content_type != 'application/json': + self.set_status(406, 'Content-Type must be application/json') + return + data = self.request.body if not data: return @@ -177,6 +196,18 @@ class JsonRpcHandler(tornado.web.RequestHandler): self.set_header('Accept', 'application/json') self.set_header('Content-Type', 'application/json; utf-8') + def options(self): + origin = self.request.headers.get('Origin', None) + if not check_origin(origin, self.request.headers, + self.origin_whitelist): + self.set_status(403, 'Access denied for origin %s' % origin) + return + + self.set_header("Access-Control-Allow-Origin", "%s" % origin) + self.set_header("Access-Control-Allow-Headers", "Content-Type") + self.set_status(204) + self.finish() + class ClientListHandler(tornado.web.RequestHandler): From 94ba9b6642f4836edaa33df53f6d482110329ad6 Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Tue, 10 Apr 2018 23:59:28 +0100 Subject: [PATCH 2/8] HTTP: Content-Type other than application/json is a 415 client error. Also Fixed up formatting following code review. --- mopidy/http/handlers.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/mopidy/http/handlers.py b/mopidy/http/handlers.py index 2bc037a6..c9d86804 100644 --- a/mopidy/http/handlers.py +++ b/mopidy/http/handlers.py @@ -14,6 +14,7 @@ from mopidy import core, models from mopidy.compat import urllib from mopidy.internal import encoding, jsonrpc + logger = logging.getLogger(__name__) @@ -167,9 +168,9 @@ class JsonRpcHandler(tornado.web.RequestHandler): self.finish() def post(self): - content_type = self.request.headers.get("Content-Type", '') + content_type = self.request.headers.get('Content-Type', '') if content_type != 'application/json': - self.set_status(406, 'Content-Type must be application/json') + self.set_status(415, 'Content-Type must be application/json') return data = self.request.body @@ -198,13 +199,13 @@ class JsonRpcHandler(tornado.web.RequestHandler): def options(self): origin = self.request.headers.get('Origin', None) - if not check_origin(origin, self.request.headers, - self.origin_whitelist): + if not check_origin( + origin, self.request.headers, self.origin_whitelist): self.set_status(403, 'Access denied for origin %s' % origin) return - self.set_header("Access-Control-Allow-Origin", "%s" % origin) - self.set_header("Access-Control-Allow-Headers", "Content-Type") + self.set_header('Access-Control-Allow-Origin', '%s' % origin) + self.set_header('Access-Control-Allow-Headers', 'Content-Type') self.set_status(204) self.finish() From ecb5a7038a40a744b20f403e9b6281d4814d11c3 Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Wed, 11 Apr 2018 00:42:08 +0100 Subject: [PATCH 3/8] docs: http/allowed_origins config setting description --- docs/ext/http.rst | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/ext/http.rst b/docs/ext/http.rst index 730c05b8..6dc91782 100644 --- a/docs/ext/http.rst +++ b/docs/ext/http.rst @@ -98,3 +98,11 @@ See :ref:`config` for general help on configuring Mopidy. be published. Set to an empty string to disable Zeroconf for HTTP. + +.. confval:: http/allowed_origins + + A whitelist of domains allowed to perform Cross-Origin Resource Sharing + (CORS) requests. Entries must be in the format ``hostname``:``port``. + + If you want to access Mopidy's web server from a different web server, you + will need to add an entry for that server in this list. From 1b863b417b7d36ed51bce6986d9ce8759383df5f Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Wed, 11 Apr 2018 01:54:37 +0100 Subject: [PATCH 4/8] HTTP: New RPC CORS tests and fixed existing. --- tests/http/test_server.py | 42 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 39 insertions(+), 3 deletions(-) diff --git a/tests/http/test_server.py b/tests/http/test_server.py index bb1d8cf0..aa721f29 100644 --- a/tests/http/test_server.py +++ b/tests/http/test_server.py @@ -20,6 +20,7 @@ class HttpServerTest(tornado.testing.AsyncHTTPTestCase): 'port': 6680, 'static_dir': None, 'zeroconf': '', + 'allowed_origins': [], } } @@ -128,7 +129,8 @@ class MopidyRPCHandlerTest(HttpServerTest): def test_should_return_rpc_error(self): cmd = tornado.escape.json_encode({'action': 'get_version'}) - response = self.fetch('/mopidy/rpc', method='POST', body=cmd) + response = self.fetch('/mopidy/rpc', method='POST', body=cmd, headers={ + 'Content-Type': 'application/json'}) self.assertEqual( {'jsonrpc': '2.0', 'id': None, 'error': @@ -139,7 +141,8 @@ class MopidyRPCHandlerTest(HttpServerTest): def test_should_return_parse_error(self): cmd = '{[[[]}' - response = self.fetch('/mopidy/rpc', method='POST', body=cmd) + response = self.fetch('/mopidy/rpc', method='POST', body=cmd, headers={ + 'Content-Type': 'application/json'}) self.assertEqual( {'jsonrpc': '2.0', 'id': None, 'error': @@ -154,7 +157,8 @@ class MopidyRPCHandlerTest(HttpServerTest): 'id': 1, }) - response = self.fetch('/mopidy/rpc', method='POST', body=cmd) + response = self.fetch('/mopidy/rpc', method='POST', body=cmd, headers={ + 'Content-Type': 'application/json'}) self.assertEqual( {'jsonrpc': '2.0', 'id': 1, 'result': mopidy.__version__}, @@ -168,6 +172,38 @@ class MopidyRPCHandlerTest(HttpServerTest): self.assertIn('Cache-Control', response.headers) self.assertIn('Content-Type', response.headers) + def test_should_require_correct_content_type(self): + cmd = tornado.escape.json_encode({ + 'method': 'core.get_version', + 'params': [], + 'jsonrpc': '2.0', + 'id': 1, + }) + + response = self.fetch('/mopidy/rpc', method='POST', body=cmd, headers={ + 'Content-Type': 'text/plain'}) + + self.assertEqual(response.code, 415) + self.assertEqual( + response.reason, 'Content-Type must be application/json') + + def test_different_origin_returns_access_denied(self): + response = self.fetch('/mopidy/rpc', method='OPTIONS', headers={ + 'Host': 'me:6680', 'Origin': 'http://evil:666'}) + + self.assertEqual(response.code, 403) + self.assertEqual( + response.reason, 'Access denied for origin http://evil:666') + + def test_same_origin_returns_cors_headers(self): + response = self.fetch('/mopidy/rpc', method='OPTIONS', headers={ + 'Host': 'me:6680', 'Origin': 'http://me:6680'}) + + self.assertEqual( + response.headers['Access-Control-Allow-Origin'], 'http://me:6680') + self.assertEqual( + response.headers['Access-Control-Allow-Headers'], 'Content-Type') + class HttpServerWithStaticFilesTest(tornado.testing.AsyncHTTPTestCase): From 7caba4a05d5496f8757741b409e0523a11c5a9b6 Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Thu, 12 Apr 2018 20:41:37 +0100 Subject: [PATCH 5/8] docs: improved http/allowed_origins description. --- docs/ext/http.rst | 5 +++-- mopidy/http/handlers.py | 18 +++++++++--------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/docs/ext/http.rst b/docs/ext/http.rst index 6dc91782..8fd78a58 100644 --- a/docs/ext/http.rst +++ b/docs/ext/http.rst @@ -101,8 +101,9 @@ See :ref:`config` for general help on configuring Mopidy. .. confval:: http/allowed_origins - A whitelist of domains allowed to perform Cross-Origin Resource Sharing - (CORS) requests. Entries must be in the format ``hostname``:``port``. + A list of domains allowed to perform Cross-Origin Resource Sharing (CORS) + requests. Values should be in the format ``hostname:port`` and separated + by either a comma or newline. If you want to access Mopidy's web server from a different web server, you will need to add an entry for that server in this list. diff --git a/mopidy/http/handlers.py b/mopidy/http/handlers.py index c9d86804..f565c05c 100644 --- a/mopidy/http/handlers.py +++ b/mopidy/http/handlers.py @@ -20,7 +20,7 @@ logger = logging.getLogger(__name__) def make_mopidy_app_factory(apps, statics): def mopidy_app_factory(config, core): - origin_whitelist = { + allowed_origins = { x.lower() for x in config['http']['allowed_origins'] if x } return [ @@ -29,7 +29,7 @@ def make_mopidy_app_factory(apps, statics): }), (r'/rpc', JsonRpcHandler, { 'core': core, - 'origin_whitelist': origin_whitelist, + 'allowed_origins': allowed_origins, }), (r'/(.+)', StaticFileHandler, { 'path': os.path.join(os.path.dirname(__file__), 'data'), @@ -148,20 +148,20 @@ def set_mopidy_headers(request_handler): 'X-Mopidy-Version', mopidy.__version__.encode('utf-8')) -def check_origin(origin, request_headers, origin_whitelist): +def check_origin(origin, request_headers, allowed_origins): if origin is None: logger.debug('Origin was not set') return False - origin_whitelist.add(request_headers.get('Host', None)) + allowed_origins.add(request_headers.get('Host')) parsed_origin = urllib.parse.urlparse(origin).netloc.lower() - return parsed_origin and parsed_origin in origin_whitelist + return parsed_origin and parsed_origin in allowed_origins class JsonRpcHandler(tornado.web.RequestHandler): - def initialize(self, core, origin_whitelist): + def initialize(self, core, allowed_origins): self.jsonrpc = make_jsonrpc_wrapper(core) - self.origin_whitelist = origin_whitelist + self.allowed_origins = allowed_origins def head(self): self.set_extra_headers() @@ -198,9 +198,9 @@ class JsonRpcHandler(tornado.web.RequestHandler): self.set_header('Content-Type', 'application/json; utf-8') def options(self): - origin = self.request.headers.get('Origin', None) + origin = self.request.headers.get('Origin') if not check_origin( - origin, self.request.headers, self.origin_whitelist): + origin, self.request.headers, self.allowed_origins): self.set_status(403, 'Access denied for origin %s' % origin) return From 51741a7cbce685ce8b128e127599325ead56988d Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Thu, 12 Apr 2018 23:24:44 +0100 Subject: [PATCH 6/8] HTTP: Apply allowed_origins to Websocket requests also. --- docs/ext/http.rst | 5 +++-- mopidy/http/handlers.py | 8 ++++---- tests/http/test_handlers.py | 4 +++- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/docs/ext/http.rst b/docs/ext/http.rst index 8fd78a58..6ede2f25 100644 --- a/docs/ext/http.rst +++ b/docs/ext/http.rst @@ -102,8 +102,9 @@ See :ref:`config` for general help on configuring Mopidy. .. confval:: http/allowed_origins A list of domains allowed to perform Cross-Origin Resource Sharing (CORS) - requests. Values should be in the format ``hostname:port`` and separated - by either a comma or newline. + requests. This applies to both JSON-RPC and Websocket requests. Values + should be in the format ``hostname:port`` and separated by either a comma or + newline. If you want to access Mopidy's web server from a different web server, you will need to add an entry for that server in this list. diff --git a/mopidy/http/handlers.py b/mopidy/http/handlers.py index f565c05c..1a16d43b 100644 --- a/mopidy/http/handlers.py +++ b/mopidy/http/handlers.py @@ -26,6 +26,7 @@ def make_mopidy_app_factory(apps, statics): return [ (r'/ws/?', WebSocketHandler, { 'core': core, + 'allowed_origins': allowed_origins, }), (r'/rpc', JsonRpcHandler, { 'core': core, @@ -101,8 +102,9 @@ class WebSocketHandler(tornado.websocket.WebSocketHandler): # One callback per client to keep time we hold up the loop short loop.add_callback(functools.partial(_send_broadcast, client, msg)) - def initialize(self, core): + def initialize(self, core, allowed_origins): self.jsonrpc = make_jsonrpc_wrapper(core) + self.allowed_origins = allowed_origins def open(self): self.set_nodelay(True) @@ -137,9 +139,7 @@ class WebSocketHandler(tornado.websocket.WebSocketHandler): self.close() def check_origin(self, origin): - # Allow cross-origin WebSocket connections, like Tornado before 4.0 - # defaulted to. - return True + return check_origin(origin, self.request.headers, self.allowed_origins) def set_mopidy_headers(request_handler): diff --git a/tests/http/test_handlers.py b/tests/http/test_handlers.py index 3ad0c7b6..23472499 100644 --- a/tests/http/test_handlers.py +++ b/tests/http/test_handlers.py @@ -46,7 +46,9 @@ class WebSocketHandlerTest(tornado.testing.AsyncHTTPTestCase): def get_app(self): self.core = mock.Mock() return tornado.web.Application([ - (r'/ws/?', handlers.WebSocketHandler, {'core': self.core}) + (r'/ws/?', handlers.WebSocketHandler, { + 'core': self.core, 'allowed_origins': [] + }) ]) def connection(self): From 1d6e081171b425a3358dfe9e0cb85f8e602ffcf0 Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Sun, 15 Apr 2018 17:26:16 +0100 Subject: [PATCH 7/8] docs: mention that same-origin requests are always allowed. --- docs/ext/http.rst | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/docs/ext/http.rst b/docs/ext/http.rst index 6ede2f25..4b521b97 100644 --- a/docs/ext/http.rst +++ b/docs/ext/http.rst @@ -106,5 +106,7 @@ See :ref:`config` for general help on configuring Mopidy. should be in the format ``hostname:port`` and separated by either a comma or newline. - If you want to access Mopidy's web server from a different web server, you - will need to add an entry for that server in this list. + Same-origin requests (i.e. requests from Mopidy's web server) are always + allowed and so you don't need an entry for those. However, if your requests + originate from a different web server, you will need to add an entry for + that server in this list. From ae4dab65e48e1681290d98c295b015610e9798e3 Mon Sep 17 00:00:00 2001 From: Nick Steel Date: Sun, 15 Apr 2018 17:46:46 +0100 Subject: [PATCH 8/8] docs: added changelog entry --- docs/changelog.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index eddbeed6..f858fe16 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -27,6 +27,12 @@ Feature release. - File: Fix extraneous encoding of path. (PR: :issue:`1611`) +- HTTP: Protect RPC and Websocket interfaces against CSRF by blocking requests + that originiate from servers other than those specified in the new config + value :confval:`http/allowed_origins`. An artifact of this is that all + JSON-RPC requests must now always set the header + ``Content-Type: application/json``. (PR:1668 :issue:`1659`) + - MPD: Added ``idle`` to the list of available commands. (Fixes: :issue:`1593`, PR: :issue:`1597`)