298892 - Node spoofing (using e.g. XHTML) to bypass security checks

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[moz_bug_r_a4]

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]

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

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

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]

Maybe another day or two to test and make sure we're ready to take a fix for this.

Comment 5

[Chase Phillips]

jst says, "remember to look for nodeName"

Comment 6

[Daniel Veditz [:dveditz]]

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]

What exactly is left to do on trunk for this bug?

Comment 8

[Daniel Veditz [:dveditz]]

Attachment: Check instanceof rather than spoofable localName

Comment 9

[[e-mail]]

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

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

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-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

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

Fixed on trunk and branches

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

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

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

Attachment: fix syntax error

Comment 18

[Daniel Veditz [:dveditz]]

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

checked in biesi's fix.

Comment 20

[moz_bug_r_a4]

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]

attachment 188087 [details] in Bug 299520 comment 1
A exploit testcase that is using xhtml <A>.

Comment 22

[[e-mail]]

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

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

Can anyone jump in here and get patches ready for landing on the Aviary branch
for Firefox and Thunderbird?

Comment 25

[Jay Patel [:jay]]

Reassigning to jst to patch up browser and mail.  Dveditz can patch the 1.7
branch when those patches are ready.

Comment 26

[[e-mail]]

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

Comment on attachment 188372 [details] [diff] [review]
Branches patch

r/sr=dveditz for the branch/FE ports

Comment 28

[Jay Patel [:jay]]

Let's get those patches checked in the appropriate places asap. a=jay again. :-)

Comment 29

[Jay Patel [:jay]]

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

Comment on attachment 188089 [details] [diff] [review]
Remove findParentNode (xpfe version)

a=dveditz

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 188372 [details] [diff] [review]
Branches patch

a=dveditz for branches

Comment 32

[Daniel Veditz [:dveditz]]

Neil's patch checked into the aviary1.0.1 branch

Comment 33

[Daniel Veditz [:dveditz]]

Fixed on the 1.7 branch

Comment 34

[[e-mail]]

xpfe patch checked in to trunk.

Comment 35

[Daniel Veditz [:dveditz]]

Checked neil's patch into the trunk.

Please open any variations or similar problems as new bugs

Comment 36

[Jay Patel [:jay]]

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

Adding distributors

Comment 38

[Daniel Veditz [:dveditz]]

Security advisories published

Comment 39

[Juha-Matti Laurio]

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 [:bzbarsky]]

This fix caused bug 301873.