Last Comment Bug 298892 - Node spoofing (using e.g. XHTML) to bypass security checks
: Node spoofing (using e.g. XHTML) to bypass security checks
Status: RESOLVED FIXED
[sg:fix] ready for landing
: fixed-aviary1.0.5, fixed1.7.9
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
:
Mentors:
Depends on: 290420 299901 301873
Blocks:
  Show dependency treegraph
 
Reported: 2005-06-27 05:12 PDT by moz_bug_r_a4
Modified: 2007-04-01 15:11 PDT (History)
10 users (show)
dveditz: blocking1.7.9+
dveditz: blocking‑aviary1.0.5+
benjamin: blocking1.8b3+
bob: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Loading chrome via viewImage() (1.81 KB, application/xml)
2005-06-27 05:17 PDT, moz_bug_r_a4
no flags Details
Arbitrary code execution via setWallpaper() (2.03 KB, application/xml)
2005-06-27 05:19 PDT, moz_bug_r_a4
no flags Details
Check instanceof rather than spoofable localName (21.79 KB, patch)
2005-06-29 11:15 PDT, Daniel Veditz [:dveditz]
mconnor: review+
neil: superreview+
jaymoz: approval‑aviary1.0.5+
jaymoz: approval1.7.9+
Details | Diff | Splinter Review
fix syntax error (1.23 KB, patch)
2005-06-30 10:54 PDT, Christian :Biesinger (don't email me, ping me on IRC)
neil: review+
neil: superreview+
Details | Diff | Splinter Review
Remove findParentNode (xpfe version) (2.17 KB, patch)
2005-07-03 03:09 PDT, neil@parkwaycc.co.uk
timeless: review+
dveditz: superreview+
dveditz: approval1.8b3+
Details | Diff | Splinter Review
Branches patch (5.83 KB, patch)
2005-07-05 15:56 PDT, neil@parkwaycc.co.uk
dveditz: review+
dveditz: superreview+
dveditz: approval‑aviary1.0.5+
dveditz: approval1.7.9+
Details | Diff | Splinter Review

Description moz_bug_r_a4 2005-06-27 05:12:08 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.9) Gecko/20050626 Firefox/1.0.5

According to Bonsai, some url-checks of image.src have been added for bug 292774
and bug 292737 (I cannot see bug 292774 and bug 292737).  But, those can be
bypassed.

The following url-check has been added for bug 292774.

    viewImage : function (e) {
        urlSecurityCheck( this.imageURL, document )
        openUILink( this.imageURL, e );
    },

The following url-checks have been added for bug 292737.

 browser.js:
    else if (makeURL(this.target.src).scheme == "javascript")
        disableSetWallpaper = true;

 setWallpaper.xul:
    if (makeURL(window.arguments[0].src).scheme == "javascript")
      return false;


Vulnerable code from nsContextMenu.prototype.setTarget:

        if ( this.target.nodeType == Node.ELEMENT_NODE ) {
             if ( this.target.localName.toUpperCase() == "IMG" ) {
                this.onImage = true;
                this.imageURL = this.target.src;

Exploit:

1) In an XHTML document, since tags are case-sensitive, <IMG> element is not a
HTMLImageElement, it is a HTMLUnknownElement that does not have |src| property.
thus, content can make chrome access content-defined getter function of |src|
property of <IMG> element.

2) The approach for bypassing url-check is the same as Bug 249332 (Bypassing
CheckLoadURI using custom getters and changing toString returns).


Note: The trunk is not affected thanks to the fix for Bug 281988.


Reproducible: Always

Steps to Reproduce:
Comment 1 moz_bug_r_a4 2005-06-27 05:17:19 PDT
Created attachment 187391 [details]
Loading chrome via viewImage()

I've confirmed that this works on:
Firefox 1.0.5 2005-06-26-05
Mozilla 1.7.9 2005-06-24-08
Comment 2 moz_bug_r_a4 2005-06-27 05:19:44 PDT
Created attachment 187392 [details]
Arbitrary code execution via setWallpaper()

I've confirmed that this works on:
Firefox 1.0.5 2005-06-26-05
Comment 3 Daniel Veditz [:dveditz] 2005-06-27 13:51:33 PDT
This is more or less bug 290324 for Firefox itself, except using .xhtml instead
of creating dynamic XUL nodes.

http://lxr.mozilla.org/aviary101branch/search?string=localName.

Most or all of those places would be safer doing an instanceof check instead.
Patch coming up
Comment 4 Chase Phillips 2005-06-27 15:46:53 PDT
Maybe another day or two to test and make sure we're ready to take a fix for this.
Comment 5 Chase Phillips 2005-06-27 15:47:22 PDT
jst says, "remember to look for nodeName"
Comment 6 Daniel Veditz [:dveditz] 2005-06-27 18:04:13 PDT
the patch in bug 290420 covers the same issues in browser.js, but misses the
actual contextMenu.js spot.

Some of these are fixed on the trunk. This xhtml <IMG> example in particular is
changed to use instanceof
Comment 7 Benjamin Smedberg [:bsmedberg] 2005-06-28 10:48:47 PDT
What exactly is left to do on trunk for this bug?
Comment 8 Daniel Veditz [:dveditz] 2005-06-29 11:15:47 PDT
Created attachment 187685 [details] [diff] [review]
Check instanceof rather than spoofable localName
Comment 9 neil@parkwaycc.co.uk 2005-06-29 13:44:03 PDT
Comment on attachment 187685 [details] [diff] [review]
Check instanceof rather than spoofable localName

Mostly ok, but if you've got time for a few tweaks then keep reading...

>                             if ( area.nodeType == Node.ELEMENT_NODE
>                                  &&
>-                                 area.localName.toUpperCase() == "AREA" ) {
>+                                 area instanceof HTMLAreaElement ) {
( area instanceof HTMLAreaElement ) implies area is an element node.

>+                    if ( ( elem instanceof HTMLQuoteElement && 'cite' in elem && elem.cite)  ||
>+                         ( elem instanceof HTMLTableElement && 'summary' in elem && elem.summary) ||
( elem instanceof HTMLQuoteElement ) implies 'cite' in elem (although not
elem.cite of course). Same goes for table summary.

>+                         ( ( elem instanceof HTMLInsElement || elem instanceof HTMLDelElement ) &&
>                            ( ( 'cite' in elem && elem.cite ) ||
>                              ( 'dateTime' in elem && elem.dateTime ) ) )               ||
>                          ( 'title' in elem && elem.title )                             
I discovered that we can use ( elem instanceof HTMLModElement ) which covers
both INS and DEL. Again, the 'cite' and 'dateTime' in elem are implied. ( elem
instanceof HTMLElement ) could be used instead of 'title' in elem.

>+      if (node instanceof HTMLInputElement)
>+        return (node.type == "text" || node.type =="password")
>+      else
>+        return (node instanceof HTMLTextAreaElement);
I like this fix but I would like it more if you added a space between == and
"password" and more still if you fixed the other two files in the same way ;-)

>+      if (e.type.toLowerCase() == "text" || e.type.toLowerCase() == "hidden" ||
>+          e instanceof HTMLTextAreaElement)
I don't think this is spoofable because we already know this is a form element.
That said, you could probably check e.type everywhere.
Comment 10 Jay Patel [:jay] 2005-06-29 14:14:10 PDT
Comment on attachment 187685 [details] [diff] [review]
Check instanceof rather than spoofable localName

Let's get this in on the branches.  Leaving it up to dveditz to decide whether
or not we want the tweaks. a=jay
Comment 11 Daniel Veditz [:dveditz] 2005-06-29 14:32:16 PDT
we do want the tweaks, waste of Neil's time if we don't use his effort to get
better.
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-06-29 17:06:47 PDT
Comment on attachment 187685 [details] [diff] [review]
Check instanceof rather than spoofable localName

>-      if (node.localName.toUpperCase() == "INPUT") {
>-        var attrib = "";
>-        var type = node.getAttribute("type");
>-
>-        if (type)
>-          attrib = type.toUpperCase();
>-
>-        return( (attrib != "IMAGE") &&
>-                (attrib != "CHECKBOX") &&
>-                (attrib != "RADIO") &&
>-                (attrib != "SUBMIT") &&
>-                (attrib != "RESET") &&
>-                (attrib != "FILE") &&
>-                (attrib != "HIDDEN") &&
>-                (attrib != "RESET") &&
>-                (attrib != "BUTTON") );
>-      } else  {
>-        return(node.localName.toUpperCase() == "TEXTAREA");
>-      }
>+      if (node instanceof HTMLInputElement)
>+        return (node.type == "text" || node.type =="password")
>+      else
>+        return (node instanceof HTMLTextAreaElement);

There's a reason that the old code here was checking that the type is not one
of a long list:  inputs with an unrecognized type attribute are treated as
type="text".  This behavior is important for future compatibility.  Any chance
this could be fixed?
Comment 13 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-06-29 17:14:18 PDT
Er, actually, never mind.  Now I see that the .type property (but not
getAttribute()) is normalized to "text" in those cases.
Comment 14 Daniel Veditz [:dveditz] 2005-06-29 19:08:23 PDT
Fixed on trunk and branches
Comment 15 Christian :Biesinger (don't email me, ping me on IRC) 2005-06-30 10:34:42 PDT
guys, this broke seamonkey's context menu due to a syntax error
Error: missing ) after condition
Source File: chrome://communicator/content/nsContextMenu.js
Line: 398, Column: 25
Source Code:
                         elem.getAttributeNS(XMLNS, "lang") ) {
Comment 16 Christian :Biesinger (don't email me, ping me on IRC) 2005-06-30 10:42:16 PDT
it would've been nice if the patch that was actaully checked in had been
attached here...
Comment 17 Christian :Biesinger (don't email me, ping me on IRC) 2005-06-30 10:54:54 PDT
Created attachment 187837 [details] [diff] [review]
fix syntax error
Comment 18 Daniel Veditz [:dveditz] 2005-06-30 13:02:48 PDT
Sorry my earlier sr= submit didn't appear to go through, but looks like neil
covered it anyway.

I double-checked what was checked in and this affects only trunk suite. The
initial patch was developed for the branches and xpfe moved things around a bit
on trunk, and I dropped the || during the conflict merge (but did remember to
add it in the browser.js version).

The thunderbird version mail/base/content/nsContextMenu.js didn't get the added
"|| elem.getAttributeNS(XMLNS, "lang")" from bug 207274 -- is that going to
matter? I guess I'll comment over there.
Comment 19 Daniel Veditz [:dveditz] 2005-06-30 13:14:47 PDT
checked in biesi's fix.
Comment 20 moz_bug_r_a4 2005-07-03 01:50:04 PDT
findParentNode() is remaining.

Vulnerable code from contentAreaClick():
  linkNode = findParentNode(event.originalTarget, "a");

On the trunk, this can be combined with other bug to execute arbitrary code.
Comment 21 moz_bug_r_a4 2005-07-03 02:27:28 PDT
attachment 188087 [details] in Bug 299520 comment 1
A exploit testcase that is using xhtml <A>.
Comment 22 neil@parkwaycc.co.uk 2005-07-03 03:09:50 PDT
Created attachment 188089 [details] [diff] [review]
Remove findParentNode (xpfe version)

I changed the useful invocation of findParentNode into a while loop.

The other use is unnecessary because scrollbar.xml blocks clicks on scrollbars.
Comment 23 Daniel Veditz [:dveditz] 2005-07-05 11:48:10 PDT
Comment on attachment 188089 [details] [diff] [review]
Remove findParentNode (xpfe version)

sr=dveditz
This is Suite only, need mozilla/mail and mozilla/browser versions (yay
forking!)
Comment 24 Jay Patel [:jay] 2005-07-05 13:47:34 PDT
Can anyone jump in here and get patches ready for landing on the Aviary branch
for Firefox and Thunderbird?
Comment 25 Jay Patel [:jay] 2005-07-05 15:13:34 PDT
Reassigning to jst to patch up browser and mail.  Dveditz can patch the 1.7
branch when those patches are ready.
Comment 26 neil@parkwaycc.co.uk 2005-07-05 15:56:13 PDT
Created attachment 188372 [details] [diff] [review]
Branches patch

I just removed all references to findParentNode and replaced them with
something that I think should be equivalent although I haven't even checked my
typing.
Comment 27 Daniel Veditz [:dveditz] 2005-07-05 16:19:59 PDT
Comment on attachment 188372 [details] [diff] [review]
Branches patch

r/sr=dveditz for the branch/FE ports
Comment 28 Jay Patel [:jay] 2005-07-05 16:23:01 PDT
Let's get those patches checked in the appropriate places asap. a=jay again. :-)
Comment 29 Jay Patel [:jay] 2005-07-05 16:25:34 PDT
Dveditz: Do we need to get this on the Trunk as well?  If so, can you a= for
1.8b3 and get it checked in there as well?  
Comment 30 Daniel Veditz [:dveditz] 2005-07-06 03:02:58 PDT
Comment on attachment 188089 [details] [diff] [review]
Remove findParentNode (xpfe version)

a=dveditz
Comment 31 Daniel Veditz [:dveditz] 2005-07-06 03:03:42 PDT
Comment on attachment 188372 [details] [diff] [review]
Branches patch

a=dveditz for branches
Comment 32 Daniel Veditz [:dveditz] 2005-07-06 03:38:59 PDT
Neil's patch checked into the aviary1.0.1 branch
Comment 33 Daniel Veditz [:dveditz] 2005-07-06 03:50:31 PDT
Fixed on the 1.7 branch
Comment 34 neil@parkwaycc.co.uk 2005-07-06 04:17:26 PDT
xpfe patch checked in to trunk.
Comment 35 Daniel Veditz [:dveditz] 2005-07-06 05:30:56 PDT
Checked neil's patch into the trunk.

Please open any variations or similar problems as new bugs
Comment 36 Jay Patel [:jay] 2005-07-06 19:06:36 PDT
v.fixed on aviary with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.9)
Gecko/20050706 Firefox/1.0.5
Comment 37 Daniel Veditz [:dveditz] 2005-07-12 11:34:34 PDT
Adding distributors
Comment 38 Daniel Veditz [:dveditz] 2005-07-12 18:04:31 PDT
Security advisories published
Comment 39 Juha-Matti Laurio 2005-07-13 17:40:41 PDT
This is MFSA2005-55 and assigned as http://secunia.com/advisories/16043/ (issue
#8). Is it possible to update 'Alias' field to include SA16043 now?
Comment 40 Boris Zbarsky [:bz] 2005-10-12 08:03:22 PDT
This fix caused bug 301873.

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