Status

()

Core
Security
--
critical
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Paul Nickerson, Assigned: Martijn Wargers (dead))

Tracking

(4 keywords)

1.8 Branch
mozilla1.8final
fixed-seamonkey1.0.2, fixed-seamonkey1.1a, fixed1.8.1, verified1.8.0.4
Points:
---
Dependency tree / graph
Bug Flags:
blocking-aviary1.0.9 ?
blocking1.8.0.4 +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
Created attachment 214199 [details]
Executes script at google.com

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

Comment 2

12 years ago
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.
(Reporter)

Comment 3

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

Comment 4

12 years ago
Yes, the spaces are stripped off.
(Assignee)

Comment 5

12 years ago
Comment on attachment 214258 [details] [diff] [review]
patch

Might as well ask review.
Attachment #214258 - Flags: review?(mconnor)
(Assignee)

Updated

12 years ago
Attachment #214258 - Attachment is obsolete: true
Attachment #214258 - Flags: review?(mconnor)
(Assignee)

Comment 6

12 years ago
Created attachment 214302 [details] [diff] [review]
patch2

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

Comment 8

11 years ago
Created attachment 214753 [details] [diff] [review]
xpfe patch

The point being that after the location changes we lose our context ;-)
Attachment #214753 - Flags: superreview?(bzbarsky)
Attachment #214753 - Flags: review?(jag)

Comment 9

11 years ago
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+
(Assignee)

Comment 11

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

Comment 12

11 years ago
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+
(Reporter)

Comment 13

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

Comment 14

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

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

Comment 16

11 years ago
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.

Updated

11 years ago
Attachment #214302 - Flags: review?(mconnor) → review+

Comment 17

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

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

Comment 19

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

11 years ago
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.
(Reporter)

Comment 21

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

Comment 22

11 years ago
(In reply to comment #21)
> ..., we could use the same fix as the
> forward/backward navigation script execution bug...
Which bug is that?

(Reporter)

Comment 23

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

11 years ago
Code wise I'm sure we can make the "right" thing happen, but what is the right thing from a UI point of view?
(Reporter)

Comment 25

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

Comment 26

11 years ago
(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?

(Assignee)

Comment 27

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

Updated

11 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 2 alpha2

Comment 28

11 years ago
Created attachment 215521 [details] [diff] [review]
xpfe enhancement
Attachment #215521 - Flags: superreview?(jag)
Attachment #215521 - Flags: review?(jag)

Comment 29

11 years ago
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 30

11 years ago
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)

Comment 31

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

Comment 32

11 years ago
(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 36

11 years ago
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 37

11 years ago
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+

Comment 38

11 years ago
xpfe fix checked in to the 1.8.0 branch.
(Assignee)

Comment 39

11 years ago
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
Last Resolved: 11 years ago
Keywords: fixed-seamonkey1.0.2, fixed1.8.0.4, fixed1.8.1
Resolution: --- → FIXED

Comment 42

11 years ago
Yes, 1.1a has the enhanced version.
Keywords: fixed-seamonkey1.1a
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
Keywords: fixed1.8.0.4 → verified1.8.0.4

Updated

11 years ago
Depends on: 332874
Group: security

Comment 44

11 years ago
Created attachment 225478 [details] [diff] [review]
1.0.x patch for 329521 and 329468

proposed patch for mfsa2006-34

Updated

11 years ago
Attachment #225478 - Flags: review?(caillon)
You need to log in before you can comment on or make changes to this bug.