285438 - Drag and drop gestures can be hijacked to load priviliged xul

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

Description

[Michael Krax]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.7.6) Gecko/20050223 Firefox/1.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.7.6) Gecko/20050223 Firefox/1.0.1

Even though Firefox 1.0.1 patched one of the key bugs behind the firescrolling
exploit #281807 (the ability of plugins to load chrome files in a hidden frame)
the ability to hijack a drag and drop operation and open a privileged xul file
is still available.



Reproducible: Always

Steps to Reproduce:
1. Open http://www.mikx.de/firescrolling2/
2. Drag the scrollbar





The demo opens "chrome://global/content/alerts/alert.xul" when dragging the
scrollbar the first time. This XUL file automaticly runs an inline script to
turns the window into a tray notification alert. This demo is just an example of
an annoying usage, but if the browser or an extension contains an inline script
that uses an eval/setTimeout with a parameter an attacker can influence it turns
into an arbitrary code execution bug. Also update or uninstall scripts could be
a valuable target. I doubt most extension developers think about problems that
could occure if a XUL file in their extensions is opened directly. 

Created a new bug report to bring some attention back to the entire problem of
privileged xul called from a normal website. #281807 isn't fixed - just the
exploit is blocked by fixing #280664. What about a more general approach to
block it? Is there any legitimate use to open a chrome:// url directly? Beside
the about:config shortcut?

Comment 1

[Daniel Veditz [:dveditz]]

Confirming, presuming this will block.

Web content shouldn't be able to load chrome: documents, imho including
about:config (which should instead launch its own non-browser window).

Comment 2

[Jesse Ruderman]

Dup of bug 250725?

Comment 3

[Boris Zbarsky [:bzbarsky]]

One possible approach here is to make use of the sourceDocument of the
dragSession.  If we're dropping, and have a sourceDocument, do a CheckLoadURI to
see whether the sourceDocument would have been allowed to open this URI.  If
not, don't allow the load.

This means that drags from us to outside will be unaffected, as will links from
outside to us.  Drags from one part of the app to another, however, would get
the same security check that would happen if you just clicked the link in the
original document... (except we'd do the check even if text is what's being
dragged).

Note that this makes dragging be not quite like copy/paste ui-wise, so ccing UI
people for their opinion on the suggestion.

Comment 4

[[e-mail]]

Is it feasible to completely "unlinkify" insecure links, in the same way that we
refuse to link to malformed URIs (e.g. data:text/html,<a href="#">unlinked), so
that they cannot be dragged?

Comment 5

[Johnny Stenback (:jst)]

Attachment: Not a complete fix, but does block this exploit

This is by no means a complete fix for this bug, nor do I yet fully understand
our drag and drop code, even closely. But this patch does fix the bug, but I'm
not entirely sure how. The idea behind this is to add a security check (per
bz's suggestion above) to check if the drag-source doc can load the dropped
link. But there's problems with our drag session code, it apparently forgets
the source document when we switch windows, or something. And the drag session
is a singleton, at least on windows, and it's never properly re-initialized for
that same drag again...

I've gotta take off for the weekend, so this is as far as I got. bz, can you,
or someone else look at this and see if it makes any sense, and maybe even
complete this fix?

Comment 6

[Daniel Veditz [:dveditz]]

(In reply to comment #2)
> Dup of bug 250725?

Yes, essentially.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Is it feasible to completely "unlinkify" insecure links

Yes, but this would be a pretty severe pageload hit (would need to
security-check a link before resolving style) and would keep us from dragging
them outside the browser, which is not desirable either.

jst, the patch looks like about what I thought about, yes.  I didn't realize our
drag code was quite that broken, though... :(

I'll see what I can do, but I won't be able to look into this in a reasonable
way till sometime pretty late Sunday night at best.

Comment 8

[Jesse Ruderman]

*** Bug 250725 has been marked as a duplicate of this bug. ***

Comment 9

[chris hofmann]

any updates on where we are for this?  holding 1.0.2's and 1.7.6 for the
finished patch.

Comment 10

[Johnny Stenback (:jst)]

Attachment: Fix (Win32 only so far).

This fixes our broken drag n' drop code on Win32, and it makes us do the
appropriate security checks when dropping a URL from one of our own windows.
Probably only fixes this bug in Firefox. Haven't had a chance to look at the
suite yet.

Comment 11

[Johnny Stenback (:jst)]

Attachment: Fix (Win32 + Gtk 1 & 2).

This fixes our busted drag n' drop code (on Win32 and Gtk 1 & 2) and patches
this security bug (which I haven't been able to patch w/o fixing our busted
drag n' drop code). The problem with the dnd code is that there's no reliable
way to find out the source of the drag in todays code when the user drags the
mouse out of the mozilla window and then back in. Not sure if this is really a
problem on the Mac, but I'm guessing it is. We'd need someone to look at the
Mac side of this, as I don't right now have access to a Mac system.

bz, what do you think about this for fixing the drag n' drop code, and patching
this hole (at least for Firefox)?

Comment 12

[[e-mail]]

Attachment: FE fix for suite

Comment 13

[[e-mail]]

Attachment: Fix tabbrowser.xml?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 177454 [details] [diff] [review]
Fix (Win32 + Gtk 1 & 2).

So... I can try to review this, but I really don't know all the
interrelationships in this widget code well.  Does anyone?  If so, they should
also review....

>Index: widget/src/gtk/nsWindow.cpp
>@@ -3445,18 +3442,38 @@ nsWindow::OnDragLeave(void)
>+        // initiated in a differnt app. End the drag session, since

"different"

Other than that, looks ok.  Scary, but ok.

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 177471 [details] [diff] [review]
Fix tabbrowser.xml?

Yeah, I think we want this, and we'll want the same change in the toolkit
version of this file.

Comment 16

[Johnny Stenback (:jst)]

Attachment: Aviary change that was checked in (browser content area fix only)

This has been checked in on the aviary branch, but this doesn't *completely*
fix this bug. But it gets the scary changes in there so that we can test those
as much as possible...

Comment 17

[Johnny Stenback (:jst)]

Attachment: Mac portion of this change (aviary branch)

This is the mac portion of this change. The Mac was behaving quite well wrt
drag n' drop already, so this was the only change I need, AFAICT.

Comment 18

[Johnny Stenback (:jst)]

Mac part landed on trunk and aviary branch too.

Now I need to start looking at adding the security checks in all our drop
handlers where we need it. And all this needs to go in on the 1.7 branch as well.

Comment 19

[Rich Walsh]

I maintain the d&d code for OS/2 and want to ensure that I implement the
platform-specific parts of this fix properly.  So, is the following correct?

When the app originates a drag, the combined actions of nsBaseDragService &
native code should ensure that:
- when the drag starts, mDoingDrag & mSourceDocument will always be set;
- during a drag, they will not be changed, regardless of where the drag goes;
- when the drag actually ends (by virtue of a drop or a cancel), these members
will always be cleared.

If this is correct then no changes should be required in the OS/2 code.

Comment 20

[Johnny Stenback (:jst)]

(In reply to comment #19)
> I maintain the d&d code for OS/2 and want to ensure that I implement the
> platform-specific parts of this fix properly.  So, is the following correct?
> 
> When the app originates a drag, the combined actions of nsBaseDragService &
> native code should ensure that:
> - when the drag starts, mDoingDrag & mSourceDocument will always be set;

And mSourceNode too.

> - during a drag, they will not be changed, regardless of where the drag goes;
> - when the drag actually ends (by virtue of a drop or a cancel), these members
> will always be cleared.
> 
> If this is correct then no changes should be required in the OS/2 code.
> 

And also, when dragging from another app into Mozilla, we need the properly
start a drag session (with mSourceNode and mSourceDocument set to null) and end
it when the user drops in mozilla, or when the user drags back out of mozilla,
or when the drag is interrupted on top of mozilla.

Comment 21

[Johnny Stenback (:jst)]

(In reply to comment #19)
Oh, and most importantly, when starting a drag in mozilla, mSourceNode and
mSourceDocument must not be reset until the drag is truly done. I.e. no ending
the drag session just because we leave the mozilla window or whatever window.

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 177567 [details] [diff] [review]
Aviary change that was checked in (browser content area fix only)

Geez, I screwed up this diff good. The first file is right, but the rest are
reversed diffs.

Comment 23

[Johnny Stenback (:jst)]

Attachment: Complete (Win, gtk, mac) widget changes for aviary/1.7 branches.

This is the widget changes that have been checked in on the branches.

Comment 24

[sairuh (rarely reading bugmail)]

Johnny, is this fixed for aviary1.0.2? (if so, I'll add the keyword to help with
our queries.)

also, I noticed you 07:23 checkin this am --would that require a respin for the
mac, windows or linux ffox (or tbird) bits? or was that specific to other platforms?

Comment 25

[Daniel Veditz [:dveditz]]

Do we need to spin off sub-bugs for the remaining platforms? Looks like
 - cocoa (Camino uses this, right?)
 - beos
 - os2
 - photon
 - qt
 - xlib

We can file individual subbugs and leave them up to contributors, but we should
link them to this one so the platform owners have a chance to find it.

Comment 26

[Rich Walsh]

(In reply to comment #21)
> Oh, and most importantly, when starting a drag in mozilla, mSourceNode and

> mSourceDocument must not be reset until the drag is truly done. I.e. no
> ending the drag session just because we leave the mozilla window or whatever
> window.

This is particularly important to me because my code relies on mSourceNode to
distinguish between Mozilla and native drags, so its value must be preserved.

Possible problem:  any code can call EndDragSession() and clear mSourceNode and
mSourceDocument while a Mozilla drag is still in progress. If the caller is
simply "misinformed", doing so will screw up my d&d handler. However, if the
caller is malicious, does this give it the opportunity to sidestep this whole
security fix?

Comment 27

[Boris Zbarsky [:bzbarsky]]

If a malicious caller can get a drag service, you already have a malicious
caller with full XPConnect access...

That said, I don't see why the start/endDragSession stuff is on a (scriptable)
interface. It seems like a widget implementation detail to me, one that the
widget impls should just handle directly...

Comment 28

[Johnny Stenback (:jst)]

Attachment: Do security check when dropping links.

This patches the places where I believe we want to do a security check on the
source of a dropped URL. Those places are: URL bar, Go button, downloads
button, content area, tabbrowser. This patch patches both Firefox and the
suite.

Comment 29

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 177673 [details] [diff] [review]
Do security check when dropping links.

>Index: toolkit/content/widgets/tabbrowser.xml
>+            DragDropSecurityCheck(aEvent, aDragSession, url);
>+

Can I get you to make this a method *on* tabbrowser or something else similarly
appropriate? Right now this makes an outward dependency of toolkit on
browser/base and xpfe/browser forcing anyone using <tabbrowser> to implement
this method. 

Otherwise the rest of the code looks fine.

Comment 30

[Christian :Biesinger (don't email me, ping me on IRC)]

+function DragDropSecurityCheck(aEvent, aDragSession, aUrl)

maybe this should be in some chrome://global location? (xpfe/global /
toolkit/content)

Comment 31

[Johnny Stenback (:jst)]

Attachment: Make the security check a method of the tabbrowser for firefox, and fix nsContentAreaDragDrop.cpp to do the same security check too.

Comment 32

[Johnny Stenback (:jst)]

(In reply to comment #30)
> +function DragDropSecurityCheck(aEvent, aDragSession, aUrl)
> 
> maybe this should be in some chrome://global location? (xpfe/global /
> toolkit/content)

Maybe. Find a spot and I'll move it, if not, it can be moved later when
someone's got the time to figure out where this should live.

Comment 33

[Mike Connor [:mconnor]]

contentAreaUtils.js will be moving to toolkit at some point rsn, and seems an
appropriate spot.  Once that's done we can move the method out of tabbrowser,
but that shouldn't hold this off.  Barring objections to that, please file a bug
against me to add that to the moved file and I'll take care of that.

Comment 34

[Boris Zbarsky [:bzbarsky]]

I buy security-checking drags to all places that load the dragged text
automtically.  But why url bar?

Comment 35

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 177682 [details] [diff] [review]
Make the security check a method of the tabbrowser for firefox, and fix nsContentAreaDragDrop.cpp to do the same security check too.

r=[e-mail] for:

1. all of it for the appropriate branches
2. the browser/toolkit sections for the trunk. 

get a xpfe peer to sign off on the xpfe changes for the trunk.

Comment 36

[Daniel Veditz [:dveditz]]

Comment on attachment 177682 [details] [diff] [review]
Make the security check a method of the tabbrowser for firefox, and fix nsContentAreaDragDrop.cpp to do the same security check too.

sr=dveditz
a=dveditz for branches

Comment 37

[Johnny Stenback (:jst)]

(In reply to comment #34)
> I buy security-checking drags to all places that load the dragged text
> automtically.  But why url bar?

Because that's how the URL bar works in Firefox, drag a URL to the URL bar and
we'll load the URL, not just paste it into the URL bar.

Comment 38

[Daniel Veditz [:dveditz]]

(In reply to comment #34)
> I buy security-checking drags to all places that load the dragged text
> automtically.  But why url bar?

Because of things like http://greyhatsecurity.org/vulntests/firefox.htm
(admittedly a forced example). Speaking of which, could this patch be extended
to block javascript: drops on the urlbar?

Comment 39

[sairuh (rarely reading bugmail)]

the test cases don't hijack windows on linux fc3 (tested with 20050317-1.0.2
firefox and 20050317-1.7.6 mozilla builds).

I played around with some dragging and dropping on both linux and mac os x
10.3.8 (latter builds: 20050317-1.0.2 firefox and 20050317-1.7.6 mozilla as
well), and haven't seen issues there yet. a broader audience would be better, so
hopefully we'll know more with feedback from public candidate builds.

Comment 40

[Tracy Walker [:tracy]]

This appears to be still broken on Windows 1.0.2 build 2005-03-17-06-aviary1.0.1

I ran the proof of concept demo. Clicked the scrollbar, which changed the
pointer to a drand and drop square icon.  Drag and dropped it to the desktop.
That created a shortcut to chrome://global/content/alerts/alert.xul.  I then
drag and dropped that back to the browser window.  That slid open what appeared
to be an alert message window at the system tray.  The contents of which were
the upper left portion of Firefox.  I drag and dropped the same shortcut to the
browser again.  This time the browser closed leaving just the alert window
sliding open and closed.

I could not reproduce the same behavior on this morning Mac Fx 1.0.2 build. 
Clicking the scrollbar allowed scrolling only.

Comment 41

[[e-mail]]

Why not put the code in nsTransferable.js - you could even enhance
retriveURLFromData as I expect all consumers could do with the extra checks.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Tracy, you should be able to drag the URL out of mozilla, and you should be able
to drag things from outside Mozilla into Mozilla.

What you should not be able to do is drag the URL from one Mozilla window to
another and get it to load.

Comment 43

[Johnny Stenback (:jst)]

(In reply to comment #41)
> Why not put the code in nsTransferable.js - you could even enhance
> retriveURLFromData as I expect all consumers could do with the extra checks.

Neil, we can't do that as some onDrop handlers *do* want to allow dropping
crome:// URLs, like the bookmarks code for example.

Comment 44

[Daniel Veditz [:dveditz]]

Reference id CAN-2005-0401

Comment 45

[Daniel Veditz [:dveditz]]

Advisory published: http://www.mozilla.org/security/announce/mfsa2005-32.html

Comment 46

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050324
Firefox/1.0+

Is it intended behaviour that dragging a word into the url bar does nothing at all ?

repro:
1.select the word "mozilla" on this page and drag it to the urlbar
2.nothing happens

exp:
www.mozilla.org to be opened (as happened before this path landed)

Comment 47

[Boris Zbarsky [:bzbarsky]]

No, that doesn't sound intended at all....  I bet the problem is that
CheckLoadURIStr just throws if you pass it something that's not a URI (which is
what happens in that case).

We probably want to fix all this code to create the URI objects itself and then
call regular CheckLoadURI instead of using CheckLoadURIStr on things that may
not be URIs....

Comment 48

[Daniel Veditz [:dveditz]]

Please file a separate bug on the regression/cleanup, don't reopen this one.

Comment 49

[Boris Zbarsky [:bzbarsky]]

Daniel, this bug is still open, hence the comments landing here.

Comment 50

[Juha-Matti Laurio]

This is assigned to 'SA14654'

Mozilla Firefox Three Vulnerabilities (Issue 1):
http://secunia.com/advisories/14654/

"Solution Status: Vendor Patch".

Comment 51

[Johnny Stenback (:jst)]

Attachment: Remaining trunk Seamonkey changes.

These are the same changes that went in for 1.7, but diffed against the trunk
(which still doesn't have this fix for Seamonkey).

Neil, you ok with me landing these changes for Seamonkey?

Comment 52

[Johnny Stenback (:jst)]

I filed bugs on the widget systems that were not fixed by the patches in this
bug to make sure the owners get a chance to check their code and fix if
necessary. See blocked bugs.

Comment 53

[[e-mail]]

Comment on attachment 178639 [details] [diff] [review]
Remaining trunk Seamonkey changes.

Two problems with this:
1. contentAreaDD.js wasn't patched
2. navigator.js is the wrong place for the security check
Since view source also uses contentAreaDD.js we can't put the secuity check in
tabbrowser.xml either. Either nsDragAndDrop.js or nsTransferable.js would seem
to be suitable as they both live in global chrome, and all three drop targets
already depend on them.

Comment 54

[Boris Zbarsky [:bzbarsky]]

jst, see also comment 47... using CheckLoadUriStr leads to broken behavior here.  :(

Comment 55

[Johnny Stenback (:jst)]

Attachment: Only do a security check in the URL bar code when dropping an actual URL

bz, what do you think about this? This attempts to do the security check only
when an actual URL is dropped into the URL bar, if a keyword or whatever is
dropped, we let it go through w/o doing the security check as there should be
no reason to do the security check in such cases.

Comment 56

[Boris Zbarsky [:bzbarsky]]

What about other places where we're using CheckLoadURIStr?  Wouldn't we want
this for all of them?

Comment 57

[Johnny Stenback (:jst)]

Hmm, yeah, I guess we would. I'll whip up a new patch...

Comment 58

[Johnny Stenback (:jst)]

Attachment: XPFE fixes per Neil's suggestion, and only do the security checks when dealing with URI's.

Comment 59

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 179132 [details] [diff] [review]
XPFE fixes per Neil's suggestion, and only do the security checks when dealing with URI's.

>Index: toolkit/content/widgets/tabbrowser.xml
>+              // Strip leading and trailing whitespace

Check with darin whether this is needed?  I think all our URI impls deal, but I
don't see anything obvious in the nsIProtocolHandler api that says that
protocol handlers must ignore leading and trailing whitespace...  If they don't
have to do that, then I guess we do need this code.

>+                  // Stop even propagation right here.

s/even/event/

>Index: xpfe/global/resources/content/nsDragAndDrop.js

>+            // Stop even propagation right here.

s/even/event/

Looks good otherwise; I assume you'll add similar code to the other places
where we added checkLoadURIStr calls in this bug?

Comment 60

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 179005 [details] [diff] [review]
Only do a security check in the URL bar code when dropping an actual URL

I'm not a module owner or peer for this code, so I really can't review it... In
particular, I don't really know the implications of setting gURLBar.value.

Mike, if you can't get to this either, is there someone you'd recommed as
reviewer?

Comment 61

[Mike Connor [:mconnor]]

Comment on attachment 179005 [details] [diff] [review]
Only do a security check in the URL bar code when dropping an actual URL

> function handleURLBarCommand(aTriggeringEvent)
> {
>   var postData = { };
>-  canonizeUrl(aTriggeringEvent, postData);
>+
>+  if (gURLBar) {
>+    var newURI = canonizeUrl(gURLBar.value, aTriggeringEvent, postData);
>+
>+    gURLBar.value = newURI;
>+  }

getShortcutOrURI actually returns a URL, ergo so does canonizeURL, so this
should be newURL, please.

>+        var ioService = Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService);
>+        var canonizedUri = canonizeUrl(url);
>+
>+        var tmpUri = null;
>+        try {
>+          var tmpUri = ioService.newURI(canonizedUri, null, null);
>+        } catch(e) {
>+        }

Again, canonizedUrl, not canonizedUri please.  (I'll fix the function name and
callers to prevent confusion in the future.)  
Please fix the js strict warning (redeclaration).  
Please use |tmpURI = makeURI(canonizedUrl);| here.  (Please note: on the aviary
branch it's still called makeURL, but does the same thing.)  I need to fix that
helper function to take a charset and convert the existing cases as well, but
that's a cleanup bug that I'll file later today.

r=me with those changes.

Comment 62

[[e-mail]]

(In reply to comment #59)
>(From update of attachment 179132 [details] [diff] [review] [edit])
>>+              // Strip leading and trailing whitespace
Except the provided regexp actually strips leading OR trailing whitespace, but
not both - you're missing a /g on the end. Rest of the review when I wake up.

Comment 63

[Johnny Stenback (:jst)]

(In reply to comment #61)
> getShortcutOrURI actually returns a URL, ergo so does canonizeURL, so this
> should be newURL, please.

Unless I'm remembering incorrectly, about:blank etc are not URLs, and this deals
with such URIs too doesn't it? Seems more correct to me to use URI here. But
whatever, I can change this if you want me to.

Comment 64

[Mike Connor [:mconnor]]

Yeah, at least in Firefox code, anything we call a uri has been constructed
properly, so the convention at least holds that we know that a "url" needs to
get converted before being passed to functions that need a URI and not a string.

Comment 65

[[e-mail]]

Comment on attachment 179132 [details] [diff] [review]
XPFE fixes per Neil's suggestion, and only do the security checks when dealing with URI's.

Sorry for the delay, I'd forgotton about this review. r=me with the regexp
fixed.

Comment 66

[Ginn Chen]

I changed chrome://global/content/alerts/alert.xul to
chrome://communicator/content/pref/pref.xul in the testcase.
I found it is still reproducible with Mozilla 1.7.6
Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.7.6) Gecko/20050319

Did I miss something?

Comment 67

[Ginn Chen]

(In reply to comment #66)
> I changed chrome://global/content/alerts/alert.xul to
> chrome://communicator/content/pref/pref.xul in the testcase.
> I found it is still reproducible with Mozilla 1.7.6
> Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.7.6) Gecko/20050319
> 
> Did I miss something?

I tried my testcases on firefox 1.0.x debug build and mozilla 1.7.x debug build.
On firefox, it outputs 'uncaught exception: Drop of chrome://...xul denied".
On mozilla, the drop succeeds.

Comment 68

[Ginn Chen]

Attachment: patch contentAreaDD.js for mozilla1.7 branch

This bug is still reproducible with Mozilla 1.7.6
contentAreaDD.js should also be patched.

Comment 69

[Ginn Chen]

Remove fixed1.7.6, because it's still there.

Comment 70

[Johnny Stenback (:jst)]

Comment on attachment 180472 [details] [diff] [review]
patch contentAreaDD.js for mozilla1.7 branch

sr=jst

Comment 71

[Daniel Veditz [:dveditz]]

Comment on attachment 180472 [details] [diff] [review]
patch contentAreaDD.js for mozilla1.7 branch

Good catch!

r=dveditz
a=dveditz for 1.7 branch (but a- for aviary branch)

Comment 72

[Ginn Chen]

Checking in xpfe/communicator/resources/content/contentAreaDD.js;
/cvsroot/mozilla/xpfe/communicator/resources/content/contentAreaDD.js,v  <-- 
contentAreaDD.js
new revision: 1.32.80.2; previous revision: 1.32.80.1
done

Landed 1.7 branch
Please update the Advisory.

Comment 73

[Boris Zbarsky [:bzbarsky]]

Ginn, is this fixed in 1.7.7?  If so, is there a reason you left the blocking? flag?

Comment 74

[Daniel Veditz [:dveditz]]

awarded a bug bounty

Comment 75

[Marc Bejarano]

what is the real current status of this bug?  it certainly isn't "NEW" ;)

Comment 76

[Ian Neal]

Comment on attachment 179132 [details] [diff] [review]
XPFE fixes per Neil's suggestion, and only do the security checks when dealing with URI's.

That is because it looks like some changes were only made to the mozilla 1.7.x
and aviary 1.0.x branches but, this patch has not landed on the trunk, and
might need to be unbitrotted to do so as well as needing an a=.

Comment 77

[Ian Neal]

Attachment: xpfe/toolkit trunk patch v2.0 (Checked in)

Changes since trunk patch attachment 179132 [details] [diff] [review]
* Fixed comment typo - even instead of event
* Added g flag to both .replace methods in xpfe/toolkit's tabbrowser.xml files
as requested by Neil

Carrying forward r/sr and requesting a= for xpfe/toolkit trunk landing for this
security fix that has already been fixed on the branches

Comment 78

[Ian Neal]

Comment on attachment 190298 [details] [diff] [review]
xpfe/toolkit trunk patch v2.0 (Checked in)

Checking in
toolkit/content/widgets/tabbrowser.xml;
new revision: 1.94; previous revision: 1.93
xpfe/browser/resources/content/navigatorDD.js;
new revision: 1.103; previous revision: 1.102
xpfe/communicator/resources/content/contentAreaDD.js;
new revision: 1.35; previous revision: 1.34
xpfe/global/resources/content/nsDragAndDrop.js;
new revision: 1.28; previous revision: 1.27
xpfe/global/resources/content/bindings/tabbrowser.xml;
new revision: 1.123; previous revision: 1.122
done

Comment 79

[Asa Dotzler [:asa]]

Is there anything here that still needs to happen for the 1.8 branch? Is the
review request for attachment 177682 [details] [diff] [review] still needed?

Comment 80

[Johnny Stenback (:jst)]

To the best of my knowledge this is all fixed on the 1.8 branch too, if not,
please remove the fixed1.8 keyword.

Comment 81

[Jesse Ruderman]

The patch for this bug went in before the branch point for Gecko 1.8, so this is
fixed1.8.

Comment 82

[Daniel C]

Comment on attachment 177682 [details] [diff] [review]
Make the security check a method of the tabbrowser for firefox, and fix nsContentAreaDragDrop.cpp to do the same security check too.

Can the review request be removed here?

Comment 83

[Asa Dotzler [:asa]]

Comment on attachment 177682 [details] [diff] [review]
Make the security check a method of the tabbrowser for firefox, and fix nsContentAreaDragDrop.cpp to do the same security check too.

Is this review request still needed?