217195 - security hole in markLinkVisited (exploit with link.href setter = eval)

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Jesse Ruderman]

Impact: full compromise.

Interaction required: The user must open a link from the exploit page in a new
window or new tab.

Vulnerable code: 

// from markLinkVisited in contentAreaUtils.js
var oldHref = linkNode.href;
linkNode.href = "";
linkNode.href = oldHref;

Exploit: Set a link node's |href setter| to the built-in function |eval|.  This
makes the third line pasted from markLinkVisited call eval with oldHref as the
parameter.

See also bug 187195, that line should be linkNode.setAttribute("href", oldHref).

Comment 1

[Jesse Ruderman]

Attachment: demo

Comment 2

[Jesse Ruderman]

When you try the demo, the second alert, which says 
"[object nsXPCComponents_Classes]", shows that the script has chrome privs.

Comment 3

[Jesse Ruderman]

Attachment: patch

* Fixes this security hole.
* Fixes a minor security hole where the attacker makes chrome convert
linkNode.href to a string for you (similar to bug 202994).
* Fixes bug 187195.

Comment 4

[chris hofmann]

who would be good reviewers?   let get this one in soon..

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

Rather than do this, let's land the patch for bug 169000 and do this the right
way.  I had been waiting to land that until something came up which could use
it, and this definitely does.  We can use the API I wrote up there to get the
real getter method here.  The problem with this patch his that setAttribute and
getAttribute can also be over-ridden...

Comment 6

[Brendan Eich [:brendan]]

Need a spot-fix for 1.5 final -- shouldn't we go with this bug's patch?

/be

Comment 7

[Darin Fisher]

brendan: from caillon's comment, it sounds like this spot fix is not a valid
fix.  the problem just gets shifted.

Comment 8

[Jesse Ruderman]

> The problem with this patch his that setAttribute and
> getAttribute can also be over-ridden...

I don't think that's exploitable, at least not in the same way.  If a page
replaces setAttribute with eval, chrome evals the string "href", which evaluates
to a string without doing any real computation.

Comment 9

[Jesse Ruderman]

Also, my patch would be correct even without this security bug.  See bug 187195,
"when unvisited link becomes visited, href attribute becomes absolute".

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

Brendan, this is a firebird-only bug.  Is there an imminent firebird release we
need this for?

The real fix IMO would be to get rid of the weird hack we're doing with setting
the href to "" and then back to its original value just to get it to pick up a
visited state.  What we really need is a "setAsVisited()" or similar method on
somewhere which does the right thing here.

Comment 11

[Brendan Eich [:brendan]]

Yes, Firebird 0.7 is coming up fast, coincident with Mozilla 1.5.

Cleaning up the hack with a method sounds good, but how will the method be
implemented?  Probably in XBL, via the hack ;-).

/be

Comment 12

[Christopher Aillon (sabbatical, not receiving bugmail)]

I was thinking of something in c++, actually.  Possibly nsILink or
nsILinkHandler?  They are both currently unscriptable, though...

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

Is there anything that needs to happen other than to set the link state and
force a repaint?

Comment 14

[Ben Goodger (use ben at mozilla dot org for email)]

Fixed, branch and trunk, 0.7

Comment 15

[Brendan Eich [:brendan]]

I think we need a general solution whereby native methods such as eval cannot be
set as thunks to be called from chrome, to run arbitrary input with chrome
privs.  I've opened bug 223041 on this problem.

/be

Comment 16

[Christopher Aillon (sabbatical, not receiving bugmail)]

Opening.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Was there a reason not to just use an XPCWrapper here?  That would avoid the
issues with pages overriding getAttribute/setAttribute too....