Closed Bug 329521 Opened 15 years ago Closed 15 years ago

View Image xss

Categories

(Core :: Security, defect)

1.8 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.8final

People

(Reporter: pvnick, Assigned: martijn.martijn)

References

Details

(4 keywords, Whiteboard: [sg:high] need to test seamonkey)

Attachments

(5 files, 1 obsolete file)

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.
Added a testcase. Right click on the image and press "View Image"
Attached patch patch (obsolete) — Splinter Review
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.
(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?
Yes, the spaces are stripped off.
Comment on attachment 214258 [details] [diff] [review]
patch

Might as well ask review.
Attachment #214258 - Flags: review?(mconnor)
Attachment #214258 - Attachment is obsolete: true
Attachment #214258 - Flags: review?(mconnor)
Attached patch patch2Splinter Review
Better patch.
From the surrounding code, I get the feeling no exception should be thrown.
Attachment #214302 - Flags: review?(mconnor)
Assignee: nobody → martijn.martijn
Need to test SeaMonkey and see if we need an equivalent patch there, too.
Flags: blocking1.8.0.3?
Flags: blocking-firefox2+
Flags: blocking-aviary1.0.9?
Whiteboard: [sg:high] need to test seamonkey
Attached patch xpfe patchSplinter Review
The point being that after the location changes we lose our context ;-)
Attachment #214753 - Flags: superreview?(bzbarsky)
Attachment #214753 - Flags: review?(jag)
Comment on attachment 214753 [details] [diff] [review]
xpfe patch

I like this approach much better.
Attachment #214753 - Flags: review?(jag) → review+
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?
Attachment #214753 - Flags: superreview?(bzbarsky) → superreview+
Hmm, so that means the context menu disappears when the url changes, I deliberately tried to avoid that.
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.
Attachment #214753 - Flags: approval1.8.0.3?
Attachment #214753 - Flags: approval-branch-1.8.1?(cbiesinger)
Attachment #214753 - Flags: approval-aviary1.0.9?
Attachment #214753 - Flags: approval-branch-1.8.1?(cbiesinger) → approval-branch-1.8.1+
(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?
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.
(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.
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.
Attachment #214302 - Flags: review?(mconnor) → review+
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.
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.
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.
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.
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.
(In reply to comment #21)
> ..., we could use the same fix as the
> forward/backward navigation script execution bug...
Which bug is that?

(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.
Code wise I'm sure we can make the "right" thing happen, but what is the right thing from a UI point of view?
(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.
(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?

(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).
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 2 alpha2
Attached patch xpfe enhancementSplinter Review
Attachment #215521 - Flags: superreview?(jag)
Attachment #215521 - Flags: review?(jag)
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.
Attachment #215521 - Flags: superreview?(jag)
Attachment #215521 - Flags: superreview+
Attachment #215521 - Flags: review?(jag)
Attachment #215521 - Flags: review+
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
Attachment #215521 - Flags: approval-branch-1.8.1?(cbiesinger)
If gContextMenu has a target that's not in content, something's rather wrong. 
(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.
Attachment #215521 - Flags: approval-branch-1.8.1?(cbiesinger) → approval-branch-1.8.1+
Flags: blocking1.8.0.3? → blocking1.8.0.3+
Comment on attachment 214302 [details] [diff] [review]
patch2

mconnor: is this patch ok for the 1.8 and 1.8.0 branches?
Attachment #214302 - Flags: approval1.8.0.3?
Attachment #214302 - Flags: approval-branch-1.8.1?(mconnor)
Comment on attachment 214753 [details] [diff] [review]
xpfe patch

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #214753 - Flags: approval1.8.0.3? → approval1.8.0.3+
Component: General → Security
Flags: review+
Flags: blocking-firefox2+
Product: Firefox → Core
Target Milestone: Firefox 2 alpha2 → ---
Version: unspecified → Trunk
Comment on attachment 214302 [details] [diff] [review]
patch2

Yeah, should be fine everywhere.
Attachment #214302 - Flags: approval1.8.0.3?
Attachment #214302 - Flags: approval1.8.0.3+
Attachment #214302 - Flags: approval-branch-1.8.1?(mconnor)
Attachment #214302 - Flags: approval-branch-1.8.1+
Comment on attachment 215521 [details] [diff] [review]
xpfe enhancement

c.f. attachment 214753 [details] [diff] [review], only asking for the right flags first time.
Attachment #215521 - Flags: approval1.8.0.3?
Attachment #215521 - Flags: approval1.7.14?
Comment on attachment 215521 [details] [diff] [review]
xpfe enhancement

approved for 1.8.0 branch, a=darin for drivers
Attachment #215521 - Flags: approval1.8.0.3? → approval1.8.0.3+
xpfe fix checked in to the 1.8.0 branch.
If Neil's patch gets checked in, I still think my patch (patch2) is necessary, because I think it also fixes bug 329583.
Blocks: 329583
"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
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → mozilla1.8final
Version: Trunk → 1.8 Branch
(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

Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Yes, 1.1a has the enhanced version.
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
Depends on: 332874
Group: security
proposed patch for mfsa2006-34
Attachment #225478 - Flags: review?(caillon)
You need to log in before you can comment on or make changes to this bug.