1182537 - Use channel->ascynOpen2 in dom/base/Navigator.cpp

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1182537_asyncopen2_navigator.patch

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8638262 [details] [diff] [review]
bug_1182537_asyncopen2_navigator.patch

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

::: dom/base/Navigator.cpp
@@ +1101,5 @@
>    rv = NS_NewChannel(getter_AddRefs(channel),
>                       uri,
>                       doc,
> +                     nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL |
> +                     nsILoadInfo::SEC_DISALLOW_SCRIPT,

This should use CORS, no? Doesn't seem like we are right now, but I think we should per spec, no? Makes a difference during redirects.

::: dom/security/nsContentSecurityManager.cpp
@@ +69,5 @@
> +
> +  // map nsILoadInfo flags to nsIScriptSecurityManager flags
> +  uint32_t flags = nsIScriptSecurityManager::STANDARD;
> +  if (securityMode & nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL) {
> +    flags |= nsIScriptSecurityManager::DISALLOW_INHERIT_PRINCIPAL;

This is not correct. DATA_IS_NULL doesn't mean that inheriting protocols are forbidden, it means that they end up as a null-principal.

I think we should explicitly check for data: in the sendBeacon code and leave it at that.

@@ +183,5 @@
> +      mimeTypeGuess = EmptyCString();
> +      requestingContext = aLoadInfo->LoadingNode();
> +      MOZ_ASSERT(requestingContext
> +                 ? (requestingContext->NodeType() == nsIDOMNode::DOCUMENT_NODE)
> +                 : true, "type_beacon requires requestingContext of type Document");

!requestiongContext || requestingContext->NodeType() == DOCUMENT_NODE

Comment 3

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1182537_asyncopen2_navigator.patch

You are right, we definitely need cors, but in fact, thinks are not quite that simple. We also need a way to communicate the bits for the nsINetworkInterceptController, hence I added yet another flag, or is there a simpler way of doing that?

Anything else I missed?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This is a big mess.

The reason we're setting an intercept handler is because normally we get that through the loadgroup. But since no loadgroup is used here the SW folks added a horrible hack to the CORS code which makes it deal with things that aren't CORS related, nor otherwise generic, in any way. 

There's a few ways we can fix this.

1. The reason that we're not setting a loadgroup here is to avoid having the request cancelled when
   the loadgroup dies. But that's breaking a lot of other functionality, like the intercept handler,
   appid/browserflag and privatebrowsing, that the loadgroup normally provide.
   So we could add code to loadgroups that make it possible to add a request to a loadgroup without
   causing that request to get cancelled when the loadgroup is cancelled.

2. Setting a intercept handler as the notificationcallback on the *channel* before setting up the CORS
   listener would have solved the problem that the SW folks were facing, without having to resort to
   ugly hacks in the CORS code.

3. A more generic solution of 2 is to set the docshell as the notificationcallback on the channel
   before setting up the CORS listener. That would likely solve many issues that the current lack of
   loadgroup is currently causing.

4. We can do what the <a ping> code is doing here and simply create a new loadgroup which has the
   docshell as notification callbacks.
   http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#732


I'd recommend doing 3 or 4.

Either way we need to get rid of this intercept handler hack since that's also causing us to add *two* cors-listeners on the preflight. Currently the sendBeacon code adds one cors-listener, and then NS_StartCORSPreflight adds a second one.

Comment 5

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1182537_asyncopen2_navigator.patch

(In reply to Jonas Sicking (:sicking) from comment #4)
> 4. We can do what the <a ping> code is doing here and simply create a new
> loadgroup which has the
>    docshell as notification callbacks.

Jonas, thanks for clarification. 4 sounds reasonable to me. I set up the interceptcontroller exactly the same way as <ping> does. Nevertheless I can't claim I fully understand what's going on here, but I suppose this is what you had in mind, right?

Comment 6

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1182537_asyncopen2_navigator.patch

Jonas, as discussed over IRC I took the DISALLOW_SCRIPT bits out of the patch again because javascript: URLs are disabled by default now anyway.

I am still not sure if the interceptController bits are correct though.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8640084 [details] [diff] [review]
bug_1182537_asyncopen2_navigator.patch

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

The SendPing code does some crazy stuff. No idea why it does things that way but I don't think we should copy it. Do the below instead but get a review from bz as well.

::: dom/base/Navigator.cpp
@@ +1020,5 @@
>    return mGeolocation;
>  }
>  
>  class BeaconStreamListener final : public nsIStreamListener
> +                                 , public nsIInterfaceRequestor

This isn't right.

necko doesn't request interface through the streamlistener of the channel, but rather through the notificationCallbacks of the channel. So adding additional interfaces on the streamlistener will do nothing.

See 

http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsNetUtil.h#629
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsIChannel.idl#83
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsILoadGroup.idl#76

This is the code that fails because we don't have a loadgroup. Normally the docshell is the notificationcallbacks on the loadgroup.

@@ +1075,5 @@
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +BeaconStreamListener::GetInterface(const nsIID& aIID, void** aResult)

So this is also not needed.

@@ +1271,5 @@
> +  channel->SetLoadGroup(loadGroup);
> +
> +  BeaconStreamListener* beaconListener = new BeaconStreamListener();
> +  nsCOMPtr<nsINetworkInterceptController> interceptController =
> +    do_QueryInterface(docShell);

This is also not needed.

@@ +1276,5 @@
> +  beaconListener->SetInterceptController(interceptController);
> +  nsCOMPtr<nsIStreamListener> listener(beaconListener);
> +
> +  // Observe redirects as well:
> +  nsCOMPtr<nsIInterfaceRequestor> callbacks = do_QueryInterface(listener);

Just change 'listener' to 'docShell' here instead.


If you do this, you can delete the stuff about mozIThirdPartyUtil and nsIPrivateBrowsingChannel above.


You will however have to make the Beacon listener hold a strong reference to the loadGroup. And then release that reference in OnRequestStart. Though only set the reference if AsyncOpen2 succeeds.

@@ +1298,5 @@
> +                                                                 principal,
> +                                                                 true);
> +
> +    rv = cors->Init(channel, DataURIHandling::Allow);
> +    NS_ENSURE_SUCCESS(rv, false);

This isn't needed. NS_StartCORSPreflight adds a CORSlistenerproxy to the preflight channel.

Comment 8

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1182537_asyncopen2_navigator.patch

(In reply to Jonas Sicking (:sicking) from comment #7)
> The SendPing code does some crazy stuff. No idea why it does things that way
> but I don't think we should copy it. Do the below instead but get a review
> from bz as well.

Will do.

> > +  nsCOMPtr<nsIInterfaceRequestor> callbacks = do_QueryInterface(listener);
> Just change 'listener' to 'docShell' here instead.
> 
> If you do this, you can delete the stuff about mozIThirdPartyUtil and
> nsIPrivateBrowsingChannel above.

I did that, but I have to admit I don't fully understand how that works under the hood, so please take another close look if that is what you wanted to have done.

> You will however have to make the Beacon listener hold a strong reference to
> the loadGroup. And then release that reference in OnRequestStart. Though
> only set the reference if AsyncOpen2 succeeds.

Should we set the loadgroup after the if-else or really only if asyncOpen2() succeeds?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8640599 [details] [diff] [review]
bug_1182537_asyncopen2_navigator.patch

The explicit scheme check does not make me a happy camper, but ok.

>+  BeaconStreamListener* beaconListener = new BeaconStreamListener();

This needs to be an nsRefPtr.  Otherwise after your call to nsCORSListenerProxy() this object could be dead for all you know.

>+    nsRefPtr<nsCORSListenerProxy> cors = new nsCORSListenerProxy(beaconListener,

Does the "SEC_REQUIRE_CORS_DATA_INHERITS" bit you passed to NewChannel not handle this?

>+  // make the beaconListener hold a strong reference to the loadgroup

To answer your question, yes this should only happen if AsyncOpen succeeds. Otherwise you leak.

r=me, I think

Comment 10

[Christoph Kerschbaumer [:ckerschb}]

Comment on attachment 8640599 [details] [diff] [review]
bug_1182537_asyncopen2_navigator.patch

Thanks Boris - Jonas, just to make sure, can you have another look if I ended up adding all the code you requested?

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8640599 [details] [diff] [review]
bug_1182537_asyncopen2_navigator.patch

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

::: dom/base/Navigator.cpp
@@ +1259,5 @@
>  
> +    nsCOMPtr<nsIPrincipal> principal = doc->NodePrincipal();
> +    nsRefPtr<nsCORSListenerProxy> cors = new nsCORSListenerProxy(beaconListener,
> +                                                                 principal,
> +                                                                 true);

This isn't needed, NS_StartCORSPreflight will add a cors-listener to the preflight, and AsyncOpen2 will add one to the request itself.


Oooh! But we do need to make sure that NS_StartCORSPreflight calls AsyncOpen2!

Maybe make NS_StartCORSPreflight call AsyncOpen2 if the loadinfo sets one of the mode-flags.

@@ +1284,5 @@
>      return false;
>    }
> +  // make the beaconListener hold a strong reference to the loadgroup
> +  // which is released in ::OnStartRequest
> +  beaconListener->setLoadGroup(loadGroup);

Actually, just do this if NS_SUCCEEDED(rv). I.e. if we successfully started the request.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Boris Zbarsky [:bz] from comment #9)
> The explicit scheme check does not make me a happy camper, but ok.

I think it's ok since this isn't a security check. It's rather a "pinging a data: URL is meaningless" check.

Comment 13

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1182537_asyncopen2_navigator.patch

(In reply to Jonas Sicking (:sicking) from comment #11)
> @@ +1284,5 @@
> >      return false;
> >    }
> > +  // make the beaconListener hold a strong reference to the loadgroup
> > +  // which is released in ::OnStartRequest
> > +  beaconListener->setLoadGroup(loadGroup);
> 
> Actually, just do this if NS_SUCCEEDED(rv). I.e. if we successfully started
> the request.

There is an if (NS_FAILED(rv)) block that returns false right above that code. In other words we only set the loadgroup if opening succeeded.

Comment 14

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1182537_asyncopen2_corspreflight.patch

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8640693 [details] [diff] [review]
bug_1182537_asyncopen2_corspreflight.patch

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

Somewhat related, we should probably make NS_StartCORSPreflight fail, rather than use a system principal, if the passed in channel doesn't have a loadinfo.

::: dom/security/nsCORSListenerProxy.cpp
@@ +1182,5 @@
>      AddResultToCache(aRequest);
>  
> +    nsCOMPtr<nsILoadInfo> loadInfo = mOuterChannel->GetLoadInfo();
> +    nsSecurityFlags securityMode = loadInfo ? loadInfo->GetSecurityMode()
> +                                            : 0; // no security mode flags

Please assert that securityMode is either SEC_REQUIRE_CORS_DATA_INHERITS or 0. Otherwise we shouldn't get here.

@@ +1273,5 @@
> +  rv = aRequestChannel->GetLoadInfo(getter_AddRefs(loadInfo));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsSecurityFlags securityMode = loadInfo ? loadInfo->GetSecurityMode()
> +                                          : 0; // no security mode flags

Same here.

@@ +1348,5 @@
>    preflightListener = corsListener;
>  
>    // Start preflight
> +  if (securityMode == nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS) {
> +    rv = preflightChannel->AsyncOpen2(preflightListener);

Sorry, my bad, you also need to remove the CORS-listener-proxy here. Since AsyncOpen2 will install one.

@@ +1351,5 @@
> +  if (securityMode == nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS) {
> +    rv = preflightChannel->AsyncOpen2(preflightListener);
> +  }
> +  else {
> +    rv = preflightChannel->AsyncOpen(preflightListener, nullptr);

But here we still need the listener :(

Comment 16

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1182537_asyncopen2_corspreflight.patch

I agree, we should fail if we do not have a LoadInfo available as of now!

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8640798 [details] [diff] [review]
bug_1182537_asyncopen2_corspreflight.patch

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

::: dom/security/nsCORSListenerProxy.cpp
@@ +1339,5 @@
> +  }
> +  else {
> +    // Set up listener which will start the original channel
> +    nsCOMPtr<nsIStreamListener> preflightListener =
> +      new nsCORSPreflightListener(aRequestChannel, aListener, nullptr, aPrincipal,

You'll still need this listener for the AsyncOpen2 case. This listener is what actually calls AsyncOpen(2) on the channel if the preflight succeeds. See nsCORSPreflightListener::OnStartRequest

Comment 18

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1182537_asyncopen2_corspreflight.patch

Makes sense, we still want the nsCORSPreflightListener in both cases, but asyncOpen(2) sets up the nsCORSListenerProxy for us, whereas the regular open doesn't.

That should be correct now.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8640880 [details] [diff] [review]
bug_1182537_asyncopen2_corspreflight.patch

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

::: dom/security/nsCORSListenerProxy.cpp
@@ +1192,5 @@
> +               securityMode == nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS,
> +               "how did we end up here?");
> +
> +    if (securityMode == nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS) {
> +      rv = mOuterChannel->AsyncOpen2(mOuterListener);

Add an assertion that mOuterContext is null

@@ +1354,2 @@
>  
> +    rv = preflightChannel->AsyncOpen(preflightListener, nullptr);

Nit: Just pass corsListener here instead of first assigning it to preflightListener.

Comment 20

[Christoph Kerschbaumer [:ckerschb}]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c00d03cf9366

Comment 21

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1182537_asyncopen2_corspreflight.patch

Let's have a look:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=00203039437b

Comment 22

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #21)
> Created attachment 8640900 [details] [diff] [review]
> bug_1182537_asyncopen2_corspreflight.patch
> 
> Let's have a look:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=00203039437b

Looks good; missed the '!' when adding the assertion last night:
Assertion failure: mOuterContext (AsyncOpen(2) does not take context as a second arg), at /builds/slave/try-lx-d-000000000000000000000/build/src/dom/security/nsCORSListenerProxy.cpp:1196

should be:
> MOZ_ASSERT(!mOuterContext, "AsyncOpen(2) does not take context as a second arg");

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a11078290110
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc7fa51079ad

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a11078290110
https://hg.mozilla.org/mozilla-central/rev/dc7fa51079ad

Comment 26

[Christoph Kerschbaumer [:ckerschb}]

It seems that Bug 1190201 is not going to land anytime before the 22nd (which is when the next uplift happens). Even then it's not a great idea to uplift a security sensitive patch to beta. I just discussed with Jonas over IRC and we both agree that we should back out [1] (only the one patch) on aurora before it goes into beta.

Ryan, could you please perform that backout for me?

[1] https://hg.mozilla.org/releases/mozilla-aurora/rev/a11078290110

Comment 27

[Ryan VanderMeulen [:RyanVM]]

I'm not an active sheriff anymore.

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/releases/mozilla-aurora/rev/7239a71b423f

Please update the status-firefox42 flag as desired (since half of this was backed out, unsure if that counts as 'fixed' or 'affected').

Comment 29

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Wes Kocher (:KWierso) from comment #28)
> Backed out in https://hg.mozilla.org/releases/mozilla-aurora/rev/7239a71b423f

Thanks!

Comment 30

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Wes Kocher (:KWierso) from comment #28)
> Please update the status-firefox42 flag as desired (since half of this was
> backed out, unsure if that counts as 'fixed' or 'affected').

Setting the flag to >> Target Milestone: mozilla43 <<

The code that remains in 42 [1] is only plumbing that remains *unused* if the loadInfo does not contain the right security flags.

[1] https://hg.mozilla.org/mozilla-central/rev/dc7fa51079ad

Comment 31

[Christoph Kerschbaumer [:ckerschb}]

Wes, I have discussed things with Jonas and it seems that Bug 1190201 is not going to land anytime soon. Could you please back out the one patch [1] of aurora before its going to hit beta? I will fix the target milestone flags afterwards. (Just to make sure, the same patch as in comment 28). Thanks.

[1] https://hg.mozilla.org/mozilla-central/rev/a11078290110

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This is conflicting with the changes from bug 1199049. Could you provided a rebased backout patch, please?

Comment 33

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Wes Kocher (:KWierso) from comment #32)
> This is conflicting with the changes from bug 1199049. Could you provided a
> rebased backout patch, please?

Wes, I filed bug 1218667 because i think that backout is going to need some discussion. I'll let you know once it's ready. Thanks!