329521 - View Image xss

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Paul Nickerson]

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]

Attachment: Executes script at google.com

Added a testcase. Right click on the image and press "View Image"

Comment 2

[Martijn Wargers (dead)]

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

(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 (dead)]

Yes, the spaces are stripped off.

Comment 5

[Martijn Wargers (dead)]

Comment on attachment 214258 [details] [diff] [review]
patch

Might as well ask review.

Comment 6

[Martijn Wargers (dead)]

Attachment: patch2

Better patch.
From the surrounding code, I get the feeling no exception should be thrown.

Comment 7

[Daniel Veditz [:dveditz]]

Need to test SeaMonkey and see if we need an equivalent patch there, too.

Comment 8

[[e-mail]]

Attachment: xpfe patch

The point being that after the location changes we lose our context ;-)

Comment 9

[jag (Peter Annema)]

Comment on attachment 214753 [details] [diff] [review]
xpfe patch

I like this approach much better.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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 (dead)]

Hmm, so that means the context menu disappears when the url changes, I deliberately tried to avoid that.

Comment 12

[[e-mail]]

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]

(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 (dead)]

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

[[e-mail]]

(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 (dead)]

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

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

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 (dead)]

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

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]

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 (dead)]

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

(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)]

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]

(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 (dead)]

(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 (dead)]

(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

[[e-mail]]

Attachment: xpfe enhancement

Comment 29

[jag (Peter Annema)]

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

[[e-mail]]

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

If gContextMenu has a target that's not in content, something's rather wrong. 

Comment 32

[[e-mail]]

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

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

Comment on attachment 214753 [details] [diff] [review]
xpfe patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 35

[Mike Connor [:mconnor]]

Comment on attachment 214302 [details] [diff] [review]
patch2

Yeah, should be fine everywhere.

Comment 36

[[e-mail]]

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]

Comment on attachment 215521 [details] [diff] [review]
xpfe enhancement

approved for 1.8.0 branch, a=darin for drivers

Comment 38

[[e-mail]]

xpfe fix checked in to the 1.8.0 branch.

Comment 39

[Martijn Wargers (dead)]

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: [e-mail]]]

"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]]

(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

[[e-mail]]

Yes, 1.1a has the enhanced version.

Comment 43

[Tracy Walker [:tracy]]

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]

Attachment: 1.0.x patch for 329521 and 329468

proposed patch for mfsa2006-34