18F / pulse

How the federal .gov domain space is doing at best practices and policies.
Other
94 stars 56 forks source link

Prevent NoneType() >= int() #722

Closed siccovansas closed 6 years ago

siccovansas commented 6 years ago

I encountered an edge case where the scanned domain used maxage instead of max-age in its HSTS header. This results in pshtt listing True in the HSTS column but having an empty HSTS Max Age column. This in turn gave a TypeError: unorderable types: NoneType() >= int() error caused by line 581. We can prevent this by checking if hsts_age is True in the if-statement leading up to that line.

This has the added advantage that HSTS headers with a max-age of 0 are also given an HSTS value of 0 (no HSTS) in Pulse instead of an HSTS value of 1 (HSTS with too low max-age). The RFC specifies that a max-age of 0 means that the HSTS policy should be removed by the user agent. So the intended effect of the website is to have no HSTS. In a scan of 23k domains I actually got 44 domains listing a max-age of 0.

konklone commented 6 years ago

I encountered an edge case where the scanned domain used maxage instead of max-age in its HSTS header. This results in pshtt listing True in the HSTS column but having an empty HSTS Max Age column. This in turn gave a TypeError: unorderable types: NoneType() >= int() error caused by line 581. We can prevent this by checking if hsts_age is True in the if-statement leading up to that line.

Thanks for catching this! Looks good to me.

This has the added advantage that HSTS headers with a max-age of 0 are also given an HSTS value of 0 (no HSTS) in Pulse instead of an HSTS value of 1 (HSTS with too low max-age). The RFC specifies that a max-age of 0 means that the HSTS policy should be removed by the user agent. So the intended effect of the website is to have no HSTS. In a scan of 23k domains I actually got 44 domains listing a max-age of 0.

Actually, this shouldn't be a problem, because pshtt correctly marks the HSTS field as False if the max-age is 0, so the conditional would have already caught this situation:

https://github.com/dhs-ncats/pshtt/blob/master/pshtt/pshtt.py#L366-L368