Last Comment Bug 329521 - View Image xss
: View Image xss
Status: RESOLVED FIXED
[sg:high] need to test seamonkey
: fixed-seamonkey1.0.2, fixed-seamonkey1.1a, fixed1.8.1, verified1.8.0.4
Product: Core
Classification: Components
Component: Security (show other bugs)
: 1.8 Branch
: All All
: -- critical (vote)
: mozilla1.8final
Assigned To: Martijn Wargers [:mwargers] (not working for Mozilla)
:
Mentors:
Depends on: 332874
Blocks: 329583
  Show dependency treegraph
 
Reported: 2006-03-06 10:01 PST by Paul Nickerson
Modified: 2006-06-13 15:24 PDT (History)
8 users (show)
dveditz: blocking‑aviary1.0.9?
dveditz: blocking1.8.0.4+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Executes script at google.com (220 bytes, text/html)
2006-03-06 10:02 PST, Paul Nickerson
no flags Details
patch (1.68 KB, patch)
2006-03-06 17:32 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details | Diff | Splinter Review
patch2 (2.35 KB, patch)
2006-03-07 04:57 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
mconnor: approval‑branch‑1.8.1+
mconnor: approval1.8.0.4+
Details | Diff | Splinter Review
xpfe patch (708 bytes, patch)
2006-03-10 18:23 PST, neil@parkwaycc.co.uk
bzbarsky: superreview+
neil: approval‑aviary1.0.9?
cbiesinger: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Splinter Review
xpfe enhancement (1.19 KB, patch)
2006-03-18 12:29 PST, neil@parkwaycc.co.uk
jag-mozilla: superreview+
neil: approval1.7.14?
cbiesinger: approval‑branch‑1.8.1+
darin.moz: approval1.8.0.4+
Details | Diff | Splinter Review
1.0.x patch for 329521 and 329468 (6.44 KB, patch)
2006-06-13 14:50 PDT, Alexander Sack
asac: review? (caillon)
Details | Diff | Splinter Review

Description Paul Nickerson 2006-03-06 10:01:39 PST
An image with a javascript url will not be executed unless the user right clicks and presses View Image. If the user does this and the page is nagivated to another site while the context menu is displayed, script can be executed in the context of the target website.
Comment 1 Paul Nickerson 2006-03-06 10:02:26 PST
Created attachment 214199 [details]
Executes script at google.com

Added a testcase. Right click on the image and press "View Image"
Comment 2 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-03-06 17:32:57 PST
Created attachment 214258 [details] [diff] [review]
patch

So something like this, I guess?
It prevents loading of javascript url when the current window url isn't the same as the url where the context menu was opened in.
Comment 3 Paul Nickerson 2006-03-06 21:33:13 PST
(In reply to comment #2)
Just a little bit of threat analysis... In your patch you check if this.bgImageURL.indexOf('javascript:') == 0). If you added a space in front of the address, making indexOf('javascript:')==1 (although still a valid address that executes script), would this circumvent your check, or is bgImageURL stripped of spaces?
Comment 4 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-03-07 03:06:34 PST
Yes, the spaces are stripped off.
Comment 5 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-03-07 03:19:40 PST
Comment on attachment 214258 [details] [diff] [review]
patch

Might as well ask review.
Comment 6 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-03-07 04:57:31 PST
Created attachment 214302 [details] [diff] [review]
patch2

Better patch.
From the surrounding code, I get the feeling no exception should be thrown.
Comment 7 Daniel Veditz [:dveditz] 2006-03-10 09:48:20 PST
Need to test SeaMonkey and see if we need an equivalent patch there, too.
Comment 8 neil@parkwaycc.co.uk 2006-03-10 18:23:44 PST
Created attachment 214753 [details] [diff] [review]
xpfe patch

The point being that after the location changes we lose our context ;-)
Comment 9 jag (Peter Annema) 2006-03-10 18:36:54 PST
Comment on attachment 214753 [details] [diff] [review]
xpfe patch

I like this approach much better.
Comment 10 Boris Zbarsky [:bz] 2006-03-10 20:57:12 PST
Comment on attachment 214753 [details] [diff] [review]
xpfe patch

Yeah, this I like a lot!

Is there a corresponding bug on toolkit?  If not, please get one filed?
Comment 11 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-03-10 23:45:15 PST
Hmm, so that means the context menu disappears when the url changes, I deliberately tried to avoid that.
Comment 12 neil@parkwaycc.co.uk 2006-03-11 05:41:26 PST
Comment on attachment 214753 [details] [diff] [review]
xpfe patch

