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.

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.

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

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 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 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);

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.

[Tracking Requested - why for this release]: Regression Carrying on flags from duplicate bug #1334074

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

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

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.

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

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

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

(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 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 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.

> 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

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!

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.

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.