1615588 - Extend nsIPromptService to support tab modal system prompts

Status: RESOLVED FIXED

(Firefox :: Security, enhancement, P1)

Description

[Paul Zühlcke [:pbz]]

Attachment: confirm-auth-tab-modal.png

We currently have tabprompts for prompts triggered by websites (window.alert, window.confirm, window.prompt). These I call "content" prompts.
We can't use content prompts for system prompting, because they are easily spoof-able and look like the belong to the web content.

The proposal is to add a "tab" prompt type which can be opened through the prompt service. It would still belong to a tab, thus not steal focus, improve usability and help with a lot of DoS issues we have with window level prompts. To prevent spoofing it should slightly overlap with the parent chrome, like we currently do it for the web payment dialog.

Where possible, our current system window prompts should be switched over to tab prompts.

I've attached a screenshot of my prototype implementation.

Comment 1

[Paul Zühlcke [:pbz]]

Attachment: basic-auth-tabprompt.gif

Comment 2

[Paul Zühlcke [:pbz]]

Attachment: Bug 1615588 - Extended nsIPromptService to support tab modal prompts. r=johannh!,MattN!

Comment 3

[Paul Zühlcke [:pbz]]

Attachment: Bug 1615588 - SearchOneOffs: Use prompt service instead of nsIPrompt. r=johannh!

Depends on D66446

Comment 4

[Paul Zühlcke [:pbz]]

Attachment: Bug 1615588 - pageActions: Use prompt service instead of nsIPrompt. r=johannh!

Depends on D66447

Comment 5

[Paul Zühlcke [:pbz]]

Attachment: Bug 1615588 - nsDocumentViewer: Use prompt service instead of nsIPrompt. r=johannh!

Depends on D66448

Comment 6

[Paul Zühlcke [:pbz]]

Attachment: Bug 1615588 - Updated prompt tests. r=johannh!

Depends on D66449

Comment 7

[Paul Zühlcke [:pbz]]

Attachment: Bug 1615588 - TabModalPrompt(Box): Added tab level system prompts. r=MattN!

This patch updates the prompt code in browser.js and the tabprompts.jsm module
to support the two new prompt types: tab and content

• Updated TabModalPromptBox to support both prompt types
• Updated TabModalPrompt styles for tab prompt type

Depends on D66446

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/264e642042b1
Extended nsIPromptService to support tab modal prompts. r=johannh,MattN
https://hg.mozilla.org/integration/autoland/rev/32bb87f48b13
TabModalPrompt(Box): Added tab level system prompts. r=MattN
https://hg.mozilla.org/integration/autoland/rev/7839b95ef76c
SearchOneOffs: Use prompt service instead of nsIPrompt. r=johannh
https://hg.mozilla.org/integration/autoland/rev/474aca043834
pageActions: Use prompt service instead of nsIPrompt. r=johannh
https://hg.mozilla.org/integration/autoland/rev/751cca7566a8
Updated prompt tests. r=marionette-reviewers,johannh,whimboo

Comment 9

[Tom S [:evilpie]]

This is amazing! It's great to have this attack vector finally fixed.

Comment 10

[Natalia Csoregi [:nataliaCs]]

Backed out for browser-chrome failures e.g. browser_beforeunload_duplicate_dialogs.js

backout: https://hg.mozilla.org/integration/autoland/rev/12522eae03eb4edaedb7b69f9ccd123b7be3ac9b

push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=297167226&revision=751cca7566a856d7d03b8a7ec8fa265fe32be473&searchStr=browser-chrome

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=297167226&repo=autoland&lineNumber=2357

[task 2020-04-10T17:51:55.641Z] 17:51:55 INFO - Buffered messages finished
[task 2020-04-10T17:51:55.641Z] 17:51:55 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js | Test timed out -
[task 2020-04-10T17:51:55.641Z] 17:51:55 INFO - GECKO(1697) | MEMORY STAT | vsize 7657MB | residentFast 336MB | heapAllocated 123MB
[task 2020-04-10T17:51:55.641Z] 17:51:55 INFO - TEST-OK | browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js | took 90188ms
[task 2020-04-10T17:51:55.641Z] 17:51:55 INFO - GECKO(1697) | [Child 1702: Main Thread]: I/DocShellAndDOMWindowLeak ++DOCSHELL 0x107410800 == 1 [pid = 1702] [id = {74362761-c8a5-3445-88dc-2f00b659188c}]
[task 2020-04-10T17:51:55.641Z] 17:51:55 INFO - GECKO(1697) | [Child 1702: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 1 (0x10dba0ac0) [pid = 1702] [serial = 14] [outer = 0x0]
[task 2020-04-10T17:51:55.641Z] 17:51:55 INFO - GECKO(1697) | [Child 1702, Main Thread] WARNING: NS_ENSURE_TRUE(mPresShell) failed: file /builds/worker/checkouts/gecko/layout/base/nsPresContext.cpp, line 844
[task 2020-04-10T17:51:55.642Z] 17:51:55 INFO - GECKO(1697) | [Child 1702: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 2 (0x10db88c00) [pid = 1702] [serial = 15] [outer = 0x10dba0ac0]
[task 2020-04-10T17:51:55.642Z] 17:51:55 INFO - GECKO(1697) | [Child 1702: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 3 (0x10dc6a800) [pid = 1702] [serial = 16] [outer = 0x10dba0ac0]
[task 2020-04-10T17:51:55.642Z] 17:51:55 INFO - checking window state
[task 2020-04-10T17:51:55.642Z] 17:51:55 INFO - Not taking screenshot here: see the one that was previously logged
[task 2020-04-10T17:51:55.642Z] 17:51:55 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js | Found a browser window after previous test timed out -
[task 2020-04-10T17:51:55.642Z] 17:51:55 INFO - Not taking screenshot here: see the one that was previously logged
[task 2020-04-10T17:51:55.642Z] 17:51:55 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_beforeunload_duplicate_dialogs.js | Found a after previous test timed out -
[task 2020-04-10T17:51:55.642Z] 17:51:55 INFO - GECKO(1697) | must wait for focus

Comment 11

[Natalia Csoregi [:nataliaCs]]

Also remote failures: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=297170440&repo=autoland&lineNumber=10609
Devtools: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=297170215&repo=autoland&lineNumber=134048

Comment 12

[Paul Zühlcke [:pbz]]

Attachment: Bug 1615588 - Extended GeckoView prompt implementation to support prompting by BrowsingContext. r=snorp

Depends on D67134

Comment 13

[Paul Zühlcke [:pbz]]

There was an issue with the patch stack order. I've fixed it and will re-land this once the GeckoView patch is r+.

Comment 14

[James Teh [:Jamie]]

I don't fully understand what's being proposed here. However, I have some a11y concerns.

(In reply to Paul Zühlcke [:pbz] from comment #0)

The proposal is to add a "tab" prompt type which can be opened through the prompt service. It would still belong to a tab, thus not steal focus,

So these prompts wouldn't get focus at all? This will very probably be a problem for keyboard and/or screen reader users, since they depend somewhat on focus to interact with things. For screen reader users, the prompt may not get reported at all, which is even worse.

and help with a lot of DoS issues we have with window level prompts.

Can you clarify the issue here? As I understand this, this doesn't change "content prompts", so what's the attack vector?

I've attached a screenshot of my prototype implementation.

Note that I'm totally blind, so I can't see the screen shot to understand the UX here.

Comment 15

[Johann Hofmann [:johannh]]

Thank you for chiming in on a11y side Jamie, we probably should have included you earlier, apologies for that. Note that for this bug we are reusing the content prompt layout and thus should hopefully not run into a lot of issues.

So these prompts wouldn't get focus at all? This will very probably be a problem for keyboard and/or screen reader users, since they depend somewhat on focus to interact with things. For screen reader users, the prompt may not get reported at all, which is even worse.

I'll let Paul speak to this but I think what is meant is that the prompt will get focus but not disallow the user to focus other pieces of UI in the browser.

Can you clarify the issue here? As I understand this, this doesn't change "content prompts", so what's the attack vector?

The prompts we're replacing can still be triggered by web content, even though they are not content prompts. The attack vector is simply spawning them repeatedly so that the user can't interact with the browser anymore. This has been and is still actively exploited in the wild.

Note that I'm totally blind, so I can't see the screen shot to understand the UX here.

For now these prompts use the same layout as content prompts (from alert etc.), but we are planning a visual redesign that would probably not impact a11y much. We should still keep you in the loop on that.

Thanks!

Comment 16

[Paul Zühlcke [:pbz]]

Thanks Johann! And thank you Jamie for bringing up the a11y concerns.

Accessibility wise the new prompt type shouldn't change anything. As Johann said, it behaves very similar to the content (website) prompts.
For the visual redesign, separating tab and content prompts more, maybe we can also make this separation clear for users with screen readers.

This e-mail to dev-platform explains the issue a bit more in depth: https://groups.google.com/forum/#!topic/mozilla.dev.platform/00OOAwDuZ8o

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd8230b13071
Extended nsIPromptService to support tab modal prompts. r=johannh,MattN,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/b1d2ccd21a7e
TabModalPrompt(Box): Added tab level system prompts. r=MattN
https://hg.mozilla.org/integration/autoland/rev/3c85f19b11ac
Extended GeckoView prompt implementation to support prompting by BrowsingContext. r=snorp
https://hg.mozilla.org/integration/autoland/rev/d4df36221ce6
nsDocumentViewer: Use prompt service instead of nsIPrompt. r=johannh,jfkthame
https://hg.mozilla.org/integration/autoland/rev/1517eb2e42e9
SearchOneOffs: Use prompt service instead of nsIPrompt. r=johannh
https://hg.mozilla.org/integration/autoland/rev/b8a3bd3cb0a8
pageActions: Use prompt service instead of nsIPrompt. r=johannh
https://hg.mozilla.org/integration/autoland/rev/4b428169709d
Updated prompt tests. r=marionette-reviewers,johannh,whimboo

Comment 18

[James Teh [:Jamie]]

Thanks for your replies.

(In reply to Paul Zühlcke [:pbz] from comment #16)

Accessibility wise the new prompt type shouldn't change anything. As Johann said, it behaves very similar to the content (website) prompts.

Just to assuage my paranoia :), could you tell me an easy way to trigger one of these so I can spot check once this lands?

For the visual redesign, separating tab and content prompts more, maybe we can also make this separation clear for users with screen readers.

I believe content prompts don't currently have a dialog title for a11y. Maybe we could use that to distinguish browser prompts? It's hard to know whether the security afforded by this extra verbosity outweighs the user annoyance caused by extra verbosity, though.

Comment 19

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/fd8230b13071
https://hg.mozilla.org/mozilla-central/rev/b1d2ccd21a7e
https://hg.mozilla.org/mozilla-central/rev/3c85f19b11ac
https://hg.mozilla.org/mozilla-central/rev/d4df36221ce6
https://hg.mozilla.org/mozilla-central/rev/1517eb2e42e9
https://hg.mozilla.org/mozilla-central/rev/b8a3bd3cb0a8
https://hg.mozilla.org/mozilla-central/rev/4b428169709d

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c1122301facc
Fix Thunderbird Prompts following bug 1615588 changes. r=mkmelin