From 8087ff620e2c6171696ff1353811153324b00bc6 Mon Sep 17 00:00:00 2001 From: pseudonym117 Date: Thu, 14 Oct 2021 03:16:49 -0500 Subject: [PATCH] sanitize region input (#197) region is sometimes user-controlled and allows for redirect attacks in order to steal API keys. This is no longer possible. --- README.rst | 2 + .../Handlers/IllegalArgumentError.py | 11 ++++++ src/riotwatcher/Handlers/SanitationHandler.py | 37 +++++++++++++++++++ src/riotwatcher/Handlers/__init__.py | 2 + src/riotwatcher/LolWatcher.py | 3 ++ src/riotwatcher/LorWatcher.py | 2 + src/riotwatcher/TftWatcher.py | 2 + src/riotwatcher/ValWatcher.py | 2 + src/riotwatcher/__init__.py | 3 +- src/riotwatcher/exceptions.py | 6 ++- src/riotwatcher/riotwatcher.py | 2 + tests/Handlers/test_SanitationHandler.py | 22 +++++++++++ tests/test_LolWatcher.py | 7 +++- tests/test_LorWatcher.py | 7 +++- tests/test_RiotWatcher.py | 7 +++- tests/test_TftWatcher.py | 7 +++- tests/test_ValWatcher.py | 7 +++- 17 files changed, 122 insertions(+), 7 deletions(-) create mode 100644 src/riotwatcher/Handlers/IllegalArgumentError.py create mode 100644 src/riotwatcher/Handlers/SanitationHandler.py create mode 100644 tests/Handlers/test_SanitationHandler.py diff --git a/README.rst b/README.rst index 9381555..25b5d60 100644 --- a/README.rst +++ b/README.rst @@ -175,6 +175,8 @@ Added startTime and endTime params for match v5 api Documented ddragon weirdness +Fix potential security issue with some common usage patterns + v3.1.4 - 8/11/2021 ~~~~~~~~~~~~~~~~~~ Add LolStatus-V4 API. Didnt realize this existed until now. diff --git a/src/riotwatcher/Handlers/IllegalArgumentError.py b/src/riotwatcher/Handlers/IllegalArgumentError.py new file mode 100644 index 0000000..2a9a7f8 --- /dev/null +++ b/src/riotwatcher/Handlers/IllegalArgumentError.py @@ -0,0 +1,11 @@ +class IllegalArgumentError(RuntimeError): + def __init__( + self, argument_name: str, argument_value: str, extra_message: str = None + ): + message = ( + f"Illegal value provided for argument '{argument_name}': '{argument_value}'" + ) + if extra_message: + message += f" - {extra_message}" + + super().__init__(message) diff --git a/src/riotwatcher/Handlers/SanitationHandler.py b/src/riotwatcher/Handlers/SanitationHandler.py new file mode 100644 index 0000000..a6db5df --- /dev/null +++ b/src/riotwatcher/Handlers/SanitationHandler.py @@ -0,0 +1,37 @@ +import re + +from . import IllegalArgumentError, RequestHandler + + +class SanitationHandler(RequestHandler): + """ + The SanitationHandler class provides some basic sanity checks to parameters to + ensure safe usage. + + Only check as of now is ensuring that region doesn't cause HTTP requests to unknown + servers, which would allow a malicious user to steal API keys. + """ + + def __init__(self): + self._region_expr = re.compile("[a-zA-Z0-9]+") + + def preview_request( + self, + region: str, + endpoint_name: str, + method_name: str, + url: str, + query_params: dict, + ): + """ + called before a request is processed. + + :param string endpoint_name: the name of the endpoint being requested + :param string method_name: the name of the method being requested + :param url: the URL that is being requested. + :param query_params: dict: the parameters to the url that is being queried, + e.g. ?key1=val&key2=val2 + """ + region_ok = self._region_expr.fullmatch(region) + if region_ok is None: + raise IllegalArgumentError("region", region) diff --git a/src/riotwatcher/Handlers/__init__.py b/src/riotwatcher/Handlers/__init__.py index bda0ead..c165570 100644 --- a/src/riotwatcher/Handlers/__init__.py +++ b/src/riotwatcher/Handlers/__init__.py @@ -1,8 +1,10 @@ +from .IllegalArgumentError import IllegalArgumentError from .RequestHandler import RequestHandler from .DeprecationHandler import DeprecationHandler from .DeserializerAdapter import DeserializerAdapter from .DictionaryDeserializer import DictionaryDeserializer from .RateLimiterAdapter import RateLimiterAdapter +from .SanitationHandler import SanitationHandler from .TypeCorrectorHandler import TypeCorrectorHandler from .ThrowOnErrorHandler import ApiError, ThrowOnErrorHandler diff --git a/src/riotwatcher/LolWatcher.py b/src/riotwatcher/LolWatcher.py index 8a3b9aa..c4a76e8 100644 --- a/src/riotwatcher/LolWatcher.py +++ b/src/riotwatcher/LolWatcher.py @@ -9,6 +9,7 @@ DeserializerAdapter, DictionaryDeserializer, RateLimiterAdapter, + SanitationHandler, ThrowOnErrorHandler, TypeCorrectorHandler, ) @@ -68,6 +69,7 @@ def __init__( if kernel_url: handler_chain = [ + SanitationHandler(), DeserializerAdapter(deserializer), ThrowOnErrorHandler(), TypeCorrectorHandler(), @@ -75,6 +77,7 @@ def __init__( ] else: handler_chain = [ + SanitationHandler(), DeserializerAdapter(deserializer), ThrowOnErrorHandler(), TypeCorrectorHandler(), diff --git a/src/riotwatcher/LorWatcher.py b/src/riotwatcher/LorWatcher.py index a9d8c53..ee5ab35 100644 --- a/src/riotwatcher/LorWatcher.py +++ b/src/riotwatcher/LorWatcher.py @@ -6,6 +6,7 @@ DeserializerAdapter, DictionaryDeserializer, RateLimiterAdapter, + SanitationHandler, ThrowOnErrorHandler, TypeCorrectorHandler, ) @@ -42,6 +43,7 @@ def __init__( raise ValueError("api_key must be set!") handler_chain = [ + SanitationHandler(), DeserializerAdapter(deserializer), ThrowOnErrorHandler(), TypeCorrectorHandler(), diff --git a/src/riotwatcher/TftWatcher.py b/src/riotwatcher/TftWatcher.py index d033919..5f416fb 100644 --- a/src/riotwatcher/TftWatcher.py +++ b/src/riotwatcher/TftWatcher.py @@ -6,6 +6,7 @@ DeserializerAdapter, DictionaryDeserializer, RateLimiterAdapter, + SanitationHandler, ThrowOnErrorHandler, TypeCorrectorHandler, ) @@ -43,6 +44,7 @@ def __init__( raise ValueError("api_key must be set!") handler_chain = [ + SanitationHandler(), DeserializerAdapter(deserializer), ThrowOnErrorHandler(), TypeCorrectorHandler(), diff --git a/src/riotwatcher/ValWatcher.py b/src/riotwatcher/ValWatcher.py index 2b38f3c..5058bcc 100644 --- a/src/riotwatcher/ValWatcher.py +++ b/src/riotwatcher/ValWatcher.py @@ -6,6 +6,7 @@ DeserializerAdapter, DictionaryDeserializer, RateLimiterAdapter, + SanitationHandler, ThrowOnErrorHandler, TypeCorrectorHandler, ) @@ -42,6 +43,7 @@ def __init__( raise ValueError("api_key must be set!") handler_chain = [ + SanitationHandler(), DeserializerAdapter(deserializer), ThrowOnErrorHandler(), TypeCorrectorHandler(), diff --git a/src/riotwatcher/__init__.py b/src/riotwatcher/__init__.py index 763a258..6d97433 100644 --- a/src/riotwatcher/__init__.py +++ b/src/riotwatcher/__init__.py @@ -1,6 +1,6 @@ from .__version__ import __author__, __title__, __version__ -from .exceptions import ApiError, TimeoutError +from .exceptions import ApiError, IllegalArgumentError, TimeoutError from .Deserializer import Deserializer from .RateLimiter import RateLimiter from .LolWatcher import LolWatcher @@ -20,5 +20,6 @@ "ValWatcher", "Handlers", "ApiError", + "IllegalArgumentError", "TimeoutError", ] diff --git a/src/riotwatcher/exceptions.py b/src/riotwatcher/exceptions.py index 8261179..4ca8834 100644 --- a/src/riotwatcher/exceptions.py +++ b/src/riotwatcher/exceptions.py @@ -1,6 +1,10 @@ import requests -from .Handlers import ApiError as _ApiError +from .Handlers import ( + ApiError as _ApiError, + IllegalArgumentError as _IllegalArgumentError, +) ApiError = _ApiError # should silence code analysis warning +IllegalArgumentError = _IllegalArgumentError TimeoutError = requests.exceptions.Timeout diff --git a/src/riotwatcher/riotwatcher.py b/src/riotwatcher/riotwatcher.py index e091211..26f63a4 100644 --- a/src/riotwatcher/riotwatcher.py +++ b/src/riotwatcher/riotwatcher.py @@ -6,6 +6,7 @@ DeserializerAdapter, DictionaryDeserializer, RateLimiterAdapter, + SanitationHandler, ThrowOnErrorHandler, TypeCorrectorHandler, ) @@ -42,6 +43,7 @@ def __init__( raise ValueError("api_key must be set!") handler_chain = [ + SanitationHandler(), DeserializerAdapter(deserializer), ThrowOnErrorHandler(), TypeCorrectorHandler(), diff --git a/tests/Handlers/test_SanitationHandler.py b/tests/Handlers/test_SanitationHandler.py new file mode 100644 index 0000000..a18d205 --- /dev/null +++ b/tests/Handlers/test_SanitationHandler.py @@ -0,0 +1,22 @@ +import pytest + +from riotwatcher.Handlers import IllegalArgumentError, SanitationHandler + + +@pytest.mark.common +@pytest.mark.unit +class TestSanitationHandler: + @pytest.mark.parametrize( + "region", ["na1", "Na1", "aN1", "enue", "americas", "europe", "euw1"] + ) + def test_valid_region_passes(self, region): + handler = SanitationHandler() + + handler.preview_request(region, None, None, None, None) + + @pytest.mark.parametrize("region", ["", "google.com/?stolen=", "+", "na1?"]) + def test_invalid_region_fails(self, region): + handler = SanitationHandler() + + with pytest.raises(IllegalArgumentError): + handler.preview_request(region, None, None, None, None) diff --git a/tests/test_LolWatcher.py b/tests/test_LolWatcher.py index 04deb69..f05b819 100644 --- a/tests/test_LolWatcher.py +++ b/tests/test_LolWatcher.py @@ -1,6 +1,6 @@ import pytest -from riotwatcher import LolWatcher +from riotwatcher import LolWatcher, IllegalArgumentError from riotwatcher._apis.league_of_legends import ( LolStatusApiV3, LolStatusApiV4, @@ -34,3 +34,8 @@ def test_uses_status_v3_when_false(self): def test_uses_status_v4_when_true(self): watcher = LolWatcher(api_key="RGAPI-this-is-a-fake", default_status_v4=True) assert isinstance(watcher.lol_status, LolStatusApiV4) + + def test_stealing_api_key_doesnt_work(self): + watcher = LolWatcher(api_key="RGAPI-this-is-a-fake") + with pytest.raises(IllegalArgumentError): + watcher.lol_status.shard_data("example.com/?stolen-request=") diff --git a/tests/test_LorWatcher.py b/tests/test_LorWatcher.py index 2573f9c..9147582 100644 --- a/tests/test_LorWatcher.py +++ b/tests/test_LorWatcher.py @@ -1,6 +1,6 @@ import pytest -from riotwatcher import LorWatcher +from riotwatcher import LorWatcher, IllegalArgumentError @pytest.mark.lor @@ -15,3 +15,8 @@ def test_allows_positional_api_key(self): def test_allows_keyword_api_key(self): LorWatcher(api_key="RGAPI-this-is-a-fake") + + def test_stealing_api_key_doesnt_work(self): + watcher = LorWatcher(api_key="RGAPI-this-is-a-fake") + with pytest.raises(IllegalArgumentError): + watcher.ranked.leaderboards("example.com/?stolen-request=") diff --git a/tests/test_RiotWatcher.py b/tests/test_RiotWatcher.py index 9dc205b..ed6f8ec 100644 --- a/tests/test_RiotWatcher.py +++ b/tests/test_RiotWatcher.py @@ -1,6 +1,6 @@ import pytest -from riotwatcher import RiotWatcher +from riotwatcher import RiotWatcher, IllegalArgumentError @pytest.mark.riot @@ -15,3 +15,8 @@ def test_allows_positional_api_key(self): def test_allows_keyword_api_key(self): RiotWatcher(api_key="RGAPI-this-is-a-fake") + + def test_stealing_api_key_doesnt_work(self): + watcher = RiotWatcher(api_key="RGAPI-this-is-a-fake") + with pytest.raises(IllegalArgumentError): + watcher.account.by_puuid("example.com/?stolen-request=", "fake-puuid") diff --git a/tests/test_TftWatcher.py b/tests/test_TftWatcher.py index 59ce58e..a2ae57b 100644 --- a/tests/test_TftWatcher.py +++ b/tests/test_TftWatcher.py @@ -1,6 +1,6 @@ import pytest -from riotwatcher import TftWatcher +from riotwatcher import TftWatcher, IllegalArgumentError @pytest.mark.tft @@ -15,3 +15,8 @@ def test_allows_positional_api_key(self): def test_allows_keyword_api_key(self): TftWatcher(api_key="RGAPI-this-is-a-fake") + + def test_stealing_api_key_doesnt_work(self): + watcher = TftWatcher(api_key="RGAPI-this-is-a-fake") + with pytest.raises(IllegalArgumentError): + watcher.summoner.by_puuid("example.com/?stolen-request=", "fake-puuid") diff --git a/tests/test_ValWatcher.py b/tests/test_ValWatcher.py index f740d04..72307a6 100644 --- a/tests/test_ValWatcher.py +++ b/tests/test_ValWatcher.py @@ -1,6 +1,6 @@ import pytest -from riotwatcher import ValWatcher +from riotwatcher import ValWatcher, IllegalArgumentError @pytest.mark.val @@ -15,3 +15,8 @@ def test_allows_positional_api_key(self): def test_allows_keyword_api_key(self): ValWatcher(api_key="RGAPI-this-is-a-fake") + + def test_stealing_api_key_doesnt_work(self): + watcher = ValWatcher(api_key="RGAPI-this-is-a-fake") + with pytest.raises(IllegalArgumentError): + watcher.match.by_id("example.com/?stolen-request=", "fake-match")