1516309 - www.youtube.com's service worker on nautil.us doesn't get blocked

Status: NEW

(Core :: Privacy: Anti-Tracking, defect, P3)

Description

[(no longer active)]

STR:

1. Log in to www.youtube.com.
2. Visit http://nautil.us/blog/the-sound-so-loud-that-it-circled-the-earth-four-times.
3. In the Cookies subpanel, http://www.youtube.com would show as unblocked.

This happens because we return true from <https://searchfox.org/mozilla-central/rev/64ef7bc9fb478ba7c292f9fa57813d3fe864201e/toolkit/components/antitracking/AntiTrackingCommon.cpp#1143> with this call stack:

#0  0x00007f9a29c8ae65 in StorageDisabledByAntiTrackingInternal(nsPIDOMWindowInner*, nsIChannel*, nsIPrincipal*, nsIURI*, unsigned int*) (aWindow=<optimized out>, aChannel=<optimized out>, aPrincipal=<optimized out>, aURI=<optimized out>, aRejectedReason=<optimized out>) at /home/ehsan/moz/src/dom/base/nsContentUtils.cpp:8313
#1  0x00007f9a29c8ae65 in nsContentUtils::StorageDisabledByAntiTracking(nsPIDOMWindowInner*, nsIChannel*, nsIPrincipal*, nsIURI*) (aWindow=0x0, aChannel=0x7f9a1ada7868, aPrincipal=<optimized out>, aURI=<optimized out>) at /home/ehsan/moz/src/dom/base/nsContentUtils.cpp:8325
#2  0x00007f9a29c8a828 in nsContentUtils::InternalStorageAllowedForPrincipal(nsIPrincipal*, nsPIDOMWindowInner*, nsIURI*, nsIChannel*) (aPrincipal=0x7f9a1b9a9790, aWindow=0x0, aURI=0x0, aChannel=0x7f9a1ada7868)
    at /home/ehsan/moz/src/dom/base/nsContentUtils.cpp:8431
#3  0x00007f9a29c8a9d8 in nsContentUtils::StorageAllowedForChannel(nsIChannel*) (aChannel=<optimized out>) at /home/ehsan/moz/src/dom/base/nsContentUtils.cpp:8131
#4  0x00007f9a2b297189 in mozilla::dom::ServiceWorkerInterceptController::ShouldPrepareForIntercept(nsIURI*, nsIChannel*, bool*) (this=<optimized out>, aURI=0x7f9a1a730900, aChannel=0x7f9a1ada7868, aShouldIntercept=0x7ffc4c16252f) at /home/ehsan/moz/src/dom/serviceworkers/ServiceWorkerInterceptController.cpp:54
#5  0x00007f9a28fd570d in mozilla::net::HttpBaseChannel::ShouldIntercept(nsIURI*) (this=<optimized out>, aURI=<optimized out>) at /home/ehsan/moz/src/netwerk/protocol/http/HttpBaseChannel.cpp:3063
#6  0x00007f9a28fe7b87 in mozilla::net::HttpChannelChild::ShouldInterceptURI(nsIURI*, bool&) (this=<optimized out>, aURI=<optimized out>, aShouldUpgrade=@0x7ffc4c162627: false)
    at /home/ehsan/moz/src/netwerk/protocol/http/HttpChannelChild.cpp:3730
#7  0x00007f9a28fe79aa in mozilla::net::HttpChannelChild::SetupRedirect(nsIURI*, mozilla::net::nsHttpResponseHead const*, unsigned int const&, nsIChannel**) (this=0x7f9a1adae000, uri=0x7f9a1a730900, responseHead=0x7f9a21b664a0, redirectFlags=@0x7f9a21b6649c: 10, outChannel=0x7ffc4c1626a0) at /home/ehsan/moz/src/netwerk/protocol/http/HttpChannelChild.cpp:1635
#8  0x00007f9a28fe7d25 in mozilla::net::HttpChannelChild::Redirect1Begin(unsigned int const&, mozilla::ipc::URIParams const&, unsigned int const&, unsigned int const&, mozilla::net::ParentLoadInfoForwarderArgs const&, mozilla::net::nsHttpResponseHead const&, nsTSubstring<char> const&, unsigned long const&) (this=0x7f9a1adae000, registrarId=@0x7f9a21b66400: 4, newOriginalURI=..., newLoadFlags=@0x7f9a21b66498: 6619140, redirectFlags=@0x7f9a21b6649c: 10, loadInfoForwarder=..., responseHead=..., securityInfoSerialization=..., channelId=@0x7f9a21b66530: 126461017063554)
    at /home/ehsan/moz/src/netwerk/protocol/http/HttpChannelChild.cpp:1679