Fix checked in to the trunk. Seeking branch approvals (using aviary1.0.9 as this product doesn't have 1.7.x) for this trivial fix.
Comment 13 Paul Nickerson 2006-03-11 12:50:49 PST
(In reply to comment #11)
> Hmm, so that means the context menu disappears when the url changes, I
> deliberately tried to avoid that.

Is there a problem with this?
Comment 14 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-03-11 13:05:26 PST
Well, not really, only that I find it slightly inconvenient. IE6 keeps the context menu open, by the way.
I don't think Neil's patch fixes this issue when frames are involved, is it?
http://wargers.org/mozilla/test/viewimage.htm
Although this issue is less critical.
Comment 15 neil@parkwaycc.co.uk 2006-03-11 13:20:57 PST
(In reply to comment #14)
>I don't think Neil's patch fixes this issue when frames are involved, is it?
It seems to cancel the context menu when any frame loads.
Comment 16 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-03-11 14:04:09 PST
Ok, I see it now with SeaMonkey 2006-03-11 build. But now you get this problem:
http://wargers.org/mozilla/test/contextmenu.htm
The context menu doesn't stay open anymore when an iframe's source changes location.
Comment 17 jag (Peter Annema) 2006-03-14 05:45:00 PST
What do you want to do with the context menu despite the fact that you're on a different page?

Aside from XSS you can also use this simply to deceive. The user thinks they're saving or sending one page but end up saving or sending whatever's currently loaded (that's how the SeaMonkey context menu currently works). Of course viewing/saving the image would appear to work (we too store the URL on the context menu object), but as Neil pointed out we'd be trying to view/save with the security context of the currently loaded page. Probably not a good idea.

The only safe things I see are Back, Forward, Reload, Stop and Select All. So if we were in an image context menu we'd have to get those to show (updated for the currently loaded page), and hide? disable? the rest. For a page context menu they'd be there, but of course need to update and hide or disable the rest. Same for frame context menu.

Might be simpler to say that when a new frame or page gets loaded we either close the context menu, or reinit it to contain just "Back", "Forward", "Reload" and "Stop" updated for the current situation.
Comment 18 jag (Peter Annema) 2006-03-14 05:58:48 PST
I guess we could record all context data on the context menu object and pre-check stuff, and that way make sure the user always acts on the thing they clicked on. Though, that might break cases where the context-click changes the baseURI after the click (and for some reason expects that to work ... unlikely, I guess).

That leaves as my only problem that having a context menu on some page act on an object on a previous page just feels wrong to me, no matter that I'd actually understand what's going on.
Comment 19 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-03-14 14:19:16 PST
Ok, well, most people apparently want the context menu to disappear/change when the page changes (I prefer current behavior, which is what Opera and IE6 are doing).
But Neil's patch is causing the regression which I mentioned in comment 16, so I don't think that patch is currently sufficient, imho.

Btw, Opera9 is also vulnerable to this, someone might want to warn them.
Comment 20 jag (Peter Annema) 2006-03-14 15:18:49 PST
I wouldn't say I want the context menu to go away, I'm just saying that most items in the context menu make less sense when a new document replaces the one you had the context menu open for.

We could do something where if the context menu is not for the document that gets replaced (nor for any object in it) we can keep the context menu open.
Comment 21 Paul Nickerson 2006-03-15 08:50:36 PST
How about we make the context menu retain the permissions of the original page? For example, instead of chrome navigating to the image when View Image is pressed, we behave as if the original page is navigating to the image from the website with the current url. That way, http://mal.com can navigate to http://good.com/image.gif, but not javascript:alert("script executed").

If that doesn't seem like a feasible solution, we could use the same fix as the forward/backward navigation script execution bug and have View Image navigate to the image from about:blank.
Comment 22 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-03-15 14:15:22 PST
(In reply to comment #21)
> ..., we could use the same fix as the
> forward/backward navigation script execution bug...
Which bug is that?

Comment 23 Paul Nickerson 2006-03-15 14:59:35 PST
(In reply to comment #22)
> Which bug is that?

https://bugzilla.mozilla.org/show_bug.cgi?id=292691
There are two bugs described on that page. One of the testcases (attachment #3 [details] [diff] [review] I think) shows the vuln I'm talking about.
Comment 24 jag (Peter Annema) 2006-03-15 17:48:56 PST
Code wise I'm sure we can make the "right" thing happen, but what is the right thing from a UI point of view?
Comment 25 Paul Nickerson 2006-03-15 18:52:44 PST
(In reply to comment #24)
> Code wise I'm sure we can make the "right" thing happen, but what is the right
> thing from a UI point of view?

Using about:blank as the navigate-from page seems like the easiest thing to do. Executing javascript when View Image is pressed is a bug in itself, which would be fixed as well, and I don't think it would break anything.
Comment 26 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-03-16 02:42:49 PST
(In reply to comment #25)
> Executing javascript when View Image is pressed is a bug in itself, which would
> be fixed as well, and I don't think it would break anything.
Well, javascript images do exist, see:
http://www.elf.org/pnglets/
http://www.246.dk/xbm.html
But the use of it is very minimal and View Image is already not working. So maybe just disable the View Image menu-item when javascript is involved?

Comment 27 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-03-16 02:51:36 PST
(In reply to comment #26)
> But the use of it is very minimal and View Image is already not working. So
> maybe just disable the View Image menu-item when javascript is involved?
But I'd rather fix View Image to make it work on javascript images (the other context Image related options are not working properly either).
Comment 28 neil@parkwaycc.co.uk 2006-03-18 12:29:49 PST
Created attachment 215521 [details] [diff] [review]
xpfe enhancement
Comment 29 jag (Peter Annema) 2006-03-18 22:04:51 PST
Comment on attachment 215521 [details] [diff] [review]
xpfe enhancement

Your for-loop's condition could be |contextWindow != content|. It'd change the loop from inclusive of content to exclusive, but since we're testing for that case early on it's safe.
Comment 30 neil@parkwaycc.co.uk 2006-03-19 01:19:27 PST
Comment on attachment 215521 [details] [diff] [review]
xpfe enhancement

c.f. attachment 214753 [details] [diff] [review]

(In reply to comment #29)
I didn't change the condition in case for some reason contextWindow.top != content
Comment 31 jag (Peter Annema) 2006-03-19 02:31:34 PST
If gContextMenu has a target that's not in content, something's rather wrong. 
Comment 32 neil@parkwaycc.co.uk 2006-03-19 05:02:30 PST
(In reply to comment #31)
>If gContextMenu has a target that's not in content, something's rather wrong. 
I didn't want to make assumptions about background tabs.
Comment 33 Daniel Veditz [:dveditz] 2006-04-03 12:24:00 PDT
Comment on attachment 214302 [details] [diff] [review]
patch2

mconnor: is this patch ok for the 1.8 and 1.8.0 branches?
Comment 34 Daniel Veditz [:dveditz] 2006-04-03 12:24:41 PDT
Comment on attachment 214753 [details] [diff] [review]
xpfe patch

approved for 1.8.0 branch, a=dveditz for drivers
Comment 35 Mike Connor [:mconnor] 2006-04-03 12:55:50 PDT
Comment on attachment 214302 [details] [diff] [review]
patch2

Yeah, should be fine everywhere.
Comment 36 neil@parkwaycc.co.uk 2006-04-03 15:50:28 PDT
Comment on attachment 215521 [details] [diff] [review]
xpfe enhancement

c.f. attachment 214753 [details] [diff] [review], only asking for the right flags first time.
Comment 37 Darin Fisher 2006-04-07 12:55:59 PDT
Comment on attachment 215521 [details] [diff] [review]
xpfe enhancement

approved for 1.8.0 branch, a=darin for drivers
Comment 38 neil@parkwaycc.co.uk 2006-04-21 03:26:01 PDT
xpfe fix checked in to the 1.8.0 branch.
Comment 39 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-04-27 11:00:15 PDT
If Neil's patch gets checked in, I still think my patch (patch2) is necessary, because I think it also fixes bug 329583.
Comment 40 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-05-02 17:37:47 PDT
"Patch2" (attachment 214302 [details] [diff] [review]) checked in on the trunk and both 1.8 branches.
mozilla/browser/base/content/browser.js 	1.479.2.43.2.2
mozilla/browser/base/content/browser.js 	1.479.2.127
mozilla/browser/base/content/browser.js 	1.621
Comment 41 Daniel Veditz [:dveditz] 2006-05-03 15:52:26 PDT
(In reply to comment #38)
> xpfe fix checked in to the 1.8.0 branch.

Was the xpfe version also checked into the 1.8 branch? Adding other release keywords per gavin and neil's comments

Comment 42 neil@parkwaycc.co.uk 2006-05-03 16:47:59 PDT
Yes, 1.1a has the enhanced version.
Comment 43 Tracy Walker [:tracy] 2006-05-04 08:46:54 PDT
verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060504 Firefox/1.5.0.4
Comment 44 Alexander Sack 2006-06-13 14:50:21 PDT
Created attachment 225478 [details] [diff] [review]
1.0.x patch for 329521 and 329468

proposed patch for mfsa2006-34

Note You need to log in before you can comment on or make changes to this bug.