Closed Bug 217195 Opened 21 years ago Closed 21 years ago

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


(Firefox :: General, defect)

Windows XP
Not set





(Reporter: jruderman, Assigned: bugzilla)



(2 files)

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

See also bug 187195, that line should be linkNode.setAttribute("href", oldHref).
Attached file demo
When you try the demo, the second alert, which says 
"[object nsXPCComponents_Classes]", shows that the script has chrome privs.
Attached patch patchSplinter Review
* 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.
Target Milestone: --- → Firebird0.7
who would be good reviewers?   let get this one in soon..
Comment on attachment 130506 [details] [diff] [review]

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?

brendan: from caillon's comment, it sounds like this spot fix is not a valid
fix.  the problem just gets shifted.
> 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.
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 ;-).

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?
QA Contact: asa
Fixed, branch and trunk, 0.7
Closed: 21 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.

Was there a reason not to just use an XPCWrapper here?  That would avoid the
issues with pages overriding getAttribute/setAttribute too....
Flags: testcase+
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.