#9  0x00007f9a2901588d in mozilla::net::Redirect1Event::Run() (this=<optimized out>) at /home/ehsan/moz/src/netwerk/protocol/http/HttpChannelChild.cpp:1553
#10 0x00007f9a28f9b1da in mozilla::net::ChannelEventQueue::RunOrEnqueue(mozilla::net::ChannelEvent*, bool) (this=<optimized out>, aCallback=<optimized out>, aAssertionWhenNotQueued=false)
    at /home/ehsan/moz/src/obj-ff-opt.noindex/dist/include/mozilla/net/ChannelEventQueue.h:210
#11 0x00007f9a28fe77db in mozilla::net::HttpChannelChild::RecvRedirect1Begin(unsigned int const&, mozilla::ipc::URIParams const&, unsigned int const&, unsigned int const&, mozilla::net::ParentLoadInfoForwarderArgs const&, mozilla::net::nsHttpResponseHead const&, nsTString<char> const&, unsigned long const&, mozilla::net::NetAddr const&) (this=0x7f9a1adae000, registrarId=@0x7ffc4c162808: 4, newUri=..., newLoadFlags=@0x7ffc4c162888: 6619140, redirectFlags=@0x7ffc4c162858: 10, loadInfoForwarder=..., responseHead=..., securityInfoSerialization=..., channelId=@0x7ffc4c162820: 126461017063554, oldPeerAddr=...)
    at /home/ehsan/moz/src/netwerk/protocol/http/HttpChannelChild.cpp:1585
#12 0x00007f9a2932d39b in mozilla::net::PHttpChannelChild::OnMessageReceived(IPC::Message const&) (this=<optimized out>, msg__=...) at /home/ehsan/moz/src/obj-ff-opt.noindex/ipc/ipdl/PHttpChannelChild.cpp:887
#13 0x00007f9a292779f5 in mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) (this=0x7f9a37965820, msg__=...) at /home/ehsan/moz/src/obj-ff-opt.noindex/ipc/ipdl/PContentChild.cpp:5445
#14 0x00007f9a29189582 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) (this=0x7f9a379174d8, aMsg=...) at /home/ehsan/moz/src/ipc/glue/MessageChannel.cpp:2159
#15 0x00007f9a29188515 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) (this=0x7f9a379174d8, aMsg=...) at /home/ehsan/moz/src/ipc/glue/MessageChannel.cpp:2086
#16 0x00007f9a29188c37 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) (this=0x7f9a379174d8, aTask=...) at /home/ehsan/moz/src/ipc/glue/MessageChannel.cpp:1935
#17 0x00007f9a2918903e in mozilla::ipc::MessageChannel::MessageTask::Run() (this=0x7f9a1adebae0) at /home/ehsan/moz/src/ipc/glue/MessageChannel.cpp:1966
#18 0x00007f9a28bb4ede in mozilla::SchedulerGroup::Runnable::Run() (this=0x7f9a1f238330) at /home/ehsan/moz/src/xpcom/threads/SchedulerGroup.cpp:299
#19 0x00007f9a28bc2b46 in nsThread::ProcessNextEvent(bool, bool*) (this=<optimized out>, aMayWait=<optimized out>, aResult=<optimized out>) at /home/ehsan/moz/src/xpcom/threads/nsThread.cpp:1157
#20 0x00007f9a28bc4808 in NS_ProcessNextEvent(nsIThread*, bool) (aThread=0x7f9a1a730900, aMayWait=false) at /home/ehsan/moz/src/xpcom/threads/nsThreadUtils.cpp:468
#21 0x00007f9a2918b97a in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (this=0x7f9a3799a470, aDelegate=0x7ffc4c1636a8) at /home/ehsan/moz/src/ipc/glue/MessagePump.cpp:88
#22 0x00007f9a291217d9 in MessageLoop::RunInternal() (this=<optimized out>) at /home/ehsan/moz/src/ipc/chromium/src/base/message_loop.cc:314
#23 0x00007f9a291217d9 in MessageLoop::RunHandler() (this=<optimized out>) at /home/ehsan/moz/src/ipc/chromium/src/base/message_loop.cc:307
#24 0x00007f9a291217d9 in MessageLoop::Run() (this=0x1) at /home/ehsan/moz/src/ipc/chromium/src/base/message_loop.cc:289
#25 0x00007f9a2b34b5b9 in nsBaseAppShell::Run() (this=0x7f9a21b43be0) at /home/ehsan/moz/src/widget/nsBaseAppShell.cpp:137
#26 0x00007f9a2c973204 in XRE_RunAppShell() () at /home/ehsan/moz/src/toolkit/xre/nsEmbedFunctions.cpp:915
#27 0x00007f9a291217d9 in MessageLoop::RunInternal() (this=<optimized out>) at /home/ehsan/moz/src/ipc/chromium/src/base/message_loop.cc:314
#28 0x00007f9a291217d9 in MessageLoop::RunHandler() (this=<optimized out>) at /home/ehsan/moz/src/ipc/chromium/src/base/message_loop.cc:307
#29 0x00007f9a291217d9 in MessageLoop::Run() (this=0x1) at /home/ehsan/moz/src/ipc/chromium/src/base/message_loop.cc:289
#30 0x00007f9a2c972eb7 in XRE_InitChildProcess(int, char**, XREChildData const*) (aArgc=<optimized out>, aArgv=<optimized out>, aChildData=<optimized out>)
    at /home/ehsan/moz/src/toolkit/xre/nsEmbedFunctions.cpp:753
