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.

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

HSTS preload list check against API #1809

Open
wants to merge 1 commit into
base: 3.2
Choose a base branch
from

Conversation

tosticated
Copy link
Contributor

First attempt at addressing #1248: adding support for HSTS preload list lookups.

  • Used the API example as provided in HSTS Browser Preloading Check ( Like SSL Labs ) #1248. This seems to be the list that other lists (Firefox, Edge, etc.) are based on. A browser may still have an older copy of the list, but the state on hstspreload.org should propagate eventually.
  • The 'bulk' variable seems to be used for any entry that is added via the form on hstspreload.org, as opposed to any manually added domains such as mail.google.com for example (does not respond with preload header, but is in fact included in the list).
  • Of course only works when --phone-out is being set.

Feedback is welcome. Especially regarding the way the results are displayed. The idea is that the check is done even when the HSTS preload header is missing. This would show unintended and/or undesired situations such as having a list addition rejected because the 'preload' header itself is missing. Based on my understanding of how the list should work, I have currently assigned the following:

hsts_implementation_1
Explanation of some columns:

  • has preload header: indicates the response of the server that testssl.sh is testing
  • preloadedDomain == name: True if testssl.sh is currently testing the 'responsible domain'. False if it is a subdomain. A subdomain may not have 'preloaded' returned, but still can have a status like 'preloaded' according to the API.
  • bulk: if the API 'bulk' variable was true or false. When irrelevant for testssl.sh severity: "N/A".

As I was writing this check, it grew from just a simple 'status' to this entire 'lookup table'. If this is too much, it can also be changed back to just viewing the status without any severity and advice.

@drwetter
Copy link
Owner

Sorry, about the late reply. First: much appreciated, thanks!

I read and tried that in December. I can't get to it and need a few days for a consolidate answer!

[[ -z "$key" ]] && return 0

# Check if key exists in response. If not, the API may have changed or something.
! grep -Fiaqw "$key" $tmpfile && debugme echo "HSTS preloadlist key unrecognized: $key" && return 21
Copy link
Owner

Choose a reason for hiding this comment

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

This grep is legacy code. Can't this be changed into something like [[ "$tmpfile" =~ \ ${key}\ ]] ? (please doublecheck)

! grep -Fiaqw "$key" $tmpfile && debugme echo "HSTS preloadlist key unrecognized: $key" && return 21

# Check if there is a match, return 10 if there is, 20 if not
grep -Fiaqw "\"$key\": $value" $tmpfile && return 10 || return 20
Copy link
Owner

Choose a reason for hiding this comment

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

See above wrt legacy

out "$indent"; pr_bold " HSTS preload list "

# Check if the domain is also the preloadedDomain. If so, it *may* be correct that the server response header does not have 'preloaded' included.
check_hsts_preloadlist_match "$NODE" "preloadedDomain" "\"$NODE\""
Copy link
Owner

Choose a reason for hiding this comment

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

Is there any reason you use here and other places string why you pass the double quotes here? If they are needed at all I would rather handle this in the function and not in the call.

debugme echo "Temporary lookupvariable: $preloadcombined"

# Determine and show the outcome
case "$(check_hsts_preloadlist_value "$NODE" "status")" in
Copy link
Owner

Choose a reason for hiding this comment

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

for a plain string there should be no need to enclose it in quotes

case $key in
"status") values=("\"unknown\"" "\"pending\"" "\"rejected\"" "\"preloaded\"") ;;
"bulk") values=("true" "false") ;;
*) return 1 ;; # No supported key provided.
Copy link
Owner

Choose a reason for hiding this comment

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

Please double check here the usage of quotes. I believe for status and bulk they can be removed

For the array values I am not sure whether they need contain quotes.

check_hsts_preloadlist_value() {
local domain="$1"
local key="$2"
local values
Copy link
Owner

Choose a reason for hiding this comment

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

local -a value=() would declare the type better.

@drwetter
Copy link
Owner

drwetter commented Feb 4, 2021

Good work! You spend a lot of time for the lookup table and the decision table you provided is quite impressive.

The information on the screen for both easy cases (HSTS and no preload anywhere, HSTS and preload eerywhere)

image

I miss use case to test every variant of this. If I sleep over it I probably can test 1-2 more than the basic variants. If you could address the minor points in the code; I'll do a few more tests and then it should be set.

Thanks for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants