Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update HSTS check #203

Draft
wants to merge 9 commits into
base: develop
Choose a base branch
from
Draft

Update HSTS check #203

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8ad8512
Changes HSTS header processing to more closely follow RFC specificati…
mcdonnnj Aug 13, 2019
888da78
Add logic to reflect that the max-age directive is required for HSTS …
mcdonnnj Aug 13, 2019
2b15f92
Clarify wording in the warning message about multiple HSTS headers.
mcdonnnj Aug 13, 2019
262e991
Bump version from 0.6.6 to 0.6.7
jsf9k Aug 13, 2019
482d025
Change the directive splitting to be clearer to what's going on. Add …
mcdonnnj Aug 13, 2019
3517b0b
Merge branch 'update_hsts_logic' of github.com:cisagov/pshtt into upd…
mcdonnnj Aug 13, 2019
cb7399f
Improve comment for HSTS max-age value check
jsf9k Aug 13, 2019
365b9d3
Change a check to only fail hsts on negative and non instead of zero …
mcdonnnj Aug 13, 2019
0f4bb9c
Rework max-age comment a little
jsf9k Aug 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pshtt/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
__version__ = '0.6.6'
__version__ = '0.6.7'
31 changes: 20 additions & 11 deletions pshtt/pshtt.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -526,31 +526,40 @@ def hsts_check(endpoint):
endpoint.hsts = False
return

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 incorrectly returning multiple HSTS headers: {}".format(header))
jsf9k marked this conversation as resolved.
Show resolved Hide resolved

# 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
jsf9k marked this conversation as resolved.
Show resolved Hide resolved

# max-age is a required directive for HSTS headers
if "max-age" not in directives:
endpoint.hsts = False
return

temp = re.split(r';\s?', second_pass)
endpoint.hsts = 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:
jsf9k marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is an argument here for letting endpoint.hsts indicate only the presence of a syntactically correct HSTS header. That value can be combined with endpoint.hsts_max_age and endpoint.hsts_all_subdomains to determine if the domain "supports HSTS." To that end, I think it makes sense to change endpoint.hsts_max_age <= 0 to endpoint.hsts_max_age < 0 since a max-age value of zero is valid according to the RFC.

What do you think, @mcdonnnj?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree on changing <= to < and only checking for syntactical correctness here. That makes sense for what this part of the code is trying to do (which is just establish that we're getting a valid header and what it contains from the response).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsf9k @mcdonnnj I understand wanting to impart valid HSTS syntax, but rather than saying HSTS=True when the max-age is set to zero, could we instead Fail "HSTS" while still reporting the HSTS header in "HSTS Header" since technically the a value of zero means "forget me as an HSTS host/I don't do HSTS"? Alternatively, we could relabel the "HSTS" column as "Valid HSTS" if you'd prefer that?

Copy link
Member

@jsf9k jsf9k Aug 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@climber-girl A valid of zero is valid HSTS, according to the RFC.

Don't forget that pshtt is not the end of the line when it comes to the BOD reporting. The pshtt results are interpreted via cisagov/pshtt_reporter, and that is where the actual report is created. In this case, they would still fail "Supports HSTS" in the report, since max-age is too small.

In the past we have tried to make pshtt and trustymail simply collect information that is then interpreted by the reporting code.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsf9k I understand that it is valid per the RFC, hence the secondary alternative I proposed to relabel the current HSTS column that is reported (which would I guess be included in the pshtt_reporter?). For the purposes of collecting info with pshtt, I'm fine with the change you and @mcdonnnj discussed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already thought that the HSTS column was referring to whether it was valid since it can already be False and still have data in the HSTS Header column. I don't think the column name needs to be changed since that is already how it is used and there may be downstream effects from changing the column name. Generally, though, I like the trustymail way of having one column for existence and then another column for validity.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@climber-girl, if the value is zero and the HSTS column is True, then the Domain Uses Strong HSTS column will still be False. Therefore the domain will "fail HSTS" in the HTTPS report. The HSTS column in the raw pshtt results only says whether a valid HSTS header is being served, but the Domain Uses Strong HSTS includes the max-age check.

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
Expand Down