From 01349c73ced4d7ad8a215c5b2ee83231ec134925 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Sat, 20 Mar 2021 15:36:00 -0700 Subject: [PATCH] bug: use better host validation for `sub` While far from perfect, this is a little better about checking the sorts of values passed in the `sub` claim. It checks for ipv6 like as well as localhost in mailto and https forms. (Yeah, you'll still need to supply `https` since that's part of the RFC. Plus, letsEncrypt is a thing, so there's that.) I've also introduced a new option `--no-strict` which turns off `sub` checks. It has to be present, but what the value is can be up to you. I'll note that this kinda ruins what VAPID is supposed to be about. The `sub` is there so that if there's a problem with your subscription, Ops can reach out to you to help fix it rather than just straight up block you. Closes: #90 --- python/py_vapid/__init__.py | 19 ++++++++++++---- python/py_vapid/main.py | 4 +++- python/py_vapid/tests/test_vapid.py | 35 +++++++++++++++++++++++++++-- 3 files changed, 51 insertions(+), 7 deletions(-) diff --git a/python/py_vapid/__init__.py b/python/py_vapid/__init__.py index 1103582..892fe0a 100644 --- a/python/py_vapid/__init__.py +++ b/python/py_vapid/__init__.py @@ -38,13 +38,16 @@ class Vapid01(object): _public_key = None _schema = "WebPush" - def __init__(self, private_key=None): + def __init__(self, private_key=None, conf=None): """Initialize VAPID with an optional private key. :param private_key: A private key object :type private_key: ec.EllipticCurvePrivateKey """ + if conf is None: + conf = {} + self.conf = conf self.private_key = private_key if private_key: self._public_key = self.private_key.public_key() @@ -259,9 +262,11 @@ def _base_sign(self, claims): cclaims = copy.deepcopy(claims) if not cclaims.get('exp'): cclaims['exp'] = str(int(time.time()) + 86400) - if not re.match(r'mailto:.+@.+\..+', - cclaims.get('sub', ''), - re.IGNORECASE): + if not self.conf.get('no-strict', False): + valid = _check_sub(cclaims.get('sub', '')) + else: + valid = cclaims.get('sub') is not None + if not valid: raise VapidException( "Missing 'sub' from claims. " "'sub' is your admin email as a mailto: link.") @@ -345,5 +350,11 @@ def verify(cls, auth): verification_token=tokens[1] ) +def _check_sub(sub): + pattern =( + r"^(mailto:.+@((localhost|[%\w]+(\.[%\w]+)+|([0-9a-f]{1,4}):+([0-9a-f]{1,4})?)))|https:\/\/(localhost|\w+\.[\w\.]+|([0-9a-f]{1,4}:+)+([0-9a-f]{1,4})?)$" + ) + return re.match(pattern, sub, re.IGNORECASE) is not None + Vapid = Vapid02 diff --git a/python/py_vapid/main.py b/python/py_vapid/main.py index 56d0f52..9760b25 100644 --- a/python/py_vapid/main.py +++ b/python/py_vapid/main.py @@ -31,6 +31,8 @@ def main(): default=False, action="store_true") parser.add_argument('--json', help="dump as json", default=False, action="store_true") + parser.add_argumet('--no-strict', help='Do not be strict about "sub"', + default=False, action="store_true") parser.add_argument('--applicationServerKey', help="show applicationServerKey value", default=False, action="store_true") @@ -52,7 +54,7 @@ def main(): if answer == 'n': print("Sorry, can't do much for you then.") exit(1) - vapid = Vapid() + vapid = Vapid(conf=args) vapid.generate_keys() print("Generating private_key.pem") vapid.save_key('private_key.pem') diff --git a/python/py_vapid/tests/test_vapid.py b/python/py_vapid/tests/test_vapid.py index d6fd743..aaf2b05 100644 --- a/python/py_vapid/tests/test_vapid.py +++ b/python/py_vapid/tests/test_vapid.py @@ -7,7 +7,7 @@ from cryptography.hazmat.primitives import serialization from mock import patch, Mock -from py_vapid import Vapid01, Vapid02, VapidException +from py_vapid import Vapid01, Vapid02, VapidException, _check_sub from py_vapid.jwt import decode TEST_KEY_PRIVATE_DER = """ @@ -228,7 +228,12 @@ def test_bad_sign(self): self.assertRaises(VapidException, v.sign, {'sub': 'mailto:foo@bar.com', - 'aud': "https://p.example.com:8080/"}) + 'aud': "https://p.example.com:8080/"}) + + def test_ignore_sub(self): + v = Vapid02.from_file("/tmp/private") + v.conf['no-strict'] = True + assert v.sign({"sub": "foo", "aud": "http://localhost:8000"}) @patch('cryptography.hazmat.primitives.asymmetric' '.ec.EllipticCurvePublicNumbers') @@ -247,3 +252,29 @@ def test_invalid_sig(self, mm): decode, 'foo.bar.a', 'aaaa') + + def test_sub(self): + valid = [ + 'mailto:me@localhost', + 'mailto:me@1.2.3.4', + 'mailto:me@1234::', + 'mailto:me@1234::5678', + 'mailto:admin@example.org', + 'https://localhost', + 'https://8001::', + 'https://8001:1000:0001', + 'https://1.2.3.4' + ] + invalid = [ + 'mailto:@foobar.com', + 'mailto:example.org', + 'mailto:0123:', + 'mailto:::1234', + 'https://somehost', + 'https://xyz:123', + ] + + for val in valid: + assert _check_sub(val) is True + for val in invalid: + assert _check_sub(val) is False \ No newline at end of file