Closed Bug 1173811 Opened 9 years ago Closed 9 years ago

FetchEvent.respondWith() should propagate opaque tainting

Categories

(Core :: DOM: Service Workers, defect)

Product:
Core
Component:
DOM: Service Workers
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla44
Tracking Flags:
Tracking Status
firefox42 --- unaffected
firefox43 --- disabled
firefox44 --- fixed
firefox-esr38 --- unaffected

People

(Reporter: bkelly, Assigned: jdm)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-sop, sec-high, Whiteboard: [b2g-adv-main2.5-])

Attachments

(3 files, 7 obsolete files)

Part 1: Propagate the response URL to intercepted channels when necessary (non-e10s)
41.23 KB, patch
abillings
: sec-approval+
Details | Diff | Splinter Review
Part 2: Propagate the response URL to intercepted channels when necessary (e10s)
20.29 KB, patch
Details | Diff | Splinter Review
Fix xpcshell tests for new finisheResponse() signature. r=jdm
4.11 KB, patch
jdm
: review+
Details | Diff | Splinter Review 
If an opaque response is passed to FetchEvent.respondWith() for an <img> request, then the tainting should be passed on as well.  For example, using this <img> tag to draw on a canvas should unset the origin-clear flag and therefore make the canvas write-only.

Maybe we do this already, but we should at least add a test.
If we don't do this already, that's a security bug.
Group: core-security
Sorry, origin-clean flag:

  https://html.spec.whatwg.org/#concept-canvas-origin-clean

The easiest way to do this would be to add a flag on the nsIChannel to indicate the taining.  The <img> element could then set a flag from the channel which the canvas could then check.
As far as I can tell from code inspection, we do not do this.  The RespondWith() code doesn't pass any info related to opaque to the channel.  Only the headers and the body.

The canvas will still do an same-origin check, but if the ServiceWorker synthesized the opaque response for a same-origin request, then I think that will fail to catch it.  (Request is same-origin, but Response is opaque from a different origin.)
Yes, I haven't seen any code to handle this either, so I'm pretty sure this is a bug.

FWIW, we have some tests for this stuff in dom/canvas/test/crossorigin.  I think ideally we would want to run those tests with a service worker present.
Where else do we have same-origin security checks?  Those should probably test for opaque Responses as well.
Daniel, what do you think of adding an nsIChannel::LOAD_CROSS_ORIGIN_TAINTED flag?
Flags: needinfo?(dveditz)
By the way, this tainting is not really spec'd yet since the html spec doesn't integrate with SW or fetch yet.  The only thing in the spec is a single sentence:

  "Renderer-side security checks about tainting for cross-origin content are tied to the transparency (or opacity) of the Response body, not URLs."
I think there are probably many more cases here than just <img>.  I expect we don't handle these right either:

1) var a = fetch(sameOriginUrl) is intercepted with respondWith(fetch(crossOriginUrl, { 'no-cors' })).  Here 'a' should be an opaque response, but we probably return a basic response.
2) new Worker(sameOriginScript) is intercepted with an opaque response.  This should cause the new Worker() to throw a SecurityError I think.

This is starting to feel like a lot of work.
Summary: FetchEvent.respondWith() should propagate opaque tainting to <img> element → FetchEvent.respondWith() should propagate opaque tainting
(In reply to Ben Kelly [:bkelly] from comment #8)
> I think there are probably many more cases here than just <img>.  I expect
> we don't handle these right either:
> 
> 1) var a = fetch(sameOriginUrl) is intercepted with
> respondWith(fetch(crossOriginUrl, { 'no-cors' })).  Here 'a' should be an
> opaque response, but we probably return a basic response.

Yeah.  Off the top of my head, we need to handle this for fetch/XHR, images, audio and video.  Can you think of anything else?

> 2) new Worker(sameOriginScript) is intercepted with an opaque response. 
> This should cause the new Worker() to throw a SecurityError I think.

Yeah, or whatever the worker code does by default for a cross-origin URL.  (I haven't checked the desired behavior.)

And obviously we should not forget SharedWorker too.
Basically anywhere we do a same-origin check on URL, we now need to check for tainted body data in the channel as well.
(In reply to Ben Kelly [:bkelly] from comment #10)
> Basically anywhere we do a same-origin check on URL, we now need to check
> for tainted body data in the channel as well.

Hmm, why?  For example, access to cross origin iframe content should not be affected since the security check on the principals should still be sufficient, right?  I think this only applies to stuff that lets you see its body without principal based checks.  Am I missing something?
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #11)
> Hmm, why?  For example, access to cross origin iframe content should not be
> affected since the security check on the principals should still be
> sufficient, right?  I think this only applies to stuff that lets you see its
> body without principal based checks.  Am I missing something?

The issue is that the body data is no longer described by the principal.  The principal check will think the iframe is same-origin, when really its content is cross-origin.

So I don't think the principal checks are sufficient with opaque responses in the mix.
We could possibly put the taint flag on the principal, but we still have the problem of the interception happening after the principal check is performed.  If that's the case then the taint flag would not be set in time for the check.
Actually, due to redirects all this code has to check the nsIChannel final URI anyway.  So we can slip in the tainted information there somehow.  We need to figure out the best way, but at least we don't have to change the ordering of checks, etc.
I think Ben is right.  For example, if you have an iframe with http://same.origin/foo and intercept it on the SW side and respondWith() a no-cors cross origin Response, the iframe would be considered same origin.

We need to test this ASAP, and disable SW on trunk if that is indeed broken.  Nikhil, got cycles for this today?
Flags: needinfo?(nsm.nikhil)
Blocks: 1167808
Might have spoken too soon about disabling on trunk, since before the patch for bug 1167808 we can't read the response body at all.  (Sorry for these small updates, talking to Ben on IRC...)
Bobby, do you have any thoughts on the best way to handle the following situation?

1) window navigates to http://foo.com
2) window opens an iframe at http://foo.com/sub
3) service worker intercepts http://foo.com/sub with fetch('http://bar.com/sub', { mode: 'no-cors' })
4) iframe is opened with URL http://foo.com/sub, but underlying frame content is from http://bar.com/sub

