From 8ad85120b1257f08db91d9cc2bbaf1ebc4a1016e Mon Sep 17 00:00:00 2001 From: Nicholas McDonnell <50747025+mcdonnnj@users.noreply.github.com> Date: Tue, 13 Aug 2019 03:31:09 -0400 Subject: [PATCH 1/8] Changes HSTS header processing to more closely follow RFC specifications. Removed regex based string manipulation in favor of simple split calls. --- pshtt/pshtt.py | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/pshtt/pshtt.py b/pshtt/pshtt.py index a16debcb..44ac5218 100644 --- a/pshtt/pshtt.py +++ b/pshtt/pshtt.py @@ -529,28 +529,31 @@ def hsts_check(endpoint): endpoint.hsts = True endpoint.hsts_header = header - # Set max age to the string after max-age - # TODO: make this more resilient to pathological HSTS headers. - # handle multiple HSTS headers, requests comma-separates them - first_pass = re.split(r',\s?', header)[0] - second_pass = re.sub(r'\'', '', first_pass) + headers = [x.strip() for x in header.split(",")] + # Multiple HSTS headers does not conform to RFCs 7230-3.2.2 and 6797-6.1 + if len(headers) > 1: + logging.warning("Host is using an invalid HSTS header format: {}".format(header)) - temp = re.split(r';\s?', second_pass) + # Put all of the directives in the HSTS header into a dictionary + directive_list = [x.strip() for x in headers[0].split(";")] + directives = dict() + for directive in directive_list: + components = directive.split("=") + directives[components[0]] = "".join(components[1:]) or True - if "max-age" in header.lower(): - endpoint.hsts_max_age = int(temp[0][len("max-age="):]) + endpoint.hsts_max_age = int(directives["max-age"]) if "max-age" in directives else None if endpoint.hsts_max_age is None or endpoint.hsts_max_age <= 0: endpoint.hsts = False return # check if hsts includes sub domains - if 'includesubdomains' in header.lower(): + if "includesubdomains" in directives: endpoint.hsts_all_subdomains = True # Check is hsts has the preload flag - if 'preload' in header.lower(): + if "preload" in directives: endpoint.hsts_preload = True except Exception as err: endpoint.unknown_error = True From 888da78e556b63932aa9b568aa5013e37e347998 Mon Sep 17 00:00:00 2001 From: Nicholas McDonnell <50747025+mcdonnnj@users.noreply.github.com> Date: Tue, 13 Aug 2019 03:40:54 -0400 Subject: [PATCH 2/8] Add logic to reflect that the max-age directive is required for HSTS headers. --- pshtt/pshtt.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/pshtt/pshtt.py b/pshtt/pshtt.py index 44ac5218..bd3de935 100644 --- a/pshtt/pshtt.py +++ b/pshtt/pshtt.py @@ -526,7 +526,6 @@ def hsts_check(endpoint): endpoint.hsts = False return - endpoint.hsts = True endpoint.hsts_header = header # handle multiple HSTS headers, requests comma-separates them @@ -542,6 +541,13 @@ def hsts_check(endpoint): components = directive.split("=") directives[components[0]] = "".join(components[1:]) or True + # max-age is a required directive for HSTS headers + if "max-age" not in directives: + endpoint.hsts = False + return + + endpoint.hsts = True + endpoint.hsts_max_age = int(directives["max-age"]) if "max-age" in directives else None if endpoint.hsts_max_age is None or endpoint.hsts_max_age <= 0: From 2b15f924f71cdc407bdd66b3dfa0fa32456933c1 Mon Sep 17 00:00:00 2001 From: Nicholas McDonnell <50747025+mcdonnnj@users.noreply.github.com> Date: Tue, 13 Aug 2019 03:57:02 -0400 Subject: [PATCH 3/8] Clarify wording in the warning message about multiple HSTS headers. --- pshtt/pshtt.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pshtt/pshtt.py b/pshtt/pshtt.py index bd3de935..6a17660d 100644 --- a/pshtt/pshtt.py +++ b/pshtt/pshtt.py @@ -532,7 +532,7 @@ def hsts_check(endpoint): headers = [x.strip() for x in header.split(",")] # Multiple HSTS headers does not conform to RFCs 7230-3.2.2 and 6797-6.1 if len(headers) > 1: - logging.warning("Host is using an invalid HSTS header format: {}".format(header)) + logging.warning("Host is incorrectly returning multiple HSTS headers: {}".format(header)) # Put all of the directives in the HSTS header into a dictionary directive_list = [x.strip() for x in headers[0].split(";")] From 262e9919810d7fda6e9874066d0925622d57f01f Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Tue, 13 Aug 2019 08:25:20 -0400 Subject: [PATCH 4/8] Bump version from 0.6.6 to 0.6.7 --- pshtt/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pshtt/__init__.py b/pshtt/__init__.py index 7c9e66e2..400a1047 100644 --- a/pshtt/__init__.py +++ b/pshtt/__init__.py @@ -1 +1 @@ -__version__ = '0.6.6' +__version__ = '0.6.7' From 482d0254334324536490f373a99c134f94c917c9 Mon Sep 17 00:00:00 2001 From: Nicholas McDonnell <50747025+mcdonnnj@users.noreply.github.com> Date: Tue, 13 Aug 2019 11:46:08 -0400 Subject: [PATCH 5/8] Change the directive splitting to be clearer to what's going on. Add a comment to explain the hsts_max_age check rationale. --- pshtt/pshtt.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/pshtt/pshtt.py b/pshtt/pshtt.py index 6a17660d..20ef6a9a 100644 --- a/pshtt/pshtt.py +++ b/pshtt/pshtt.py @@ -538,22 +538,26 @@ def hsts_check(endpoint): directive_list = [x.strip() for x in headers[0].split(";")] directives = dict() for directive in directive_list: - components = directive.split("=") - directives[components[0]] = "".join(components[1:]) or True + if "=" in directive: + token, value = directive.split("=") + directives[token] = value + else: + directives[directive] = True # max-age is a required directive for HSTS headers if "max-age" not in directives: endpoint.hsts = False return - endpoint.hsts = True - endpoint.hsts_max_age = int(directives["max-age"]) if "max-age" in directives else None + # max-age is a time in seconds from when the response is received if endpoint.hsts_max_age is None or endpoint.hsts_max_age <= 0: endpoint.hsts = False return + endpoint.hsts = True + # check if hsts includes sub domains if "includesubdomains" in directives: endpoint.hsts_all_subdomains = True From cb7399f3f4cc004a9b014755a3b8f435bbada414 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Tue, 13 Aug 2019 12:19:21 -0400 Subject: [PATCH 6/8] Improve comment for HSTS max-age value check --- pshtt/pshtt.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/pshtt/pshtt.py b/pshtt/pshtt.py index 20ef6a9a..5e54f701 100644 --- a/pshtt/pshtt.py +++ b/pshtt/pshtt.py @@ -551,7 +551,13 @@ def hsts_check(endpoint): endpoint.hsts_max_age = int(directives["max-age"]) if "max-age" in directives else None - # max-age is a time in seconds from when the response is received + # According to section 6.1.1 of the HSTS RFC, a non-negative + # integer value is required. A zero value indicates that the + # user agent should cease considering the host a known HSTS + # host, and therefore only strictly positive values indicate + # the presence of HSTS protection. See + # https://tools.ietf.org/html/rfc6797#section-6.1.1 for more + # details. if endpoint.hsts_max_age is None or endpoint.hsts_max_age <= 0: endpoint.hsts = False return From 365b9d395f3d72215b2847ac203ceafcad157afd Mon Sep 17 00:00:00 2001 From: Nicholas McDonnell <50747025+mcdonnnj@users.noreply.github.com> Date: Tue, 13 Aug 2019 12:37:06 -0400 Subject: [PATCH 7/8] Change a check to only fail hsts on negative and non instead of zero or negative and none. Added links to relevant sections explaining multiple HSTS headers. --- pshtt/pshtt.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pshtt/pshtt.py b/pshtt/pshtt.py index 5e54f701..5fe25298 100644 --- a/pshtt/pshtt.py +++ b/pshtt/pshtt.py @@ -530,7 +530,10 @@ def hsts_check(endpoint): # handle multiple HSTS headers, requests comma-separates them headers = [x.strip() for x in header.split(",")] + # Multiple HSTS headers does not conform to RFCs 7230-3.2.2 and 6797-6.1 + # See https://tools.ietf.org/html/rfc7230#section-3.2.2 and + # https://tools.ietf.org/html/rfc6797#section-6.1 if len(headers) > 1: logging.warning("Host is incorrectly returning multiple HSTS headers: {}".format(header)) @@ -558,7 +561,7 @@ def hsts_check(endpoint): # the presence of HSTS protection. See # https://tools.ietf.org/html/rfc6797#section-6.1.1 for more # details. - if endpoint.hsts_max_age is None or endpoint.hsts_max_age <= 0: + if endpoint.hsts_max_age is None or endpoint.hsts_max_age < 0: endpoint.hsts = False return From 0f4bb9c63b0ef3ae1a63d3b0f0d2c98423325b3a Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Tue, 13 Aug 2019 12:41:11 -0400 Subject: [PATCH 8/8] Rework max-age comment a little Now that we're only checking strictly for compliance to the RFC, the wording needs to be reworked a little. --- pshtt/pshtt.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/pshtt/pshtt.py b/pshtt/pshtt.py index 5fe25298..528188d5 100644 --- a/pshtt/pshtt.py +++ b/pshtt/pshtt.py @@ -554,11 +554,15 @@ def hsts_check(endpoint): endpoint.hsts_max_age = int(directives["max-age"]) if "max-age" in directives else None - # According to section 6.1.1 of the HSTS RFC, a non-negative - # integer value is required. A zero value indicates that the - # user agent should cease considering the host a known HSTS - # host, and therefore only strictly positive values indicate - # the presence of HSTS protection. See + # According to section 6.1.1 of the HSTS RFC + # (https://tools.ietf.org/html/rfc6797#section-6.1.1), a + # non-negative integer value is required. + # + # Note that a zero value indicates that the user agent should + # cease considering the host a known HSTS host, and therefore + # only strictly positive values really indicate the presence + # of HSTS protection. Here, though, we're checking strictly + # for RFC compliance. See # https://tools.ietf.org/html/rfc6797#section-6.1.1 for more # details. if endpoint.hsts_max_age is None or endpoint.hsts_max_age < 0: