910375 - New PanelUI / toolbar customization event spoofing

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[David Chan [:dchan]]

Attachment: 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

Comment 1

[David Chan [:dchan]]

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

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Comment 3

[Frederik Braun [:freddy]]

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

[Matthew N. [:MattN]]

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) {

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

https://tbpl.mozilla.org/?tree=Try&rev=bb192859092f

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 7

[[e-mail]]

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...

Comment 8

[Neil Deakin]

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.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1

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

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.2

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

Comment 11

[Frederik Braun [:freddy]]

Is this sec-low or sec-moderate? David, can you please assign a sec-rating?

Comment 12

[David Chan [:dchan]]

(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

Comment 13

[Matthew N. [:MattN]]

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.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 15

[Matthew N. [:MattN]]

(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.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Followup patch

Follow-up patch as we discussed on IRC

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 802785 [details] [diff] [review]
Followup patch

Redirecting review to Gijs since Matt is on vacation now.

Comment 18

[:Gijs (he/him)]

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)

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Followup patch v1.1

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.

Comment 20

[:Gijs (he/him)]

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)

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/projects/ux/rev/e4a85d4d3d7c

Comment 22

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/e4a85d4d3d7c

Comment 23

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/94231fdf1158

(sorry for the double comment, doing this manually as mcMerge doesn't do security sensitive bugs)

Comment 24

[Al Billings [:abillings - ex-MoCo]]

How far back does this issue go?

Comment 25

[:Gijs (he/him)]

(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?

Comment 26

[David Chan [: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

Comment 27

[:Gijs (he/him)]

(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...

Comment 28

[David Chan [:dchan]]

(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

Comment 29

[David Chan [:dchan]]

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.

Comment 30

[:Gijs (he/him)]

Attachment: 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.

Comment 31

[:Gijs (he/him)]

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)

Comment 32

[Dão Gottwald [: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?

Comment 33

[:Gijs (he/him)]

(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.

Comment 34

[Jared Wein [:jaws] (please needinfo? me)]

(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 35

[Dão Gottwald [: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;
>+  }

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.

Comment 36

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 37

[Matthew N. [:MattN]]

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.

Comment 38

[:Gijs (he/him)]

(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

[Al Billings [:abillings - ex-MoCo]]

Does that mean we shouldn't write an advisory about the issue?

Comment 40

[:Gijs (he/him)]

(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

[:Gijs (he/him)]

Matt, how do you feel about an advisory considering comment 38?

Comment 42

[Matthew N. [:MattN]]

(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.

Comment 43

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 44

[Nagasahas DS]

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