Closed Bug 345991 Opened 18 years ago Closed 18 years ago

[FIX]JS components do not get protected by XPCNativeWrapper

Categories

(Core :: XPConnect, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Keywords: fixed1.8.0.7, fixed1.8.1)

Attachments

(2 files, 2 obsolete files)

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.
Attached file Test component (obsolete) (deleted) —
Attached patch Proposed patch (obsolete) (deleted) — Splinter Review
Attachment #230757 - Flags: superreview?(brendan)
Attachment #230757 - Flags: review?(mrbkap)
Attached file Test component with buglet fixed (deleted) —
Attachment #230756 - Attachment is obsolete: true
Flags: blocking1.9a1+
Flags: blocking1.8.1?
Flags: blocking1.8.0.6?
Priority: -- → P1
Target Milestone: --- → mozilla1.9alpha
Attachment #230757 - Flags: review?(mrbkap) → review+
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
Attachment #230757 - Flags: superreview?(brendan) → superreview+
> 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. ;)
(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
Flags: blocking1.8.1? → blocking1.8.1+
Fixed on trunk.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
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.
Attachment #230757 - Flags: approval1.8.1?
Attachment #230757 - Flags: approval1.8.0.6?
Oh, I filed bug 346139 about filename prefixes.
Attachment #230757 - Flags: approval1.8.1? → approval1.8.1+
Attached patch Patch that I checked in (deleted) — Splinter Review
Attachment #230757 - Attachment is obsolete: true
Attachment #230926 - Flags: approval1.8.0.6?
Attachment #230757 - Flags: approval1.8.0.6?
Fixed on 1.8 branch.
Keywords: fixed1.8.1
Flags: in-testsuite?
Comment on attachment 230926 [details] [diff] [review] Patch that I checked in approved for 1.8.0 branch, a=dveditz for drivers
Attachment #230926 - Flags: approval1.8.0.7? → approval1.8.0.7+
Flags: blocking1.8.0.7? → blocking1.8.0.7+
Fixed for 1.8.0.7.
Keywords: fixed1.8.0.7
Depends on: 348990
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: