315004 - Malicious theme can cause arbitrary chrome scripting (theme homepage link in about theme dialog)

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Paul Nickerson]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1

Installing a theme with a malicious homepage url set in the install file can cause arbitrary scripting in the context of chrome with a little bit of user interation.

Reproducible: Always

Steps to Reproduce:
1.Navigate to http://www.greyhatsecurity.org/mozilla/themevuln/9u23ro8haslkdhf.htm and install the theme
2.Right click on the ThemeVuln entry in the themes list
3.Click About ThemeVuln
4.Click Visit Homepage on the dialog

Actual Results:  
An alert will display that says script has been executed from [url of chrome dialog]

Expected Results:  
Homepage urls should be filtered by protocol

This doesn't work when you just click Visit Homepage from the right click menu. The webpage will open, but the opener object won't exist. I don't know why this is, but I do suspect that the newly opened page has some higher rights than a normal javascript: page, perhaps allowing for cross site scripting, although I haven't been able to prove this yet.

Comment 1

[Daniel Veditz [:dveditz]]

This was regressed by Aaron's fix for bug 302402. Up until then the javascript would open in a new dialog with about:blank permissions (as it does from the Extension manager dialog itself).

The 1.0 behavior should be safe enough, but better not to run any script at all. In fact, better to whitelist this to just http(s). It's fine not to have a homepage, not fine to claim to have one and have it made up on the spot (javascript/data) or embedded in the extension itself (chrome).

A malicious extension can, of course, already do anything. But this code is shared with Themes which users expect to be well behaved.

Comment 2

[Daniel Veditz [:dveditz]]

(In reply to comment #1)
> open in a new dialog

new browser window, I mean.

Comment 3

[Scott MacGregor]

Dan, didn't we take a fix recently to make theme installation go through the white list for 1.5 to alleviate some of this?

Comment 4

[Scott MacGregor]

whoops, not sure how that happened.

Comment 5

[Daniel Veditz [:dveditz]]

(In reply to comment #3)
> Dan, didn't we take a fix recently to make theme installation go through the
> white list for 1.5 to alleviate some of this?

Yes, bug 304931. Whitelisting helps only if AMO reviewers are checking these URLs as part of the approval process and only if users aren't convinced to bypass it because "themes are safe".

We are talking about an uncommon action on an obscure dialog, but some people will click especially if there's a social engineering campaign going on.

There really isn't much risk here, but for the handful of people who would fall for it this is a critical code-execution exploit.

Comment 6

[Daniel Veditz [:dveditz]]

Attachment: only link to http/https homepages

We could simply back out Aaron's one-line patch, but that presumably reintroduces his accessibility thing and also still lets the URL execute script, although only in a safe context.

I prefer limiting the homepage schemes that we link to.

Comment 7

[Jesse Ruderman]

It would be nice to have a more general solution to holes where <label class="text-link"> gets its href from an untrusted source.  I filed bug 315166 for that.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Attachment: use nsURI

I noticed that the regexp check applied to attributes which can sometimes contain space padding. In this case it's not an issue, since that padding is removed when install.rdf is parsed, but I saw other possible ways of getting around this check, like using "http:|chrome://browser/content/browser.js" as a homepageURL. The only reason this fails is because of the check at:
http://lxr.mozilla.org/seamonkey/source/browser/base/content/browser.js#611

Seems like it's best to be safe and do the URI parsing up front rather than relying on other unrelated code that may change.

Mike Connor said r=him over IRC.

Comment 9

[Mike Schroepfer]

Comment on attachment 202057 [details] [diff] [review]
use nsURI

GIven the earlier approval of this bug - carrying the approval forward to the newly reviewed patch

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Attachment: use nsURI v2

Ugh, my previous patch didn't actually solve the issue I mentioned since it still used the original URI. This one works.

Comment 11

[Mike Connor [:mconnor]]

Comment on attachment 202060 [details] [diff] [review]
use nsURI v2

r=me, carrying over a=asa/schrep

Comment 12

[:Gavin Sharp [email: [e-mail]]]

"use nsURI v2" landed on the trunk and the 1.8 branch.

Comment 13

[Scott MacGregor]

belated blocking flag pixie dust.

Comment 14

[Bob Clary [:bc] (inactive)]

testcase-: greyhat is no more.

Comment 15

[Paul Nickerson]

I was never payed for this bug. Was it ever considered for the bounty program?

Paul