1192955 - Use channel->ascynOpen2 for PING in docshell/base/nsDocShell.cpp

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1192955_asyncOpen2_ping.patch

Comment 2

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

Comment on attachment 8645896 [details] [diff] [review]
bug_1192955_asyncOpen2_ping.patch

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

It'd also be awesome if you want to do the same cleanup that we did for the sendBeacon code. I.e. create a loadgroup and use the docshell as the notification callback on that loadgroup. That way you can remove the nsINetworkInterceptController hack that's there now. Up to you.

::: docshell/base/nsDocShell.cpp
@@ +328,5 @@
>    // Ignore non-HTTP(S)
> +  bool isHttpScheme =
> +    (NS_SUCCEEDED(aURI->SchemeIs("http", &isHttpScheme)) && isHttpScheme) ||
> +    (NS_SUCCEEDED(aURI->SchemeIs("https", &isHttpScheme)) && isHttpScheme);
> +  return isHttpScheme;

Ideally I'd change this to check that we're not pinging data: to keep things consistent with sendBeacon, but I'll leave that to you.

@@ +534,5 @@
>  {
>    nsCOMPtr<nsIURI> newURI;
>    aNewChan->GetURI(getter_AddRefs(newURI));
>  
> +  if (!CheckPingURI(newURI)) {

Remove this check. The whole thing of only allowing http URLs is somewhat unneccessary, so we should only bother doing it on the original URL IMO.

This'll mean that you'll end up with just one callsite of CheckPingURI, so you can just inline the function at that callsite.

@@ +591,5 @@
>    nsCOMPtr<nsIChannel> chan;
>    NS_NewChannel(getter_AddRefs(chan),
>                  aURI,
>                  doc,
> +                nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL,

There's some code in the redirect handler which forces the ping to be same-origin when "info->requireSameHost" is true.

Rather than enforcing that in the redirect handler, use SAME_ORIGIN here if that's true and remove the current code to handle the same-origin restriction.

I think that means that you can remove the whole redirect-handler. So the nsPingListener won't need to implement nsIChannelEventSink.

Comment 3

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1192955_asyncOpen2_ping.patch

(In reply to Jonas Sicking (:sicking) from comment #2)
> It'd also be awesome if you want to do the same cleanup that we did for the
> sendBeacon code.

That cleans things up quite drastically. Looks way better this way - thanks for all the feedback!

Comment 4

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

Comment on attachment 8646028 [details] [diff] [review]
bug_1192955_asyncOpen2_ping.patch

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

::: docshell/base/nsDocShell.cpp
@@ +426,1 @@
>    nsCOMPtr<nsIContent> mContent;

You can remove mContent as well.

@@ +510,4 @@
>      // only require the same hostname.  The scheme and port may differ.
>      if (!IsSameHost(aURI, info->referrer)) {
>        return;
>      }

You can remove this check since the channel will take care of it. Which means that you can kill the IsSameHost function entirely.

@@ +517,5 @@
>  
> +  nsSecurityFlags securityFlags =
> +    info->requireSameHost ?
> +    nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED :
> +    nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL;

Nit: I would generally lean towards inlining the flags computation inside the NS_NewChannel statement. Just to make sure that the code to create the channel is kept with the code to compute the flags.

It's a major aspect of the security policy so it's good that we can always tell with certainty what policy is used for various parts of the code.

Up to you though. Especially since it might be hard to keep the 80-column limit if you inline.

@@ +629,5 @@
>  
> +  nsRefPtr<nsPingListener> pingListener =
> +    new nsPingListener(aContent, loadGroup);
> +
> +  chan->AsyncOpen2(pingListener);

You should set the loadgroup in the listener right here, if and only if AsyncOpen2 succeeded? Otherwise you'll leak if the channel fails to open. This was a problem in the old code too, but is a bigger problem now since opening the channel can fail in more ways.

Making mLoadGroup public is the easiest way to do so.

Comment 5

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1192955_asyncOpen2_ping.patch

(In reply to Jonas Sicking (:sicking) from comment #4)

Thanks Jonas - incorporated all of your other suggestions. Carrying over r+.

Comment 6

[Christoph Kerschbaumer [:ckerschb}]

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

Comment 7

[Christoph Kerschbaumer [:ckerschb}]

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

Comment 8

[Christoph Kerschbaumer [:ckerschb}]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/fa170734154a12789670d9621be3483105b35558
changeset:  fa170734154a12789670d9621be3483105b35558
user:       Christoph Kerschbaumer <[e-mail]>
date:       Thu Aug 13 08:53:28 2015 -0700
description:
Bug 1192955 - Use channel->ascynOpen2 for PING in docshell/base/nsDocShell.cpp (r=sicking)

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/fa170734154a