Merge pull request #1668 from kingosticks/fix/cors

Protect RPC interface against CSRF

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`)
 

docs/ext/http.rst

@@ -98,3 +98,15 @@ 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 list of domains allowed to perform Cross-Origin Resource Sharing (CORS)
+    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.
+    
+    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.

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):

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 =

mopidy/http/handlers.py

@@ -11,6 +11,7 @@ import tornado.websocket
 
 import mopidy
 from mopidy import core, models
+from mopidy.compat import urllib
 from mopidy.internal import encoding, jsonrpc
 
 
@@ -19,12 +20,17 @@ logger = logging.getLogger(__name__)
 
 def make_mopidy_app_factory(apps, statics):
     def mopidy_app_factory(config, core):
+        allowed_origins = {
+            x.lower() for x in config['http']['allowed_origins'] if x
+        }
         return [
             (r'/ws/?', WebSocketHandler, {
                 'core': core,
+                'allowed_origins': allowed_origins,
             }),
             (r'/rpc', JsonRpcHandler, {
                 'core': core,
+                'allowed_origins': allowed_origins,
             }),
             (r'/(.+)', StaticFileHandler, {
                 'path': os.path.join(os.path.dirname(__file__), 'data'),
@@ -96,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)
@@ -132,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
+        return check_origin(origin, self.request.headers, self.allowed_origins)
-        # defaulted to.
-        return True
 
 
 def set_mopidy_headers(request_handler):
@@ -143,16 +148,31 @@ def set_mopidy_headers(request_handler):
         'X-Mopidy-Version', mopidy.__version__.encode('utf-8'))
 
 
+def check_origin(origin, request_headers, allowed_origins):
+    if origin is None:
+        logger.debug('Origin was not set')
+        return False
+    allowed_origins.add(request_headers.get('Host'))
+    parsed_origin = urllib.parse.urlparse(origin).netloc.lower()
+    return parsed_origin and parsed_origin in allowed_origins
+
+
 class JsonRpcHandler(tornado.web.RequestHandler):
 
-    def initialize(self, core):
+    def initialize(self, core, allowed_origins):
         self.jsonrpc = make_jsonrpc_wrapper(core)
+        self.allowed_origins = allowed_origins
 
     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(415, 'Content-Type must be application/json')
+            return
+
         data = self.request.body
         if not data:
             return
@@ -177,6 +197,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')
+        if not check_origin(
+                origin, self.request.headers, self.allowed_origins):
+            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):
 

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):

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):