Closed
Bug 910375
Opened 11 years ago
Closed 11 years ago
New PanelUI / toolbar customization event spoofing
Categories
(Firefox :: Toolbars and Customization, defect)
Firefox
Toolbars and Customization
Tracking
()
VERIFIED
FIXED
Firefox 29
Tracking | Status | |
---|---|---|
firefox26 | --- | wontfix |
firefox27 | --- | affected |
firefox28 | --- | affected |
firefox29 | + | verified |
firefox30 | --- | verified |
firefox31 | --- | verified |
firefox-esr24 | --- | unaffected |
b2g18 | --- | unaffected |
b2g-v1.2 | --- | unaffected |
People
(Reporter: dchanm+bugzilla, Assigned: jaws)
References
()
Details
(Keywords: sec-low, Whiteboard: [Australis:P4][Australis:M9][adv-main29-])
Attachments
(4 files, 3 obsolete files)
278 bytes,
text/html
|
Details | |
5.87 KB,
patch
|
MattN
:
review+
|
Details | Diff | Splinter Review |
4.67 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
4.52 KB,
patch
|
dao
:
review-
|
Details | Diff | Splinter Review |
It is possible to craft a drag and drop event in content which mimics the behavior of a chrome customization event. This attack is very unlikely as it requires the user to have the customizable page / panel open and then drag and drop from another window to the page. The impact is also very low since all an attacker can do is move icons around. The current chrome code only uses the event to pick an icon to move.
Marking this as security sensitive though it likely doesn't have to be since the code hasn't shipped yet.
STR
1. Open about:customizing
2. Open above file in another window or visit
https://people.mozilla.com/~dchan/drag.html
3. Drag the Firefox icon to one of the about:customizing panels
Result
The Subscribe icon moves
Expected
Icon doesn't move
Reporter | ||
Comment 1•11 years ago
|
||
Forgot to put build information
http://hg.mozilla.org/projects/ux/rev/3816e384573d
UX nightly 26.0a1 Linux
--enable-update-channel=nightly-ux --enable-update-packaging --with-google-api-keyfile=/builds/gapi.data --enable-crashreporter --enable-release --enable-elf-hack --enable-stdcxx-compat --enable-warnings-as-errors --enable-signmar --enable-profiling --disable-elf-hack --enable-js-diagnostics --with-ccache=/usr/bin/ccache
Assignee | ||
Comment 2•11 years ago
|
||
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #796818 -
Flags: review?(dchan+bugzilla)
Attachment #796818 -
Flags: review?
Assignee | ||
Updated•11 years ago
|
Attachment #796818 -
Flags: review? → review?(mnoorenberghe+bmo)
Reporter | ||
Updated•11 years ago
|
Attachment #796818 -
Flags: review?(dchan+bugzilla) → review+
Comment 3•11 years ago
|
||
If we get this out, the sec review can be marked as done :)
Thank you both for taking such a quick stab at it!
Comment 4•11 years ago
|
||
Comment on attachment 796818 [details] [diff] [review]
Patch
Review of attachment 796818 [details] [diff] [review]:
-----------------------------------------------------------------
I wonder if there should be a comment with each of these new blocks since this isn't a common pattern in the codebase and others may not realize it's for security reasons. Perhaps now that the check is getting lengthier we could create a helper that is more self-documenting. That function could have a comment explaining the issue being addressed. Example:
if (this.isUnwantedDragDrop(aEvent)) {
return;
}
I don't think we need to avoid comments since as dchan says, this isn't shipped code yet.
IIUC, this will prevent us from switching customization mode to it's own document (as proposed in bug 900122) without modification. In that case I believe the window would be different for the palette. I think that's fine as I think bug 900122 is unlikely to happen since there weren't noticeable perf wins and it affected addon-compat. We can always tweak this is we need.
Please make the commit message less generic by mentioning customization.
::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +710,5 @@
> _onDragOver: function(aEvent) {
> __dumpDragData(aEvent);
>
> let document = aEvent.target.ownerDocument;
> + if (aEvent.dataTransfer.mozSourceNode.ownerDocument.defaultView != this.window) {
Move all of the new blocks up as they're not using |document| and I'd rather bail as soon as possible when this happens. I would probably prefer this to go before __dumpDragData just to make sure we don't do too much with the unwanted drag.
Since mozSourceNode can be null (see the IDL), we should probably also check that it's truthy before accessing the ownerDocument otherwise we will output TypeError's to the browser console when dragging between external applications over Firefox in customization mode. Even if it's rare, we should try not to add console spew.
if (!aEvent.dataTransfer.mozSourceNode ||
aEvent.dataTransfer.mozSourceNode.ownerDocument.defaultView != this.window) {
Attachment #796818 -
Flags: review?(mnoorenberghe+bmo) → review+
Assignee | ||
Comment 5•11 years ago
|
||
Assignee | ||
Comment 6•11 years ago
|
||
mozSourceNode is null when running the tests, but it's not null when manually using a mouse to drag items. I attached a debugger and nsContentUtils::GetDragSession is returning null within nsDOMDataTransfer::GetMozSourceNode.
Neil, do you see any issues with the drag/drop code in head.js at http://hg.mozilla.org/projects/ux/annotate/a6fe43be166c/browser/components/customizableui/test/head.js#l95 ? I don't think the synthesizeDragStart is necessary there, but commenting it out doesn't fix the issue. I wonder if this is a deeper issue within ChromeUtils or EventUtils.
Flags: needinfo?(neil)
Comment 7•11 years ago
|
||
I don't think you meant to needinfo? me, I don't know anything about the code in question. I tried to needinfo? someone else but Bugzilla wouldn't let me create the needinfo? request...
Flags: needinfo?(neil)
Updated•11 years ago
|
Flags: needinfo?(enndeakin)
Comment 8•11 years ago
|
||
synthesizeDragStart/synthesizeDrop are used only for testing whether the right data is being put into the dataTransfer. Neither cause a real drop to occur, so they don't set the source node. There isn't a means of testing real drag and drops.
Flags: needinfo?(enndeakin)
Assignee | ||
Comment 9•11 years ago
|
||
Thanks for the info Enn. Matt, I've added a hidden pref to skip this check during tests. Look good to you?
0:59.55 INFO TEST-START | Shutdown
0:59.55 Browser Chrome Test Summary
0:59.55 Passed: 354
0:59.55 Failed: 0
0:59.55 Todo: 1
Attachment #796818 -
Attachment is obsolete: true
Attachment #799252 -
Flags: review?(mnoorenberghe+bmo)
Assignee | ||
Comment 10•11 years ago
|
||
Needed to wrap the Services.prefs.getBoolPref call with a try/catch in the case that the pref doesn't exist.
1:02.40 INFO TEST-START | Shutdown
1:02.40 Browser Chrome Test Summary
1:02.40 Passed: 354
1:02.40 Failed: 0
1:02.40 Todo: 1
Attachment #799252 -
Attachment is obsolete: true
Attachment #799252 -
Flags: review?(mnoorenberghe+bmo)
Attachment #799684 -
Flags: review?(mnoorenberghe+bmo)
Comment 11•11 years ago
|
||
Is this sec-low or sec-moderate? David, can you please assign a sec-rating?
Reporter | ||
Comment 12•11 years ago
|
||
(In reply to Frederik Braun [:freddyb] from comment #11)
> Is this sec-low or sec-moderate? David, can you please assign a sec-rating?
Definitely a sec-low due to how unlikely it is to be used and the minimal impact
Keywords: sec-low
Comment 13•11 years ago
|
||
Comment on attachment 799684 [details] [diff] [review]
Patch v1.2
Review of attachment 799684 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +940,5 @@
> + // set the source node. There isn't a means of testing real drag and drops,
> + // so this pref skips the check but it should only be set by test code.
> + let skipCheck = false;
> + try {
> + skipCheck = Services.prefs.getBoolPref("browser.uiCustomization.skipSourceNodeCheck");
I dislike relying on exceptions for this when we can check getPrefType(…) == Ci.nsIPrefBranch.PREF_BOOL. That way other exceptions are not swallowed and my understanding is that try…catch can affect JS perf.
It seems like there could be a perf impact of repeatedly calling these pref functions during drag/drop operations. We may want to use a pref observer and stash the value on a property. Another option would be to skip this pref stuff if you're not in a DEBUG build but that would mean we wouldn't be able to test this in non-debug builds. I'll leave it up to you to decide whether you want to do something about potential perf. It's not a commonly used code-path but it's somewhat hot.
Attachment #799684 -
Flags: review?(mnoorenberghe+bmo) → review+
Assignee | ||
Comment 14•11 years ago
|
||
https://hg.mozilla.org/projects/ux/rev/94231fdf1158
I removed the try/catch in favor of using getPrefType. Thanks, I didn't know about that method before.
I also now cache the value of the pref in a member. I didn't use a pref observer because this pref is not likely to change at runtime and as such we shouldn't take on the overhead of another observer for something that isn't important on the fly like this.
All tests passed locally when running with this patch, as well as manual testing.
Whiteboard: [Australis:P4][Australis:M9][fixed-in-ux]
Comment 15•11 years ago
|
||
(In reply to Jared Wein [:jaws] from comment #14)
> I didn't use a pref observer because this pref is not likely to change at runtime and as such we
> shouldn't take on the overhead of another observer for something that isn't
> important on the fly like this.
Doesn't this mean that clearing the user pref in your tests don't do anything for the subsequent tests since the value is memoized?
Also, I just realized that this bug didn't add a test to ensure that unwanted drops are ignored.
Assignee | ||
Comment 16•11 years ago
|
||
Follow-up patch as we discussed on IRC
Attachment #802785 -
Flags: review?(mnoorenberghe+bmo)
Assignee | ||
Comment 17•11 years ago
|
||
Comment on attachment 802785 [details] [diff] [review]
Followup patch
Redirecting review to Gijs since Matt is on vacation now.
Attachment #802785 -
Flags: review?(mnoorenberghe+bmo) → review?(gijskruitbosch+bugs)
Comment 18•11 years ago
|
||
Comment on attachment 802785 [details] [diff] [review]
Followup patch
I think if we're going this route, it makes more sense to just set the member each time we enter customize mode, rather than adding/removing pref observers all the time. If we had a reliable destructor we could use that to destroy the pref observer when the window goes away, and only init it once, but apparently we do not (for customize mode).
(of course, I'm going on the assumption that it's cheaper to read the pref than to add+remove the observer, which may or may not be true depending on how sane our pref service is)
Attachment #802785 -
Flags: review?(gijskruitbosch+bugs) → review-
Assignee | ||
Comment 19•11 years ago
|
||
We had to initialize the cached pref value anyways when entering customization mode with the previous patch, so this makes it a little simpler by ignoring any pref changes while in customization mode.
Attachment #802785 -
Attachment is obsolete: true
Attachment #805416 -
Flags: review?(gijskruitbosch+bugs)
Comment 20•11 years ago
|
||
Comment on attachment 805416 [details] [diff] [review]
Followup patch v1.1
Review of attachment 805416 [details] [diff] [review]:
-----------------------------------------------------------------
Looks great, thanks!
(and sorry for the late review)
Attachment #805416 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 21•11 years ago
|
||
Comment 22•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P4][Australis:M9][fixed-in-ux] → [Australis:P4][Australis:M9]
Comment 23•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/94231fdf1158
(sorry for the double comment, doing this manually as mcMerge doesn't do security sensitive bugs)
Updated•11 years ago
|
Target Milestone: --- → Firefox 28
Comment 24•11 years ago
|
||
How far back does this issue go?
Comment 25•11 years ago
|
||
(In reply to Al Billings [:abillings] from comment #24)
> How far back does this issue go?
That's an interesting point. David, can you reproduce this with e.g. the customize window in current release or beta versions?
Flags: needinfo?(dchan)
Reporter | ||
Comment 26•11 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #25)
> (In reply to Al Billings [:abillings] from comment #24)
> > How far back does this issue go?
>
> That's an interesting point. David, can you reproduce this with e.g. the
> customize window in current release or beta versions?
AFAIK, this issue was specific to the UX branch. I wasn't able to reproduce with current release / beta / aurora. The custom UI files don't seem to exist in the non-UX branches. I tested with the latest UX nightly and the bug appears to be fixed, no events are fired when drag-dropping cross-origin
Flags: needinfo?(dchan)
Comment 27•11 years ago
|
||
(In reply to David Chan [:dchan] from comment #26)
> (In reply to :Gijs Kruitbosch from comment #25)
> > (In reply to Al Billings [:abillings] from comment #24)
> > > How far back does this issue go?
> >
> > That's an interesting point. David, can you reproduce this with e.g. the
> > customize window in current release or beta versions?
>
> AFAIK, this issue was specific to the UX branch. I wasn't able to reproduce
> with current release / beta / aurora. The custom UI files don't seem to
> exist in the non-UX branches.
You can still drag/drop UI elements by using View > Toolbars > Customize...
Reporter | ||
Comment 28•11 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #27)
> (In reply to David Chan [:dchan] from comment #26)
> > (In reply to :Gijs Kruitbosch from comment #25)
> > > (In reply to Al Billings [:abillings] from comment #24)
> > > > How far back does this issue go?
> > >
> > > That's an interesting point. David, can you reproduce this with e.g. the
> > > customize window in current release or beta versions?
> >
> > AFAIK, this issue was specific to the UX branch. I wasn't able to reproduce
> > with current release / beta / aurora. The custom UI files don't seem to
> > exist in the non-UX branches.
>
> You can still drag/drop UI elements by using View > Toolbars > Customize...
Ah, I didn't know that. I'll re-test
Reporter | ||
Comment 29•11 years ago
|
||
It appears that release, beta and aurora are affected using the same poc.
Open 2 windows
Navigate one of them to the poc
Open View -> Toolbars -> Customize on the other
Drag Firefox icon to browser chrome, subscribe icon moves
Drag back to "Customize toolbar", subscribe icon moves back
I tested on Windows Fx 26 and Linux Fx 27, 28.
Updated•11 years ago
|
status-firefox26:
--- → wontfix
status-firefox27:
--- → affected
status-firefox28:
--- → affected
status-firefox29:
--- → affected
tracking-firefox29:
--- → +
Comment 30•11 years ago
|
||
Here's a patch for holly/m-a/m-b. Dão, does this look OK to you? It's pretty much a straight patch from the CustomizableUI one.
Comment 31•11 years ago
|
||
Comment on attachment 8360274 [details] [diff] [review]
910375-security
(In reply to :Gijs Kruitbosch from comment #30)
> Created attachment 8360274 [details] [diff] [review]
> 910375-security
>
> Here's a patch for holly/m-a/m-b. Dão, does this look OK to you? It's pretty
> much a straight patch from the CustomizableUI one.
(also, bugzilla, you suck)
Attachment #8360274 -
Flags: review?(dao)
Comment 32•11 years ago
|
||
Comment on attachment 8360274 [details] [diff] [review]
910375-security
>+ // The simulated events generated by synthesizeDragStart/synthesizeDrop in
>+ // mochitests are used only for testing whether the right data is being put
>+ // into the dataTransfer. Neither cause a real drop to occur, so they don't
>+ // set the source node. There isn't a means of testing real drag and drops,
>+ // so this pref skips the check but it should only be set by test code.
>+ let skipCheck = false;
>+ try {
>+ skipCheck = Services.prefs.getBoolPref("toolkit.uiCustomization.skipSourceNodeCheck");
>+ } catch (ex) {}
>+ if (skipCheck) {
>+ return false;
>+ }
Is this pref actually needed by any tests?
>+ /* Discard drag events that originated from a separate window to
>+ prevent content->chrome privilege escalations. */
Does aEvent.isTrusted help here?
Comment 33•11 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #32)
> Comment on attachment 8360274 [details] [diff] [review]
> 910375-security
>
> >+ // The simulated events generated by synthesizeDragStart/synthesizeDrop in
> >+ // mochitests are used only for testing whether the right data is being put
> >+ // into the dataTransfer. Neither cause a real drop to occur, so they don't
> >+ // set the source node. There isn't a means of testing real drag and drops,
> >+ // so this pref skips the check but it should only be set by test code.
> >+ let skipCheck = false;
> >+ try {
> >+ skipCheck = Services.prefs.getBoolPref("toolkit.uiCustomization.skipSourceNodeCheck");
> >+ } catch (ex) {}
> >+ if (skipCheck) {
> >+ return false;
> >+ }
>
> Is this pref actually needed by any tests?
I don't /think/ so. If you prefer I can remove it.
>
> >+ /* Discard drag events that originated from a separate window to
> >+ prevent content->chrome privilege escalations. */
>
> Does aEvent.isTrusted help here?
No, it is always true. I suspect the dragover has nothing to do with the dragstart as far as isTrusted is concerned.
Updated•11 years ago
|
Group: firefox-core-security → core-security
Assignee | ||
Comment 34•11 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #32)
> Comment on attachment 8360274 [details] [diff] [review]
> 910375-security
>
> >+ // The simulated events generated by synthesizeDragStart/synthesizeDrop in
> >+ // mochitests are used only for testing whether the right data is being put
> >+ // into the dataTransfer. Neither cause a real drop to occur, so they don't
> >+ // set the source node. There isn't a means of testing real drag and drops,
> >+ // so this pref skips the check but it should only be set by test code.
> >+ let skipCheck = false;
> >+ try {
> >+ skipCheck = Services.prefs.getBoolPref("toolkit.uiCustomization.skipSourceNodeCheck");
> >+ } catch (ex) {}
> >+ if (skipCheck) {
> >+ return false;
> >+ }
>
> Is this pref actually needed by any tests?
Yes, the customizableui tests use the synthesize drag/drops but the source node is not properly set by the synthesize code. Removing this pref on non-Australis repos shouldn't cause any issues but it will need to remain on Australis-populated repos for the customizableui tests to pass.
Updated•11 years ago
|
status-b2g18:
--- → unaffected
status-b2g-v1.2:
--- → unaffected
status-firefox-esr24:
--- → unaffected
Comment 35•11 years ago
|
||
Comment on attachment 8360274 [details] [diff] [review]
910375-security
>+ // The simulated events generated by synthesizeDragStart/synthesizeDrop in
>+ // mochitests are used only for testing whether the right data is being put
>+ // into the dataTransfer. Neither cause a real drop to occur, so they don't
>+ // set the source node. There isn't a means of testing real drag and drops,
>+ // so this pref skips the check but it should only be set by test code.
>+ let skipCheck = false;
>+ try {
>+ skipCheck = Services.prefs.getBoolPref("toolkit.uiCustomization.skipSourceNodeCheck");
>+ } catch (ex) {}
>+ if (skipCheck) {
>+ return false;
>+ }
Please get rid of this.
>+ /* Discard drag events that originated from a separate window to
>+ prevent content->chrome privilege escalations. */
>+ let mozSourceNode = aEvent.dataTransfer.mozSourceNode;
>+ // mozSourceNode is null in the dragStart event handler or if
>+ // the drag event originated in an external application.
>+ return !mozSourceNode ||
>+ mozSourceNode.ownerDocument.defaultView != this.window;
If I correctly understand the code calling isUnwantedDragEvent, then it may run for D&D actions originating from a toolbar, and if I understand mozSourceNode correctly, it would belong to the browser window in that case and fail the above test. Let me know if I misunderstood something.
Attachment #8360274 -
Flags: review?(dao) → review-
Comment 36•11 years ago
|
||
Is this also affecting FF30 and up? We're going to build our final FF29 on Monday so given this is sec-low and the patch isn't looking nearly ready we're likely a miss here for 29 but should track for 30 and up.
Updated•11 years ago
|
Comment 37•11 years ago
|
||
No, I believe this is fixed in 29 and up since the fix stayed with Australis which is getting released in 29. I just confirmed _isUnwantedDragDrop is on mozilla-beta.
status-firefox30:
--- → fixed
status-firefox31:
--- → fixed
tracking-firefox30:
? → ---
tracking-firefox31:
? → ---
Flags: needinfo?(jaws)
Target Milestone: Firefox 28 → Firefox 29
Updated•11 years ago
|
Whiteboard: [Australis:P4][Australis:M9] → [Australis:P4][Australis:M9][adv-main29+]
Comment 38•11 years ago
|
||
(In reply to Matthew N. [:MattN] from comment #37)
> No, I believe this is fixed in 29 and up since the fix stayed with Australis
> which is getting released in 29. I just confirmed _isUnwantedDragDrop is on
> mozilla-beta.
FWIW, this isn't fixed for e.g. the toolbar customization window in the library, or other non-browser.xul toolbar customization. We should still fix the toolkit issue.
Comment 39•11 years ago
|
||
Does that mean we shouldn't write an advisory about the issue?
Comment 40•11 years ago
|
||
(In reply to Al Billings [:abillings] from comment #39)
> Does that mean we shouldn't write an advisory about the issue?
I think so.
Comment 41•11 years ago
|
||
Matt, how do you feel about an advisory considering comment 38?
Flags: needinfo?(MattN+bmo)
Comment 42•11 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #38)
> (In reply to Matthew N. [:MattN] from comment #37)
> > No, I believe this is fixed in 29 and up since the fix stayed with Australis
> > which is getting released in 29. I just confirmed _isUnwantedDragDrop is on
> > mozilla-beta.
>
> FWIW, this isn't fixed for e.g. the toolbar customization window in the
> library, or other non-browser.xul toolbar customization. We should still fix
> the toolkit issue.
I think this stuff should move to another bug so we can track it appropriately with the status flags.
Since it's using the same technique on other applications/windows, I don't think we should disclose this in an advisory until that new bug is fixed.
Flags: needinfo?(MattN+bmo)
Comment 43•11 years ago
|
||
Please link back to this bug in the new bugs so when they are fixed, we don't forget to write an advisory for the whole set.
Whiteboard: [Australis:P4][Australis:M9][adv-main29+] → [Australis:P4][Australis:M9][adv-main29-]
Comment 44•11 years ago
|
||
Confirmed issue in Firefox 27.0
Verified fixed in Firefox 28, 2014-04-21
Verified fixed in Firefox 29, 2014-04-21
Verified fixed in Firefox 30, 2014-04-23
Verified fixed in Firefox 31, 2014-04-23
Updated•11 years ago
|
Status: RESOLVED → VERIFIED
Updated•11 years ago
|
See Also: → CVE-2014-1561
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•