Element embed pluginspage attribute allows javascript urls

RESOLVED FIXED

Status

()

Core
Plug-ins
--
critical
RESOLVED FIXED
13 years ago
11 years ago

People

(Reporter: Omar Khan, Assigned: jst)

Tracking

({fixed-aviary1.0.3})

Trunk
x86
Windows ME
fixed-aviary1.0.3
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b3 +
blocking-aviary1.5 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix] fixed-trunk)

Attachments

(5 attachments)

(Reporter)

Description

13 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.8b2) Gecko/20050331 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.8b2) Gecko/20050331 Firefox/1.0+

Element embed pluginspage attribute allows javascript urls, and somehow
somebody forgot to santize it.
So can execute arbitrary code.

Reproducible: Always

Steps to Reproduce:
1. Load page with an embed's pluginspage attribute set to javascript url
2. Click install missing plugins
3. Click Manual install

Actual Results:  
Arbitrary code executed

Expected Results:  
Do not execute arbitrary code.
Thank you.
(Reporter)

Comment 1

13 years ago
Created attachment 179237 [details]
Testcase

Saw bug bounty program rewarded today, so was a little bit too excited. Not
sure if dup or invalid bug.
Assignee: nobody → jst
Blocks: 256195
Severity: normal → critical
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [sg:fix]
(Assignee)

Comment 2

13 years ago
Created attachment 179337 [details] [diff] [review]
Do security check when opening URL for manual plugin installation
(Assignee)

Updated

13 years ago
Attachment #179337 - Flags: superreview?(brendan)
Attachment #179337 - Flags: review?(darin)

Comment 3

13 years ago
Comment on attachment 179337 [details] [diff] [review]
Do security check when opening URL for manual plugin installation

r=darin

maybe add a comment indicating why opener.content is being used.
Attachment #179337 - Flags: review?(darin) → review+
(Assignee)

Comment 4

13 years ago
Created attachment 179341 [details] [diff] [review]
Same as above w/o the content.open() change which isn't necessary to fix this.
(Assignee)

Comment 5

13 years ago
(In reply to comment #3)
> (From update of attachment 179337 [details] [diff] [review] [edit])
> r=darin
> 
> maybe add a comment indicating why opener.content is being used.
> 

Yeah, but on second thought I desided to not risk changing that now. I'll worry
about that later, so ignore that part of the first patch.
Comment on attachment 179341 [details] [diff] [review]
Same as above w/o the content.open() change which isn't necessary to fix this.

sr=me, noting darin's r=.

/be
Attachment #179341 - Flags: superreview+
Attachment #179341 - Flags: review+

Comment 7

13 years ago
Johnny, can you provide your testcases here?
Attachment #179341 - Flags: approval-aviary1.0.3+
Checked in to AVIARY_1_0_20050124_BRANCH, 2005-04-01 18:48 -0800.

Note:  this was *not* yet checked in to the trunk.  I'll let jst do that,
perhaps once we're ready to tolerate a little more exposure.
Keywords: fixed-aviary1.0.3
(Assignee)

Comment 9

13 years ago
Created attachment 179360 [details]
Testcase showing manual installation still works.

This is a testcase (based on the original testcase in this bug) that shows that
manual installation still works with non-evil pluginspage attribute values.
Load the testcase, click the missing plugin and click on the manual
installation URL and you should get a new window with www.mozilla.org loaded in
it. If you do, it still works.
Attachment #179360 - Attachment is patch: false
Attachment #179360 - Attachment mime type: text/plain → text/html

Comment 10

13 years ago
Verified fixed on Aviary branch - Mozilla/5.0 (Windows; U; Windows NT 5.1;
en-US; rv:1.7.7) Gecko/20050401 Firefox/1.0.3

Original testcase:  Manual plugin install no longer changes my homepage.
jst's testcase: Works as expected, opens mozilla.org in new window.
verified fixed 200504030x-1.0.3 on linux fc3 and mac os x. same results as in
comment 10: the first testcase doesn't reset my homepage, and jst's testcase
opens www.mozilla.org.
I just figured out another way to exploit this that isn't fix by the patch.  It
can be used to inject code into the xul:button's oncommand.
Created attachment 179724 [details]
testcase for code injection
I have a patch for this issue as well - do I need to file a new bug or not?
filed bug 289171 with a patch on the 2nd issue.

Updated

13 years ago
Blocks: 289187
Fix released
Group: security
Flags: blocking1.8b2+
Flags: blocking-aviary1.1+
Checked in to trunk, but leaving open since I suspect jst wanted to get the
difference between the two patches in on the trunk too.
I think for trunk we can do better and just use the window that contains the
missing plugin (the regular html dom window, not chrome window) to open the new
window, and thus skip the nsIScriptSecurityManager check.

I'll look into it next week.

Comment 19

13 years ago
Are we done with the "blocking1.8b2+" part of this fix? If so, can we move
remaining work to blocking 1.8b3+ and unset that flag or do we have more to do
here for b2?
I think this is fixed enough for 1.8b2, but leaving open for Johnny to do more
if he's still planning on it.
Whiteboard: [sg:fix] → [sg:fix] fixed-trunk
A cleaner way, without security manager checks would be:

nsPluginInstallerWizard.prototype.loadURL = function (aUrl){
  if (window.opener.getBrowser().mCurrentBrowser)
    window.opener.getBrowser().mCurrentBrowser.contentWindow.open(aUrl);
}

use the current browser's contentWinow to open the url.  This should be safe,
thought perhaps if the current browser is pointing at an chrome:// url this
could cause trouble.  Probably needs more testing.

Updated

13 years ago
Flags: blocking1.8b2+ → blocking1.8b3+
Please file a new bug on any remaining concerns. This bug as filed is fixed
everywhere.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Blocks: 256197
No longer blocks: 256195

Updated

12 years ago
Attachment #179337 - Flags: superreview?(brendan)

Updated

12 years ago
Flags: testcase+

Updated

11 years ago
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.