Closed Bug 1334693 Opened 8 years ago Closed 7 years ago

Investigate and isolate SPDY/HTTP2 state by first-party domain when privacy.firstparty.isolate = true

Categories

(Core :: Networking: HTTP, defect, P1)

Product:
Core
Component:
Networking: HTTP
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: arthur, Assigned: jhao)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [tor][necko-would-take][OA])

Attachments

(3 files, 1 obsolete file)

Bug 1334693 - Add origin attributes to Http2Stream::PushHashKey().
59 bytes, text/x-review-board-request
mcmanus
: review+
Details
Bug 1334693 - Add origin attributes to HTTP2 coalescing keys.
59 bytes, text/x-review-board-request
mcmanus
: review+
Details
Bug 1334693 - Test http2 push and origin attributes.
59 bytes, text/x-review-board-request
mcmanus
: review+
Details 
SPDY and HTTP2 may store state that can be used for third-party tracking. We should identify all such state and isolate it by origin attributes (or first party domain) when isolation is activated.

(In Tor Browser, we currently disable SPDY to avoid such issues.)
spdy is probably not a going concerned - we stopped shipping it in favor of h2.

I just brainstormed a bit on origin attributes for h2 and related things..

* I'd take an OA patch for the alt-svc hashkey (AltSvcMapping::MakeHashKey()).. as long as it didn't impact default config of mainline FF of course
* did the dns cache ever find a way to scope by OA? That's pretty much the same mechanism as alt-svc.
* and I'm 99% sure the http cache did get OA added, but I'm just double checking as a cached 3xx is similar
* there is also the alt-used header (1 bit) that is already suppressed for PB and anonymous channels.. but a pref for tor would be a good addition and easy to add

in h2..  it would probably be good to add OA to http2stream::pushhashkey().. right now its not as impt because the scope of that is the loadgroup and not global, but it might go global so it would be good to have a tor partition in there

also, I'm pretty sure the CI work that added OA to ConnecitonInfo will give the OA protection you want for h2 connection coalescing, but it might be worth reading that code with that use case in mind to make sure before shipping.
Flags: needinfo?(mcmanus)
Whiteboard: [tor] → [tor][necko-would-take]
Whiteboard: [tor][necko-would-take] → [tor][necko-would-take][OA]
Priority: -- → P1
Assignee: nobody → jhao
Status: NEW → ASSIGNED
(In reply to Patrick McManus [:mcmanus] from comment #1)
> spdy is probably not a going concerned - we stopped shipping it in favor of
> h2.
> 
> I just brainstormed a bit on origin attributes for h2 and related things..
> 
> * I'd take an OA patch for the alt-svc hashkey
> (AltSvcMapping::MakeHashKey()).. as long as it didn't impact default config
> of mainline FF of course

Landed in bug 1334690.

> * did the dns cache ever find a way to scope by OA? That's pretty much the
> same mechanism as alt-svc.

This could be done in bug 1337893.

> * and I'm 99% sure the http cache did get OA added, but I'm just double
> checking as a cached 3xx is similar
> * there is also the alt-used header (1 bit) that is already suppressed for
> PB and anonymous channels.. but a pref for tor would be a good addition and
> easy to add

Since alt-svc is isolated by first party domain, and Tor browser is always in private browsing mode, I think we don't need to do anything, do we?
 
> in h2..  it would probably be good to add OA to http2stream::pushhashkey()..
> right now its not as impt because the scope of that is the loadgroup and not
> global, but it might go global so it would be good to have a tor partition
> in there

This seems to be the only thing left, and I plan do that in this bug.

> also, I'm pretty sure the CI work that added OA to ConnecitonInfo will give
> the OA protection you want for h2 connection coalescing, but it might be
> worth reading that code with that use case in mind to make sure before
> shipping.

We could test that in bug 1337868.
Depends on: 1334690, 1337893, 1337868
Comment on attachment 8839335 [details]
Bug 1334693 - Add origin attributes to Http2Stream::PushHashKey().

https://reviewboard.mozilla.org/r/113992/#review115612

::: netwerk/protocol/http/Http2Stream.cpp:374
(Diff revision 1)
>      outOrigin = fullOrigin;
>    }
>  
>    outKey = outOrigin;
> +  nsAutoCString suffix;
> +  originAttributes.CreateSuffix(suffix);

