Closed Bug 881830 Opened 11 years ago Closed 10 years ago

CORS requests that STS upgrades to https fail

Categories

(Core :: Networking, defect)

Product:
Core
Component:
Networking
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla38
Tracking Flags:
Tracking Status
firefox38 --- fixed

People

(Reporter: shuhao, Assigned: mayhemer)

References

Details

Attachments

(1 file)

v1
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.
OS: Linux → All
Version: unspecified → Trunk
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)
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).
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.
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.
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.
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?
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
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.
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)
Flags: needinfo?(annevk)
Does STS describe its behavior as a redirect? I thought it was more like an upgrade.
Flags: needinfo?(annevk)
It doesn't describe its behavior as either; it just says to fetch a different URI than the one you were planning to fetch....
Summary: Setting crossOrigin on img before src in javascript causes image load error → CORS requests that STS upgrades to https fail
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.
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.
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.
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)
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...
I like REDIRECT_STS.
Flags: needinfo?(honzab.moz)
(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?
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.
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)
> 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)
Trying some other necko people, since Jason is not answering.  :(
Flags: needinfo?(mcmanus)
Flags: needinfo?(honzab.moz)
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.
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.  ;)
My apologies--I'll find someone to fix this.
Flags: needinfo?(mcmanus)
Flags: needinfo?(honzab.moz)
Assignee: nobody → honzab.moz
Flags: needinfo?(jduell.mcbugs)
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
Comment on attachment 8562146 [details] [diff] [review]
v1

r=me.  Thank you for fixing this!
Attachment #8562146 - Flags: review?(bzbarsky) → review+
(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
(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)
Also, there is a separate run for this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=60b0be0c2ff5
This one's clear.
Keywords: checkin-needed
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
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.
(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.
(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.
(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.
Blocks: 1134856
(In reply to Jonas Sicking (:sicking) from comment #48)
> However the issue that's remaining is that:

Bug 1134856.
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
(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)
(Exactly "dev version of HTTPS Everywhere Firefox")
> (Exactly "dev version of HTTPS Everywhere Firefox")

see "development" XPIs at https://eff.org/https-everywhere
Flags: needinfo?(yan)
Blocks: 1149250
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: