Drag and drop gestures can be hijacked to load priviliged xul

RESOLVED FIXED

Status

()

Core
Drag and Drop
--
major
RESOLVED FIXED
13 years ago
8 years ago

People

(Reporter: Michael Krax, Assigned: jst)

Tracking

(Blocks: 1 bug, {fixed-aviary1.0.2, fixed1.7.7, fixed1.8})

Trunk
fixed-aviary1.0.2, fixed1.7.7, fixed1.8
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.6 +
blocking-aviary1.0.2 +
blocking1.8b5 +
blocking-aviary1.5 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix] CAN-2005-0401, URL)

Attachments

(9 attachments, 6 obsolete attachments)

5.88 KB, patch
Details | Diff | Splinter Review
20.80 KB, patch
Brian Ryner (not reading)
: superreview+
Details | Diff | Splinter Review
22.57 KB, patch
Details | Diff | Splinter Review
23.96 KB, patch
Details | Diff | Splinter Review
14.84 KB, patch
dveditz
: superreview+
dveditz
: approval1.7.6+
Details | Diff | Splinter Review
4.07 KB, patch
neil@parkwaycc.co.uk
: review-
Details | Diff | Splinter Review
4.01 KB, patch
mconnor
: review+
Details | Diff | Splinter Review
1.09 KB, patch
dveditz
: review+
dveditz
: approval1.7.7+
Details | Diff | Splinter Review
9.90 KB, patch
Ian Neal
: review+
Ian Neal
: superreview+
bsmedberg
: approval1.8b4+
Details | Diff | Splinter Review
(Reporter)

Description

13 years ago
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?
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).
Severity: normal → major
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-aviary1.1+
Flags: blocking-aviary1.0.2+
Whiteboard: [sg:fix]

Comment 2

13 years ago
Dup of bug 250725?
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

13 years ago
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?
(Assignee)

Comment 5

13 years ago
Created attachment 177191 [details] [diff] [review]
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?
(In reply to comment #2)
> Dup of bug 250725?

Yes, essentially.
> 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

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

Comment 9

13 years ago
any updates on where we are for this?  holding 1.0.2's and 1.7.6 for the
finished patch.
(Assignee)

Comment 10

13 years ago
Created attachment 177443 [details] [diff] [review]
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.
(Assignee)

Comment 11

13 years ago
Created attachment 177454 [details] [diff] [review]
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)?
Attachment #177443 - Attachment is obsolete: true
Attachment #177454 - Flags: review?(bzbarsky)

Comment 12

13 years ago
Created attachment 177470 [details] [diff] [review]
FE fix for suite

Comment 13

13 years ago
Created attachment 177471 [details] [diff] [review]
Fix tabbrowser.xml?
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.
Attachment #177454 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 15

13 years ago
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.
Attachment #177471 - Flags: review+
Attachment #177454 - Flags: superreview+
Assignee: nobody → jst
(Assignee)

Comment 16

13 years ago
Created attachment 177567 [details] [diff] [review]
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...
(Assignee)

Comment 17

13 years ago
Created attachment 177568 [details] [diff] [review]
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.
(Assignee)

Updated

13 years ago
Attachment #177568 - Flags: superreview?(bryner)
Attachment #177568 - Flags: review?(bryner)
Attachment #177568 - Flags: superreview?(bryner)
Attachment #177568 - Flags: superreview+
Attachment #177568 - Flags: review?(bryner)
Attachment #177568 - Flags: review+
(Assignee)

Comment 18

13 years ago
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.
Flags: blocking1.7.6+

Comment 19

13 years ago
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.
(Assignee)

Comment 20

