1729640 - Isolate sites that users are logged in or have passwords saved for using Android Fission

Status: RESOLVED FIXED

(Toolkit :: Password Manager, enhancement, P2)

Description

[Dimi Lee [:dimi]]

In Bug 1723797, we added a pref to only isolate High-Value sites in the mobile platform.
This bug is used to track how we can identify High-Value sites with password-related heuristics.

Right now, the plan is identifying a High-Value site when the site meets any of the following conditions:

1. The site has a saved login in the password manager
2. The user has logged in to the site

For condition (2), it will be done by detecting when users filled-in their username/password in a site and then submit the login form.

Additional notes:

• Login via OAuth is not covered in this bug.

Comment 1

[Dimi Lee [:dimi]]

One issue we have to solve in the bug is that geckoview only supports fetching logins async, which means we can’t check whether a site has a saved login synchronously.

As discussed offline with Andrea, if synchronously checking whether a site has a saved login is not possible, one possible approach is checking saved logins BEFORE starting the load and then writing the permission to the permission manager when the result is returned. So when the load happens (ShouldIsolate func in Bug 1723797), the permission might be there. But I guess that also means we can’t guarantee if the result can come back in time.

If we want to support checking logins synchronously, I think we might have to maintain logins either in storage-geckoview.js or in another component that uses geckoview’s API to retrieve the logins (Add a LoginDetectionService, for example). Maintaining the logins in another component has the benefit that we only need to maintain the list when the feature is enabled.
But no matter which solution we take, i guess we’ll need

1. Support getAllLoginsAsync
2. Be notified when a login is added or removed from geckoview.

Hi Agi, I would like to know your opinion on this and if you have any suggestions, really appreciated the help!

Comment 2

[[ex-Mozilla] Agi Sferro | :agi]

In general I would much rather keep checking logins asynchronously, most of GeckoView expects that things are done asynchronously and this would be a big departure from that.