#31 0x000055f7c156934b in content_process_main(mozilla::Bootstrap*, int, char**) (bootstrap=0x7f9a379296b0, argc=<optimized out>, argv=<optimized out>)
    at /home/ehsan/moz/src/browser/app/../../ipc/contentproc/plugin-container.cpp:49
#32 0x000055f7c156934b in main(int, char**, char**) (argc=<optimized out>, argv=0x7ffc4c1649c8, envp=0x7ffc4c164a60) at /home/ehsan/moz/src/browser/app/nsBrowserApp.cpp:265

Looks like in this case we need to do the classification before this point.  What do you think, baku?

Comment 1

[Andrea Marchesini [:baku]]

During a redirect, we create a new HttpChannelChild object, we call ShouldInterceptURI() and in case that method returns true, we store |shouldUpgrade| and a couple of extra data. At some point, AsyncOpen() will be called on that new HttpChannelChild object and we will use TrackingDummyChannel() to check if the channel must be intercepted or not.

So, I don't think we need to move the classifier in an earlier state. Andrew, am I right here?

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

== Shorter
Yeah, I think :baku's right here, although by my guess we've already done a dummy channel check that we've thrown away, so you certainly have the option to save that value off earlier on if you really want to.  (However, that would be specific to child intercept, so I'm not sure I'd recommend adding more complexity there right now.)

== Longer
Restating the context as I understand it (under *child intercept*):
- http://nautil.us/blog/the-sound-so-loud-that-it-circled-the-earth-four-times embeds an iframe with src of "//www.youtube.com/embed/BUREX8aFbMs" which uses Strict-Transport-Security and so is subject to an upgrade.
- Secure upgrading...
  - In the non-ServiceWorker case is handled in nsHttpChannel::BeforeConnect which triggers an async redirect which the child will perceive via RecvRedirect1Begin
  - But in the ServiceWorker case HttpChannelChild::ShouldInterceptURI is invoked as part of HttpChannelChild::AsyncOpen and if it's the one to decide a secure upgrade is in order, it will invoke HttpChannelChild::BeginNonIPCRedirect to perform the upgrade, but the parent won't be asked.

So I hypothesize based on :ehsan's stack without accompanying logs that:
- The http://youtube.com was already considered for intercept with a dummy tracking channel and the decision was not made to intercept.
- So the channel got created to go to the parent where the parent channel decided to perform the secure upgrade which now triggers a redirect.
- This leaves us in a situation similar to where we went to the network and got a redirect.  SetupRedirect marks the new redirected HttpChannelChild with mPostRedirectChannelShouldIntercept which means that in the future, after :ehsan's stack, AsyncOpen() will see the flag and bypass the call to ShouldInterceptURI but _will_ run the dummy channel check when it gets around to it.

I do assume something is going wrong somewhere, though...

Comment 3

[(no longer active)]

Sorry for the delay in the response, and also for not explaining this as well as I could have in comment 0.

So first let's agree on some ground truth.

• On Nightly, www.youtube.com is on the Disconnect Tracking Protection (Strict) list https://github.com/mozilla-services/shavar-prod-lists/blob/6cb4367c87585dd21ff83e4e530d3563f05d5f3b/disconnect-blacklist.json#L7520.
• As such, iframes loading content from www.youtube.com should never be allowed to be intercepted by a service worker registered for https://www.youtube.com.

With that in mind, let's look at this code:

https://searchfox.org/mozilla-central/rev/bee8cf15c901b9f4b0c074c9977da4bbebc506e3/netwerk/protocol/http/HttpChannelChild.cpp#1636

Here, we call ShouldInterceptURI(). This is frame #6 in the call stack in comment 0. From there we end up doing a storage access check as can be seen in the call stack, which incorrectly succeeds here https://searchfox.org/mozilla-central/rev/bee8cf15c901b9f4b0c074c9977da4bbebc506e3/toolkit/components/antitracking/AntiTrackingCommon.cpp#1183. This means ShouldInterceptURI() incorrectly returns true, and we end up incorrectly setting mPostRedirectChannelShouldIntercept after that, and so on.

