Some functions in nsIDOMWindowUtils can be used from unprivileged context

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Martijn Wargers (zombie), Assigned: Surya Ismail)

Tracking

Trunk
x86
Windows XP
Points:
---
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

11 years ago
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.
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.
Flags: blocking1.9?
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.
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?
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.
Assignee: nobody → jst
Flags: blocking1.9? → blocking1.9+
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.
Assignee: jst → suryaismail
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 6

11 years ago
Created attachment 284268 [details] [diff] [review]
Adds UniversalXPConnect check to focus
Attachment #284268 - Flags: review?(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...
Attachment #284268 - Flags: superreview+
Attachment #284268 - Flags: review?(jst)
Attachment #284268 - Flags: review+
Attachment #284268 - Flags: approval1.9+
Fix checked in. Thank you Surya!
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 9

11 years ago
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
Flags: in-testsuite?
(Assignee)

Comment 10

11 years ago
Created attachment 294166 [details] [diff] [review]
MochiTest for events

Sorry for delay
(Assignee)

Updated

11 years ago
Attachment #294166 - Flags: review?(jwalden+bmo)
(Reporter)

Comment 11

11 years ago
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.
Attachment #294166 - Flags: review?(jwalden+bmo) → review+
(Reporter)

Comment 12

11 years ago
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!
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.