At this point the iframe should be treated as cross-origin, but the principals appear to be same origin.

We need to propagate the opaque Response cross-origin tainting through the nsIChannel somehow.  Some possibilities:

1) Add an nsIPrincipal::SetCrossOriginTainting() method.  Security checks would then look at this flag.
2) Add an nsIChannel load flag.  The security checks would then have to be modified to look for this flag.  Not sure if the flags are passed in currently, though.
3) Something else?

What do you think?
Flags: needinfo?(dveditz) → needinfo?(bobbyholley)
Nikhil correctly points out case 1 of comment 8 should reject with network error because the first fetch is mode 'cors'.

The original problem stands though if the first fetch is fetch(sameOriginUrl, { mode: 'no-cors' }).  An interception that returns an opaque response won't result in the original fetch returning an opaque response.
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #15)
> I think Ben is right.  For example, if you have an iframe with
> http://same.origin/foo and intercept it on the SW side and respondWith() a
> no-cors cross origin Response, the iframe would be considered same origin.

<iframe> creates its own browsing context and therefore does not necessarily use the SW of the parent. <iframe> is analogous to top-level document browsing contexts basically.

Now, if you reply with a no-cors cross-origin Response in such a situation, per https://fetch.spec.whatwg.org/#http-fetch that would fail as the response is neither "basic" nor "default" while the request is a client request.
Ehsan and Nikhil think we can use the nsHttpChannelInternet CorsMode flag for the tainting.  Its still a bit murky for me as this is not running in cors, but I will let them run with it.
Flags: needinfo?(bobbyholley)
For posterity - principals are supposed to be immutable, so mutable flags on them are a definite non-starter. The only mutable thing about principals right now is document.domain (probably the most costly legacy thing on the web), and we're hoping to deprecate it eventually.
Assignee: nobody → nsm.nikhil
Flags: needinfo?(nsm.nikhil)
Another case that came up this morning:

If a style sheet is cross-origin, you should not be able to script it.  Since "style" is not considered a client request that means we need to make sure opaque tainting enforces this.
<script> is in the same league. Exceptions of cross-origin scripts need not be visible through the error event unless CORS is used.
I think there are a class of requests that use nsContentUtils::SameOriginChecker() as well.  For example, XMLDocument.load().  In these cases the CorsMode flag is not checked.

