Closed Bug 1510281 Opened 11 months ago Closed 10 months ago

Use a private and isolated context for search suggestions

Categories

(Firefox :: Search, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 66
Tracking Status
firefox64 --- wontfix
firefox65 --- verified
firefox66 --- verified

People

(Reporter: mak, Assigned: mak)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

We want to always use a private context, and isolate the origin.
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/093960021b8b
Use a private and isolated context for search suggestions. r=mkaply
https://hg.mozilla.org/mozilla-central/rev/093960021b8b
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Comment on attachment 9027925 [details]
Bug 1510281 - Use a private and isolated context for search suggestions. r=mkaply!

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: Fetching search suggestions may store/share unexpected cookies. This is an old defect but being privacy-related we want it sooner than later.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: In a new profile:
1. Type about:preferences#privacy in the address bar, wait for search suggestions to appear, confirm with Enter
2. Open Manage Data
3. Check there's no Google cookie

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The code changes are simple

String changes made/needed: none
Attachment #9027925 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Comment on attachment 9027925 [details]
Bug 1510281 - Use a private and isolated context for search suggestions. r=mkaply!

don't send cookies for search suggestions, approved for 64.0b14
Attachment #9027925 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 9027925 [details]
Bug 1510281 - Use a private and isolated context for search suggestions. r=mkaply!

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is privacy related, see comment 4

User impact if declined: Fetching search suggestions may store unwanted cookies

Fix Landed on Version: 65, 64

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Flipping a default argument, simple changes

String or UUID changes made by this patch: none
Attachment #9027925 - Flags: approval-mozilla-esr60?
Marco, I don't believe this was a good way to fix this bug, I'd like to ask you to please reconsider your fix here.

Private browsing mode shouldn't be abused when you don't want cookies for a channel.  Using private browsing mode has a lot of other implications, that in this case are actually undesirable for privacy.  Let me explain why.

We try to partition the private context from the non-private context, by ensuring that things like the HTTP cache aren't shared between the two.  We try to some extent to make it impossible for a remote server to be able to tie a private and a non-private session coming from the same Firefox client to the same user.

With this patch, effectively key presses in the address bar in a private window can be tied to key presses in a non-private window, which is quite bad.  :(

I think the right way to fix this bug is to:

 a) Revert this patch to ensure that private and non-private search engine channels will continue to remain partitioned away, and
 b) Apply the LOAD_ANONYMOUS flag to all XHRs outgoing from the fetch function here by doing |xhr.channel.loadFlags = Ci.nsIChannel.LOAD_ANONYMOUS;| to ensure that no credentials will be submitted for those requests, and
 c) Apply a unique (per search engine) firstPartyDomain origin attribute to the XHRs in addition.  This will ensure that the traffic will be paritioned per each search engine, that no two search engines can collaborate with each other to see the user's key presses across each other's networks, and that the user's keypresses cannot be tied to the user's normal activity on the search engine's website.

What do you think?
Flags: needinfo?(mak77)
Comment on attachment 9027925 [details]
Bug 1510281 - Use a private and isolated context for search suggestions. r=mkaply!

resetting approval flag to give some time to address the concern in comment 7.
Attachment #9027925 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?
Comment on attachment 9027925 [details]
Bug 1510281 - Use a private and isolated context for search suggestions. r=mkaply!

From comment 7, holding off on this for ESR.
Attachment #9027925 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60-
(In reply to :Ehsan Akhgari from comment #7)
>  b) Apply the LOAD_ANONYMOUS flag to all XHRs outgoing from the fetch
> function here by doing |xhr.channel.loadFlags =
> Ci.nsIChannel.LOAD_ANONYMOUS;| to ensure that no credentials will be
> submitted for those requests, and

This will still store cookies though, it would just avoid us sending them back.
That's I/O and data we don't want.
Is there an alternative way than PB to avoid storing cookies?

>  c) Apply a unique (per search engine) firstPartyDomain origin attribute to
> the XHRs in addition.  This will ensure that the traffic will be paritioned
> per each search engine, that no two search engines can collaborate with each
> other to see the user's key presses across each other's networks

