From 08d37a4b0fb15e22e8c82abb8d22ffa4acf58975 Mon Sep 17 00:00:00 2001 From: Jack Robison Date: Mon, 25 May 2020 10:02:13 -0400 Subject: [PATCH 1/4] add `allowed_origin` to config -raise 403 error if a request doesn't have a matching origin --- lbry/conf.py | 1 + lbry/extras/daemon/daemon.py | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/lbry/conf.py b/lbry/conf.py index e9b293933..9e08e842c 100644 --- a/lbry/conf.py +++ b/lbry/conf.py @@ -625,6 +625,7 @@ class Config(CLIConfig): previous_names=['upload_log', 'upload_log', 'share_debug_info'] ) track_bandwidth = Toggle("Track bandwidth usage", True) + allowed_origin = String("Allowed origin header for api calls, use * to allow all", 'null') # media server streaming_server = String('Host name and port to serve streaming media over range requests', diff --git a/lbry/extras/daemon/daemon.py b/lbry/extras/daemon/daemon.py index 756ff6bc7..15a82e38b 100644 --- a/lbry/extras/daemon/daemon.py +++ b/lbry/extras/daemon/daemon.py @@ -566,6 +566,11 @@ class Daemon(metaclass=JSONRPCServerType): log.info("finished shutting down") async def handle_old_jsonrpc(self, request): + origin = request.headers.get('Origin', 'null') + origin = None if origin == 'null' else origin + if origin != self.conf.allowed_origin != '*': + log.warning("API request from origin '%s' is not allowed", origin) + raise web.HTTPForbidden() data = await request.json() params = data.get('params', {}) include_protobuf = params.pop('include_protobuf', False) if isinstance(params, dict) else False From ee0aabda1d5f228f9e26ea7be477a6d3af51a3cd Mon Sep 17 00:00:00 2001 From: Lex Berezhny Date: Wed, 3 Jun 2020 13:28:32 -0400 Subject: [PATCH 2/4] backwards compatible allowed_origin, default browsers not allowed --- lbry/conf.py | 4 ++- lbry/extras/daemon/daemon.py | 7 ++-- lbry/extras/daemon/security.py | 5 +++ .../lbrynet_daemon/test_allowed_origin.py | 36 +++++++++++++++++++ 4 files changed, 47 insertions(+), 5 deletions(-) create mode 100644 lbry/extras/daemon/security.py create mode 100644 tests/unit/lbrynet_daemon/test_allowed_origin.py diff --git a/lbry/conf.py b/lbry/conf.py index 9e08e842c..deb9222f2 100644 --- a/lbry/conf.py +++ b/lbry/conf.py @@ -625,7 +625,9 @@ class Config(CLIConfig): previous_names=['upload_log', 'upload_log', 'share_debug_info'] ) track_bandwidth = Toggle("Track bandwidth usage", True) - allowed_origin = String("Allowed origin header for api calls, use * to allow all", 'null') + allowed_origin = String( + "Allowed `Origin` header value for API request (sent by browser), use * to allow " + "all hosts; default is to only allow API requests with no `Origin` value.", "") # media server streaming_server = String('Host name and port to serve streaming media over range requests', diff --git a/lbry/extras/daemon/daemon.py b/lbry/extras/daemon/daemon.py index 15a82e38b..2c121ff8c 100644 --- a/lbry/extras/daemon/daemon.py +++ b/lbry/extras/daemon/daemon.py @@ -47,6 +47,7 @@ from lbry.extras.daemon.componentmanager import ComponentManager from lbry.extras.daemon.json_response_encoder import JSONResponseEncoder from lbry.extras.daemon import comment_client from lbry.extras.daemon.undecorated import undecorated +from lbry.extras.daemon.security import is_request_allowed from lbry.file_analysis import VideoFileAnalyzer from lbry.schema.claim import Claim from lbry.schema.url import URL @@ -566,10 +567,8 @@ class Daemon(metaclass=JSONRPCServerType): log.info("finished shutting down") async def handle_old_jsonrpc(self, request): - origin = request.headers.get('Origin', 'null') - origin = None if origin == 'null' else origin - if origin != self.conf.allowed_origin != '*': - log.warning("API request from origin '%s' is not allowed", origin) + if is_request_allowed(request, self.conf): + log.warning("API request from origin '%s' is not allowed", request.headers.get('Origin')) raise web.HTTPForbidden() data = await request.json() params = data.get('params', {}) diff --git a/lbry/extras/daemon/security.py b/lbry/extras/daemon/security.py new file mode 100644 index 000000000..18a8326ec --- /dev/null +++ b/lbry/extras/daemon/security.py @@ -0,0 +1,5 @@ +def is_request_allowed(request, conf) -> bool: + origin = request.headers.get('Origin', 'null') + if origin == 'null' or conf.allowed_origin in ('*', origin): + return True + return False diff --git a/tests/unit/lbrynet_daemon/test_allowed_origin.py b/tests/unit/lbrynet_daemon/test_allowed_origin.py new file mode 100644 index 000000000..7d679a3dc --- /dev/null +++ b/tests/unit/lbrynet_daemon/test_allowed_origin.py @@ -0,0 +1,36 @@ +import unittest + +from aiohttp.test_utils import make_mocked_request as request + +from lbry.testcase import AsyncioTestCase +from lbry.conf import Config +from lbry.extras.daemon.security import is_request_allowed as allowed + + +class TestAllowedOrigin(unittest.TestCase): + + def test_allowed_origin_default(self): + conf = Config() + # no Origin is always allowed + self.assertTrue(allowed(request('GET', '/'), conf)) + # some clients send Origin: null (eg, https://github.com/electron/electron/issues/7931) + self.assertTrue(allowed(request('GET', '/', headers={'Origin': 'null'}), conf)) + # deny all other Origins + self.assertFalse(allowed(request('GET', '/', headers={'Origin': 'localhost'}), conf)) + self.assertFalse(allowed(request('GET', '/', headers={'Origin': 'hackers.com'}), conf)) + + def test_allowed_origin_star(self): + conf = Config(allowed_origin='*') + # everything is allowed + self.assertTrue(allowed(request('GET', '/'), conf)) + self.assertTrue(allowed(request('GET', '/', headers={'Origin': 'null'}), conf)) + self.assertTrue(allowed(request('GET', '/', headers={'Origin': 'localhost'}), conf)) + self.assertTrue(allowed(request('GET', '/', headers={'Origin': 'hackers.com'}), conf)) + + def test_allowed_origin_specified(self): + conf = Config(allowed_origin='localhost') + # no origin and only localhost are allowed + self.assertTrue(allowed(request('GET', '/'), conf)) + self.assertTrue(allowed(request('GET', '/', headers={'Origin': 'null'}), conf)) + self.assertTrue(allowed(request('GET', '/', headers={'Origin': 'localhost'}), conf)) + self.assertFalse(allowed(request('GET', '/', headers={'Origin': 'hackers.com'}), conf)) From f3ee6603de9b4365b30b331694c903b972b4c4a9 Mon Sep 17 00:00:00 2001 From: Lex Berezhny Date: Wed, 3 Jun 2020 13:55:20 -0400 Subject: [PATCH 3/4] improve allowed_origin request handling --- lbry/extras/daemon/daemon.py | 6 ++--- lbry/extras/daemon/security.py | 24 +++++++++++++++++++ .../lbrynet_daemon/test_allowed_origin.py | 20 +++++++++++++++- 3 files changed, 45 insertions(+), 5 deletions(-) diff --git a/lbry/extras/daemon/daemon.py b/lbry/extras/daemon/daemon.py index 2c121ff8c..7508c1b5e 100644 --- a/lbry/extras/daemon/daemon.py +++ b/lbry/extras/daemon/daemon.py @@ -47,7 +47,7 @@ from lbry.extras.daemon.componentmanager import ComponentManager from lbry.extras.daemon.json_response_encoder import JSONResponseEncoder from lbry.extras.daemon import comment_client from lbry.extras.daemon.undecorated import undecorated -from lbry.extras.daemon.security import is_request_allowed +from lbry.extras.daemon.security import ensure_request_allowed from lbry.file_analysis import VideoFileAnalyzer from lbry.schema.claim import Claim from lbry.schema.url import URL @@ -567,9 +567,7 @@ class Daemon(metaclass=JSONRPCServerType): log.info("finished shutting down") async def handle_old_jsonrpc(self, request): - if is_request_allowed(request, self.conf): - log.warning("API request from origin '%s' is not allowed", request.headers.get('Origin')) - raise web.HTTPForbidden() + ensure_request_allowed(request, self.conf) data = await request.json() params = data.get('params', {}) include_protobuf = params.pop('include_protobuf', False) if isinstance(params, dict) else False diff --git a/lbry/extras/daemon/security.py b/lbry/extras/daemon/security.py index 18a8326ec..80fe0ea93 100644 --- a/lbry/extras/daemon/security.py +++ b/lbry/extras/daemon/security.py @@ -1,3 +1,27 @@ +import logging +from aiohttp import web + +log = logging.getLogger(__name__) + + +def ensure_request_allowed(request, conf): + if is_request_allowed(request, conf): + return + if conf.allowed_origin: + log.warning( + "API requests with Origin '%s' are not allowed, " + "configuration 'allowed_origin' limits requests to: '%s'", + request.headers.get('Origin'), conf.allowed_origin + ) + else: + log.warning( + "API requests with Origin '%s' are not allowed, " + "update configuration 'allowed_origin' to enable this origin.", + request.headers.get('Origin') + ) + raise web.HTTPForbidden() + + def is_request_allowed(request, conf) -> bool: origin = request.headers.get('Origin', 'null') if origin == 'null' or conf.allowed_origin in ('*', origin): diff --git a/tests/unit/lbrynet_daemon/test_allowed_origin.py b/tests/unit/lbrynet_daemon/test_allowed_origin.py index 7d679a3dc..531863ce4 100644 --- a/tests/unit/lbrynet_daemon/test_allowed_origin.py +++ b/tests/unit/lbrynet_daemon/test_allowed_origin.py @@ -1,10 +1,11 @@ import unittest from aiohttp.test_utils import make_mocked_request as request +from aiohttp.web import HTTPForbidden from lbry.testcase import AsyncioTestCase from lbry.conf import Config -from lbry.extras.daemon.security import is_request_allowed as allowed +from lbry.extras.daemon.security import is_request_allowed as allowed, ensure_request_allowed as ensure class TestAllowedOrigin(unittest.TestCase): @@ -34,3 +35,20 @@ class TestAllowedOrigin(unittest.TestCase): self.assertTrue(allowed(request('GET', '/', headers={'Origin': 'null'}), conf)) self.assertTrue(allowed(request('GET', '/', headers={'Origin': 'localhost'}), conf)) self.assertFalse(allowed(request('GET', '/', headers={'Origin': 'hackers.com'}), conf)) + + def test_ensure_default(self): + conf = Config() + ensure(request('GET', '/'), conf) + with self.assertLogs() as log: + with self.assertRaises(HTTPForbidden): + ensure(request('GET', '/', headers={'Origin': 'localhost'}), conf) + self.assertIn("'localhost' are not allowed", log.output[0]) + + def test_ensure_specific(self): + conf = Config(allowed_origin='localhost') + ensure(request('GET', '/', headers={'Origin': 'localhost'}), conf) + with self.assertLogs() as log: + with self.assertRaises(HTTPForbidden): + ensure(request('GET', '/', headers={'Origin': 'hackers.com'}), conf) + self.assertIn("'hackers.com' are not allowed", log.output[0]) + self.assertIn("'allowed_origin' limits requests to: 'localhost'", log.output[0]) From 7296c7df1a14be669385e2144cfe71a3060c9c6f Mon Sep 17 00:00:00 2001 From: Lex Berezhny Date: Wed, 3 Jun 2020 14:19:16 -0400 Subject: [PATCH 4/4] Origin: null no longer allowed --- lbry/extras/daemon/security.py | 10 ++++++---- tests/unit/lbrynet_daemon/test_allowed_origin.py | 7 +++---- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/lbry/extras/daemon/security.py b/lbry/extras/daemon/security.py index 80fe0ea93..c6ecde6d8 100644 --- a/lbry/extras/daemon/security.py +++ b/lbry/extras/daemon/security.py @@ -23,7 +23,9 @@ def ensure_request_allowed(request, conf): def is_request_allowed(request, conf) -> bool: - origin = request.headers.get('Origin', 'null') - if origin == 'null' or conf.allowed_origin in ('*', origin): - return True - return False + origin = request.headers.get('Origin') + return ( + origin is None or + origin == conf.allowed_origin or + conf.allowed_origin == '*' + ) diff --git a/tests/unit/lbrynet_daemon/test_allowed_origin.py b/tests/unit/lbrynet_daemon/test_allowed_origin.py index 531863ce4..230210202 100644 --- a/tests/unit/lbrynet_daemon/test_allowed_origin.py +++ b/tests/unit/lbrynet_daemon/test_allowed_origin.py @@ -12,11 +12,10 @@ class TestAllowedOrigin(unittest.TestCase): def test_allowed_origin_default(self): conf = Config() - # no Origin is always allowed + # lack of Origin is always allowed self.assertTrue(allowed(request('GET', '/'), conf)) - # some clients send Origin: null (eg, https://github.com/electron/electron/issues/7931) - self.assertTrue(allowed(request('GET', '/', headers={'Origin': 'null'}), conf)) # deny all other Origins + self.assertFalse(allowed(request('GET', '/', headers={'Origin': 'null'}), conf)) self.assertFalse(allowed(request('GET', '/', headers={'Origin': 'localhost'}), conf)) self.assertFalse(allowed(request('GET', '/', headers={'Origin': 'hackers.com'}), conf)) @@ -32,8 +31,8 @@ class TestAllowedOrigin(unittest.TestCase): conf = Config(allowed_origin='localhost') # no origin and only localhost are allowed self.assertTrue(allowed(request('GET', '/'), conf)) - self.assertTrue(allowed(request('GET', '/', headers={'Origin': 'null'}), conf)) self.assertTrue(allowed(request('GET', '/', headers={'Origin': 'localhost'}), conf)) + self.assertFalse(allowed(request('GET', '/', headers={'Origin': 'null'}), conf)) self.assertFalse(allowed(request('GET', '/', headers={'Origin': 'hackers.com'}), conf)) def test_ensure_default(self):