We need to do the opaque tainting there as well.  (And maybe even block CORS responses?)
I think we could allow CORS responses there (provided the API doesn't expose headers if we lack header filtering at that level), but initially we might want to restrict to same-origin and synthetic responses.
No longer blocks: 1167808
Attached patch opaque propagation test (obsolete) — — Splinter Review 
I don't have time to write a fix before I go on PTO, but I hope this test helps if someone takes it up while I'm gone. The test needs some modification to pass on error.
One approach to deal with this issue in a more centralized way, suggested by Jonas, is to do our security checks centrally in OverrideWithSynthesizedResponse.  That is called on the original channel, so we have the information about the initial load (things such as the nsContentPolicyType, which we should be able to use to tell whether we are dealing with a client request for example, the original URI and so on), and information about the synthesized response (we should be able to get whatever we need from FinishSynthesizedResponse), so at least in theory we should have all that we need in order to do our security checks in one place.
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #27)
> One approach to deal with this issue in a more centralized way, suggested by
> Jonas, is to do our security checks centrally in
> OverrideWithSynthesizedResponse.  That is called on the original channel, so
> we have the information about the initial load (things such as the
> nsContentPolicyType, which we should be able to use to tell whether we are
> dealing with a client request for example, the original URI and so on), and
> information about the synthesized response (we should be able to get
> whatever we need from FinishSynthesizedResponse), so at least in theory we
> should have all that we need in order to do our security checks in one place.

Note that doing this may depend on the work that is being done in bug 1143922.  Jonas said that the first part where we update the callers to indicate what type of load they expect (CORS, same origin, etc.) is a few days away from landing...
FWIW, XMLDocument.load() currently has a same-origin restriction since we want to avoid dealing with any potential issues with having a Document change origins. Especially since Document objects can have a script global attached to them, and we definitely don't want to change origins of a script global.

If possible I'd prefer to leave same-origin restrictions as same-origin for now.
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #28)
> Note that doing this may depend on the work that is being done in bug
> 1143922.  Jonas said that the first part where we update the callers to
> indicate what type of load they expect (CORS, same origin, etc.) is a few
> days away from landing...

Sorry, I was unclear.

What will land in a few days is the *one* callsite which will indicate policy. Getting that landed will land a bunch of infrastructure as well though. Remaining callsites will be a lot easier but will likely take a few weeks.
I still feel like adding something to the principal origin attributes would be better.  This would require the nsIChannel to mint a new principal with the same URI, but with the tainting attribute flipped.  So all our code that looks for redirects by principal would just work.  The new principal would also still be immutable.

Its unclear to me if we would need two attributes, though.  One for opaque responses and one for cors responses.
Why can't we just mint an nsNullPrincipal instead of creating a principal with different OriginAttributes?
(In reply to Bobby Holley (:bholley) from comment #32)
> Why can't we just mint an nsNullPrincipal instead of creating a principal
> with different OriginAttributes?

I guess my worry is that things like window.location and other attributes would not reflect the URI anymore.  Maybe that's not a valid concern.
(In reply to Ben Kelly [:bkelly] from comment #33)
> (In reply to Bobby Holley (:bholley) from comment #32)
> > Why can't we just mint an nsNullPrincipal instead of creating a principal
> > with different OriginAttributes?
> 
> I guess my worry is that things like window.location and other attributes
> would not reflect the URI anymore.  Maybe that's not a valid concern.

Ideally none of those APIs should be getting the URI from the principal, since then they'd also break with iframe sandbox.
What about for something like a <script> element?  If the js script gets a null principal, it cannot then do XHR or fetch() requests.  Right?

Or does the <script> element use its parent document's principal for loading?
(In reply to Ben Kelly [:bkelly] from comment #35)
> What about for something like a <script> element?  If the js script gets a
> null principal, it cannot then do XHR or fetch() requests.  Right?

It cannot freely XHR to the domain of the document no - but isn't that the whole point? You're trying to make this thing cross-origin, right?
(In reply to Ben Kelly [:bkelly] from comment #35)
> What about for something like a <script> element?  If the js script gets a
> null principal, it cannot then do XHR or fetch() requests.  Right?
> 
> Or does the <script> element use its parent document's principal for loading?

The code for <script> doesn't look at the channel result principal at all. The script inside a script element runs with the principal of the page.

I would in fact expect most gecko code to not look at the result principal of channels. Generally we do security checks on the URI before initiating a load since we want to block the load from happening.

One exception that I can think of is the code for <img>, which might use the channel result principal if the image is drawn into a <canvas>. But I think this is an exception rather than a rule.
Assignee: nobody → ehsan
I'm half way through this bug.

The good news so far is that the tainting checks happen through checking principal subsumption that we get right.  The bad news is that I need to adjust the tests to make them work properly with the service worker interception, and I'm trying to find a way to keep most of the same structure for the tests.  I'll post the patches when they're ready, but I think we can open this bug up.
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #38)
> The good news so far is that the tainting checks happen through checking
> principal subsumption that we get right.  The bad news is that I need to
> adjust the tests to make them work properly with the service worker
> interception, and I'm trying to find a way to keep most of the same
> structure for the tests.  I'll post the patches when they're ready, but I
> think we can open this bug up.

Can you explain how the principal subsumption works for interception?  I didn't think we changed the principal on an intercepted channel.
Group: core-security → dom-core-security
Depends on: 1202085
Josh has a WIP patch for this.  The solution that Jonas suggested in a meeting today is as follows:

1) We will no longer always resurrect the redirect final channel URI from the SW on the original channel.
2) Instead, iff an opaque response is passed to respondWith(), we will simulate a redirect on the original channel using the opaque response's internal URL.
Assignee: ehsan → josh
No longer depends on: 1202085
Attached patch WIP (obsolete) — — Splinter Review 
Checkpoint that passes relevant tests for non-e10s.
Comment on attachment 8661455 [details] [diff] [review]
WIP

Review of attachment 8661455 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/cache/DBSchema.cpp
@@ +1745,5 @@
>                                     serializedInfo);
>    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>  
>    rv = state->BindInt32ByName(NS_LITERAL_CSTRING("response_redirected"),
> +                              aResponse.channelInfo().opaque() ? 1 : 0);

You shouldn't need this here.  We already store if the Response is opaque in response_type.  I think we can probably remove the response_redirected column completely.
Comment on attachment 8661455 [details] [diff] [review]
WIP

Review of attachment 8661455 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/fetch/ChannelInfo.h
@@ +107,2 @@
>    bool mInited;
> +  bool mIsOpaque;

Again, I don't understand why we need these fields in the ChannelInfo.  We should simply do the redirect to InnerResponse::GetURL() when FetchEvent.respondWith() resolves with a Response.type === 'opaque'.  It doesn't seem this should require values on ChannelInfo at all.

Sorry, if this is just part of the wip and was going to be removed.
Depends on: 1201160
Blocks: 1201160
No longer depends on: 1201160
Status: NEW → ASSIGNED
Attached patch WIP (obsolete) — — Splinter Review 
You appear to be correct. All the same tests pass after these changes. e10s is still unimplemented.
Attachment #8661455 - Attachment is obsolete: true
So, Anne raises the point that we should really be tainting resources in the following case:

  1) frame a.com loads script a.com/one.js
  2) a.com/one.js redirects to b.com/some.js
  3) b.com/some.js redirects to a.com/two.js

In this case Anne suggest we should treat a.com/two.js as cross-origin for the purposes of non-cors tainting.  So for scripts we should mute exceptions, for images we should make canvases write-only, and for stylesheets we should not let SWs intercepted sub-resources.

As far as I can tell, our redirect tainting does not do this.  We only look at the final principal coming out of the channel.  So we treat a.com/two.js as same-origin.

Jonas, do you agree this is a problem?  Should we write a separate bug for this?  Or do we have a good argument for why this is not a problem and should instead change the fetch spec?
Flags: needinfo?(jonas)
Comment 46 definitely seems separate from this bug since comment 46 is unrelated to service workers.