Is this actually possible if we don't send back any cookies through LOAD_ANONYMOUS?

Aryx, I fear we need a backout :(
Flags: needinfo?(mak77) → needinfo?(aryx.bugmail)
Flags: needinfo?(ehsan)
(In reply to Marco Bonardo [::mak] from comment #11)
> (In reply to :Ehsan Akhgari from comment #7)
> >  b) Apply the LOAD_ANONYMOUS flag to all XHRs outgoing from the fetch
> > function here by doing |xhr.channel.loadFlags =
> > Ci.nsIChannel.LOAD_ANONYMOUS;| to ensure that no credentials will be
> > submitted for those requests, and
> 
> This will still store cookies though, it would just avoid us sending them
> back.
> That's I/O and data we don't want.
> Is there an alternative way than PB to avoid storing cookies?

What do you mean by that?  At the HTTP layer that flag prevents setting cookies <https://searchfox.org/mozilla-central/rev/f2028b4c38bff2a50ed6aa1763f6dc5ee62b0cc4/netwerk/protocol/http/HttpBaseChannel.cpp#2531> as well as sending them <https://searchfox.org/mozilla-central/rev/f2028b4c38bff2a50ed6aa1763f6dc5ee62b0cc4/netwerk/protocol/http/HttpBaseChannel.cpp#3517>.

The only catch is that if you set LOAD_ANONYMOUS on a channel and then load a document from it, and allow that document to run JS, that document will be able to use |document.cookie| to read/write cookies.  But we don't use that flag in any such context right now and this new use case wouldn't be such an example either.

> >  c) Apply a unique (per search engine) firstPartyDomain origin attribute to
> > the XHRs in addition.  This will ensure that the traffic will be paritioned
> > per each search engine, that no two search engines can collaborate with each
> > other to see the user's key presses across each other's networks
> 
> Is this actually possible if we don't send back any cookies through
> LOAD_ANONYMOUS?

Yes it is possible, tracking is possible through other means besides HTTP Cookies, for example HTTP ETags, see http://lucb1e.com/rp/cookielesscookies/.  The identifiers stored in ETags can then be exchanged between parties using HTTP redirects.  (The web sucks for privacy basically!)
Flags: needinfo?(ehsan)
Thank you, I will revert the changes and implement your suggestions.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backout from beta on request from mak: https://hg.mozilla.org/releases/mozilla-beta/rev/a5dfb5153b78bc8fd4838c885c92318fe45a3116

Should this bug be open? Then status-firefox65 should be affected. If it only got reopened for the beta backout, please set the general status back to Resolved Fixed. Thank you.
Flags: needinfo?(aryx.bugmail)
I'm sorry moz-phab doesn't seem to happily replace one of the changesets, fighting with it.
Attachment #9027925 - Attachment is obsolete: true
Attachment #9027925 - Flags: approval-mozilla-beta?
I'm sorry Ehsan, I don't know if the phabricator review reset works, the UI looks broken... Thus setting a needinfo too. Please check the diff comments.
Flags: needinfo?(ehsan)
I actually kind of noticed your question by accident there.  :-)  So the needinfo was prudent!  Responded on the review.
Flags: needinfo?(ehsan)
Based on IRC discussion we're going to hold off on this for 64, since we're close to building release candidates and there's a bit of risk involved.
Blocks: 1511339
Next problem: now we don't store cookies nor cache, but the engine host + firstPartyDomain are stored in siteSecurityServiceState.txt for security reasons (HSTS).

This would not be a big deal provided it's one row per engine, but in the OpenSearch/External case, since we don't have a unique identifier, we'd store one row per browser session (firstPartyDomain differs every time). These rows are only removed in case of a Clear History, so they may accumulate.

