1328460 - Don't HSTS priming requests on non-standard ports or IP addresses

Status: VERIFIED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Kate McKinley [:kmckinley, :Kate]]

When mixed-content is encountered on a non-standard port, HSTS priming does not know what the new port (if any) should be. The HSTS spec says that it applies to hosts, so all ports on a host should be upgraded (:80->:443, all others preserve the port, see https://tools.ietf.org/html/rfc6797#section-8.3). Some prior discussion in bug 613645 on the applicability of HSTS to other ports.

This causes the problem in bug 1325680 where we attempt a TLS-protected connection to the web platform tests web server, which is interpretes as an invalid HTTP request. See bug 1182569 and 1246540 where web platform tests have additional preferences added to disable HSTS priming.

Comment 1

[Daniel Veditz [:dveditz]]

The test server shouldn't be failing on a HEAD request -- that's clearly not HSTS priming's fault. Any client/program could in theory be hitting that server with a HEAD request and it ought to be robust. In practice it's a test server insulated from the internet and it becomes your problem because it's a blocker.

That said, it doesn't make a lot of sense to do priming on non-standard ports. As you asked, what port would you ping? If the page worked with http://foo.bar:123/ then "upgrading" to https://foo.bar:123/ is going to fail -- that's not a TLS server. If the original link was broken because it was a TLS server and not HTTP then why bother trying to fix it?

There might be an edge case where an entire website turned on TLS and just couldn't re-write all their links, and so "upgrade-insecure-requests" makes sense. But I don't think we need to throw HSTS priming on top of that. Doing it for standard ports only ought to be fine.

tossing my needinfo? to rbarnes for a sanity check.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

I think Dan is right and we shouldn't do HSTS priming for non standard ports. FWIW 'upgrade-insecure-requests' spec [1] says:

4.1.6 If request’s urls port is 80, set request’s urls port to 443.
Note: This will only change the URL’s port if the port is explicitly set to 80. If the port is not set, or if it is set to some non-standard value, the user agent will not modify that value. This implementation makes the same tradeoffs as HSTS (see [RFC6797], and specifically step #5 of Section 8.3, and item #6 in Appendix A).

[1] https://www.w3.org/TR/upgrade-insecure-requests/#upgrade-request

Comment 3

[Kate McKinley [:kmckinley, :Kate]]

I'm inclined to agree, and updated the bug to reflect. We also shouldn't be priming on IP addresses as we will never HSTS upgrade them.

Comment 4

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Bug 1328460 - Don't send priming to IP or non-standard ports

Review commit: https://reviewboard.mozilla.org/r/102324/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102324/

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8823835 [details]
Bug 1328460 - Don't send priming to IP or non-standard ports

https://reviewboard.mozilla.org/r/102324/#review102906

::: dom/security/nsMixedContentBlocker.cpp:67
(Diff revision 1)
>  // Default HSTS Priming failure timeout to 7 days, in seconds
>  uint32_t nsMixedContentBlocker::sHSTSPrimingCacheTimeout = (60 * 24 * 7);
>  
> +bool
> +IsEligibleForHSTSPriming(bool aIsHttpScheme, nsIURI* aContentLocation) {
> +  nsresult rv;

nit: move nsresult rv to first use (a little further down).

::: dom/security/nsMixedContentBlocker.cpp:71
(Diff revision 1)
> +IsEligibleForHSTSPriming(bool aIsHttpScheme, nsIURI* aContentLocation) {
> +  nsresult rv;
> +
> +  if (!aIsHttpScheme) {
> +    return false;
> +  }

I know you have already the information about isHTTPScheme from the callsite, but it seems weired and error prone to pass both, I would just pass
IsEligibleForHSTSPriming(nsIURI* aContentLocation)
and then do the work again.

::: dom/security/nsMixedContentBlocker.cpp:74
(Diff revision 1)
> +  if (!aIsHttpScheme) {
> +    return false;
> +  }
> +
> +  int32_t port = -1;
> +  rv = aContentLocation->GetPort(&port);

NS_ENSURE_SUCCESS(rv, false);

::: dom/security/nsMixedContentBlocker.cpp:81
(Diff revision 1)
> +    return false;
> +  }
> +
> +  if (port != -1 && port != 80) {
> +    return false;
> +  }

Instead of using ->GetPort() and hardcoding port 80, you could use NS_GetDefaultPort() which seems more robust to use here in my opinion

::: dom/security/nsMixedContentBlocker.cpp:89
(Diff revision 1)
> +  rv = aContentLocation->GetHost(hostname);
> +  if (NS_FAILED(rv)) {
> +    return false;
> +  }
> +  PRNetAddr hostAddr;
> +  bool isIPAddress = (PR_StringToNetAddr(hostname.get(), &hostAddr) == PR_SUCCESS);

nit: instead of assigning the result to isIPAddress just do

return !(PR_StringToNetAddr(hostname.get(), &hostAddr) == PR_SUCCESS);

Comment 6

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8823835 [details]
Bug 1328460 - Don't send priming to IP or non-standard ports

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102324/diff/1-2/

Comment 7

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8823835 [details]
Bug 1328460 - Don't send priming to IP or non-standard ports

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102324/diff/2-3/

Comment 8

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8823835 [details]
Bug 1328460 - Don't send priming to IP or non-standard ports

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102324/diff/3-4/

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8823835 [details]
Bug 1328460 - Don't send priming to IP or non-standard ports

https://reviewboard.mozilla.org/r/102324/#review103744

::: dom/security/nsMixedContentBlocker.cpp:78
(Diff revision 4)
> +  }
> +
> +  int32_t port = -1;
> +  rv = aContentLocation->GetPort(&port);
> +  NS_ENSURE_SUCCESS(rv, false);
> +  int32_t defaultPort = NS_GetDefaultPort("https", nullptr);

nit: remove the second argument 'nullptr' it's the default argument within NS_GetDefaultPort anyway.

::: dom/security/nsMixedContentBlocker.cpp:80
(Diff revision 4)
> +  int32_t port = -1;
> +  rv = aContentLocation->GetPort(&port);
> +  NS_ENSURE_SUCCESS(rv, false);
> +  int32_t defaultPort = NS_GetDefaultPort("https", nullptr);
> +
> +  if (port != -1 && port != defaultPort) {

Nit: add a comment that HSTS priming only applies to default ports.

::: dom/security/nsMixedContentBlocker.cpp:89
(Diff revision 4)
> +  nsAutoCString hostname;
> +  rv = aContentLocation->GetHost(hostname);
> +  NS_ENSURE_SUCCESS(rv, false);
> +
> +  PRNetAddr hostAddr;
> +  return !(PR_StringToNetAddr(hostname.get(), &hostAddr) == PR_SUCCESS);

Nit:
return (PR_StringToNetAddr(hostname.get(), &hostAddr) != PR_SUCCESS);

Comment 10

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8823835 [details]
Bug 1328460 - Don't send priming to IP or non-standard ports

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102324/diff/4-5/

Comment 11

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8823835 [details]
Bug 1328460 - Don't send priming to IP or non-standard ports

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102324/diff/5-6/

Comment 12

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8823835 [details]
Bug 1328460 - Don't send priming to IP or non-standard ports

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102324/diff/6-7/

Comment 13

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8823835 [details]
Bug 1328460 - Don't send priming to IP or non-standard ports

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102324/diff/7-8/

Comment 14

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8823835 [details]
Bug 1328460 - Don't send priming to IP or non-standard ports

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102324/diff/8-9/

Comment 15

[Richard Barnes [:rbarnes]]

Chiming in late here, but I agree with the conclusion everyone has come to, i.e., that we shouldn't bother with priming on non-standard ports.

Comment 16

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8823835 [details]
Bug 1328460 - Don't send priming to IP or non-standard ports

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102324/diff/9-10/

Comment 17

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8823835 [details]
Bug 1328460 - Don't send priming to IP or non-standard ports

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102324/diff/10-11/

Comment 18

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8823835 [details]
Bug 1328460 - Don't send priming to IP or non-standard ports

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102324/diff/11-12/

Comment 20

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

[Tracking Requested - why for this release]: Regression

Carrying on flags from duplicate bug #1334074

Comment 21

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8823835 [details]
Bug 1328460 - Don't send priming to IP or non-standard ports

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102324/diff/12-13/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5e4a1cccae92
Don't send priming to IP or non-standard ports r=ckerschb

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5e4a1cccae92

Comment 24

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Requesting an uplift request, as Firefox 51, 52 and 53 are also affected.

Comment 25

[Kate McKinley [:kmckinley, :Kate]]

Bug 1335134 has been created to disable HSTS priming (via pref flip) on Release and Beta code base, and bug 1335224 has been created to create a go-faster addon to disable the pref.

Comment 26

[Julien Cristau [:jcristau]]

We're shipping a system add-on to disable HSTS priming on release and flipping the pref to false in beta 3.

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Kate, is this fixed in aurora 53? If not, want to uplift this patch?

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Christoph, maybe you can weigh in on whether this is needed for Aurora53 or not?

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #28)
> Christoph, maybe you can weigh in on whether this is needed for Aurora53 or
> not?

Pinged Kate on IRC, she will look at it.

Comment 30

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8823835 [details]
Bug 1328460 - Don't send priming to IP or non-standard ports

Approval Request Comment
[Feature/Bug causing the regression]: 1246540
[User impact if declined]: HSTS Priming requests will be sent that have no chance to succeed, potentially delaying page load
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: 1334838 to prevent orange tests
[Is the change risky?]: This change should lower risk
[Why is the change risky/not risky?]: This change reduces the number of connections eligible for HSTS priming
[String changes made/needed]: None

Comment 31

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8823835 [details]
Bug 1328460 - Don't send priming to IP or non-standard ports

Let's bring the fix and tests onto aurora 53.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/436ada403de0

Comment 33

[ovidiu boca[:Ovidiu]]

> Bug 1328460 - Don't send priming to IP or non-standard ports
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No

Hi Liz,

Can you please clarify this out, do you need manual testing from QA on this bug? From Kate's comment 30 I understood that manual QA is not needed, but I saw your qe-verify+ flag.  
Can you add some steps for verifying this, if manual testing is required. 
Thanks

Comment 34

[Liz Henry (:lizzard) (relman/hg->git project)]

Ovidiu, I just want to make sure we know this is behaving correctly on 53 now. There are STR in https://bugzilla.mozilla.org/show_bug.cgi?id=1335224#c7 and elsewhere in the comments in that bug and in bug 1335134. Thanks!

Comment 35

[ovidiu boca[:Ovidiu]]

I tested on Mac OS X 10.10, Windows 10 x64 and Ubuntu 16.04 with FF Developer Edition 53.0a2 (2017-02-21) and I can confirm the fix, I used the steps from https://bugzilla.mozilla.org/show_bug.cgi?id=1335224#c7. 
I also verified on Nighlty 54.0a1 and it works as expected. 

Thanks Liz.

Comment 36

[Mihai Boldan, Desktop QA [:mboldan]]

I've also manage to reproduce the issue on Firefox 53.0a1 (2017-01-01), under Windows 10x64,  using STR from https://bugzilla.mozilla.org/show_bug.cgi?id=1335224#c7 .

The issue is no longer reproducible on Firefox 54.0b2.
Tests were performed under Windows 10x64, Mac OS X 10.12.1, Ubuntu 16.04x64.