Please file a new bug and we can discuss there.
Flags: needinfo?(jonas)
Note, I am adding a test in bug 1212669 that is affected by these changes.  Specifically, I'm making some tests in test_fetch_cors.js check for final response type after redirect.  For now I am skipping these checks when isSWPresent is true, but after we the patches here land we can remove that exception.
Attachment #8661986 - Attachment is obsolete: true
Here's the design:
* when the code for respondWith() encounters an Opaque response, it extracts the response's URL and stores it away
* when we get to FinishedSynthesizedResponse, we pass the stored URL (or default to the request's URL if there is none) and compare it against the request's URL
* if the URLs are different (ie. we are rewriting an HTTP request with a tainted response), we initiate a redirection for the intercepted channel to the opaque response's URL to ensure that the channel principal will be correct
* if the URLs are the same (ie. we are rewriting an HTTP request with a non-tainted response), we continue with the existing code path of faking the response with the original request's URL (ie. via a cache entry in the parent process, and via a stream listener in the child process)

The non-e10s code path should be relatively straightforward to read given the above description. The e10s code path is more complex, since we want to avoid talking to the parent process at all costs:
* we initiate a faux-redirect to the tainted response's URL by performing most of the same steps that Redirect1Begin used to perform
* when the redirect verify callback occurs, and it's successful, and we're performing the redirection to facilitate synthesizing a tainted response, we directly call Redirect3Complete and proceed to synthesize the desired response for the new redirected channel
* none of these steps involve communicating with the parent process. This new code path should be distinct from the code path that _does_ talk to the parent process (ie. when we synthesize a response that will trigger a redirection) because we should never be able to obtain an opaque, tainted response that is a redirect.
Comment on attachment 8671621 [details] [diff] [review]
Part 1: Propagate the response URL to intercepted channels when necessary (non-e10s)

Ben, can you look at the dom/workers bits?
Attachment #8671621 - Flags: review?(bkelly)
Honza, these patches not only impact the implementation of intercepting HTTP channels, they also add a fake redirect mode for child processes. Are you able to review these?
Flags: needinfo?(honzab.moz)
Comment on attachment 8671621 [details] [diff] [review]
Part 1: Propagate the response URL to intercepted channels when necessary (non-e10s)

Review of attachment 8671621 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/ServiceWorkerEvents.cpp
@@ +363,5 @@
>      return;
>    }
>  
> +  nsCString responseURL;
> +  if (response->Type() == ResponseType::Opaque) {

Maybe add a comment about why we only need to override the final URL in the opaque case?

@@ +364,5 @@
>    }
>  
> +  nsCString responseURL;
> +  if (response->Type() == ResponseType::Opaque) {
> +    ir->GetUnfilteredUrl(responseURL);

So, currently Cache API is not using GetUnfilteredUrl() when saving the opaque Response in the database.  This means that we may encounter opaque responses with an empty string unfiltered URL.  I think we should treat these as errors.

Also, can you please fix Cache API since the unfiltered URL is now relevant for opaque responses?

You just need to change this to GetUnfilteredUrl():

https://dxr.mozilla.org/mozilla-central/source/dom/cache/TypeUtils.cpp#205

Also, can you add a case to the existing tests to make sure opaque tainting works as expected when the Response has passed through Cache API?

::: modules/libjar/InterceptedJARChannel.cpp
@@ +92,5 @@
>    if (NS_WARN_IF(!mChannel)) {
>      return NS_ERROR_NOT_AVAILABLE;
>    }
>  
> +  MOZ_ASSERT(aFinalURLSpec.IsEmpty());

How can we assert this here?  What stops a service worker from passing an opaque response back for a jar request?  It could do a cross origin request to the network.

Of course, we aren't really going to support JAR channels on release.  So not sure it matters a lot.

Maybe just return error instead of the assert?

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +202,5 @@
>    }
>  
>    NS_IMETHOD Run()
>    {
> +    mChannel->FinishSynthesizedResponse(EmptyCString());

Maybe add a comment that we always use emptry string in the parent process because the internal redirect is only necessary for child-side listeners?

::: netwerk/protocol/http/InterceptedChannel.cpp
@@ +230,5 @@
>  
> +  bool equal = false;
> +  originalURI->Equals(responseURI, &equal);
> +  if (!equal) {
> +    mChannel->StartRedirectChannelToURI(responseURI, nsIChannelEventSink::REDIRECT_INTERNAL);

This needs error handling.

@@ +361,5 @@
>    EnsureSynthesizedResponse();
>  
> +  nsCOMPtr<nsIURI> responseURL;
> +  nsresult rv = NS_NewURI(getter_AddRefs(responseURL), aFinalURLSpec);
> +  NS_ENSURE_SUCCESS(rv, rv);

I guess this part is completed in P2.

::: netwerk/protocol/http/nsCORSListenerProxy.cpp
@@ +1048,5 @@
> +  // Make cookie-less if needed. If this redirection is occurring for
> +  // a channel with a synthesized response, modifying the load flags
> +  // will break the code used to implement network interception and
> +  // must therefore be avoided.
> +  if (mIsPreflight || (!mWithCredentials && !isSynthesized)) {

Can we make the synthesized channel detect the LOAD_ANONYMOUS and remove it instead?  FetchDriver also adds LOAD_ANONYMOUS in the case of no-cors requests that redirect with same-origin credentials.  Seems troublesome to hunt down all the places that might add the flag to add this exception.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +2013,5 @@
>      if (httpRedirect) {
> +        if (mInterceptCache == INTERCEPTED) {
> +            httpRedirect->ForceIntercepted(mInterceptionID);
> +        } else {
> +            httpRedirect->ForceNoIntercept();

ForceNoIntercept() has been removed.  It seems you need to rebase.  Sorry!

::: testing/web-platform/mozilla/meta/service-workers/service-worker/fetch-request-css-base-url.https.html.ini
@@ +1,4 @@
> +[fetch-request-css-base-url.https.html]
> +  type: testharness
> +  [CSS's base URL must be the request URL even when fetched from other URL.]
> +    expected: FAIL

Maybe reference the bug that will fix it?

  bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1201160

::: testing/web-platform/mozilla/tests/service-workers/service-worker/resources/fetch-canvas-tainting-iframe.html
@@ +139,5 @@
>          create_test_promise(
>              remote_image_url +
>              '&mode=same-origin&url=' + encodeURIComponent(image_url),
>              '',
> +            TAINTED),

This one is interesting.  I think we are doing the wrong thing.

The service worker provides an untainted basic response to respondWith().  We still consider it tainted, though, because the original URL is cross-origin.

Since the spec doesn't look at final URL to determine taint-vs-no-taint, I think we are wrong here.  This should be NOT_TAINTED.  We probably need the long term tainting solution to fix this correctly.

When we upstream this it should be NOT_TAINTED.  Since this is an all or nothing test, maybe leave a loud comment about it?
Attachment #8671621 - Flags: review?(bkelly) → review+
To clarify, if GetUnfilteredUrl() returns empty string for an opaque response, we should fail the FetchEvent.respondWith().
Anne, can you confirm my interpretation of the spec here?

If an img.src is set to a cross-origin URL without CORS enabled, but is intercepted with a basic response, is the final image tainted or not?

I believe not, since it seems related to only the Response type that is returned from the fetch algorithm.  In gecko, however, we are going to still treat this tainted (for now) because we look at the final URL of the load which is still cross-origin in this case.
Flags: needinfo?(annevk)
Josh, can you include the no-cors part of the tests in bug 1212669 attachment 8671435 [details] [diff] [review]?  (Without the isSWPresent() disablement check.)

For various reasons it will be difficult for me to land them directly on trunk/aurora.  You can reasonably land them here as part of this fix, though, and will not draw attention to bug 1212669.
Flags: needinfo?(josh)
(In reply to Ben Kelly [:bkelly] from comment #61)
> If an img.src is set to a cross-origin URL without CORS enabled, but is
> intercepted with a basic response, is the final image tainted or not?

I think it is, because the response tainting flag is set. Not sure that's great though.
Flags: needinfo?(annevk)
(In reply to Anne (:annevk) from comment #63)
> (In reply to Ben Kelly [:bkelly] from comment #61)
> > If an img.src is set to a cross-origin URL without CORS enabled, but is
> > intercepted with a basic response, is the final image tainted or not?
> 
> I think it is, because the response tainting flag is set. Not sure that's
> great though.

Ok, then our behavior is correct for now.  Please comment here or file a new gecko bug if that changes.

Thanks!
Comment on attachment 8671621 [details] [diff] [review]
Part 1: Propagate the response URL to intercepted channels when necessary (non-e10s)

Review of attachment 8671621 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/InterceptedChannel.cpp
@@ +230,5 @@
>  
> +  bool equal = false;
> +  originalURI->Equals(responseURI, &equal);
> +  if (!equal) {
> +    mChannel->StartRedirectChannelToURI(responseURI, nsIChannelEventSink::REDIRECT_INTERNAL);

when this fails you probably need to fail the channel or fail this method.

@@ +361,5 @@
>    EnsureSynthesizedResponse();
>  
> +  nsCOMPtr<nsIURI> responseURL;
> +  nsresult rv = NS_NewURI(getter_AddRefs(responseURL), aFinalURLSpec);
> +  NS_ENSURE_SUCCESS(rv, rv);

does this belong here at all?

::: netwerk/protocol/http/nsCORSListenerProxy.cpp
@@ +1048,5 @@
> +  // Make cookie-less if needed. If this redirection is occurring for
> +  // a channel with a synthesized response, modifying the load flags
> +  // will break the code used to implement network interception and
> +  // must therefore be avoided.
> +  if (mIsPreflight || (!mWithCredentials && !isSynthesized)) {

Re: Ben Kelly

Would conversion of the interception code from the cache to something else avoid this problem?

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5385,5 @@
> +{
> +    MarkIntercepted();
> +    mResponseCouldBeSynthesized = true;
> +    mInterceptionID = aInterceptionID;
> +    return NS_OK;

should there be any BEFORE_OPEN or so checks here?
Attachment #8671621 - Flags: review+
Comment on attachment 8671624 [details] [diff] [review]
Part 2: Propagate the response URL to intercepted channels when necessary (e10s)

Review of attachment 8671624 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1091,5 @@
>    rv = gHttpHandler->GetIOService(getter_AddRefs(ioService));
>    if (NS_FAILED(rv)) {
>      // Veto redirect.  nsHttpChannel decides to cancel or continue.
>      OnRedirectVerifyCallback(rv);
>      return;

probably better would be to let this method return nsresult and put the burden of calling OnRedir..(rv) on the caller.

@@ +1182,5 @@
> +  nsCOMPtr<nsIChannel> newChannel;
> +  StartRedirect(responseURI,
> +                responseHead,
> +                mSecurityInfo,
> +                getter_AddRefs(newChannel));

you must return (break execution) when this method fails to create the channel.  also see the above comment

@@ +1437,5 @@
> +  if (mRedirectingForSubsequentSynthesizedResponse) {
> +    nsCOMPtr<nsIHttpChannelChild> httpChannelChild = do_QueryInterface(mRedirectChannelChild);
> +    MOZ_ASSERT(httpChannelChild);
> +    RefPtr<HttpChannelChild> redirectedChannel =
> +        static_cast<HttpChannelChild*>(httpChannelChild.get());

I love to see these tricks... how can you be sure it's the right object?  don't forget we can redirect to different schemes as well.

::: netwerk/protocol/http/HttpChannelChild.h
@@ +264,5 @@
>  
> +  void StartRedirect(nsIURI* uri,
> +                     const nsHttpResponseHead* responseHead,
> +                     nsISupports* securityInfo,
> +                     nsIChannel** outChannel);

and what does this method do?

after looking at the code this should be SetupRedirect().

::: netwerk/protocol/http/InterceptedChannel.h
@@ +98,5 @@
>    nsCOMPtr<nsIInputStream> mSynthesizedInput;
>  
>    // Listener for the synthesized response to fix up the notifications before they reach
>    // the actual channel.
> +  RefPtr<InterceptStreamListener> mStreamListener;

nsRefPtr?
Attachment #8671624 - Flags: review+
Here is the test patch from bug 1212669.  It was previously r+'d by sicking in that bug.  Please include this here so we can avoid uplifting bug 1212669.

Thanks!
Flags: needinfo?(josh)
Attachment #8672625 - Flags: review+
(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #66)
> @@ +1437,5 @@
> > +  if (mRedirectingForSubsequentSynthesizedResponse) {
> > +    nsCOMPtr<nsIHttpChannelChild> httpChannelChild = do_QueryInterface(mRedirectChannelChild);
> > +    MOZ_ASSERT(httpChannelChild);
> > +    RefPtr<HttpChannelChild> redirectedChannel =
> > +        static_cast<HttpChannelChild*>(httpChannelChild.get());
> 
> I love to see these tricks... how can you be sure it's the right object? 
> don't forget we can redirect to different schemes as well.

We only intercept or synthesize http channels, though.  Even if the service worker does a fetch() that is redirected to file://, the result is still synthesized back on to the original http channel.
Blocks: 1212904
Flags: needinfo?(honzab.moz)
Comment on attachment 8672625 [details] [diff] [review]
P3 Test response type of no-cors fetch() with redirects. r=sicking

Sorry for the noise here.  Dan asked me to land this test later in bug 1214361.
Attachment #8672625 - Attachment is obsolete: true
Depends on: 1215290
(In reply to Honza Bambas (:mayhemer) from comment #65) 
> ::: netwerk/protocol/http/nsCORSListenerProxy.cpp
> @@ +1048,5 @@
> > +  // Make cookie-less if needed. If this redirection is occurring for
> > +  // a channel with a synthesized response, modifying the load flags
> > +  // will break the code used to implement network interception and
> > +  // must therefore be avoided.
> > +  if (mIsPreflight || (!mWithCredentials && !isSynthesized)) {
> 
> Re: Ben Kelly
> 
> Would conversion of the interception code from the cache to something else
> avoid this problem?

Yes, since the problem we're working around is that LOAD_ANONYMOUS changes the cache key.
Attached patch Part 1 interdiff (obsolete) — — Splinter Review 
This stuff seems tricky enough that it's worth getting eyes on the changes I made according to the review comments.
Attachment #8674718 - Flags: review?(bkelly)
Comment on attachment 8674718 [details] [diff] [review]
Part 1 interdiff

Review of attachment 8674718 [details] [diff] [review]:
-----------------------------------------------------------------

This mostly looks good, but I want to make sure we understand the synthesized redirect case with the LOAD_ANONYMOUS flag.  Can you re-flag after addressing comments here?

Thanks!

::: dom/cache/TypeUtils.cpp
@@ +201,5 @@
>                                        InternalResponse& aIn, ErrorResult& aRv)
>  {
>    aOut.type() = aIn.Type();
>  
> +  aIn.GetUnfilteredUrl(aOut.url());

Oh, I landed this in bug 1215290.

::: dom/workers/ServiceWorkerEvents.cpp
@@ +369,5 @@
>    nsCString responseURL;
>    if (response->Type() == ResponseType::Opaque) {
>      ir->GetUnfilteredUrl(responseURL);
> +    if (responseURL.IsEmpty()) {
> +      autoCancel.SetCancelStatus(NS_ERROR_OPAQUE_INTERCEPTION_DISABLED);

This is the wrong error code.  I think you should just use NS_ERROR_INTERCEPTION_FAILED for the "unexpected interception" error case.

We could add a new error type here, but I don't think its worth it since this really corner casey.

Can you please put an NS_WARN_IF() on the empty string check here, though?

::: modules/libjar/InterceptedJARChannel.cpp
@@ +96,5 @@
> +  if (!aFinalURLSpec.IsEmpty()) {
> +    // We don't support rewriting responses for JAR channels where the principal
> +    // needs to be modified.
> +    return NS_ERROR_NOT_IMPLEMENTED;
> +  }

Man, we should just remove the jar interception support. :-\

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +277,5 @@
> +  // known cases that attempt to mark channels as anonymous due
> +  // to cross-origin redirects; since the response is entirely synthesized
> +  // this is an unnecessary precaution.
> +  if (synthesized && aLoadFlags != mLoadFlags) {
> +    aLoadFlags &= ~LOAD_ANONYMOUS;

Does this work properly if the synthesized response is a 302 redirect?  If the channel is currently anonymous and then synthetically redirected, it seems we want to continue being anonymous during the redirect.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +205,5 @@
>    {
> +    // We can always pass an empty string here since the only synthesized
> +    // responses that are processed in the parent process are redirections.
> +    // Non-empty response URLs are only present in opaque responses which
> +    // don't include redirections.

This comment confuses me.  I thought we could use EmptyCString() here because we only really care about setting the final URL properly in the child.  And a synthesized response requiring a redirect will trigger the proper redirect callbacks on the child.

::: netwerk/protocol/http/InterceptedChannel.cpp
@@ +230,5 @@
>  
>    bool equal = false;
>    originalURI->Equals(responseURI, &equal);
>    if (!equal) {
> +    nsresult rv = mChannel->StartRedirectChannelToURI(responseURI, nsIChannelEventSink::REDIRECT_INTERNAL);

nit: line length / wrapping issue here

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5387,5 @@
>  
>  NS_IMETHODIMP
>  nsHttpChannel::ForceIntercepted(uint64_t aInterceptionID)
>  {
> +    ENSURE_CALLED_BEFORE_ASYNC_OPEN();

It seems we should also check or assert that LOAD_BYPASS_SERVICE_WORKER is not set here.

::: testing/web-platform/mozilla/tests/service-workers/service-worker/resources/fetch-rewrite-worker.js
@@ +84,5 @@
> +          if (params['cache']) {
> +            return self.caches.open("cached-fetches").then(function(cache) {
> +              return cache.put(request, response).then(function() {
> +                return cache.match(request).then(function(cached_response) {
> +                  resolve(cached_response);

So, this leaves state on disk.  Can you make the cache name random using Date.now() and then do caches.delete() before resolving the final response?

It would be a bit nicer to write this as a normal promise chain.

return self.caches.open(name).then(function(cache) {
  return cache.put();
}).then(function() {
  return cache.match();
}).then(function(r) {
  cached_response = r;
  return caches.delete(name);
}).then(function() {
  resolve(cached_response);
};
Attachment #8674718 - Flags: review?(bkelly)
(In reply to Ben Kelly [:bkelly] from comment #74)
> Comment on attachment 8674718 [details] [diff] [review]
> Part 1 interdiff
> 
> Review of attachment 8674718 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: netwerk/protocol/http/HttpBaseChannel.cpp
> @@ +277,5 @@
> > +  // known cases that attempt to mark channels as anonymous due
> > +  // to cross-origin redirects; since the response is entirely synthesized
> > +  // this is an unnecessary precaution.
> > +  if (synthesized && aLoadFlags != mLoadFlags) {
> > +    aLoadFlags &= ~LOAD_ANONYMOUS;
> 
> Does this work properly if the synthesized response is a 302 redirect?  If
> the channel is currently anonymous and then synthetically redirected, it
> seems we want to continue being anonymous during the redirect.

Note that we're only removing the flag from aLoadFlags, which is ORed with mLoadFlags. If the channel is already anonymous, we won't be disabling that.
(In reply to Josh Matthews [:jdm] from comment #75)
> Note that we're only removing the flag from aLoadFlags, which is ORed with
> mLoadFlags. If the channel is already anonymous, we won't be disabling that.

I'm really confused then.  I thought we needed the LOAD_ANONYMOUS flag removed in the synthetic case.  Why is it ok for an existing LOAD_ANONYMOUS to be there, but we need to block a later one?

Is it really just that we want to block it during the fake redirect?  Maybe this would be easier to understand if it was |if (mFakeRedirectInProgress)|.
It's not that LOAD_ANONYMOUS breaks interception, it's that when we redirect from one channel to another we require the cache key to be the same for the rewritten response to be accessible. If a channel consumer modifies the load flags during a redirect to add LOAD_ANONYMOUS, this changes the cache key that the redirected channel attempts to open.
(In reply to Josh Matthews [:jdm] from comment #77)
> It's not that LOAD_ANONYMOUS breaks interception, it's that when we redirect
> from one channel to another we require the cache key to be the same for the
> rewritten response to be accessible. If a channel consumer modifies the load
> flags during a redirect to add LOAD_ANONYMOUS, this changes the cache key
> that the redirected channel attempts to open.

Ok, but doing this in SetLoadFlags() is not obviously about redirect at all.  It looks like it affects every use ever.  I think its hard to understand under what conditions you're trying to actually block here.
And its still not clear to me if we support this.  Window does:

  fetch(sameOriginURL, { credentials: 'same-origin' });

And the service worker does:

  e.respondWith(Response.redirect(crossOriginURL));

Can the CORS proxy set the anonymous flag when it attempts to follow the synthetic redirect?
Ben, would comment 79 not fail with a network error in main fetch step 8?
(In reply to Anne (:annevk) from comment #80)
> Ben, would comment 79 not fail with a network error in main fetch step 8?

Hmm, under which condition?  Note the request in comment 79 is in cors mode, but with same-origin credentials.
I read credentials as mode for some reason. Sorry.
(In reply to Ben Kelly [:bkelly] from comment #79)
> And its still not clear to me if we support this.  Window does:
> 
>   fetch(sameOriginURL, { credentials: 'same-origin' });
> 
> And the service worker does:
> 
>   e.respondWith(Response.redirect(crossOriginURL));
> 
> Can the CORS proxy set the anonymous flag when it attempts to follow the
> synthetic redirect?

So I'm not super up on the SW syntax yet, so I might misunderstand things. But wouldn't "Response.redirect(crossOriginURL)" trigger something that looks like a server 302 response (or one of the other redirect codes).

This would then cause the original nsHttpChannel which the DOM code created fire an AsyncOnChannelRedirect.

This would then set the LOAD_ANONYMOUS flag on the new channel.

Or am I misunderstanding the question?
(In reply to Jonas Sicking (:sicking) from comment #83)
> (In reply to Ben Kelly [:bkelly] from comment #79)
> > And its still not clear to me if we support this.  Window does:
> > 
> >   fetch(sameOriginURL, { credentials: 'same-origin' });
> > 
> > And the service worker does:
> > 
> >   e.respondWith(Response.redirect(crossOriginURL));
> > 
> > Can the CORS proxy set the anonymous flag when it attempts to follow the
> > synthetic redirect?
> 
> So I'm not super up on the SW syntax yet, so I might misunderstand things.
> But wouldn't "Response.redirect(crossOriginURL)" trigger something that
> looks like a server 302 response (or one of the other redirect codes).
> 
> This would then cause the original nsHttpChannel which the DOM code created
> fire an AsyncOnChannelRedirect.
> 
> This would then set the LOAD_ANONYMOUS flag on the new channel.
> 
> Or am I misunderstanding the question?

Correct.  And I want to make sure it still works after the patch I reviewed in comment 74.  Removing the LOAD_ANONYMOUS flag in SetLoadFlags() for a synthesized channel seems it might break this.  I'd really like to see the code there more isolated to the fake redirect for opaque tainting.
I guess a synthesized 30x just does a ContinueConnect() instead of a full synthesize, so shouldn't be a problem.  A clearer comment might be ok.

It might run afoul of some FetchDriver asserts which attempt to verify the flag is set correctly during redirects.
Remove the LOAD_ANONYMOUS flag for a synthesized channel probably has no effect, since the synthesized channel *shouldn't* read cookies, put anything into cache, or use the socket cache.

But of course, that also begs the question of why remove it. So yeah, I'd also like to understand if there's a purpose to that.
Comment on attachment 8674718 [details] [diff] [review]
Part 1 interdiff

Review of attachment 8674718 [details] [diff] [review]:
-----------------------------------------------------------------

Changing this to r=me with comments addressed.  Based on discussion here we think its safe to strip LOAD_ANONYMOUS in SetLoadInfo() on synthesized channels.

Can we at least add a comment pointing to bug 1201683 to remove this code block when the intercept-via-cache stuff is gone?
Attachment #8674718 - Flags: review+
(In reply to Jonas Sicking (:sicking) from comment #86)
> Remove the LOAD_ANONYMOUS flag for a synthesized channel probably has no
> effect, since the synthesized channel *shouldn't* read cookies, put anything
> into cache, or use the socket cache.
> 
> But of course, that also begs the question of why remove it. So yeah, I'd
> also like to understand if there's a purpose to that.

See comment 77.  I believe it is a side effect of how we implement interception by via http cache entries.  We have bug 1201683 filed to stop using cache entries in the future.
Attachment #8671621 - Attachment is obsolete: true
Attachment #8671624 - Attachment is obsolete: true
Attachment #8629724 - Attachment is obsolete: true
Attachment #8674718 - Attachment is obsolete: true
These patches (preceded by bug 1194848) are ready to land. What's the protocol? Do I need to get sec-approval for them?
(In reply to Josh Matthews [:jdm] from comment #91)
> These patches (preceded by bug 1194848) are ready to land. What's the
> protocol? Do I need to get sec-approval for them?

I think so, although maybe not since opaque interception is disabled by default on trunk and aurora.  Asking for sec-approval seems safest, though.
Comment on attachment 8676543 [details] [diff] [review]
Part 1: Propagate the response URL to intercepted channels when necessary (non-e10s)

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Without difficulty.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The comments spell out the issue that is being corrected, and the tests demonstrate a significant SOP violation for canvas tainting.

Which older supported branches are affected by this flaw?
Aurora, but not by default since opaque interception is controlled by a pref.

If not all supported branches, which bug introduced the flaw?
It's been there since the beginning of the fetch implementation.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I don't anticipate much difficulty creating backports. The code in question is disabled by default, so the risk should be minimal.

How likely is this patch to cause regressions; how much testing does it need?
The code changes are isolated to a code path that isn't executed today, so the risk of regression should be minimal.
Attachment #8676543 - Flags: sec-approval?
Comment on attachment 8676543 [details] [diff] [review]
Part 1: Propagate the response URL to intercepted channels when necessary (non-e10s)

sec-approval+ for trunk. We should take this on Aurora as well. If this patch applies cleanly there, please nominate for Aurora.
Attachment #8676543 - Flags: sec-approval? → sec-approval+
(In reply to Al Billings [:abillings] from comment #94)
> sec-approval+ for trunk. We should take this on Aurora as well. If this
> patch applies cleanly there, please nominate for Aurora.

Do we really want to uplift this to aurora?  It does carry some risk and the feature in question is disabled in 43.  We will merge trunk to aurora in 2 weeks.

I think I would vote not to uplift this.
Flags: needinfo?(abillings)
That is fine by me and a well made point. Just trunk then.
Flags: needinfo?(abillings)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ad51d302ba94629ea1050732c1365438f98a14c
Bug 1173811 - Part 1: Propagate the response URL to intercepted channels when necessary (non-e10s). r=mayhemer,bkelly

https://hg.mozilla.org/integration/mozilla-inbound/rev/56bc1314e660a052012bf87de1494d4b5835e315
Bug 1173811 - Part 2: Propagate the response URL to intercepted channels when necessary (e10s). r=mayhemer,bkelly
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/46cc647fabf3 for timeouts in the xpcshell test netwerk/test/unit/test_synthesized_response.js, so far on Android and Linux64 debug, but those are the only ones that have finished so far.
I ran into these failures in my try build too.  This fixes the xpcshell tests for me.

Josh, feel free to just fold this into your other patches if you prefer.

Thanks!
Attachment #8677445 - Flags: review?(josh)
Attachment #8677445 - Flags: review?(josh) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ee908af4695ef2c0a171b36fba84f4b82cb84c1
Bug 1173811 - Part 1: Propagate the response URL to intercepted channels when necessary (non-e10s). r=mayhemer,bkelly

https://hg.mozilla.org/integration/mozilla-inbound/rev/01c1f46069567aa2477b4fe70b24fbf14c6f3459
Bug 1173811 - Part 2: Propagate the response URL to intercepted channels when necessary (e10s). r=mayhemer,bkelly
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44: verified disabled → fixed
Resolution: --- → FIXED
Group: dom-core-security → core-security-release
Group: core-security-release
Whiteboard: [b2g-adv-main2.5-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: