Closed Bug 1170197 Opened 4 years ago Closed 4 years ago

A channel suspend()-ed in http-on-modify-request shouldn't send out any traffic until resume()-d

Categories

(Core :: Networking: HTTP, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
e10s + ---
firefox41 --- affected
firefox42 --- fixed

People

(Reporter: mao, Assigned: dragana)

Details

Attachments

(3 files, 5 obsolete files)

Especially in e10s world (but before, too), policy decisions about blocking a HTTP load may need to be performed asynchronously once acquired information available to a http-on-modify-request observer, e.g. to perform DNS resolution and/or query the child process for DOM-related information.

This could be done by calling nsIHTTPChannel.suspend() in the observer, performing the asynchronous checks, then when all the information is available calling nsIHTTPChannel.resume(), if necessary after nsIHTTPChannel.cancel().

Ideally, if the channel is cancel()-led before being resume()-d, no TCP connection should be made and no network traffic should be generated by this channel.

This is clearly not the case, as tested by running the attached Scratchpad script in browser context and sniffing the traffic to the 82.103.140.144 IPv4 address (hackademix.com).

The observed unwanted traffic is obviously a deal breaker for many security and privacy related use cases, e.g. to implement reliable CSRF protection, requiring preemptive DNS awareness but no ongoing HTTP traffic until we're sure both the origin and the destination are kosher.

The Scratchpad test-case also shows the work-around NoScript is currently using, i.e. calling redirectTo() with the same URL before suspend() in order to prevent unwanted traffic during the asynchronous job. This is obviously undesirable because of the extra complexity and risks (e.g. checking that we're not in a redirect loop, interfering with other redirectTo() clients or redirection-related policies and so on...). 

Fortunately NoScript caches as much information as possible to ensure asynchronous policy checks are performed only when strictly necessary (usually on first connection to a certain domain), but I expect this approach to become more and more clunky, inefficient and problems-prone while we progress with the e10s porting, due to the increasingly asynchronous attitude of the browser.
Supersedes previous because of its annoying usage of window.open() instead of gBrowser.addTab().
Attachment #8613567 - Attachment is obsolete: true
This makes a lot of sense. Maybe honza or dragana would like to take a crack at it?
Flags: needinfo?(honzab.moz)
Flags: needinfo?(dd.mozilla)
I can take it.
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Flags: needinfo?(dd.mozilla)
Flags: needinfo?(honzab.moz)
Flags: needinfo?(mrbkap)
Depends on: 1171407
No longer depends on: 1171407
This reminded me of :mmc telling me about bug 1100024, but it looks like the solution in that bug was tracking-protection specific.
Flags: needinfo?(mrbkap)
(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #4)
> This reminded me of :mmc telling me about bug 1100024, but it looks like the
> solution in that bug was tracking-protection specific.

In facts, it seems they wouldn't have needed any ad-hoc (and apparently quite complicated) solution if this was already fixed.
Attached patch bug_1170197_v1.patch (obsolete) — Splinter Review
I am not sure if you are the right person to review this. (I have seen you on yammer saying that you took over Monica's work).

If nsHttpChannel::Suspend was called before ContinueBeginConnect, no network activities will be started before resume.
Attachment #8617369 - Flags: feedback?(francois)
Comment on attachment 8617369 [details] [diff] [review]
bug_1170197_v1.patch

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

I'm probably not the best person to review the code/approach, but I'm very interested in seeing how we can remove the hack that disables speculative parsing when there's a local match in the URL classifier :)
Attachment #8617369 - Flags: feedback?(francois)
(In reply to François Marier [:francois] from comment #7)
> Comment on attachment 8617369 [details] [diff] [review]
> bug_1170197_v1.patch
> 
> Review of attachment 8617369 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm probably not the best person to review the code/approach, but I'm very
> interested in seeing how we can remove the hack that disables speculative
> parsing when there's a local match in the URL classifier :)

I will have someone review this from necko side but I would like that someone take  look how does effects URL classifier.
Attached patch bug_1170197_v1.patch (obsolete) — Splinter Review
Attachment #8617369 - Attachment is obsolete: true
Attachment #8617762 - Flags: review?(mcmanus)
Comment on attachment 8617762 [details] [diff] [review]
bug_1170197_v1.patch

(In reply to Dragana Damjanovic [:dragana] from comment #8)
> I will have someone review this from necko side but I would like that
> someone take  look how does effects URL classifier.

Ok, I will take a good look at it when I come back from PTO on Tuesday.
Attachment #8617762 - Flags: feedback?(francois)
Comment on attachment 8617762 [details] [diff] [review]
bug_1170197_v1.patch

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

I think this is begging for a test.. even a simple tcp server that fails the test if it gets a connect is probably sufficient. I can review that patch separately.

can you add a comment in beginConnect (or maybe better, in AsyncOpen()) trying to explain the stages where asyncAbort() needs to be called on returning an error?

::: netwerk/base/nsChannelClassifier.h
@@ -33,5 @@
>      bool mIsAllowListed;
>      // True if the channel has been suspended.
>      bool mSuspendedChannel;
>      nsCOMPtr<nsIChannel> mChannel;
> -    nsCOMPtr<nsIHttpChannelInternal> mChannelInternal;

good riddance!

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5133,5 @@
> +        LOG(("Waiting until resume to do async connect [this=%p]\n", this));
> +        mCallOnResume = &nsHttpChannel::ContinueBeginConnect;
> +        rv = NS_OK;
> +    } else if (mCanceled) {
> +        // We may have been cancelled already, by nsChannelClassifierin that

typo -missing space
Attachment #8617762 - Flags: review?(mcmanus) → review+
Attached patch test (obsolete) — Splinter Review
Attachment #8624320 - Flags: review?(mcmanus)
Attached patch bug_1170197_v1.patch (obsolete) — Splinter Review
only comments updated
Attachment #8617762 - Attachment is obsolete: true
Attachment #8617762 - Flags: feedback?(francois)
Attachment #8624352 - Flags: review+
Attachment #8624352 - Flags: feedback?(francois)
Attachment #8624352 - Attachment is obsolete: true
Attachment #8624352 - Flags: feedback?(francois)
Attachment #8624363 - Flags: review+
Attachment #8624363 - Flags: feedback?(francois)
Comment on attachment 8624320 [details] [diff] [review]
test

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

::: netwerk/test/unit/test_suspend_channel_before_connect.js
@@ +23,5 @@
> +}
> +
> +TestServer.prototype = {
> +  onSocketAccepted: function(socket, trans) {
> +    do_check_true(false, "Socket should not have try to connect!");

/try/tried
Attachment #8624320 - Flags: review?(mcmanus) → review+
Attachment #8624363 - Flags: feedback?(francois) → feedback+
Attached patch testSplinter Review
typo fixed.
Attachment #8624320 - Attachment is obsolete: true
Attachment #8626836 - Flags: review+
Keywords: checkin-needed
can we get a try run here ?
Flags: needinfo?(dd.mozilla)
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/785ec4b923d6
https://hg.mozilla.org/mozilla-central/rev/5d4a4c59baae
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.