autofocus could be used to steal user's focus

RESOLVED FIXED in mozilla2.0b9

Status

()

defect
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
mozilla2.0b9
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

9 years ago
We should define how much could focus be moved with autofocus and what is already doable.
I don't quite understand this bug.
Does autofocus introduce some new way to steal focus?
.focus() can do that always in many cases.
(Assignee)

Comment 2

9 years ago
I agree. But Jonas is concerned about cross-domain focus change for example. But this should probably not be a problem if we do something in bug 601031.
If we do not fix bug 601031, we should probably make sure that autofocus can't be too harmful.
Ah, we should probably restrict autofocus if focus is already set to some element
in the content document tree. This way loading an iframe couldn't steal focus.
(Assignee)

Comment 4

9 years ago
We already restrict the autofocus if the focus is set to one element but we only check the document so it might be doable to move the focus to another document with iframe's.

For example, if there is an iframe inserted inside the document which has an autofocus attribute, stealing the focus is easy. Though, in that case, we might be safe considering the iframe has been created by the document. Do we want to prevent that?

The real issue is when an iframe as the focus and another iframe is inserted in the parent document with an autofocus attribute. In that case, we should prevent focus to move.
(Assignee)

Updated

9 years ago
Blocks: 601031
(Assignee)

Comment 5

9 years ago
So, currently, we can't autofocus if:
1. The document has something focused.
Example <input autofocus><input autofocus>
2. The window has focused node.
Example: <iframe><input autofocus></iframe><input autofocus>, the <iframe> is a focused node.

So, this is not taking into account this case:
<iframe><input autofocus></iframe> then inserting the exact same <iframe> in the document.

It's easily fixable (I've a patch in progress). We only have to check that window.top has a focused node instead of window.

However, I really doubt this might be a security issue given that you need script to insert dynamically an <iframe> to steal user's focus and the point here is that autofocus is opening a security hole when the user has script disabled.
And, to be cynical, given the level of non-workiness of autofocus, the user would have to be really unlucky to be attacked with that feature ;)
(Assignee)

Comment 6

9 years ago
Posted file Testcase
(Assignee)

Comment 7

9 years ago
Posted patch Patch v1 (obsolete) — Splinter Review
Is there a way to get a nsGlobalWindow or is that even safe to cast nsPIDOMWindow to nsGlobalWindow? Because what I'm doing is basically nsGlobalWindow::GetTop().
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attachment #494570 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

9 years ago
Whiteboard: [needs review]
(Assignee)

Comment 8

9 years ago
This is fixing a test which was using iframe to test autofocus.
Attachment #494570 - Attachment is obsolete: true
Attachment #494678 - Flags: review?(Olli.Pettay)
Attachment #494570 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

9 years ago
Whiteboard: [needs review] → [needs review][passed-try]
Comment on attachment 494678 [details] [diff] [review]
Part 1 - Don't let a frame to steal focus from another frame.

I would probably QI to window, but this should be safe too.
Attachment #494678 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 10

9 years ago
(In reply to comment #9)
> Comment on attachment 494678 [details] [diff] [review]
> Patch v1.1
> 
> I would probably QI to window, but this should be safe too.

nsGlobalWindow::GetTop doesn't QI so I guess this is safe.
(Assignee)

Comment 11

9 years ago
Comment on attachment 494678 [details] [diff] [review]
Part 1 - Don't let a frame to steal focus from another frame.

Asking approval given that it's an issue that was raised during the security review of this feature. The only risk is to break websites that do use autofocus this way but we don't want them to do that and we can assume no one does that for non-malicious reasons.
Attachment #494678 - Flags: approval2.0?
(Assignee)

Updated

9 years ago
Whiteboard: [needs review][passed-try] → [needs approval][passed-try]
(Assignee)

Comment 12

9 years ago
The only abuse you can do of autofocus now is removing an iframe which has autofocus then append another one with autofocus. We could prevent that by checking that the top document is already loaded and decline autofocus in that case.
(Assignee)

Updated

9 years ago
Attachment #494678 - Attachment description: Patch v1.1 → Part 1 - Don't let a frame to steal focus from another frame.
(Assignee)

Comment 13

9 years ago
I'm not sure if we really want that and I'm wondering if we should use windom.mDocumentLoaded instead of nsIDocument::READYSTATE_COMPLETE. Finally, some tests will have to be fixed.
Attachment #494686 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 14

9 years ago
Jonas and Olli, I think this last patch is fixing all possible security holes (even if I'm not sure there would be usable without script enabled as said in comment 5).
Do you see anything else?
"fixes all security holes" is sort of bold wording as I wouldn't ever say that any patch "fixes all security holes" for any feature.

Anyhow, the only thing I'd like to make sure is that loading a page with <input autofocus> in a "background" tab, i.e. a tab other than the one the user is currently viewing, doesn't make us switch to that tab.

Is that the case?
(Assignee)

Comment 16

9 years ago
(In reply to comment #15)
> "fixes all security holes" is sort of bold wording as I wouldn't ever say that
> any patch "fixes all security holes" for any feature.

Yeah, I agree. I should have weight my words. It seems to prevent all focus stealing situation I've taught about.

> Anyhow, the only thing I'd like to make sure is that loading a page with <input
> autofocus> in a "background" tab, i.e. a tab other than the one the user is
> currently viewing, doesn't make us switch to that tab.
> 
> Is that the case?

It's actually focusing the element exactly like any other focus call. focus() doesn't switch tabs. However, the focus is correctly done when called from a background tab (we have a test checking that). I can add a test to check that the tab doesn't change if you want.
Comment on attachment 494686 [details] [diff] [review]
Part 2 - Don't let a frame steal focus when the document has been loaded

You could keep the check also in the bind, though I assume
autofocus wont be used too often so it doesn't really matter.
Attachment #494686 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Updated

9 years ago
Attachment #494686 - Flags: approval2.0?
(Assignee)

Updated

9 years ago
Whiteboard: [needs approval][passed-try] → [needs-landing][passed-try]
(Assignee)

Comment 18

9 years ago
Olli, I had to change all tests because mochitests are run in an iframe and we can't use autofocus in an iframe anymore if the top document is loaded. Could you review the test changes? I'm going to attach an inter diff.
Attachment #498173 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

9 years ago
Attachment #494686 - Attachment is obsolete: true
(Assignee)

Comment 19

9 years ago
Posted patch Inter diffSplinter Review
Attachment #498173 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 20

9 years ago
Pushed:
http://hg.mozilla.org/mozilla-central/rev/14ca1420a81d
http://hg.mozilla.org/mozilla-central/rev/dc8dea324142
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs-landing][passed-try]
Target Milestone: --- → mozilla2.0b9
(Assignee)

Updated

9 years ago
Depends on: 620078
Depends on: 779114
Depends on: 812889
Depends on: 820320
You need to log in before you can comment on or make changes to this bug.