397571 - Some functions in nsIDOMWindowUtils can be used from unprivileged context

Status: RESOLVED FIXED

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

Description

[Martijn Wargers (dead)]

See discussion at:
http://groups.google.com/group/mozilla.dev.quality/browse_thread/thread/2708e1fb33220f4b/e7b9be697c7ec449?lnk=st

Some functions in nsIDOMWindowUtils can be accessed from unprivileged context, for instance the bookmarklet Vlad describes here: http://blog.vlad1.com/archives/2005/10/28/74/

and the focus method here: http://martijn.martijn.googlepages.com/focus.htm

Boris doesn't think that's a good idea.

Comment 1

[Boris Zbarsky [:bzbarsky]]

We need to either close this off from websites (all of nsIDOMWindowUtils) or security-audit the things hanging off it.  People have been adding stuff while assuming that it's not accessible from content, as far as I know.

In particular, I see no reason that sites should be able to force redraw without going back to the event loop.  It encourages scripts that hog the CPU.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

The Redraw() stuff was added to WindowUtils on jst's suggestion with the explicit goal that it be accessible from content (e.g. see the bookmarklet).  It's not really useful for scripts in normal use, so worst case it could be used for a script to spike the CPU... but they can already do that in other ways, so I don't think this is any worse.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I see.  Then perhaps we need to more clearly document that some things on nsIWindowUtils are content-accessible.  I assume we've audited the existing things and made sure they're safe?

Comment 4

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

The only thing that looks potentially scary is the focus function given how rickety our focus code is. So we should add UniversalXPConnect checks to that function and a big-ass warning at the top of the interface file. Also add documentation to the other functions that do UniversalXPConnect checks that they do so.

Comment 5

[Johnny Stenback (:jst)]

Surya, this would be a great bug to start with. All that's needed here is a change to add a security check in the nsDOMWindowUtils::Focus() method (just like what's done in nsDOMWindowUtils::SendMouseEvent()) and some comment changes to make the method descriptions in the interface nsIDOMWindowUtils match the implementation wrt whether the implementation checks if the caller is chrome or not (has UniversalXPConnect enabled). Let me know if you have any questions etc.

Comment 6

[Surya Ismail]

Attachment: Adds UniversalXPConnect check to focus

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 284268 [details] [diff] [review]
Adds UniversalXPConnect check to focus 

Looks good! r+sr+a=jst

I'll land this for you in a bit...

Comment 8

[Johnny Stenback (:jst)]

Fix checked in. Thank you Surya!

Comment 9

[Jeff Walden [:Waldo]]

Surya, could you write a Mochitest that double-checks that these methods/properties all throw/don't throw correctly when called from unprivileged content?  See the following URLs for details, or look at examples in the tree (e.g. content/html/document/test):

http://developer.mozilla.org/en/docs/Writing_MochiTest-based_unit_tests
http://developer.mozilla.org/en/docs/Mochitest

Comment 10

[Surya Ismail]

Attachment: MochiTest for events

Sorry for delay

Comment 11

[Martijn Wargers (dead)]

Comment on attachment 294166 [details] [diff] [review]
MochiTest for events

I tested this and it looks ok to me.
I'll check this in shortly when the tree gets green.

I guess you could add garbageCollect() and redraw() to the test if you like.

Comment 12

[Martijn Wargers (dead)]

Checking in Makefile.in;
/cvsroot/mozilla/dom/tests/mochitest/bugs/Makefile.in,v  <--  Makefile.in
new revision: 1.17; previous revision: 1.16
done
RCS file: /cvsroot/mozilla/dom/tests/mochitest/bugs/test_bug397571.html,v
done
Checking in test_bug397571.html;
/cvsroot/mozilla/dom/tests/mochitest/bugs/test_bug397571.html,v  <--  test_bug39
7571.html
initial revision: 1.1
done

Checked in the Mochitest, thanks!