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

RESOLVED FIXED in Firebird0.7

Status

()

Firefox
General
--
critical
RESOLVED FIXED
14 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: Blake Ross)

Tracking

unspecified
Firebird0.7
x86
Windows XP
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

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

Comment 1

14 years ago
Created attachment 130347 [details]
demo
(Reporter)

Comment 2

14 years ago
When you try the demo, the second alert, which says 
"[object nsXPCComponents_Classes]", shows that the script has chrome privs.
(Reporter)

Comment 3

14 years ago
Created attachment 130506 [details] [diff] [review]
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.
(Assignee)

Updated

14 years ago
Target Milestone: --- → Firebird0.7

Comment 4

14 years ago
who would be good reviewers?   let get this one in soon..
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...
Need a spot-fix for 1.5 final -- shouldn't we go with this bug's patch?

/be

Comment 7

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

Comment 8

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

Comment 9

14 years ago
Also, my patch would be correct even without this security bug.  See bug 187195,
"when unvisited link becomes visited, href attribute becomes absolute".
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.
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
I was thinking of something in c++, actually.  Possibly nsILink or
nsILinkHandler?  They are both currently unscriptable, though...
Is there anything that needs to happen other than to set the link state and
force a repaint?

Updated

14 years ago
QA Contact: asa
Fixed, branch and trunk, 0.7
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
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
Opening.
Group: security
Was there a reason not to just use an XPCWrapper here?  That would avoid the
issues with pages overriding getAttribute/setAttribute too....

Updated

12 years ago
Flags: testcase+

Updated

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