Closed
Bug 397571
Opened 17 years ago
Closed 17 years ago
Some functions in nsIDOMWindowUtils can be used from unprivileged context
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: martijn.martijn, Assigned: suryaismail)
References
()
Details
Attachments
(2 files)
4.37 KB,
patch
|
jst
:
review+
jst
:
superreview+
jst
:
approval1.9+
|
Details | Diff | Splinter Review |
3.43 KB,
patch
|
martijn.martijn
:
review+
|
Details | Diff | Splinter Review |
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•17 years ago
|
||
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.
Comment 3•17 years ago
|
||
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+
Comment 5•17 years ago
|
||
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•17 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•17 years ago
|
||
Attachment #284268 -
Flags: review?(jst)
Comment 7•17 years ago
|
||
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+
Comment 8•17 years ago
|
||
Fix checked in. Thank you Surya!
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 9•17 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•17 years ago
|
||
Sorry for delay
Assignee | ||
Updated•17 years ago
|
Attachment #294166 -
Flags: review?(jwalden+bmo)
Reporter | ||
Comment 11•17 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•17 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+
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•