(In reply to Dimi Lee [:dimi] from comment #1)

If we want to support checking logins synchronously, I think we might have to maintain logins either in storage-geckoview.js or in another component that uses geckoview’s API to retrieve the logins (Add a LoginDetectionService, for example). Maintaining the logins in another component has the benefit that we only need to maintain the list when the feature is enabled.
But no matter which solution we take, i guess we’ll need

1. Support getAllLoginsAsync

Maybe I'm missing something but it looks like we do support getAllLoginsAsync?

2. Be notified when a login is added or removed from geckoview.

I would argue that maybe we don't need this? when new logins are added to GeckoView that would normally mean that a user logged in to a website and decided to save the credentials, AFAIK the only other way to add a login is to do it manually, but I would think it's OK if we don't catch manually-added logins for this? They will be caught in the next restart anyway (which happens very frequently on Android).

So my suggestion would be to fetch logins around startup to compile the list of "high value" websites and then ignore the ones added manually until the next session/restart.

Comment 3

[Dimi Lee [:dimi]]

(In reply to [PTO until 22nd] Agi Sferro | :agi | ni? for questions | ⏰ PST | he/him from comment #2)

In general I would much rather keep checking logins asynchronously, most of GeckoView expects that things are done asynchronously and this would be a big departure from that.

(In reply to Dimi Lee [:dimi] from comment #1)

If we want to support checking logins synchronously, I think we might have to maintain logins either in storage-geckoview.js or in another component that uses geckoview’s API to retrieve the logins (Add a LoginDetectionService, for example). Maintaining the logins in another component has the benefit that we only need to maintain the list when the feature is enabled.
But no matter which solution we take, i guess we’ll need

1. Support getAllLoginsAsync

Maybe I'm missing something but it looks like we do support getAllLoginsAsync?

I guess not? It looks like the function either returns a empty list or throws Cr.NS_ERROR_NOT_IMPLEMENTED.

2. Be notified when a login is added or removed from geckoview.

I would argue that maybe we don't need this? when new logins are added to GeckoView that would normally mean that a user logged in to a website and decided to save the credentials, AFAIK the only other way to add a login is to do it manually, but I would think it's OK if we don't catch manually-added logins for this? They will be caught in the next restart anyway (which happens very frequently on Android).

That is a good point! I agree that manually adding/removing logins is an edge case that we can ignore. Manually adding/removing logins rarely happens and Restart happens very frequently on Android. As for the case when a user logged in to a website (no matter they choose to save logins or not), theoretically, it will be caught by the form submission heuristic.

So my suggestion would be to fetch logins around startup to compile the list of "high value" websites and then ignore the ones added manually until the next session/restart.

Yes, I agree.

Comment 4

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Dimi Lee [:dimi] from comment #3)

(In reply to Dimi Lee [:dimi] from comment #1)

If we want to support checking logins synchronously, I think we might have to maintain logins either in storage-geckoview.js or in another component that uses geckoview’s API to retrieve the logins (Add a LoginDetectionService, for example). Maintaining the logins in another component has the benefit that we only need to maintain the list when the feature is enabled.
But no matter which solution we take, i guess we’ll need

1. Support getAllLoginsAsync

Maybe I'm missing something but it looks like we do support getAllLoginsAsync?

I guess not? It looks like the function either returns a empty list or throws Cr.NS_ERROR_NOT_IMPLEMENTED.

You're correct, I misread the code earlier. So yeah we can implement that as part of this bug I guess (leaving the ni open in case I missed something, I'll come back to it after my PTO).

Comment 5

[[ex-Mozilla] Agi Sferro | :agi]

Let me know if you need any more information from me :)

Comment 6

[Dimi Lee [:dimi]]

(In reply to Agi Sferro | :agi | ni? for questions | ⏰ PST | he/him from comment #5)

Let me know if you need any more information from me :)

Hi Agi,
Yes, Since the plan is getting all logins while initialization, we'll need geckoview add an API to support fetching all logins.
Could someone in your team work on that?

Comment 7

[[ex-Mozilla] Agi Sferro | :agi]

Redirecting to etoop.

Comment 8

[Dimi Lee [:dimi]]

Attachment: Bug 1729640 - P1. Extend AddHighValuePermission to support 'highValueHasSavedLogin' and 'highValueIsLoggedIn' permission.

This patch:

1. adds two 'highValueHasSavedLogin' and 'highValueIsLoggedIn' permission
2. moves 'AddHighValuePermission' from HttpBaseChannel to ProcessIsolation
   to support more high-value permission type.

Comment 9

[Dimi Lee [:dimi]]

Attachment: Bug 1729640 - P2. Add a nsILoginManager API to fetch all saved logins asynchronously.

The new 'searchAllLoginsAsync' API works the same as the getAllLoginsAsync API.
The purpose of adding the API is to make C++ caller easier to use.

Depends on D127101

Comment 10

[Dimi Lee [:dimi]]

Attachment: Bug 1729640 - P3. Notify event when a form with password is submitted

Depends on D127103

Comment 11

[Dimi Lee [:dimi]]

Attachment: Bug 1729640 - P4. Setting high-value permission for sites that are considered is logged in.

Sites that match the following heuristic are considered high-value:

1. Sites that have a password stored in the password manager
2. Sites that we detect that users submit a form with a password

This patch adds LoginDetectionService to detect login attempts.
When LoginDetectionService finds a site that matches any of the above
heuristics, it adds the corresponding high-value permission to the
permission manager.

Depends on D127104

Comment 12

[Dimi Lee [:dimi]]

Attachment: Bug 1729640 - P5. Add a preference to control the expiration time of login related high-value permission.

Right now, all high-value permissions have the same expiration time, but
we might want to set difference expiration time for different
high-value permission type in the future.

Depends on D127105

Comment 13

[Dimi Lee [:dimi]]

Attachment: Bug 1729640 - P6. Add a preference to enable setting `HighValueIsLoggedIn` permission when fission is off.

The preference should be enabled before rolling out "fission for authenticated sites"
to makes sure we have the permissions there when the feature is on.

Depends on D127106

Comment 14

[Dimi Lee [:dimi]]

Attachment: Bug 1729640 - P7. Add test cases for isolating sites by login related heuristics

Depends on D127107

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae9c5de3902a
P1. Extend AddHighValuePermission to support 'highValueHasSavedLogin' and 'highValueIsLoggedIn' permission. r=necko-reviewers,farre,kershaw
https://hg.mozilla.org/integration/autoland/rev/c7ffe85380d7
P2. Add a nsILoginManager API to fetch all saved logins asynchronously. r=sfoster,tgiles
https://hg.mozilla.org/integration/autoland/rev/66376bee8cef
P3. Notify event when a form with password is submitted r=sfoster,tgiles,geckoview-reviewers,agi
https://hg.mozilla.org/integration/autoland/rev/f44a8f37d84c
P4. Setting high-value permission for sites that are considered is logged in. r=farre,sfoster,tgiles
https://hg.mozilla.org/integration/autoland/rev/43dd05680186
P5. Add a preference to control the expiration time of login related high-value permission. r=farre
https://hg.mozilla.org/integration/autoland/rev/9ec5346cf9d9
P6. Add a preference to enable setting `HighValueIsLoggedIn` permission when fission is off. r=farre
https://hg.mozilla.org/integration/autoland/rev/4d3532da5652
P7. Add test cases for isolating sites by login related heuristics r=farre

Comment 16

[Norisz Fay [:noriszfay]]

Backed out for causing Bp-hybrid bustages on LoginDetectionService.o

Backout link: https://hg.mozilla.org/integration/autoland/rev/93d5e38b8aef8a821d0d3ba97b449facec1daee3
Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=Z52p_5WaQB6_QgmWLvv8kQ.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=4d3532da565253e343ebab4e85a36c8176509e20
Failure log: https://treeherder.mozilla.org/logviewer?job_id=356742049&repo=autoland

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7c50c85f911
P1. Extend AddHighValuePermission to support 'highValueHasSavedLogin' and 'highValueIsLoggedIn' permission. r=necko-reviewers,farre,kershaw
https://hg.mozilla.org/integration/autoland/rev/4ce2eebab0c4
P2. Add a nsILoginManager API to fetch all saved logins asynchronously. r=sfoster,tgiles
https://hg.mozilla.org/integration/autoland/rev/fbd6928e34ef
P3. Notify event when a form with password is submitted r=sfoster,tgiles,geckoview-reviewers,agi
https://hg.mozilla.org/integration/autoland/rev/b8fd0343ca42
P4. Setting high-value permission for sites that are considered is logged in. r=farre,sfoster,tgiles
https://hg.mozilla.org/integration/autoland/rev/5ff8d3adc6eb
P5. Add a preference to control the expiration time of login related high-value permission. r=farre
https://hg.mozilla.org/integration/autoland/rev/fcd4b833ad73
P6. Add a preference to enable setting `HighValueIsLoggedIn` permission when fission is off. r=farre
https://hg.mozilla.org/integration/autoland/rev/5b2a1799ce4e
P7. Add test cases for isolating sites by login related heuristics r=farre

Comment 18

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/c7c50c85f911
https://hg.mozilla.org/mozilla-central/rev/4ce2eebab0c4
https://hg.mozilla.org/mozilla-central/rev/fbd6928e34ef
https://hg.mozilla.org/mozilla-central/rev/b8fd0343ca42
https://hg.mozilla.org/mozilla-central/rev/5ff8d3adc6eb
https://hg.mozilla.org/mozilla-central/rev/fcd4b833ad73
https://hg.mozilla.org/mozilla-central/rev/5b2a1799ce4e

Comment 19

[Natalia Csoregi [:nataliaCs]]

Backed out for causing permafailures on browser_web_process_isolation.js

• backout: https://hg.mozilla.org/mozilla-central/rev/1f57920fc04062c989c749169a31d147227a2962

• push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=EWiijLLIQ_e05TciFNoQog.0&revision=5b2a1799ce4e1a4098eb8420930589872a772c66

• failure log: https://treeherder.mozilla.org/logviewer?job_id=357040694&repo=autoland&lineNumber=32687

[task 2021-11-04T20:29:58.922Z] 20:29:58     INFO - TEST-PASS | dom/ipc/tests/browser_web_process_isolation.js | Frame org_after_isLoggedIn[0][0] has the expected number of children - 
[task 2021-11-04T20:29:58.922Z] 20:29:58     INFO - Leaving test bound test_isolate_high_value
[task 2021-11-04T20:29:58.924Z] 20:29:58     INFO - Buffered messages finished
[task 2021-11-04T20:29:58.925Z] 20:29:58     INFO - TEST-UNEXPECTED-FAIL | dom/ipc/tests/browser_web_process_isolation.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort. - 
[task 2021-11-04T20:29:58.926Z] 20:29:58     INFO - GECKO(8951) | MEMORY STAT | vsize 3088MB | residentFast 445MB | heapAllocated 192MB
[task 2021-11-04T20:29:58.927Z] 20:29:58     INFO - TEST-OK | dom/ipc/tests/browser_web_process_isolation.js | took 113992ms

Comment 20

[Dimi Lee [:dimi]]

Attachment: Bug 1729640 - P8. Request longer timeout for browser_web_process_isolation test. r=farre

Open the test tab takes some time in this testcase
(https://searchfox.org/mozilla-central/rev/d2f8488b6a704443a5c5bfc6d2878171b5f0d393/dom/ipc/tests/browser_web_process_isolation.js#31-43)
, and we open the tab multiple times in this testcase.

Since we add 2 more test scenarios for the testcase, this testcase need more time to finish.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f8ab662198b7
P1. Extend AddHighValuePermission to support 'highValueHasSavedLogin' and 'highValueIsLoggedIn' permission. r=necko-reviewers,farre,kershaw
https://hg.mozilla.org/integration/autoland/rev/c1a757f547b9
P2. Add a nsILoginManager API to fetch all saved logins asynchronously. r=sfoster,tgiles
https://hg.mozilla.org/integration/autoland/rev/a765b54eec8c
P3. Notify event when a form with password is submitted r=sfoster,tgiles,geckoview-reviewers,agi
https://hg.mozilla.org/integration/autoland/rev/11e52a262096
P4. Setting high-value permission for sites that are considered is logged in. r=farre,sfoster,tgiles
https://hg.mozilla.org/integration/autoland/rev/ea5fc06ac247
P5. Add a preference to control the expiration time of login related high-value permission. r=farre
https://hg.mozilla.org/integration/autoland/rev/c05cc9249ca5
P6. Add a preference to enable setting `HighValueIsLoggedIn` permission when fission is off. r=farre
https://hg.mozilla.org/integration/autoland/rev/767daf581e65
P7. Add test cases for isolating sites by login related heuristics r=farre
https://hg.mozilla.org/integration/autoland/rev/8a0a41762955
P8. Request longer timeout for browser_web_process_isolation test. r=farre

Comment 22

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/f8ab662198b7
https://hg.mozilla.org/mozilla-central/rev/c1a757f547b9
https://hg.mozilla.org/mozilla-central/rev/a765b54eec8c
https://hg.mozilla.org/mozilla-central/rev/11e52a262096
https://hg.mozilla.org/mozilla-central/rev/ea5fc06ac247
https://hg.mozilla.org/mozilla-central/rev/c05cc9249ca5
https://hg.mozilla.org/mozilla-central/rev/767daf581e65
https://hg.mozilla.org/mozilla-central/rev/8a0a41762955

Comment 23

[Chris Peterson [:cpeterson]]

Setting status-firefox95=disabled. We don't need to uplift this fix to Beta 95 because Android Fission is not enabled yet.