Last Comment Bug 345991 - [FIX]JS components do not get protected by XPCNativeWrapper
: [FIX]JS components do not get protected by XPCNativeWrapper
Status: RESOLVED FIXED
: fixed1.8.0.7, fixed1.8.1
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.9alpha1
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 348990
Blocks:
  Show dependency treegraph
 
Reported: 2006-07-26 10:31 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2010-10-31 11:48 PDT (History)
11 users (show)
bzbarsky: blocking1.9a1+
mtschrep: blocking1.8.1+
dveditz: blocking1.8.0.7+
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test component (3.06 KB, text/plain)
2006-07-26 10:33 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
Proposed patch (1.45 KB, patch)
2006-07-26 10:34 PDT, Boris Zbarsky [:bz] (still a bit busy)
mrbkap: review+
brendan: superreview+
mtschrep: approval1.8.1+
Details | Diff | Splinter Review
Test component with buglet fixed (3.06 KB, text/plain)
2006-07-26 10:36 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
Patch that I checked in (1.18 KB, patch)
2006-07-27 11:03 PDT, Boris Zbarsky [:bz] (still a bit busy)
dveditz: approval1.8.0.7+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2006-07-26 10:31:43 PDT
STEPS TO REPRODUCE:

1)  Drop the attached testComponent.js file into your dist/bin/components
2)  Start the browser
3)  Load 

data:text/html,<img src="http://www.mozilla.org/images/mozilla-banner.gif">Test

EXPECTED RESULTS:  Image loads

ACTUAL RESULTS:  Broken image, indicating that the HTMLImageElement was not XPCNativeWrappered before being passed to the JS component.

NOTES: I thought I fixed this in bug 337095, but I was wrong.  The only reason it worked there is that the adblock code uses the subscript loader to load policy.js, and uses a chrome URI to point to it, so policy.js ends up with the system script flag set.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2006-07-26 10:33:15 PDT
Created attachment 230756 [details]
Test component
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2006-07-26 10:34:17 PDT
Created attachment 230757 [details] [diff] [review]
Proposed patch
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2006-07-26 10:36:10 PDT
Created attachment 230759 [details]
Test component with buglet fixed
Comment 4 Brendan Eich [:brendan] 2006-07-26 13:34:51 PDT
Comment on attachment 230757 [details] [diff] [review]
Proposed patch

>Index: js/src/xpconnect/loader/mozJSComponentLoader.cpp
>===================================================================
>RCS file: /home/bzbarsky/mozilla/cvs-mirror/mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp,v
>retrieving revision 1.124
>diff -u -p -d -8 -r1.124 mozJSComponentLoader.cpp
>--- js/src/xpconnect/loader/mozJSComponentLoader.cpp	10 Jun 2006 00:47:40 -0000	1.124
>+++ js/src/xpconnect/loader/mozJSComponentLoader.cpp	26 Jul 2006 17:27:27 -0000
>@@ -1115,16 +1115,24 @@ mozJSComponentLoader::GlobalForLocation(
>                 nativePath.get());
> #endif
>         return NS_ERROR_FAILURE;
>     }
> 
>     // Ensure that we clean up the script on return.
>     JSScriptHolder scriptHolder(cx, script);
> 
>+    // Flag this script as a system script
>+    // XXXbz we actually want to flag this exact filename, not anything that
>+    // starts with this filename... Maybe we need a way to do that?  On the
>+    // other hand, the fact that this is in our components dir means that if
>+    // someone snuck a malicious file into this dir we're screwed anyway...  So
>+    // maybe flagging as a prefix is fine.

See the comment in jsdbgapi.h:

 * Associate flags with a script filename prefix in rt, so that any subsequent
 * script compilation will inherit those flags if the script's filename is the
 * same as prefix, or if prefix is a substring of the script's filename.

In other words, there's no `dirname nativePath` computed here.  The prefix is the string you pass in, and if it's a leaf filename, not a directory name, then only that filename is flagged.

So your XXXbz comment can go away ;-).

sr=me with that.

/be
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2006-07-26 17:00:04 PDT
> The prefix is the string you pass in, and if it's a leaf filename, not a 
> directory name,

If my prefix is "/home/bzbarsky/good.js" then "/home/bzbarsky/good.js.evil.js" is flagged.

So I stand by my XXX comment.  ;)
Comment 6 Brendan Eich [:brendan] 2006-07-26 17:03:46 PDT
(In reply to comment #5)
> > The prefix is the string you pass in, and if it's a leaf filename, not a 
> > directory name,
> 
> If my prefix is "/home/bzbarsky/good.js" then "/home/bzbarsky/good.js.evil.js"
> is flagged.
> 
> So I stand by my XXX comment.  ;)

I'll see that and raise you a FIXME.  Could you file a bug and cite it with a FIXME: # comment?  The bug should ask that prefix matching require either '\0' or '/' after the end of the prefix.  Thanks,

/be
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2006-07-27 10:50:01 PDT
Fixed on trunk.
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2006-07-27 10:51:02 PDT
Comment on attachment 230757 [details] [diff] [review]
Proposed patch

This patch causes JS components to get XPCNativeWrapper goodness, since they run as chrome.  Safety depends on whether JS components are currently doing unsafe stuff with content-objects -- the ones that are might break...  I feel that this is what we actually want.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2006-07-27 10:51:26 PDT
Oh, I filed bug 346139 about filename prefixes.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2006-07-27 11:03:13 PDT
Created attachment 230926 [details] [diff] [review]
Patch that I checked in
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2006-07-27 11:05:51 PDT
Fixed on 1.8 branch.
Comment 12 Daniel Veditz [:dveditz] 2006-08-11 11:44:31 PDT
Comment on attachment 230926 [details] [diff] [review]
Patch that I checked in

approved for 1.8.0 branch, a=dveditz for drivers
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2006-08-11 19:17:37 PDT
Fixed for 1.8.0.7.

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