601030 - autofocus could be used to steal user's focus

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mounir Lamouri (:mounir)]

We should define how much could focus be moved with autofocus and what is already doable.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

I don't quite understand this bug.
Does autofocus introduce some new way to steal focus?
.focus() can do that always in many cases.

Comment 2

[Mounir Lamouri (:mounir)]

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.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 4

[Mounir Lamouri (:mounir)]

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.

Comment 5

[Mounir Lamouri (:mounir)]

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 ;)

Comment 6

[Mounir Lamouri (:mounir)]

Attachment: Testcase

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

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().

Comment 8

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - Don't let a frame to steal focus from another frame.

This is fixing a test which was using iframe to test autofocus.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 10

[Mounir Lamouri (:mounir)]

(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.

Comment 11

[Mounir Lamouri (:mounir)]

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.

Comment 12

[Mounir Lamouri (:mounir)]

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.

Comment 13

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Don't let a frame steal focus when the document has been loaded

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.

Comment 14

[Mounir Lamouri (:mounir)]

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?

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

"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?

Comment 16

[Mounir Lamouri (:mounir)]

(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 17

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 18

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Don't let a frame steal focus when the document has been loaded

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.

Comment 19

[Mounir Lamouri (:mounir)]

Attachment: Inter diff

Comment 20

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/14ca1420a81d
http://hg.mozilla.org/mozilla-central/rev/dc8dea324142