From b544fbf0561b01252f7947a0e848032c6244b3f0 Mon Sep 17 00:00:00 2001 From: thecontrarycat <1482576+thecontrarycat@users.noreply.github.com> Date: Mon, 2 Nov 2020 16:10:14 +0000 Subject: [PATCH 1/2] Update checking of cookies so that SameSite=None is treated as a valid entry if the cookie is also marked as Secure. SameSite; and SameSite=true; should both be invalid. Also only grant the bonus score if all session cookies have a valid SameSite attribute --- httpobs/scanner/analyzer/headers.py | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/httpobs/scanner/analyzer/headers.py b/httpobs/scanner/analyzer/headers.py index d54db337..cdd31485 100644 --- a/httpobs/scanner/analyzer/headers.py +++ b/httpobs/scanner/analyzer/headers.py @@ -281,7 +281,7 @@ def cookies(reqs: dict, expectation='cookies-secure-with-httponly-sessions') -> cookies-without-secure-flag-but-protected-by-hsts: Cookies don't have secure, but site uses HSTS cookies-session-without-secure-flag-but-protected-by-hsts: Same, but session cookie cookies-without-secure-flag: Cookies set without secure flag - cookies-samesite-flag-invalid: Cookies set with invalid SameSite value (must be either unset, Strict, or Lax) + cookies-samesite-flag-invalid: Cookies set with invalid SameSite value (must be either unset, Strict, Lax or None) cookies-session-without-secure-flag: Session cookies lack the Secure flag cookies-session-without-httponly-flag: Session cookies lack the HttpOnly flag cookies-not-found: No cookies found in HTTP requests @@ -322,6 +322,7 @@ def cookies(reqs: dict, expectation='cookies-secure-with-httponly-sessions') -> else: jar = {} + samesiteCount = 0 # There are certain cookies we ignore, because they are set by service providers and sites have # no control over them. @@ -335,12 +336,18 @@ def cookies(reqs: dict, expectation='cookies-secure-with-httponly-sessions') -> if key.lower() == 'httponly' and getattr(cookie, 'httponly') is False: cookie.httponly = True elif key.lower() == 'samesite' and getattr(cookie, 'samesite') is False: - if cookie._rest[key] in (None, True) or cookie._rest[key].strip().lower() == 'lax': + if cookie._rest[key].strip().lower() == 'lax': cookie.samesite = 'Lax' + samesiteCount += 1 output['sameSite'] = True elif cookie._rest[key].strip().lower() == 'strict': cookie.samesite = 'Strict' + samesiteCount += 1 output['sameSite'] = True + elif cookie._rest[key].strip().lower() == 'none': + cookie.samesite = 'None' + samesiteCount += 1 + output['sameSite'] = True else: output['result'] = only_if_worse('cookies-samesite-flag-invalid', output['result'], @@ -354,6 +361,11 @@ def cookies(reqs: dict, expectation='cookies-secure-with-httponly-sessions') -> sessionid = any(i in cookie.name.lower() for i in ('login', 'sess')) anticsrf = True if 'csrf' in cookie.name.lower() else False + if not cookie.secure and cookie.samesite == 'None': + output['result'] = only_if_worse('cookies-samesite-flag-invalid', + output['result'], + goodness) + if not cookie.secure and hsts: output['result'] = only_if_worse('cookies-without-secure-flag-but-protected-by-hsts', output['result'], @@ -388,7 +400,7 @@ def cookies(reqs: dict, expectation='cookies-secure-with-httponly-sessions') -> # Store whether or not we saw SameSite cookies, if cookies were set if output['result'] is None: - if output['sameSite']: + if output['sameSite'] and samesiteCount == len(jar): output['result'] = 'cookies-secure-with-httponly-sessions-and-samesite' else: output['result'] = 'cookies-secure-with-httponly-sessions' From 0488c3cf2472a4cb8cfdc57470805a7ff100451f Mon Sep 17 00:00:00 2001 From: thecontrarycat <1482576+thecontrarycat@users.noreply.github.com> Date: Wed, 11 Nov 2020 16:46:05 +0000 Subject: [PATCH 2/2] Improve the way we detect whether all cookies have a valid SameSite attribute. Fix failing tests due to None and Bool values not being strings. Add additional tests to cover the changed behaviour. --- httpobs/scanner/analyzer/headers.py | 24 ++-- httpobs/tests/unittests/test_headers.py | 172 ++++++++++++++++++++++-- 2 files changed, 169 insertions(+), 27 deletions(-) diff --git a/httpobs/scanner/analyzer/headers.py b/httpobs/scanner/analyzer/headers.py index cdd31485..db64734c 100644 --- a/httpobs/scanner/analyzer/headers.py +++ b/httpobs/scanner/analyzer/headers.py @@ -290,6 +290,9 @@ def cookies(reqs: dict, expectation='cookies-secure-with-httponly-sessions') -> expectation: test expectation pass: whether the site's configuration met its expectation result: short string describing the result of the test + sameSite: True if all session cookies have a valid SameSite attribute + False if any session cookie has an invalid or missing SameSite attribute + None if there are no session cookies """ output = { @@ -322,7 +325,6 @@ def cookies(reqs: dict, expectation='cookies-secure-with-httponly-sessions') -> else: jar = {} - samesiteCount = 0 # There are certain cookies we ignore, because they are set by service providers and sites have # no control over them. @@ -336,18 +338,13 @@ def cookies(reqs: dict, expectation='cookies-secure-with-httponly-sessions') -> if key.lower() == 'httponly' and getattr(cookie, 'httponly') is False: cookie.httponly = True elif key.lower() == 'samesite' and getattr(cookie, 'samesite') is False: - if cookie._rest[key].strip().lower() == 'lax': + samesiteVal = '' if cookie._rest[key] == None else str(cookie._rest[key]) + if samesiteVal.strip().lower() == 'lax': cookie.samesite = 'Lax' - samesiteCount += 1 - output['sameSite'] = True - elif cookie._rest[key].strip().lower() == 'strict': + elif samesiteVal.strip().lower() == 'strict': cookie.samesite = 'Strict' - samesiteCount += 1 - output['sameSite'] = True - elif cookie._rest[key].strip().lower() == 'none': + elif samesiteVal.strip().lower() == 'none': cookie.samesite = 'None' - samesiteCount += 1 - output['sameSite'] = True else: output['result'] = only_if_worse('cookies-samesite-flag-invalid', output['result'], @@ -400,11 +397,12 @@ def cookies(reqs: dict, expectation='cookies-secure-with-httponly-sessions') -> # Store whether or not we saw SameSite cookies, if cookies were set if output['result'] is None: - if output['sameSite'] and samesiteCount == len(jar): - output['result'] = 'cookies-secure-with-httponly-sessions-and-samesite' - else: + if any(c for c in session.cookies if c.samesite == False): output['result'] = 'cookies-secure-with-httponly-sessions' output['sameSite'] = False + else: + output['result'] = 'cookies-secure-with-httponly-sessions-and-samesite' + output['sameSite'] = True # Save the cookie jar output['data'] = jar if len(str(jar)) < 32768 else {} diff --git a/httpobs/tests/unittests/test_headers.py b/httpobs/tests/unittests/test_headers.py index 054d3e9d..fd52ac8f 100644 --- a/httpobs/tests/unittests/test_headers.py +++ b/httpobs/tests/unittests/test_headers.py @@ -420,7 +420,7 @@ def test_secure_with_httponly_sessions_and_samesite(self): value='bar') self.reqs['session'].cookies.set_cookie(cookie) - cookie = Cookie(name='SESSIONID_SAMESITE_LAX_TRUE', + cookie = Cookie(name='SESSIONID_SAMESITE_LAX', comment=None, comment_url=None, discard=False, @@ -433,7 +433,101 @@ def test_secure_with_httponly_sessions_and_samesite(self): port=443, port_specified=443, rfc2109=False, - rest={'HttpOnly': True, 'SameSite': True}, + rest={'HttpOnly': True, 'SameSite': 'Lax'}, + secure=True, + version=1, + value='bar') + self.reqs['session'].cookies.set_cookie(cookie) + + cookie = Cookie(name='SESSIONID_SAMESITE_NONE', + comment=None, + comment_url=None, + discard=False, + domain='mozilla.com', + domain_initial_dot=False, + domain_specified='mozilla.com', + expires=None, + path='/', + path_specified='/', + port=443, + port_specified=443, + rfc2109=False, + rest={'HttpOnly': True, 'SameSite': 'None'}, + secure=True, + version=1, + value='bar') + self.reqs['session'].cookies.set_cookie(cookie) + + result = cookies(self.reqs) + + self.assertEquals('cookies-secure-with-httponly-sessions-and-samesite', result['result']) + self.assertEquals({ + 'SESSIONID_SAMESITE_STRICT': { + 'domain': 'mozilla.com', + 'expires': None, + 'httponly': True, + 'max-age': None, + 'path': '/', + 'port': 443, + 'samesite': 'Strict', + 'secure': True}, + 'SESSIONID_SAMESITE_LAX': { + 'domain': 'mozilla.com', + 'expires': None, + 'httponly': True, + 'max-age': None, + 'path': '/', + 'port': 443, + 'samesite': 'Lax', + 'secure': True}, + 'SESSIONID_SAMESITE_NONE': { + 'domain': 'mozilla.com', + 'expires': None, + 'httponly': True, + 'max-age': None, + 'path': '/', + 'port': 443, + 'samesite': 'None', + 'secure': True} + }, + result['data']) + self.assertTrue(result['pass']) + self.assertTrue(result['sameSite']) + + def test_secure_with_httponly_sessions_and_samesite_not_awarded_if_not_all_cookies_samesite(self): + cookie = Cookie(name='SESSIONID_SAMESITE_STRICT', + comment=None, + comment_url=None, + discard=False, + domain='mozilla.com', + domain_initial_dot=False, + domain_specified='mozilla.com', + expires=None, + path='/', + path_specified='/', + port=443, + port_specified=443, + rfc2109=False, + rest={'HttpOnly': True, 'SameSite': 'Strict'}, + secure=True, + version=1, + value='bar') + self.reqs['session'].cookies.set_cookie(cookie) + + cookie = Cookie(name='SESSIONID_NO_SAMESITE', + comment=None, + comment_url=None, + discard=False, + domain='mozilla.com', + domain_initial_dot=False, + domain_specified='mozilla.com', + expires=None, + path='/', + path_specified='/', + port=443, + port_specified=443, + rfc2109=False, + rest={'HttpOnly': True}, secure=True, version=1, value='bar') @@ -458,7 +552,7 @@ def test_secure_with_httponly_sessions_and_samesite(self): value='bar') self.reqs['session'].cookies.set_cookie(cookie) - cookie = Cookie(name='SESSIONID_SAMESITE_LAX_NONE', + cookie = Cookie(name='SESSIONID_SAMESITE_NONE', comment=None, comment_url=None, discard=False, @@ -471,7 +565,7 @@ def test_secure_with_httponly_sessions_and_samesite(self): port=443, port_specified=443, rfc2109=False, - rest={'HttpOnly': True, 'SameSite': None}, + rest={'HttpOnly': True, 'SameSite': 'None'}, secure=True, version=1, value='bar') @@ -479,7 +573,7 @@ def test_secure_with_httponly_sessions_and_samesite(self): result = cookies(self.reqs) - self.assertEquals('cookies-secure-with-httponly-sessions-and-samesite', result['result']) + self.assertEquals('cookies-secure-with-httponly-sessions', result['result']) self.assertEquals({ 'SESSIONID_SAMESITE_STRICT': { 'domain': 'mozilla.com', @@ -489,16 +583,15 @@ def test_secure_with_httponly_sessions_and_samesite(self): 'path': '/', 'port': 443, 'samesite': 'Strict', - 'secure': True - }, - 'SESSIONID_SAMESITE_LAX_TRUE': { + 'secure': True}, + 'SESSIONID_NO_SAMESITE': { 'domain': 'mozilla.com', 'expires': None, 'httponly': True, 'max-age': None, 'path': '/', 'port': 443, - 'samesite': 'Lax', + 'samesite': False, 'secure': True}, 'SESSIONID_SAMESITE_LAX': { 'domain': 'mozilla.com', @@ -508,21 +601,20 @@ def test_secure_with_httponly_sessions_and_samesite(self): 'path': '/', 'port': 443, 'samesite': 'Lax', - 'secure': True - }, - 'SESSIONID_SAMESITE_LAX_NONE': { + 'secure': True}, + 'SESSIONID_SAMESITE_NONE': { 'domain': 'mozilla.com', 'expires': None, 'httponly': True, 'max-age': None, 'path': '/', 'port': 443, - 'samesite': 'Lax', + 'samesite': 'None', 'secure': True} }, result['data']) self.assertTrue(result['pass']) - self.assertTrue(result['sameSite']) + self.assertFalse(result['sameSite']) def test_anticsrf_without_samesite(self): cookie = Cookie(name='CSRFTOKEN', @@ -550,6 +642,58 @@ def test_anticsrf_without_samesite(self): self.assertFalse(result['pass']) self.assertFalse(result['sameSite']) + def test_samesite_invalid_empty(self): + cookie = Cookie(name='SESSIONID', + comment=None, + comment_url=None, + discard=False, + domain='mozilla.com', + domain_initial_dot=False, + domain_specified='mozilla.com', + expires=None, + path='/', + path_specified='/', + port=443, + port_specified=443, + rfc2109=False, + rest={'HttpOnly': True, 'SameSite': None}, + secure=True, + version=1, + value='bar') + self.reqs['session'].cookies.set_cookie(cookie) + + result = cookies(self.reqs) + + self.assertEquals('cookies-samesite-flag-invalid', result['result']) + self.assertFalse(result['pass']) + self.assertIsNone(result['sameSite']) + + def test_samesite_invalid_true(self): + cookie = Cookie(name='SESSIONID', + comment=None, + comment_url=None, + discard=False, + domain='mozilla.com', + domain_initial_dot=False, + domain_specified='mozilla.com', + expires=None, + path='/', + path_specified='/', + port=443, + port_specified=443, + rfc2109=False, + rest={'HttpOnly': True, 'SameSite': True}, + secure=True, + version=1, + value='bar') + self.reqs['session'].cookies.set_cookie(cookie) + + result = cookies(self.reqs) + + self.assertEquals('cookies-samesite-flag-invalid', result['result']) + self.assertFalse(result['pass']) + self.assertIsNone(result['sameSite']) + def test_samesite_invalid(self): cookie = Cookie(name='SESSIONID', comment=None,