Closed
Bug 329521
Opened 19 years ago
Closed 19 years ago
View Image xss
Categories
(Core :: Security, defect)
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)
220 bytes,
text/html
|
Details | |
2.35 KB,
patch
|
mconnor
:
approval-branch-1.8.1+
mconnor
:
approval1.8.0.4+
|
Details | Diff | Splinter Review |
708 bytes,
patch
|
bzbarsky
:
superreview+
neil
:
approval-aviary1.0.9?
Biesinger
:
approval-branch-1.8.1+
dveditz
:
approval1.8.0.4+
|
Details | Diff | Splinter Review |
1.19 KB,
patch
|
jag+mozilla
:
superreview+
neil
:
approval1.7.14?
Biesinger
:
approval-branch-1.8.1+
darin.moz
:
approval1.8.0.4+
|
Details | Diff | Splinter Review |
6.44 KB,
patch
|
Details | Diff | Splinter Review |
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•19 years ago
|
||
Added a testcase. Right click on the image and press "View Image"
Assignee | ||
Comment 2•19 years ago
|
||
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•19 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•19 years ago
|
||
Yes, the spaces are stripped off.
Assignee | ||
Comment 5•19 years ago
|
||
Comment on attachment 214258 [details] [diff] [review]
patch
Might as well ask review.
Attachment #214258 -
Flags: review?(mconnor)
Assignee | ||
Updated•19 years ago
|
Attachment #214258 -
Attachment is obsolete: true
Attachment #214258 -
Flags: review?(mconnor)
Assignee | ||
Comment 6•19 years ago
|
||
Better patch.
From the surrounding code, I get the feeling no exception should be thrown.
Attachment #214302 -
Flags: review?(mconnor)
Updated•19 years ago
|
Assignee: nobody → martijn.martijn
Comment 7•19 years ago
|
||
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•19 years ago
|
||
The point being that after the location changes we lose our context ;-)
Attachment #214753 -
Flags: superreview?(bzbarsky)
Attachment #214753 -
Flags: review?(jag)
Comment 9•19 years ago
|
||
Comment on attachment 214753 [details] [diff] [review]
xpfe patch
I like this approach much better.
Attachment #214753 -
Flags: review?(jag) → review+
![]() |
||
Comment 10•19 years ago
|
||
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•19 years ago
|
||
Hmm, so that means the context menu disappears when the url changes, I deliberately tried to avoid that.
Comment 12•19 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?
Updated•19 years ago
|
Attachment #214753 -
Flags: approval-branch-1.8.1?(cbiesinger) → approval-branch-1.8.1+
Reporter | ||
Comment 13•19 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•19 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•19 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•19 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•19 years ago
|
Attachment #214302 -
Flags: review?(mconnor) → review+
Comment 17•19 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•19 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•19 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•19 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•19 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•19 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•19 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•19 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•19 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•19 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•19 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•19 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 2 alpha2
Comment 28•19 years ago
|
||
Attachment #215521 -
Flags: superreview?(jag)
Attachment #215521 -
Flags: review?(jag)
Comment 29•19 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•19 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•19 years ago
|
||
If gContextMenu has a target that's not in content, something's rather wrong.
Comment 32•19 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.
Updated•19 years ago
|
Attachment #215521 -
Flags: approval-branch-1.8.1?(cbiesinger) → approval-branch-1.8.1+
Updated•19 years ago
|
Flags: blocking1.8.0.3? → blocking1.8.0.3+
Comment 33•19 years ago
|
||
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 34•19 years ago
|
||
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+
Updated•19 years ago
|
Component: General → Security
Flags: review+
Flags: blocking-firefox2+
Product: Firefox → Core
Target Milestone: Firefox 2 alpha2 → ---
Version: unspecified → Trunk
Comment 35•19 years ago
|
||
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•19 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•19 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•19 years ago
|
||
xpfe fix checked in to the 1.8.0 branch.
Assignee | ||
Comment 39•19 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
Comment 40•19 years ago
|
||
"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
Comment 41•19 years ago
|
||
(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: 19 years ago
Resolution: --- → FIXED
Comment 43•19 years ago
|
||
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•19 years ago
|
Group: security
Comment 44•19 years ago
|
||
proposed patch for mfsa2006-34
Updated•19 years ago
|
Attachment #225478 -
Flags: review?(caillon)
You need to log in
before you can comment on or make changes to this bug.
Description
•