Last Comment Bug 249332 - Bypassing CheckLoadURI using custom getters and changing toString returns
: Bypassing CheckLoadURI using custom getters and changing toString returns
Status: RESOLVED FIXED
[sg:fix] have patch - need reviews ca...
: fixed-aviary1.0, fixed1.4.4, fixed1.7.5, testcase
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Daniel Veditz [:dveditz]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2004-06-30 20:13 PDT by Jesse Ruderman
Modified: 2005-01-26 04:03 PST (History)
6 users (show)
asa: blocking1.7.5+
chofmann: blocking‑aviary1.0+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
demo (379 bytes, text/html)
2004-06-30 20:15 PDT, Jesse Ruderman
no flags Details
Use XPCNativeWrapper() to ensure we're getting the right href. (8.76 KB, patch)
2004-10-15 12:55 PDT, Johnny Stenback (:jst, jst@mozilla.com)
caillon: review-
Details | Diff | Splinter Review
Fixing caillon's issues, and fixing SeaMonkey too (11.77 KB, patch)
2004-10-15 14:04 PDT, Johnny Stenback (:jst, jst@mozilla.com)
caillon: review+
dveditz: superreview+
asa: approval‑aviary+
asa: approval1.7.5+
Details | Diff | Splinter Review
Final branch patch for checkin. (12.21 KB, patch)
2004-10-18 10:56 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review

Description Jesse Ruderman 2004-06-30 20:13:55 PDT
A page can make a link that, when middle-clicked, opens a chrome URL in a new tab.  

Chrome JavaScript first sends link.href to CheckLoadURI, then uses it to open a
tab.  In the exploit, link.href is an object with a toString function that
returns different values depending on how many times it has been called.

The script uses a custom href getter to make the chrome JavaScript see link.href
as an object.  link.href can't *be* an object because there is a standard href
setter for link elements.
Comment 1 Jesse Ruderman 2004-06-30 20:15:08 PDT
Created attachment 152083 [details]
demo

This demo is fragile, but it works in Mozilla and Firefox.
Comment 2 chris hofmann 2004-09-28 19:46:42 PDT
think we can get a fix for this in 1.0?
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2004-10-15 12:55:46 PDT
Created attachment 162225 [details] [diff] [review]
Use XPCNativeWrapper() to ensure we're getting the right href.

This fixes this bug, and probably others, but who knows if I caught all of
them. There's a lot of JS code here, and probably elsewhere too, that may very
well suffer from this same problem :(
Comment 4 Christopher Aillon (sabbatical, not receiving bugmail) 2004-10-15 13:20:38 PDT
Comment on attachment 162225 [details] [diff] [review]
Use XPCNativeWrapper() to ensure we're getting the right href.

>     // Generate fully-qualified URL for clicked-on link.
>     linkURL : function () {
>-        if (this.link.href) {
>-          return this.link.href;
>+        var href = new XPCNativeWrapper(this.link, "href").href;
>+        if (href) {
>+          return href;
>         }
>-        var href = this.link.getAttributeNS("http://www.w3.org/1999/xlink","href");
>+        href = this.link.getAttributeNS("http://www.w3.org/1999/xlink","href");

If you were worred about this.link.ref, we also need to be worried about
this.link.getAttributeNS() since that can be overriden just as easily.

>         if (!href || !href.match(/\S/)) {
>           throw "Empty href"; // Without this we try to save as the current doc, for example, HTML case also throws if empty
>         }
>         href = makeURLAbsolute(this.link.baseURI,href);

and this.link.baseURI.

>         return href;
>     },
>     // Get text of link.
>     linkText : function () {

I think its probably best to make this function call linkURL() if we can't get
any attribute text (though we may also want to add wrappers for the
getAttribute() calls, since they can be spoofed as well -- just not exploited).

>         var text = gatherTextUnder( this.link );
>         if (!text || !text.match(/\S/)) {
>           text = this.link.getAttribute("title");
>           if (!text || !text.match(/\S/)) {
>             text = this.link.getAttribute("alt");
>             if (!text || !text.match(/\S/)) {
>-              if (this.link.href) {                
>+              if (this.link.href) {
>                 text = this.link.href;
>               } else {
>                 text = getAttributeNS("http://www.w3.org/1999/xlink", "href");
>                 if (text && text.match(/\S/)) {
>                   text = makeURLAbsolute(this.link.baseURI, text);
>                 }
>               }
>             }
>           }
>@@ -4925,61 +4927,68 @@ function asyncOpenWebPanel(event)
>        // hack.
>        target = linkNode.getAttribute("target");
>        if (fieldNormalClicks && 
>            (!target || target == "_content" || target  == "_main")) 
>          // IE uses _main, SeaMonkey uses _content, we support both
>        {
>          if (!linkNode.href) return true;

Probably want to use linkNode.href up here too.

>          if (linkNode.getAttribute("onclick")) return true;

And maybe wrap getAttribute

>          var postData = { };
>-         var url = getShortcutOrURI(linkNode.href, postData);
>+         var href = new XPCNativeWrapper(linkNode, "href").href;
>+         var url = getShortcutOrURI(href, postData);
>          if (!url)
>            return true;
>-         markLinkVisited(linkNode.href, linkNode);
>+         markLinkVisited(href, linkNode);
>          loadURI(url, null, postData.value);
>          event.preventDefault();
>          return false;
>        }
>        else if (linkNode.getAttribute("rel") == "sidebar") {
>          // This is the Opera convention for a special link that - when clicked - allows
>          // you to add a sidebar panel.  We support the Opera convention here.  The link's
>          // title attribute contains the title that should be used for the sidebar panel.
>+         var href = new XPCNativeWrapper(linkNode, "href").href;
>          openDialog("chrome://browser/content/bookmarks/addBookmark2.xul", "",
>                     "centerscreen,chrome,dialog,resizable,dependent",
>                     linkNode.getAttribute("title"), 

Might as well wrap getAttribute while we're at it here, too.

>-                    linkNode.href, null, null, null, null, true);
>+                    href, null, null, null, null, true);
>          event.preventDefault();
>          return false;
>        }
>        else if (target == "_search") {
>          // Used in WinIE as a way of transiently loading pages in a sidebar.  We
>          // mimic that WinIE functionality here and also load the page transiently.
>-         openWebPanel(gNavigatorBundle.getString("webPanels"), linkNode.href);
>+         var href = new XPCNativeWrapper(linkNode, "href").href;
>+         openWebPanel(gNavigatorBundle.getString("webPanels"), href);
>          event.preventDefault();
>          return false;
>        }

Maybe create the wrapper unconditionally before the if since all three branches
above use it, at least for title?  2 of 3 use the getAttribute so maybe just
use it unconditionally anyway.

>      }
>-     else
>-       handleLinkClick(event, linkNode.href, linkNode);
>+     else {
>+       var href = new XPCNativeWrapper(linkNode, "href").href;
>+       handleLinkClick(event, href, linkNode);
>+     }
>+
>      return true;
>    } else {
>      // Try simple XLink
>      var href;
>      linkNode = target;
>      while (linkNode) {
>        if (linkNode.nodeType == Node.ELEMENT_NODE) {
>          href = linkNode.getAttributeNS("http://www.w3.org/1999/xlink", "href");

Need to wrap getAttributeNS() too

>          break;
>        }
>        linkNode = linkNode.parentNode;

And I think linkNode.parentNode, else someone could return a random object with
a bogus getAttributeNS.  Yes, this whole thing sucks.  We really need to have
something in XPConnect to handle this shit better, i.e. if in chrome reading
content, it should call native methods when we have them.
Comment 5 Daniel Veditz [:dveditz] 2004-10-15 13:29:30 PDT
This doesn't fix Mozilla though. That one might be in contentAreaClick.js, let
me try it.
http://lxr.mozilla.org/mozilla/source/xpfe/communicator/resources/content/contentAreaClick.js#153
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2004-10-15 13:56:11 PDT
(In reply to comment #4)
> >     // Get text of link.
> >     linkText : function () {
> 
> I think its probably best to make this function call linkURL() if we can't get
> any attribute text (though we may also want to add wrappers for the
> getAttribute() calls, since they can be spoofed as well -- just not exploited).

The link text can be set by the the page to anything it wants, so nothing to
protect against there.

> >          if (linkNode.getAttribute("onclick")) return true;
> 
> And maybe wrap getAttribute

That code is so bogus that I didn't bother :)

> And I think linkNode.parentNode, else someone could return a random object with

that's fine, as long as we follow the link we get the href for here, which I
believe we do.

> a bogus getAttributeNS.  Yes, this whole thing sucks.  We really need to have
> something in XPConnect to handle this shit better, i.e. if in chrome reading
> content, it should call native methods when we have them.

Yah. But not for 1.0.

Fixed the rest, I think.
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2004-10-15 14:04:19 PDT
Created attachment 162235 [details] [diff] [review]
Fixing caillon's issues, and fixing SeaMonkey too

Dan, I tested the same change in the code you pointed at, and it does fix the
problem.
Comment 8 Daniel Veditz [:dveditz] 2004-10-17 06:43:22 PDT
Comment on attachment 162235 [details] [diff] [review]
Fixing caillon's issues, and fixing SeaMonkey too

sr=dveditz
Comment 9 Christopher Aillon (sabbatical, not receiving bugmail) 2004-10-18 10:42:15 PDT
Comment on attachment 162235 [details] [diff] [review]
Fixing caillon's issues, and fixing SeaMonkey too

(In reply to comment #6)
> (In reply to comment #4)
> > >     // Get text of link.
> > >     linkText : function () {
> > 
> > I think its probably best to make this function call linkURL() if we can't get
> > any attribute text (though we may also want to add wrappers for the
> > getAttribute() calls, since they can be spoofed as well -- just not exploited).
> 
> The link text can be set by the the page to anything it wants, so nothing to
> protect against there.


Agreed, I noted there wasn't an exploit, but it would be good to use the same
code path when they both try and do the same thing.


The rest looks cool, though I have a small concern that all the places are
caught in the seamonkey side (there is considerable more firefox patch than
seamonkey patch).  But I'll trust you here.

r=caillon
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2004-10-18 10:51:35 PDT
(In reply to comment #9)
> > The link text can be set by the the page to anything it wants, so nothing to
> > protect against there.
> 
> 
> Agreed, I noted there wasn't an exploit, but it would be good to use the same
> code path when they both try and do the same thing.
> 

Yeah, fair enough, I changed that code to use a wrapper as well, and fixed a bug
where it was getting an xlink attribute of itself in stead of the link :)

Thanks for the reviews!
Comment 11 Johnny Stenback (:jst, jst@mozilla.com) 2004-10-18 10:56:48 PDT
Created attachment 162491 [details] [diff] [review]
Final branch patch for checkin.
Comment 12 Asa Dotzler [:asa] 2004-10-18 10:59:58 PDT
Comment on attachment 162235 [details] [diff] [review]
Fixing caillon's issues, and fixing SeaMonkey too

a=asa for branch checkins.
Comment 13 Johnny Stenback (:jst, jst@mozilla.com) 2004-10-18 12:02:38 PDT
Fixed on trunk and branches.
Comment 14 Daniel Veditz [:dveditz] 2005-01-24 13:41:28 PST
Security Advisories published, clearing confidential flag

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