The only solutions we could think of are:
1. Effectively fix bug 1511339 before this one, so that each engine has a persistent id, and then we'll have at a maximum one row per engine
2. add a LOAD_FLAG to bypass HSTS caching and use it here (less secure)
3. make hsts caching obey LOAD_INHIBIT_PERSISTENT_CACHING and LOAD_INHIBIT_CACHING (less secure)
4. if the growth of siteSecurityServiceState.txt is not a problem or it has a good expiration policy, live with the problem for now until we actually fix bug 1511339 (How do we clean the leftovers then?)

Ehsan suggested asking Keeler about this, and how much the siteSecurityServiceState.txt growing may be a blocking issue. We may pick one of the solutions based on that outcome.
Flags: needinfo?(dkeeler)
Attachment #9028478 - Attachment description: Bug 1510281 - Use a private and isolated context for search suggestions. r=mkaply!,Ehsan! → Bug 1510281 - Use a private and isolated context for search suggestions.
SiteSecurityServiceState.txt growing unboundedly shouldn't be a problem because its size is capped. I'm fairly confident that these entries won't accumulate and replace useful entries because the eviction strategy is to remove the entry with the lowest "score", which is essentially the number of distinct days an entry has been accessed. If a user has many short-lived sessions, the engine host entries will all have low scores and can be evicted. If a user has one super-long-lived session, there won't be a flood of these entries anyway.

That said, fixing bug 1511339 would have the benefit of retaining any HSTS state for these hosts.

Options 2 and 3 would probably be a fair bit of work to implement something we don't really even want to do (I think we would basically be creating another memory-only table that we would have to consult in addition to the others, which complicates an already very convoluted implementation).
Flags: needinfo?(dkeeler)
Thank you Dana, I think we can proceed with 4. for now and we'll try to fix bug 1511339 asap, so that we can also retain security at the top.
Great!  Thanks for getting back so quickly Dana.  Let's proceed with option 4 then.  Reviewing right now.
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/48a91a80b4df
Backed out changeset 093960021b8b because we want to use a different approach. r=mkaply
https://hg.mozilla.org/integration/autoland/rev/546585eae6c5
Use a private and isolated context for search suggestions. r=mkaply,Ehsan
https://hg.mozilla.org/mozilla-central/rev/48a91a80b4df
https://hg.mozilla.org/mozilla-central/rev/546585eae6c5
Status: REOPENED → RESOLVED
Closed: 11 months ago10 months ago
Resolution: --- → FIXED
Target Milestone: Firefox 65 → Firefox 66
Comment on attachment 9028478 [details]
Bug 1510281 - Use a private and isolated context for search suggestions.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: This is a privacy improvement we want to take for our users, we should not be storing any data permanently about search suggestion. In any case we need at least the backout in the other patch.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: See comment 4. In addition about:cache may also be verified.

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The change is pretty limited and it's even safer than the previous one from a privacy point of view.

String changes made/needed: none
Attachment #9028478 - Flags: approval-mozilla-beta?
Comment on attachment 9028475 [details]
Bug 1510281 - Backed out changeset 093960021b8b because we want to use a different approach. r=mkaply!

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: We must take this backout regardless of the patch, because the code shipping in 65 may be worst from a privacy point of view.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Just a backout

String changes made/needed:
Attachment #9028475 - Flags: approval-mozilla-beta?
I verified this issue on Mac OS X 10.14, Ubuntu 14.04 and Windows 10 x64 with FF Nightly 66.0a1(2018-12-11) and I can confirm the fix.
Status: RESOLVED → VERIFIED
Comment on attachment 9028475 [details]
Bug 1510281 - Backed out changeset 093960021b8b because we want to use a different approach. r=mkaply!

[Triage Comment]
It's early in the cycle. Approved for 65.0b4.
Attachment #9028475 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9028478 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I can confirm also the fix on the latest Beta 65.0b4 (20181211223337) with Windows 10 x64, macOS 10.13 and Ubuntu 16.04 x86.
Flags: qe-verify+
Depends on: 1520125
Regressions: 1538737
No longer depends on: 1520125
Regressions: 1520125
Duplicate of this bug: 1406544
You need to log in before you can comment on or make changes to this bug.