The bug here is that GetIsThirdPartyTrackingResource() here https://searchfox.org/mozilla-central/rev/bee8cf15c901b9f4b0c074c9977da4bbebc506e3/toolkit/components/antitracking/AntiTrackingCommon.cpp#1181 is returning false. But not because the channel isn't a third-party tracking resource, but because the channel hasn't been classified yet. That is simply because nobody has called AsyncOpen() on the newly created post-redirect channel yet.

Until the result of the classification becomes available, any caller of GetIsThirdPartyTrackingResource() just silently receives a false return value and nobody will tell them the result they got was actually wrong.

(In reply to Andrea Marchesini [:baku] from comment #1)

During a redirect, we create a new HttpChannelChild object, we call
ShouldInterceptURI() and in case that method returns true, we store
|shouldUpgrade| and a couple of extra data. At some point, AsyncOpen() will
be called on that new HttpChannelChild object and we will use
TrackingDummyChannel() to check if the channel must be intercepted or not.

You are forgetting about the fact that storage checks now send notifications to the UI, and the UI makes the result of the notifications visible to the user, aren't you? As mentioned in the STR in comment 0, it is very clear that the result of this incorrect storage check is not just |shouldUpgrade| and a couple of extra data, but also "http://www.youtube.com" being marked as being allowed on the page.

So I still think the bug as filed is quite valid, and we either need to move this storage check to happen after the result of the classification is available, or classify earlier in this case (or maybe copy the flags from the pre-redirect channel in SetupReplacementChannel or something).

Comment 4

[Andrea Marchesini [:baku]]

I have a fix for this. I'm testing my patches.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: SW.patch

The notification is still sent, but that should be a separate bug than fixing the SW channel redirect.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 9037856 [details] [diff] [review]
SW.patch

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

- It's not clear what the intent of this fix is in relation to the discussion of the bug thus far based on the commit message or code comments provided, with the mention of the UI notification just raising questions.  In particular, I'm really confused about why the AsyncOpen dummy-channel-check doesn't actually work when we get around to observing `mPostRedirectChannelShouldIntercept` being true and why we need the additional dummy-channel-check.  Is the false UI update actually causing the third-party check to succeed later because the UI sets a permission?  Does that mean that the permission will still be set, but that the additional earlier check at least lets us get the right answer before the UI bug triggers?  Please provide an improved commit message.
- This needs some kind of test coverage or the commit message should indicate what existing test coverage covers this.  (Which I assume is none or a disabled test that this patch should enable?).  Please provide.
- A necko peer needs to review this too, if only so they can agree that the child intercept logic is ugly and the sooner we get rid of it the better.
- This would be much easier to review in phabricator, which has some understanding of code refactoring by marking re-located/re-indented blocks.  But thankfully it's a small patch (which you are always most excellent about, thank you!)

Restating what's going on as far as I understand right now and which may be useful in composing a commit message maybe:
- SetupRedirect is being made async because the dummy channel check is async.
- We are interposing a dummy-channel-storage-is-okay check between ShouldInterceptURI() returning true indicating that the scope is controlled and calling `ForceIntercepted(true, shouldUpgrade)` which sets mPostRedirectChannelShouldIntercept to true.  This is the logic that :ehsan identified in comment 3 paragraph 5 as problematic because it starts us down the wrong path.  (But I'm not sure why we don't get stopped later down that path.)
  - It's appropriate to call ShouldInterceptURI first to determine whether the scope is controlled, as it would be very bad to do a dummy check all the time.
  - Unfortunately that check has the side-effect of triggering a UI update that says third-party content is allowed.
  - The call stack of the check looks something like: HttpChannelChild::ShouldInterceptURI => HttpBaseChannel::ShouldIntercept => ServiceWorkerInterceptController::ShouldPrepareForIntercept => nsContentUtils::StorageAllowedForChannel => nsContentUtils::InternalStorageAllowedForPrincipal => nsContentUtils::StorageDisabledByAntiTracking => StorageDisabledByAntiTrackingInternal => AntiTrackingCommon::IsFirstPartyStorageAccessGrantedFor => nsIHttpChannel::GetIsThirdPartyTrackingResource.
  - It's nsContentUtils::StorageDisabledByAntiTracking that calls AntiTrackingCommon::NotifyBlockingDecision that does the UI updates, probably.
  - It's nsIHttpChannel::GetIsThirdPartyTrackingResource that's wrong because the channel hasn't been classified yet.

Comment 7

[(no longer active)]

I think the patch here should definitely come with some tests, most likely by adding/modifying a case in https://searchfox.org/mozilla-central/source/browser/base/content/test/trackingUI/browser_trackingUI_cookies_subview.js maybe in addition to other tests. :-)