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

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Giorgio Maone [:ma1]]

Attachment: Scratchpad-based test case, run in browser context

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.

Comment 1

[Giorgio Maone [:ma1]]

Attachment: Scratchpad-based test case, run in browser context

Supersedes previous because of its annoying usage of window.open() instead of gBrowser.addTab().

Comment 2

[Patrick McManus [:mcmanus]]

This makes a lot of sense. Maybe honza or dragana would like to take a crack at it?

Comment 3

[Dragana Damjanovic [:dragana]]

I can take it.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

This reminded me of :mmc telling me about bug 1100024, but it looks like the solution in that bug was tracking-protection specific.

Comment 5

[Giorgio Maone [:ma1]]

(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.

Comment 6

[Dragana Damjanovic [:dragana]]

Attachment: bug_1170197_v1.patch

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.

Comment 7

[François Marier [: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 :)

Comment 8

[Dragana Damjanovic [:dragana]]

(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.

Comment 9

[Dragana Damjanovic [:dragana]]

Attachment: bug_1170197_v1.patch

Comment 10

[François Marier [:francois]]

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.

Comment 11

[Patrick McManus [:mcmanus]]

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

Comment 12

[Dragana Damjanovic [:dragana]]

Attachment: test

Comment 13

[Dragana Damjanovic [:dragana]]

Attachment: bug_1170197_v1.patch

only comments updated

Comment 14

[Dragana Damjanovic [:dragana]]

Attachment: bug_1170197_v1.patch

Comment 15

[Patrick McManus [:mcmanus]]

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

Comment 16

[Dragana Damjanovic [:dragana]]

Attachment: test

typo fixed.

Comment 17

[Carsten Book [:Tomcat]]

can we get a try run here ?

Comment 18

[Dragana Damjanovic [:dragana]]

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

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/785ec4b923d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d4a4c59baae

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/785ec4b923d6
https://hg.mozilla.org/mozilla-central/rev/5d4a4c59baae