can you put the OA inside a delimiter like "/[OA]".. mostly just for human interpretation
Attachment #8839335 - Flags: review?(mcmanus) → review+
Hi Patrick, thanks for the review.  I wanted to write a test but it's not clear to me how.  Do you have any suggestions?
Flags: needinfo?(mcmanus)
in test_http2.js there are several push tests.. seems like they could be repeated with OA added into them
Flags: needinfo?(mcmanus)
Thanks, Patrick, for your advice.  So I'll probably start with requesting the /pushapi1, which should set a push cache, then see if I can get a cache hit in another origin attributes.  What I don't know is, how am I supposed to know whether the push cache is hit?

BTW, should this line be `headerText + i` instead of `headerText + 1`?
http://searchfox.org/mozilla-central/rev/b1044cf7c2000c3e75e8181e893236a940c8b6d2/netwerk/test/unit/test_http2.js#171
Flags: needinfo?(mcmanus)
nick, would you have a change to dig into :jhao's questions about the test?
Flags: needinfo?(mcmanus) → needinfo?(hurley)
(In reply to Jonathan Hao [:jhao] from comment #7)
> Thanks, Patrick, for your advice.  So I'll probably start with requesting
> the /pushapi1, which should set a push cache, then see if I can get a cache
> hit in another origin attributes.  What I don't know is, how am I supposed
> to know whether the push cache is hit?

If the response comes from the push cache, there will be a "pushed: yes" header on the response. Conversely, any response that was *not* pushed (and therefore did not come from the push cache) will not have a "pushed:" header at all.

> BTW, should this line be `headerText + i` instead of `headerText + 1`?
> http://searchfox.org/mozilla-central/rev/
> b1044cf7c2000c3e75e8181e893236a940c8b6d2/netwerk/test/unit/test_http2.js#171

Yeah, seems like it should. Hopefully that changes doesn't expose any bugs! :)
Flags: needinfo?(hurley)
I tried setting the origin attributes in the channel's loadinfo, but they didn't propagate to the http2 push cache.  I'm trying to find what's wrong.
Hi Patrick and Nick,

Thank you for your previous answer.

After digging the logs, it looks like the Http2Session is always the one that is created at the beginning of the test, as well as its socket transport, even though the channel's loadInfo's originAttributes has changed.

If we want http2 to be isolated by origin attributes, it seems that we might have to use a different session when the origin attributes are different.  

What do you think?
Flags: needinfo?(mcmanus)
Flags: needinfo?(hurley)
Nevertheless, I don't really know when and how we create Http2Session, so I'm not quite sure whether/how we should isolate http2 sessions.
I suspect you'll need to add the OA to the coalescingKey (its currently the anonymous flag and an ip address)
Flags: needinfo?(mcmanus)
Flags: needinfo?(hurley)
fwiw this means that the bit that quotes me in comment 2

> also, I'm pretty sure the CI work that added OA to ConnecitonInfo will give
> the OA protection you want for h2 connection coalescing, but it might be
> worth reading that code with that use case in mind to make sure before
> shipping.

was me being over optimistic (but at least I wanted to verify it :))
Attachment #8842301 - Attachment is obsolete: true
Comment on attachment 8844834 [details]
Bug 1334693 - Add origin attributes to HTTP2 coalescing keys.

https://reviewboard.mozilla.org/r/118168/#review120048
Attachment #8844834 - Flags: review?(mcmanus) → review+
Comment on attachment 8844835 [details]
Bug 1334693 - Test http2 push and origin attributes.

https://reviewboard.mozilla.org/r/118170/#review120050
Attachment #8844835 - Flags: review?(mcmanus) → review+
Pushed by jhao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/85c4356eebea
Add origin attributes to HTTP2 coalescing keys. r=mcmanus
https://hg.mozilla.org/integration/autoland/rev/f48b85ae4eca
Add origin attributes to Http2Stream::PushHashKey(). r=mcmanus
https://hg.mozilla.org/integration/autoland/rev/8042ff76ef53
Test http2 push and origin attributes. r=mcmanus
Pushed by jhao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cf6b24b0ca8a
Add origin attributes to HTTP2 coalescing keys. r=mcmanus
https://hg.mozilla.org/integration/autoland/rev/dbec5f37f86f
Add origin attributes to Http2Stream::PushHashKey(). r=mcmanus
https://hg.mozilla.org/integration/autoland/rev/8d89f6b0b071
Test http2 push and origin attributes. r=mcmanus
Flags: needinfo?(jhao)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: