1334690 - Isolate AlternateService mappings by Origin Attributes

Status: RESOLVED FIXED

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

Description

[Arthur Edelstein [:arthur]]

Alt-Svc headers cause state to be stored in the browser, which is a super-cookie vector. For this reason Tor Browser currently disables these headers (see https://trac.torproject.org/16673). We would like to propose isolating stored Alt-Svc data by first-party domain when "privacy.firstparty.isolate" is enabled.

Comment 1

[Patrick McManus [:mcmanus]]

also be sure to suppress the alt-used request header

Comment 2

[Tanvi Vyas[:tanvi]]

This should happen for private browsing and containers to, so making the subject more generic.

Comment 3

[Jonathan Hao (inactive) [:jhao]]

Attachment: Bug 1334690 - Test that AlternateService is isolated by origin attributes.

Review commit: https://reviewboard.mozilla.org/r/112694/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112694/

Comment 4

[Jonathan Hao (inactive) [:jhao]]

Attachment: Bug 1334690 - Isolate AlternateService mappings by Origin Attributes.

Review commit: https://reviewboard.mozilla.org/r/112696/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112696/

Comment 5

[Patrick McManus [:mcmanus]]

Comment on attachment 8837571 [details]
Bug 1334690 - Isolate AlternateService mappings by Origin Attributes.

https://reviewboard.mozilla.org/r/112696/#review114254

nice and straightforward. thanks

Comment 6

[Patrick McManus [:mcmanus]]

Comment on attachment 8837570 [details]
Bug 1334690 - Test that AlternateService is isolated by origin attributes.

https://reviewboard.mozilla.org/r/112694/#review114252

::: netwerk/protocol/http/nsHttpChannel.cpp:5897
(Diff revision 1)
>      SetDoNotTrack();
>  
>      OriginAttributes originAttributes;
>      NS_GetOriginAttributes(this, originAttributes);
>  
> +    LOG(("JONATHAN: pid = %d", originAttributes.mPrivateBrowsingId));

delete

Comment 7

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8837570 [details]
Bug 1334690 - Test that AlternateService is isolated by origin attributes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112694/diff/1-2/

Comment 8

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8837571 [details]
Bug 1334690 - Isolate AlternateService mappings by Origin Attributes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112696/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/19cbf2e84f80
Test that AlternateService is isolated by origin attributes. r=mcmanus
https://hg.mozilla.org/integration/autoland/rev/43e48c2dde48
Isolate AlternateService mappings by Origin Attributes. r=mcmanus

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/19cbf2e84f80
https://hg.mozilla.org/mozilla-central/rev/43e48c2dde48

Comment 11

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8837571 [details]
Bug 1334690 - Isolate AlternateService mappings by Origin Attributes.

Approval Request Comment
[Feature/Bug causing the regression]: Preexisting issue.
[User impact if declined]:
1. The first party isolation will be imcomplete.  Tor browser need this patch in ESR 52 to enable Alternative Service for their users.
2. Containers users won't have isolated alternative service cache, either.
[Is this code covered by automated tests?]: Yes, in the other patch test_altsvc.js.
[Has the fix been verified in Nightly?]: Yes, the test is green.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: The other patch.
[Is the change risky?]: Slightly.
[Why is the change risky/not risky?]: Most users will have default origin attributes, and shouldn't feel anything different.
[String changes made/needed]: None.

Comment 12

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8837570 [details]
Bug 1334690 - Test that AlternateService is isolated by origin attributes.

Approval Request Comment

The same as the previous request.

Comment 13

[Ethan Tseng [:ethan]]

Hey, Jonathan, thanks for fixing this so quickly!

Comment 14

[Patrick McManus [:mcmanus]]

re uplift - is Tor expecting to enable h2 in their fork of ESR 52 (which they currently do not).

This is h2 only code (as we implement it - 7540 does allow h1, but we don't do that for other reasons..)

Comment 15

[Ethan Tseng [:ethan]]

(In reply to Patrick McManus [:mcmanus] from comment #14)
> re uplift - is Tor expecting to enable h2 in their fork of ESR 52 (which
> they currently do not).
> This is h2 only code (as we implement it - 7540 does allow h1, but we don't
> do that for other reasons..)

Arthur, do you know the answer?

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 8837571 [details]
Bug 1334690 - Isolate AlternateService mappings by Origin Attributes.

we're past code freeze for 52, this seems late for non-crash, non-regression bugs.

Comment 17

[Arthur Edelstein [:arthur]]

(In reply to Patrick McManus [:mcmanus] from comment #14)
> re uplift - is Tor expecting to enable h2 in their fork of ESR 52 (which
> they currently do not).
> 
> This is h2 only code (as we implement it - 7540 does allow h1, but we don't
> do that for other reasons..)

I think at least initially, we will keep h2 disabled in TBB/ESR52, to give us time to review privacy properties and make any needed patches.