Closed Bug 1247733 Opened 9 years ago Closed 9 years ago

<use> referenced elements fail to load, in a page that was upgraded to HTTPS via HSTS

Categories

(Core :: Networking: HTTP, defect)

Product:
Core
Component:
Networking: HTTP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla47
Tracking Flags:
Tracking Status
firefox44 --- affected
firefox45 --- fixed
firefox46 --- fixed
firefox47 --- fixed
firefox-esr38 --- affected

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(4 files, 3 obsolete files)

part 1: Create a helper function for nsStandardURL's code to add/remove/replace a port in the URL string
(deleted), patch
valentin
: review+
Details | Diff | Splinter Review
part 2: give nsIStandardURL a "SetDefaultPort" API, and use it when upgrading HTTP connections to HTTPS
(deleted), patch
valentin
: review+
Details | Diff | Splinter Review
part 4: Add xpcshell test for nsIStandardURL setDefaultPort() API
(deleted), patch
valentin
: review+
Details | Diff | Splinter Review
part 3 v3: Add mochitest to ensure that SVG <use> is rendered correctly in documents that have been upgraded using HSTS
(deleted), patch
valentin
: review+
Details | Diff | Splinter Review
STR: 1. Load http://miketaylr.com/bzla/ytplay.html (note the HTTP in the URI there -- *not* https) EXPECTED RESULTS: White play button. ACTUAL RESULTS: No white play button. We're mistakenly failing during a URI comparison; more details coming. This is the underlying cause of bug 1244495.
(If you load the HTTPS version of the page *directly* -- e.g. https://miketaylr.com/bzla/ytplay.html directly -- then you get EXPECTED RESULTS. But if you visit the HTTP version & let the browser upgrade you (via 301 redirect), then you get ACTUAL RESULTS.)
So we fail a URI-comparison check with this backtrace: #0 0x00007f66f06951f7 in nsStandardURL::EqualsInternal(nsIURI*, nsStandardURL::RefHandlingEnum, bool*) (this=0x7f66d568db00, unknownOther=0x7f66d568c600, refHandlingMode=nsStandardURL::eIgnoreRef, result=0x7ffdd8ad611f) at $SRC/netwerk/base/nsStandardURL.cpp:1843 #1 0x00007f66f0695789 in nsStandardURL::EqualsExceptRef(nsIURI*, bool*) (this=0x7f66d568db00, unknownOther=0x7f66d568c600, result=0x7ffdd8ad611f) at $SRC/netwerk/base/nsStandardURL.cpp:1809 #2 0x00007f66f20a8ffd in nsReferencedElement::Reset(nsIContent*, nsIURI*, bool, bool) (this=0x7f66d2240970, aFromContent=0x7f66d2240820, aURI=0x7f66d568db00, aWatch=true, aReferenceImage=false) at $SRC/dom/base/nsReferencedElement.cpp:90 #3 0x00007f66f39cee89 in mozilla::dom::SVGUseElement::LookupHref() (this=0x7f66d2240820) at $SRC/dom/svg/SVGUseElement.cpp:406 #4 0x00007f66f39ce2c5 in mozilla::dom::SVGUseElement::CreateAnonymousContent() (this=0x7f66d2240820) at $SRC/dom/svg/SVGUseElement.cpp:219 The URI "mSpec" values are actually identical (aside from the #ref). The problem is that one of the URIs returns Port() = 443, and the other one returns Port() = 80. This makes nsStandardURL::EqualsInternal fail when comparing them.
The URI with Port() == 80 is our element's OwnerDoc()->GetDocumentURI(). It also doesn't seem to have the port *explicitly* set to 80 -- it simply has: mDefaultPort = 80 mPort = -1 which makes its Port() method return 80. (And it has "https", not "http", in its mSpec.)
HSTS might be involved here (or might not be). My site sends the following after the 301 Strict-Transport-Security: max-age=31536000; includeSubdomains; And Youtube sends the following after the 301: Strict-Transport-Security: max-age=604800
>It also doesn't seem to have the port *explicitly* set to 80 -- it simply has: mDefaultPort = 80 >(And it has "https", not "http", in its mSpec.) Gah. That means someone did a SetScheme() on it. Want to breakpoint on nsStandardURL::SetScheme and see who the someone was?
Flags: needinfo?(dholbert)
But I will bet the someone is HttpBaseChannel::GetSecureUpgradedURI.
Component: SVG → Networking: HTTP
Summary: <use> referenced elements fail to load, in a page that was upgraded to HTTPS via 301 redirect → <use> referenced elements fail to load, in a page that was upgraded to HTTPS via HSTS
Oh, and that does SetPort(-1), but that does not change mDefaultPort, obviously...
Yeah, I can confirm that we have a HttpBaseChannel with https in mSpec.mData ("https://miketaylr.com/bzla/ytplay.html") but with mDefaultPort = 80.
YouTube says: HSTS was rolled out for YouTube around that time [last week] and it looks like this could be a problem in the way Firefox handles the HSTS redirect. In a fresh Firefox instance (or after clearing the HSTS cache) an HTTP embed will get a 301 redirect to HTTPS. In that scenario the controls appear as expected. After the first visit, YouTube has set an HSTS so a subsequent load of an HTTP embed gets a 307 (internal) redirect to HTTPS. Under those circumstances the controls disappear. The problem seems to reproduce consistently with 307 redirects and not reproduce with 301s. I've tested Chrome and Safari as well, but their controls are working with 307 redirects.
status-firefox44: --- → affected
status-firefox45: --- → affected
status-firefox46: --- → affected
status-firefox-esr38: --- → affected
I think HSTS is indeed required here, actually; if I delete all of my history/state (ctrl+shift+del) and then visit Mike's HTTP url, I get EXPECTED RESULTS. But from that point on [after I have the HSTS setting in my cache], I get ACTUAL RESULTS.
OK -- if I clear my cache and then load the HTTP version of mike's testcase, then I do see a call to HttpBaseChannel::GetSecureUpgradedURI happening in the parent process.
Attached patch fix v1 (strawman) (obsolete) (deleted) — — Splinter Review
Here's a strawman fix, to make GetSecureUpgradedURI use 443 instead of -1 for its upgraded HTTPS URI. This seems to be what it's intending to do (it's assuming incorrectly that -1 will give this behavior). I've verified that this fixes Mike's testcase as well as a youtube testcase. (visiting each with a HTTP uri) There might be a cleaner way to do this, though...
(In reply to Daniel Holbert [:dholbert] from comment #16) > There might be a cleaner way to do this, though... Ideally, it'd be nice to be able to set mDefaultPort at the same time that we set mScheme. But, there's no setter exposed for mDefaultPort -- it gets set in "Init". (Also, we don't necessarily even know that we've got a nsStandardURL in this code; we're just dealing with nsIURI pointers here, which happen to be nsStandardURLs in this case but which could be other types of URIs in other cases.) So maybe the attached patch is the best we can easily do.
Comment on attachment 8718552 [details] [diff] [review] fix v1 (strawman) bz, does this seem sane to you?
Attachment #8718552 - Flags: feedback?(bzbarsky)
Comment on attachment 8718552 [details] [diff] [review] fix v1 (strawman) I think necko needs to improve its APIs here. Setting port explicitly to 443 like this will cause the 443 to appear in the URL bar and whatnot, right?
Attachment #8718552 - Flags: feedback?(bzbarsky)
In particular, maybe nsIStandardURL needs to grow a default port setter....
(In reply to Boris Zbarsky [:bz] from comment #19) > Setting port explicitly to > 443 like this will cause the 443 to appear in the URL bar and whatnot, right? (I suspected it might, but it doesn't actually seem to affect the URL bar with this bug's STR at least.)
Historical note: the buggy "SetScheme" call here dates back to https://hg.mozilla.org/mozilla-central/rev/5dc3c2d2dd4f#l5.253 , bug 495115, i.e. the bug that added HSTS support.
Depends on: 495115
Assignee: nobody → dholbert
This patch is just a refactoring patch, to merge 3 slightly-different codepaths from nsStandardURL::SetPort to use a single common helper-function. (The 3 slightly different codepaths here are for removing a port from mSpec, replacing a port with a new one, and inserting a port into mSpec where there previously was none. These 3 operations can all be represented with a single substring-replacement function. I'm still just using a single string buffer in the helper function, so I think it's just as efficient as each codepath that it's replacing.) This patch doesn't intend to change behavior at all -- I'm posting it here because I plan on using this helper-function in the next patch.
Attachment #8718552 - Attachment is obsolete: true
Attachment #8718741 - Flags: review?(mcmanus)
This patch gives nsIStandardURL a "default port setter", as suggested in comment 20. The new setter takes advantage of the previous patch's utility-function, to remove the port from mSpec, if it happens to match the new default. (Otherwise, we shouldn't need to modify mSpec.) If we have a nsStandardURL, then this new setter is all we need, port-wise -- we don't need to worry about checking for port 80, because that will already be represented as -1 (for "use default") in our nsStandardURL. But if we *don't* have a nsStandardURL, we need the old GetPort/SetPort codepath. I'm not 100% sure we even can get that codepath (e.g. whether it's possible for this function to be invoked with a data URI, say). I'm skeptical that this function is useful when invoked on a non-nStandardURL, but I'm preserving the old code inside of an "else" clause just in case. I'll attach a "diff -w" (no-whitespace-changes) version of the patch, too, to make that clearer.
Attachment #8718744 - Flags: review?(mcmanus)
I still need to write an automated test here, but I've verified that the attached patches make this bug's STR produce EXPECTED RESULTS, at miketaylr's test page as well as an HTTP youtube embed page from bug 1244495.
Comment on attachment 8718741 [details] [diff] [review] part 1: Create a helper function for nsStandardURL's code to add/remove/replace a port in the URL string Review of attachment 8718741 [details] [diff] [review]: ----------------------------------------------------------------- sgtm but :valentin is the defacto owner of URI code (lucky him) - so r?:valentin
Attachment #8718741 - Flags: review?(mcmanus) → review?(valentin.gosu)
Comment on attachment 8718744 [details] [diff] [review] part 2: give nsIStandardURL a "SetDefaultPort" API, and use it when upgrading HTTP connections to HTTPS Review of attachment 8718744 [details] [diff] [review]: ----------------------------------------------------------------- sgtm but :valentin is the defacto owner of URI code (lucky him) - so r?:valentin
Attachment #8718744 - Flags: review?(mcmanus) → review?(valentin.gosu)
Attachment #8718744 - Flags: review?(valentin.gosu) → review+
Attachment #8718744 - Attachment description: part 2: give nsIStandardURL a "SetDefaultPort" API → part 2: give nsIStandardURL a "SetDefaultPort" API, and use it when upgrading HTTP connections to HTTPS
Attachment #8718741 - Flags: review?(valentin.gosu) → review+
(In reply to Worcester12345 from comment #30) > Failing here. Thanks - yeah, this should be failing for everyone right now. (BTW: I'm tagging your comment as "metoo", to autohide it, since the giant list of addons takes up a lot of space & hence makes the bug a bit harder to follow, without adding any information that's relevant to this bug, since this isn't add-on related.)
Flags: in-testsuite?
(I'm working on making a mochitest for this -- not done yet, but I'm most of the way there I think. Hoping to post that for review later this evening or over the weekend.)
Thanks for the reviews valentin! I've got two test patches, too -- would you mind reviewing those as well? The first one is a regression test for this bug -- making sure that <use> renders correctly in pages that have been upgraded from HTTP to HTTPS. I've verified locally that it fails without the patch, and passes with the patch.
Attachment #8719107 - Flags: review?(valentin.gosu)
...and this last patch adds an xpcshell unit test for the new API that I'm adding in this bug, setDefaultPort().
Attachment #8719108 - Flags: review?(valentin.gosu)
(just noticed I used the wrong bug number in the mochitest -- fixed in this updated version.)
Attachment #8719107 - Attachment is obsolete: true
Attachment #8719107 - Flags: review?(valentin.gosu)
Attachment #8719109 - Flags: review?(valentin.gosu)
(Sorry, one more mochitest tweak - noticed some redundant CSS, and cleaned that up & reposted.)
Attachment #8719109 - Attachment is obsolete: true
Attachment #8719109 - Flags: review?(valentin.gosu)
Attachment #8719112 - Flags: review?(valentin.gosu)
Attachment #8719108 - Flags: review?(valentin.gosu) → review+
Comment on attachment 8719112 [details] [diff] [review] part 3 v3: Add mochitest to ensure that SVG <use> is rendered correctly in documents that have been upgraded using HSTS Review of attachment 8719112 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for adding tests as well.
Attachment #8719112 - Flags: review?(valentin.gosu) → review+
Depends on: 1248452
No longer blocks: 1244495
BTW: this can't land quite yet, because we have some similarly-incorrect HTTP<->HTTPS conversions elsewhere in our code that'll cause spurious URI-equality-failures, when URIs produced by those incorrect conversions are compared to the now-correctly-produced URI that comes out of HttpBaseChannel::GetSecureUpgradedURI. I've filed bug 1248452 on fixing those other conversions to share the same HTTP->HTTPS conversion codepath -- once those patches are reviewed & landed, we can land this bug's patches as well.
Depends on: 1247806
Flags: in-testsuite? → in-testsuite+
Can or should this change be uplifted?
@ Daniel, is this fix safe to uplift to Aurora 46? This bug is not a Firefox regression, but it was uncovered by YouTube's recent HSTS changes so it would be nice to get the fix to release users soon.
Flags: needinfo?(dholbert)
Given that 45 is the next ESR, we may want to consider uplifting to Beta as well depending on the risk.
I'd feel safe uplifting to Aurora. I'll request approval on this bug & its two dependencies. I'm a little worried about a Beta uplift; this has nonzero risk of breakage, even assuming it's correct (as evidenced by bug 1248452, which is about other code needing to be independently fixed to accommodate this bug's good changes). It's possible that there could be additional URI-comparison scenarios that will start incorrectly failing & break content in some way, which we haven't yet caught. I'd be open to taking that risk & doing a beta uplift, though, and I agree the HTTP-youtube-embed use-case is pretty important & compelling.
Flags: needinfo?(dholbert)
We might really want to construct a more targeted fix for Beta, narrowly focused on making the SVG <use> URI-comparison succeed in this one particular place (at comment 2), e.g. by checking the URIs' ports manually, and then overriding them so that the Equals() invocation will succeed as long as the default-port is the only thing that's (incorrectly) causing Equals() to fail. And in that case, we might want to land that sort of patch on Aurora, too, to get it a little testing before it goes out to Beta/Release users.
Blocks: 1249450
(In reply to Daniel Holbert [:dholbert] (offline 2/18-2/21) from comment #47) > I'd feel safe uplifting to Aurora. I'll request approval on this bug & its > two dependencies. Holding off on requesting uplift here for now, because I think there's a nonzero risk of regressions until we fix bug 1249450. I'll spin off a helper bug for coming up with a targeted backportable fix as described in comment 48.
I filed bug 1249452 on coming up with a backportable targeted fix.
(We've got a r+'d backportable patch for this over on bug 1249452 now -- once backport approval is granted over there, we can address this bug on branches.)
This is now fixed on Firefox 45 & 46 (beta & aurora) via bug 1249452 -- see bug 1249452 comment 8.
status-firefox45: affected → fixed
status-firefox46: affected → fixed
Depends on: 1274044
Blocks: 1329045
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: