249332 - Bypassing CheckLoadURI using custom getters and changing toString returns

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Jesse Ruderman]

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]

Attachment: demo

This demo is fragile, but it works in Mozilla and Firefox.

Comment 2

[chris hofmann]

think we can get a fix for this in 1.0?

Comment 3

[Johnny Stenback (:jst)]

Attachment: 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)]

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]]

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)]

(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)]

Attachment: 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]]

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)]

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)]

(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)]

Attachment: Final branch patch for checkin.

Comment 12

[Asa Dotzler [:asa]]

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)]

Fixed on trunk and branches.

Comment 14

[Daniel Veditz [:dveditz]]

Security Advisories published, clearing confidential flag