Closed
Bug 881830
Opened 11 years ago
Closed 10 years ago
CORS requests that STS upgrades to https fail
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla38
Tracking | Status | |
---|---|---|
firefox38 | --- | fixed |
People
(Reporter: shuhao, Assigned: mayhemer)
References
Details
Attachments
(1 file)
5.91 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
If you try to set crossOrigin before setting img src, the image would not load and it tosses an error event without a message. Demo: http://jsfiddle.net/xnBMF/1/ This does not happen in Chrome. This also apparently does not happen on the Mac OS version. I cannot confirm whether or not if this bug exists on Windows.
Correction, apparently there are reports saying that this does not work on Mac OS either. :dzbarsky's nightly works, apparently.
Updated•11 years ago
|
OS: Linux → All
Version: unspecified → Trunk
![]() |
||
Comment 2•11 years ago
|
||
Both a nightly and Firefox 21 work for me here on Mac. Furthermore, a both 32-bit and 64-bit Linux nightlies work fine too, for me. Can you reproduce this in safe mode? What about with a clean profile?
Flags: needinfo?(shwu)
Works if I have a new profile. Safe-mode does not help. I still have at least 5 other people complaining that it doesn't work so I don't think we can mark this as WORKSFORME yet. Any idea how to pinpoint the problem?
Flags: needinfo?(shwu)
![]() |
||
Comment 4•11 years ago
|
||
Start copying over bits of the profile over to a clean profile until the problem appears? Start with prefs.js. Once you identify which file triggers it (and my bet is on the prefs file), then see what part of it is relevant (e.g. for the prefs file by just bisecting it).
![]() |
||
Comment 5•11 years ago
|
||
And to be clear, I didn't say anything about marking this worksforme, because clearly it's a problem in some configurations; we just need to figure out which ones, so it can be reproduced as needed.
So I'm not too familiar with how the firefox profile works. Anything I can take a look at so I know the relevant bits in order to see if I can narrow down the config? So far, prefs.js copied over and it has not failed yet.
![]() |
||
Comment 7•11 years ago
|
||
I'm not sure there's any documentation on profiles so much. I would maybe try with some of the sqlite files (permissions, cookies, content-prefs). The other thing worth doing is doing HTTP logs in both working and non-working bits using the directions at https://developer.mozilla.org/en-US/docs/Mozilla/Debugging/HTTP_logging and attaching them here.
Comment 8•11 years ago
|
||
Using new profiles on both Mac and Win, FF25 nightly - I can see the problem on Windows, but not Mac. Let me know if I can help.
![]() |
||
Comment 9•11 years ago
|
||
That's .... really odd. This behavior should not be platform-dependent, unless it's just a timing issue somehow, or there are system extensions on Windows... Can you reproduce on Windows in a debug build? If so, are you willing to do some debugging?
Comment 10•11 years ago
|
||
Boris - sadly, I do not have a development environment on Windows, so I'm not sure how much help I can be debugging a build there. But to answer your question, it can be reproduced in a Windows debug build. Also, I've found out that this bug happens when: - img.crossDomain is set - image is served via HTTP (but not HTTPS) See updated sample here: http://people.mozilla.com/~mwobensmith/files/img_cross_origin.htm
![]() |
||
Comment 11•11 years ago
|
||
So some notes: 1) If .src is set before .crossOrigin, then the load is just done without CORS, because the load starts when .src is set. Per spec, setting .crossOrigin should cancel the old load and start a new one, but we don't seem to do that. See bug 696451. 2) If .crossOrigin is set before .src then we do a CORS-enabled load. I can reproduce this filing with the permissions.sqlite in one particular profile I have, but not with a clean profile.
![]() |
||
Comment 12•11 years ago
|
||
AHA! So what happens in the failure case for the testcase in comment 10 is the following: 1) We start the load of http://support.mozilla.org/media/uploads/gallery/images/0a85171f1802a3b0d9f46ffb997ddc02-1260326970-447-1.png 2) We detect that we have STS stuff for that url (because in that profile, I do, apparently, for this host). 3) We do a redirect to https://support.mozilla.org/media/uploads/gallery/images/0a85171f1802a3b0d9f46ffb997ddc02-1260326970-447-1.png 4) This dispatches a channel-redirect notification with flags set to REDIRECT_PERMANENT but NOT REDIRECT_INTERNAL. This last seems like a bug. 5) The CORS specification requires that redirects also be subject to CORS checks, so we fail the load, since the pre-redirect channel has no Access-Control-Allow-Origin header. It's not clear to me how CORS and STS are supposed to interact here. Is our current behavior correct or not? If it's _not_ correct, then we need to somehow whitelist STS redirects. Or something. Right now the CORS code allows same-URI redirects with the INTERNAL flag, but this redirect satisfies neither criterion.
Component: Layout: Images → Networking
Flags: needinfo?(sstamm)
Flags: needinfo?(jonas)
Flags: needinfo?(honzab.moz)
![]() |
||
Updated•11 years ago
|
Flags: needinfo?(annevk)
Comment 13•11 years ago
|
||
Does STS describe its behavior as a redirect? I thought it was more like an upgrade.
Flags: needinfo?(annevk)
![]() |
||
Comment 14•11 years ago
|
||
It doesn't describe its behavior as either; it just says to fetch a different URI than the one you were planning to fetch....
![]() |
||
Updated•11 years ago
|
Summary: Setting crossOrigin on img before src in javascript causes image load error → CORS requests that STS upgrades to https fail
Comment 15•11 years ago
|
||
So yeah, in that case there's no network redirect and CORS should just work. I guess we should call STS out in Fetch to make this explicit.
Comment 16•11 years ago
|
||
I suggest we make the HSTS redirect use REDIRECT_INTERNAL and/or do whatever we need to do so that CORS doesn't choke on the redirect. An implementation shouldn't be required to try to synthesize a CORS response for the HSTS rewriting, and also that sounds less efficient. Also, we have exposed the logic that HSTS uses for internal redirects to extensions, and this logic is used by extensions like HTTPS Everywhere. It would be unlikely that HTTPS Everywhere would work well if we did anything different than what I suggest above.
![]() |
||
Comment 17•11 years ago
|
||
We probably just want a new redirect flag for this, because we don't want to treat general REDIRECT_INTERNAL as OK in CORS, imo.
Comment 18•11 years ago
|
||
I think I agree with Brian in comment 16. The target of the request has instructed us to upgrade the URL to HTTPS by using STS, so we should honor that and let this succeed. REDIRECT_INTERNAL_TOHTTPS?
Flags: needinfo?(sstamm)
![]() |
||
Comment 19•11 years ago
|
||
Just REDIRECT_STS should be fine, I would think... Note that this does allow https://foo.com to carry out a sort of attack on http://foo.com, if they're controlled by different entities, but STS sort of allows that anyway...
(In reply to Boris Zbarsky (:bz) from comment #17) > We probably just want a new redirect flag for this, because we don't want to > treat general REDIRECT_INTERNAL as OK in CORS, imo. Why not?
![]() |
||
Comment 22•11 years ago
|
||
Because I have no confidence that we don't have bogus REDIRECT_INTERNAL things somewhere that _should_ get checked. If we don't, so much the better.
![]() |
||
Comment 23•11 years ago
|
||
So as far as comment 22 goes, note that .url files do a REDIRECT_INTERNAL redirect. See also the discussion, such as it is, in bug 692843.
Oh, to be young and innocent as I was back in bug 464954, with all the free time in the world. Seriously though, I don't remember why I was opposed using the REDIRECT_INTERNAL flag. Using REDIRECT_INTERNAL seem more correct to me. But I'm also fine with inventing a new flag specifically for CORS. We'd likely need to update some of the internal state of the CORS code though when a redirect happens. As for .url files. Maybe they shouldn't use REDIRECT_INTERNAL? Or maybe we could introduce a flag for PLEASE_DO_CORS_CHECK_EVEN_THOUGH_THIS_IS_AN_INTERNAL_REDIRECT and set that specifically for .url redirects.
Flags: needinfo?(jonas)
![]() |
||
Comment 25•11 years ago
|
||
> Maybe they shouldn't use REDIRECT_INTERNAL?
That's possible, yes.
Jason, do who might have time to sort out the redirect flags stuff here?
Flags: needinfo?(jduell.mcbugs)
![]() |
||
Comment 27•11 years ago
|
||
Trying some other necko people, since Jason is not answering. :(
Flags: needinfo?(mcmanus)
Flags: needinfo?(honzab.moz)
Comment 28•11 years ago
|
||
Although I see duplicate 981307 has been marked "resolved" presumably this issue has not actually been resolved, right? It looks like it was abandoned 6 months ago. I'm just hoping it will be looked at, since it certainly is not resolved and bug report 981307 has a very simple test page for this. Thank you.
![]() |
||
Comment 29•11 years ago
|
||
Bob, that bug was resolved as a duplicate of this one; that doesn't mean the issue is resolved, as you noted. This bug was waiting on Jason to respond, and that didn't happen, and I forgot to follow up on it (because I have a huge set of outstanding needinfo requests people haven't responded to, so triaging them is not workable). :( With any luck, we can get an actual necko peer to comment here, though. ;)
Comment 30•11 years ago
|
||
My apologies--I'll find someone to fix this.
Flags: needinfo?(mcmanus)
Flags: needinfo?(honzab.moz)
Updated•11 years ago
|
Assignee: nobody → honzab.moz
Flags: needinfo?(jduell.mcbugs)
![]() |
Assignee | |
Comment 31•10 years ago
|
||
After rereading this bug again, I would introduce a new flag REDIRECT_STS_UPGRADE. The CORS code would then bypass the check when BOTH following apply: - the REDIRECT_STS_UPGRADE flag is set - the only change in the URL is the schema coming from http: to https:
Status: NEW → ASSIGNED
![]() |
Assignee | |
Comment 32•10 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=60b0be0c2ff5
Attachment #8562146 -
Flags: review?(bzbarsky)
![]() |
||
Comment 33•10 years ago
|
||
Comment on attachment 8562146 [details] [diff] [review] v1 r=me. Thank you for fixing this!
Attachment #8562146 -
Flags: review?(bzbarsky) → review+
![]() |
Assignee | |
Comment 34•10 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb4fb9d0b230
Keywords: checkin-needed
Comment 35•10 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #34) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb4fb9d0b230 The Run seems to have a lot of repated failures like the m1-3 ones. is this related to this patch ?
Flags: needinfo?(honzab.moz)
Keywords: checkin-needed
![]() |
Assignee | |
Comment 36•10 years ago
|
||
(In reply to Carsten Book [:Tomcat] from comment #35) > (In reply to Honza Bambas (:mayhemer) from comment #34) > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb4fb9d0b230 > > The Run seems to have a lot of repated failures like the m1-3 ones. is this > related to this patch ? Always a different test. You had to retrigger, so we would know now.
Flags: needinfo?(honzab.moz)
![]() |
Assignee | |
Comment 37•10 years ago
|
||
Also, there is a separate run for this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=60b0be0c2ff5
Comment 39•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b94fcb274a1 In the future, including commit information on patches you're requesting checkin for would be greatly appreciated.
Keywords: checkin-needed
Comment 40•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2b94fcb274a1
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Doesn't this also require some changes to the XHR code? So that it doesn't block a preflighted request from getting HSTS redirected?
I.e. I think preflighted requests are still broken.
![]() |
Assignee | |
Comment 43•10 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #41) > Doesn't this also require some changes to the XHR code? So that it doesn't > block a preflighted request from getting HSTS redirected? Don't we have a test for this? We should write one if not.
We have tests for preflights yes. Just not for preflights on a HSTS-enabled domain. test_CrossSiteXHR.html should be testing preflights.
![]() |
Assignee | |
Comment 45•10 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #44) > We have tests for preflights yes. Just not for preflights on a HSTS-enabled > domain. > > test_CrossSiteXHR.html should be testing preflights. Should I simply update that test?
So the more I think about it, the more the problem in comment 41 is quite different from the one in this bug. This bug, if I understand correctly, was that requests from a page on https://website.com/ to http://website.com/ should *not* be considered a cross-origin request if website.com uses HSTS. Is that correct? The problem in comment 41 is that a request from http://whereever.com/ to http://website.com/ should not break if the request uses preflight and website.com uses HSTS. So first off it might be worth filing a separate bug on this. Second, the easiest way to test it would be to add an entry here: http://mxr.mozilla.org/mozilla-central/source/dom/base/test/test_CrossSiteXHR.html?force=1#880 Adding a test like: { pass: 1, method: "GET", hops: [{ server: "http://<hsts enabled domain>", allowOrigin: origin }, ], }, I have to admit I don't fully remember how that test suite works. So I'm not 100% sure that the above is correct. So please make sure that the test fails before a patch, but passes after.
Comment 47•10 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #46) > This bug, if I understand correctly, was that requests from a page on > https://website.com/ to http://website.com/ should *not* be considered a > cross-origin request if website.com uses HSTS. > > Is that correct? No, that is wrong, that should be blocked. The only scenario that makes sense here is if the user has an HSTS entry for server.example. Then on http://example.org/ (note, no TLS) http://server.example/ is attempted to be fetched. Due to the HSTS entry https://server.example/ should be fetched instead. A downgrade should never be upgraded, we check mixed content before HSTS upgrades (because they are cache-dependent). But if it happens to be an upgrade that should work.
Ah, it appears that I got my example wrong which brought mixed-content-blocking into the mix. So the issue that was fixed was that: Page on http://a.com/ uses CORS (through <img crossorigin> or XHR) to fetch a resource on http://b.com/ but where b.com uses HSTS. This should not be blocked as long as b.com serves the appropriate CORS headers. However the issue that's remaining is that: Page on http://a.com/ uses CORS with a preflight (through XHR) to fetch a resource on http://b.com/ but where b.com uses HSTS. This is still blocked even if b.com serves the appropriate CORS headers and preflights.
![]() |
Assignee | |
Comment 49•10 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #48) > However the issue that's remaining is that: Bug 1134856.
Comment 50•10 years ago
|
||
Not surprisingly, this has been causing lots of breakage with HTTPS Everywhere. Hopefully the CORS-with-preflight-case being addressed? cf https://github.com/EFForg/https-everywhere/issues/49
![]() |
Assignee | |
Comment 51•10 years ago
|
||
(In reply to yan from comment #50) > Not surprisingly, this has been causing lots of breakage with HTTPS > Everywhere. Hopefully the CORS-with-preflight-case being addressed? > > cf https://github.com/EFForg/https-everywhere/issues/49 Where can I get the addon to test with it?
Flags: needinfo?(yan)
![]() |
Assignee | |
Comment 52•10 years ago
|
||
(Exactly "dev version of HTTPS Everywhere Firefox")
Comment 53•10 years ago
|
||
> (Exactly "dev version of HTTPS Everywhere Firefox") see "development" XPIs at https://eff.org/https-everywhere
Flags: needinfo?(yan)
You need to log in
before you can comment on or make changes to this bug.
Description
•