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

RESOLVED FIXED

Status

()

Core
Security
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: moz_bug_r_a4, Assigned: jst)

Tracking

({fixed-aviary1.0.5, fixed1.7.9})

Trunk
x86
Windows XP
fixed-aviary1.0.5, fixed1.7.9
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.9 +
blocking-aviary1.0.5 +
blocking1.8b3 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix] ready for landing)

Attachments

(6 attachments)

(Reporter)

Description

12 years ago
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:
(Reporter)

Comment 1

12 years ago
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
(Reporter)

Comment 2

12 years ago
Created attachment 187392 [details]
Arbitrary code execution via setWallpaper()

I've confirmed that this works on:
Firefox 1.0.5 2005-06-26-05
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.8b3?
Flags: blocking1.7.9+
Flags: blocking-aviary1.0.5+
Summary: The fixes for bug 292774 and bug 292737 can be bypassed → Node spoofing (using e.g. XHTML) to bypass security checks
Whiteboard: [sg:fix]

Comment 4

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

Comment 5

12 years ago
jst says, "remember to look for nodeName"
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
Depends on: 290420
What exactly is left to do on trunk for this bug?
Flags: blocking1.8b3? → blocking1.8b3+
Created attachment 187685 [details] [diff] [review]
Check instanceof rather than spoofable localName
Attachment #187685 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #187685 - Flags: review?(mconnor)
Whiteboard: [sg:fix] → [sg:fix] need review

Updated

12 years ago
Attachment #187685 - Flags: review?(mconnor) → review+

Comment 9

12 years ago
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.
Attachment #187685 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+

Comment 10

12 years ago
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
Attachment #187685 - Flags: approval1.7.9+
Attachment #187685 - Flags: approval-aviary1.0.5+

Updated

12 years ago
Whiteboard: [sg:fix] need review → [sg:fix] needs landing by dveditz
we do want the tweaks, waste of Neil's time if we don't use his effort to get
better.
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?
Er, actually, never mind.  Now I see that the .type property (but not
getAttribute()) is normalized to "text" in those cases.
Fixed on trunk and branches
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed-aviary1.0.5, fixed1.7.9
Resolution: --- → FIXED
Whiteboard: [sg:fix] needs landing by dveditz → [sg:fix]
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") ) {
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
it would've been nice if the patch that was actaully checked in had been
attached here...
Created attachment 187837 [details] [diff] [review]
fix syntax error
Attachment #187837 - Flags: superreview?(dveditz)
Attachment #187837 - Flags: review?(neil.parkwaycc.co.uk)

Updated

12 years ago
Attachment #187837 - Flags: superreview?(dveditz)
Attachment #187837 - Flags: superreview+
Attachment #187837 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #187837 - Flags: review+
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.
checked in biesi's fix.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
(Reporter)

Updated

12 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 20

12 years ago
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.
(Reporter)

Comment 21

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

Comment 22

12 years ago
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.
Attachment #188089 - Flags: superreview?(dveditz)
Attachment #188089 - Flags: review?(timeless)
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!)
Attachment #188089 - Flags: superreview?(dveditz) → superreview+

Updated

12 years ago
Attachment #188089 - Flags: review?(timeless) → review+

Comment 24

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

Updated

12 years ago
Keywords: fixed-aviary1.0.5, fixed1.7.9

Comment 25

12 years ago
Reassigning to jst to patch up browser and mail.  Dveditz can patch the 1.7
branch when those patches are ready.
Assignee: dveditz → jst
Status: REOPENED → NEW

Updated

12 years ago
Whiteboard: [sg:fix] → [sg:fix] need patches for browser and mail eta 7/5

Comment 26

12 years ago
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.

Updated

12 years ago
Attachment #188089 - Flags: approval1.8b3?
Comment on attachment 188372 [details] [diff] [review]
Branches patch

r/sr=dveditz for the branch/FE ports
Attachment #188372 - Flags: superreview+
Attachment #188372 - Flags: review+

Comment 28

12 years ago
Let's get those patches checked in the appropriate places asap. a=jay again. :-)
Whiteboard: [sg:fix] need patches for browser and mail eta 7/5 → [sg:fix] ready for landing

Comment 29

12 years ago
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 on attachment 188089 [details] [diff] [review]
Remove findParentNode (xpfe version)

a=dveditz
Attachment #188089 - Flags: approval1.8b3? → approval1.8b3+
Comment on attachment 188372 [details] [diff] [review]
Branches patch

a=dveditz for branches
Attachment #188372 - Flags: approval1.7.9+
Attachment #188372 - Flags: approval-aviary1.0.5+
Neil's patch checked into the aviary1.0.1 branch
Keywords: fixed-aviary1.0.5
Fixed on the 1.7 branch
Keywords: fixed1.7.9

Comment 34

12 years ago
xpfe patch checked in to trunk.
Checked neil's patch into the trunk.

Please open any variations or similar problems as new bugs
Status: NEW → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED

Comment 36

12 years ago
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
Depends on: 299901

Updated

12 years ago
No longer depends on: 299901
Depends on: 299901
Adding distributors
Security advisories published
Group: security

Comment 39

12 years ago
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?
Depends on: 301873
This fix caused bug 301873.

Updated

12 years ago
Flags: testcase+

Updated

11 years ago
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.