Closed Bug 910375 Opened 6 years ago Closed 6 years ago

New PanelUI / toolbar customization event spoofing

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

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)

Attached file drag.html
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
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
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #796818 - Flags: review?(dchan+bugzilla)
Attachment #796818 - Flags: review?
Attachment #796818 - Flags: review? → review?(mnoorenberghe+bmo)
Attachment #796818 - Flags: review?(dchan+bugzilla) → review+
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 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+
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)
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)
Flags: needinfo?(enndeakin)
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)
Attached patch Patch v1.1 (obsolete) — Splinter Review
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)
Attached patch Patch v1.2Splinter Review
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)
Is this sec-low or sec-moderate? David, can you please assign a sec-rating?
(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 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+
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]
(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.
Attached patch Followup patch (obsolete) — Splinter Review
Follow-up patch as we discussed on IRC
Attachment #802785 - Flags: review?(mnoorenberghe+bmo)
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 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-
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 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+
https://hg.mozilla.org/mozilla-central/rev/e4a85d4d3d7c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P4][Australis:M9][fixed-in-ux] → [Australis:P4][Australis:M9]
https://hg.mozilla.org/mozilla-central/rev/94231fdf1158

(sorry for the double comment, doing this manually as mcMerge doesn't do security sensitive bugs)
Target Milestone: --- → Firefox 28
How far back does this issue go?
(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)
(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)
(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...
(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
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.
Attached patch 910375-securitySplinter Review
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 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 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?
(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.
Group: firefox-core-security → core-security
(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.
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-
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.
Flags: needinfo?(jaws)
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.
Flags: needinfo?(jaws)
Target Milestone: Firefox 28 → Firefox 29
Whiteboard: [Australis:P4][Australis:M9] → [Australis:P4][Australis:M9][adv-main29+]
(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.
Does that mean we shouldn't write an advisory about the issue?
(In reply to Al Billings [:abillings] from comment #39)
> Does that mean we shouldn't write an advisory about the issue?

I think so.
Matt, how do you feel about an advisory considering comment 38?
Flags: needinfo?(MattN+bmo)
(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)
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-]
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
See Also: → CVE-2014-1561
Group: core-security
You need to log in before you can comment on or make changes to this bug.