Last Comment Bug 217195 - security hole in markLinkVisited (exploit with link.href setter = eval)
: security hole in markLinkVisited (exploit with link.href setter = eval)
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86 Windows XP
: -- critical (vote)
: Firebird0.7
Assigned To: Blake Ross
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2003-08-24 21:27 PDT by Jesse Ruderman
Modified: 2011-08-05 23:34 PDT (History)
15 users (show)
bob: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
demo (267 bytes, text/html)
2003-08-24 21:27 PDT, Jesse Ruderman
no flags Details
patch (1.12 KB, patch)
2003-08-27 23:07 PDT, Jesse Ruderman
no flags Details | Diff | Splinter Review

Description Jesse Ruderman 2003-08-24 21:27:03 PDT
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 2003-08-24 21:27:32 PDT
Created attachment 130347 [details]
demo
Comment 2 Jesse Ruderman 2003-08-24 21:32:21 PDT
When you try the demo, the second alert, which says 
"[object nsXPCComponents_Classes]", shows that the script has chrome privs.
Comment 3 Jesse Ruderman 2003-08-27 23:07:35 PDT
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.
Comment 4 chris hofmann 2003-08-28 09:35:57 PDT
who would be good reviewers?   let get this one in soon..
Comment 5 Christopher Aillon (sabbatical, not receiving bugmail) 2003-08-28 14:49:36 PDT
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] 2003-08-28 15:04:54 PDT
Need a spot-fix for 1.5 final -- shouldn't we go with this bug's patch?

/be
Comment 7 Darin Fisher 2003-08-28 17:47:35 PDT
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 2003-08-28 18:21:40 PDT
> 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 2003-08-28 19:24:20 PDT
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) 2003-08-28 23:01:53 PDT
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] 2003-08-28 23:25:29 PDT
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) 2003-08-28 23:40:51 PDT
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) 2003-08-28 23:42:58 PDT
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) 2003-09-23 12:57:56 PDT
Fixed, branch and trunk, 0.7
Comment 15 Brendan Eich [:brendan] 2003-10-20 21:41:24 PDT
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) 2003-11-24 07:30:11 PST
Opening.
Comment 17 Boris Zbarsky [:bz] 2004-02-17 00:19:20 PST
Was there a reason not to just use an XPCWrapper here?  That would avoid the
issues with pages overriding getAttribute/setAttribute too....

Note You need to log in before you can comment on or make changes to this bug.