13 years ago
(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.
(Assignee)

Comment 21

13 years ago
(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.
(Assignee)

Comment 22

13 years ago
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.
(Assignee)

Comment 23

13 years ago
Created attachment 177618 [details] [diff] [review]
Complete (Win, gtk, mac) widget changes for aviary/1.7 branches. 

This is the widget changes that have been checked in on the branches.
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?
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

13 years ago
(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?
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...
(Assignee)

Comment 28

13 years ago
Created attachment 177673 [details] [diff] [review]
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.
Attachment #177673 - Flags: superreview?(bugs)
Attachment #177673 - Flags: review?(bugs)
(Assignee)

Updated

13 years ago
Attachment #177673 - Flags: superreview?(bugs) → superreview?(dveditz)
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.
Attachment #177673 - Flags: review?(bugs) → review-
+function DragDropSecurityCheck(aEvent, aDragSession, aUrl)

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

Comment 31

13 years ago
Created 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.
Attachment #177673 - Attachment is obsolete: true
Attachment #177682 - Flags: superreview?(dveditz)
Attachment #177682 - Flags: review?(bugs)
(Assignee)

Comment 32

13 years ago
(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.
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.
I buy security-checking drags to all places that load the dragged text
automtically.  But why url bar?
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=ben@mozilla.org 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.
Attachment #177682 - Flags: review?(bugs) → review+
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
Attachment #177682 - Flags: superreview?(dveditz)
Attachment #177682 - Flags: superreview+
Attachment #177682 - Flags: review?(bugs)
Attachment #177682 - Flags: review+
Attachment #177682 - Flags: approval1.7.6+
Attachment #177682 - Flags: approval-aviary1.0.2+
(Assignee)

Comment 37

13 years ago
(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.
(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?
(Assignee)

Updated

13 years ago
Keywords: fixed-aviary1.0.2, fixed1.7.6
(Assignee)

Updated

13 years ago
Attachment #177568 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #177471 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #177470 - Attachment is obsolete: true
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.
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

13 years ago
Why not put the code in nsTransferable.js - you could even enhance
retriveURLFromData as I expect all consumers could do with the extra checks.
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.
(Assignee)

Comment 43

13 years ago
(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.
Blocks: 286709

Updated

13 years ago
No longer blocks: 286709
Attachment #177673 - Flags: superreview?(dveditz)
Reference id CAN-2005-0401
Whiteboard: [sg:fix] → [sg:fix] CAN-2005-0401
Advisory published: http://www.mozilla.org/security/announce/mfsa2005-32.html
Group: security
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)
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....
Please file a separate bug on the regression/cleanup, don't reopen this one.
Daniel, this bug is still open, hence the comments landing here.

Comment 50

13 years ago
This is assigned to 'SA14654'

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

"Solution Status: Vendor Patch".
(Assignee)

Comment 51

13 years ago
Created attachment 178639 [details] [diff] [review]
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?
Attachment #178639 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Updated

13 years ago
Blocks: 287759
(Assignee)

Updated

13 years ago
Blocks: 287756
(Assignee)

Updated

13 years ago
Blocks: 287757
(Assignee)

Updated

13 years ago
Blocks: 287758
(Assignee)

Comment 52

13 years ago
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

13 years ago
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.
Attachment #178639 - Flags: review?(neil.parkwaycc.co.uk) → review-
jst, see also comment 47... using CheckLoadUriStr leads to broken behavior here.  :(
(Assignee)

Comment 55

13 years ago
Created attachment 179005 [details] [diff] [review]
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.
Attachment #179005 - Flags: review?(bzbarsky)
What about other places where we're using CheckLoadURIStr?  Wouldn't we want
this for all of them?
(Assignee)

Comment 57

13 years ago
Hmm, yeah, I guess we would. I'll whip up a new patch...
(Assignee)

Comment 58

13 years ago
Created attachment 179132 [details] [diff] [review]
XPFE fixes per Neil's suggestion, and only do the security checks when dealing with URI's.
Attachment #179132 - Flags: superreview?(bzbarsky)
Attachment #179132 - Flags: review?(neil.parkwaycc.co.uk)
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?
Attachment #179132 - Flags: superreview?(bzbarsky) → superreview+
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?
Attachment #179005 - Flags: review?(bzbarsky) → review?(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.
Attachment #179005 - Flags: review?(mconnor) → review+

Comment 62

13 years ago
(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.
(Assignee)

Comment 63

13 years ago
(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.
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

13 years ago
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.
Attachment #179132 - Flags: review?(neil.parkwaycc.co.uk) → review+

Comment 66

13 years ago
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?
Blocks: 289667
Blocks: 289513

Comment 67

13 years ago
(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

13 years ago
Created attachment 180472 [details] [diff] [review]
patch contentAreaDD.js for mozilla1.7 branch

This bug is still reproducible with Mozilla 1.7.6
contentAreaDD.js should also be patched.
Attachment #180472 - Flags: superreview?(dveditz)
Attachment #180472 - Flags: review?(dveditz)
Attachment #180472 - Flags: approval1.7.7?
Attachment #180472 - Flags: approval-aviary1.0.3?

Comment 69

13 years ago
Remove fixed1.7.6, because it's still there.
Flags: blocking1.7.7?
Keywords: fixed1.7.6
(Assignee)

Comment 70

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

sr=jst
Attachment #180472 - Flags: superreview?(dveditz) → superreview+
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)
Attachment #180472 - Flags: review?(dveditz)
Attachment #180472 - Flags: review+
Attachment #180472 - Flags: approval1.7.7?
Attachment #180472 - Flags: approval1.7.7+
Attachment #180472 - Flags: approval-aviary1.0.3?
Attachment #180472 - Flags: approval-aviary1.0.3-

Comment 72

13 years ago
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.
Keywords: fixed1.7.7
Ginn, is this fixed in 1.7.7?  If so, is there a reason you left the blocking? flag?

Updated

13 years ago
Flags: blocking1.7.7?
awarded a bug bounty
Blocks: 256197

Updated

12 years ago
Blocks: 298695
No longer blocks: 298695

Comment 75

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

Comment 76

12 years ago
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=.

Updated

12 years ago
Flags: blocking1.8b4?

Comment 77

12 years ago
Created attachment 190298 [details] [diff] [review]
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
Attachment #179132 - Attachment is obsolete: true
Attachment #190298 - Flags: superreview+
Attachment #190298 - Flags: review+
Attachment #190298 - Flags: approval1.8b4?

Updated

12 years ago
Flags: blocking1.8b4? → blocking1.8b4+
Attachment #190298 - Flags: approval1.8b4? → approval1.8b4+

Comment 78

12 years ago
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
Attachment #190298 - Attachment description: xpfe/toolkit trunk patch v2.0 → xpfe/toolkit trunk patch v2.0 (Checked in)

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 79

12 years ago
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?
(Assignee)

Comment 80

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

Comment 81

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

Comment 82

12 years ago
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

12 years ago
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?

Updated

12 years ago
Flags: testcase+

Updated

12 years ago
Blocks: 36867
Attachment #177682 - Flags: review?(bugs)

Updated

11 years ago
Flags: in-testsuite+ → in-testsuite?

Updated

8 years ago
QA Contact: drag-drop
You need to log in before you can comment on or make changes to this bug.