297543 - Adblock is exploitable

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.8b2) Gecko/20050612 Firefox/1.0+

Even though Bug 294799 is fixed, Adblock is still exploitable by using almost
the same way as attachment 184973 [details] in Bug 294799 comment 6.

Content window can access the object created by Adblock (e.g. 
window._AdblockFiltered, node._AdblockData, ...), and can access its __parent__ 
that is the hiddenDOMWindow (or BackstagePass object if Adblock was installed in
the browser root). thus, an attacker can use the same way as attachment 184973 [details] 
in order to run arbitrary code.

I guess Bug 294799 should not be made public, until this problem is resolved.


Reproducible: Always

Steps to Reproduce:
1. Install Adblock.
2. Open the testcase.

Comment 1

[moz_bug_r_a4]

Attachment: arbitrary code execution with Adblock

Comment 2

[Daniel Veditz [:dveditz]]

Confirming, the fix for bug 294795/294799 did not fix this one. Granted that
Adblock seems to do bad and unsafe stuff it still isn't right you can use its
__parent__ to get back to a chrome window.

Comment 3

[Brendan Eich [:brendan]]

I'll fix this with a revised approach in bug 296397.

/be

Comment 4

[Brendan Eich [:brendan]]

Fixed by the landing for bug 296397.

/be

Comment 5

[moz_bug_r_a4]

I've tested on:
Firefox 2005-06-22-18-trunk
Firefox 2005-06-22-04-aviary

The fix for bug 296397 does not stop the testcase (attachment 186090 [details]).

I guess, since the hiddenDOMWindow's URL is "about:blank" and the
hiddenDOMWindow does not have chrome privilege, security manager's access-check
succeeds.

Comment 6

[moz_bug_r_a4]

This bug is not fixed yet.

Firefox 2005-06-30-13-trunk
Firefox 2005-06-30-05-aviary

Comment 7

[Johnny Stenback (:jst)]

Yeah, indeed. The security check succeeds because of
http://lxr.mozilla.org/mozilla/source/caps/src/nsScriptSecurityManager.cpp#870.
We need to load something other than about:blank in our hidden window.
Bsmedberg's got a patch that does that in bug 298478, but that makes the hidden
window have chrome privs, which in and of itself scares me. We should do what
bsmedberg did in that bug, but use a resource: URL instead so that it's not
about:blank and does not have chrome privs.

Comment 8

[Johnny Stenback (:jst)]

Attachment: Make the hidden window load a resource: URL

Comment 9

[Benjamin Smedberg]

I don't understand why the patch in bug 298478 didn't fix this... is there code
other than the JS component loader using the hiddenwindow script context?

Comment 10

[Benjamin Smedberg]

Comment on attachment 187952 [details] [diff] [review]
Make the hidden window load a resource: URL

r=bsmedberg after an IRC discussion that left me slightly more confused tha
enlightened. We should revisit all this hiddenwindow stuff in the 1.9
timeframe, giving xpconnect control over its own global objects.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 187952 [details] [diff] [review]
Make the hidden window load a resource: URL

Looks ok, but I'm still concerned about a Ts hit -- we can watch for that and
try a data: URL instead, if that's faster (or whatever is actually faster and
not too ugly), for 1.8b4.

/be

Comment 12

[Johnny Stenback (:jst)]

Fix checked in.

Comment 13

[Johnny Stenback (:jst)]

Not fixed on the branches yet.

Comment 14

[Jay Patel [:jay]]

Comment on attachment 187952 [details] [diff] [review]
Make the hidden window load a resource: URL

Let's get this checked in on the branches. a=jay

Comment 15

[Johnny Stenback (:jst)]

Fixed on both branches now too.

Comment 16

[Johnny Stenback (:jst)]

Oh, and no noticeable Ts hit evident on tinderbox (that I can see).

Comment 17

[Benjamin Smedberg]

This caused bug 299449, which I fixed on trunk for the toolkit apps but needs
fixing on the branches also for ffox/tbird/SM

Comment 18

[moz_bug_r_a4]

Adblock is still exploitable by using eval.

    window._AdblockFiltered.eval(
        "adblockRemoveSlow(null, window, false, 'MALICIOUS_CODE');",
        window._AdblockFiltered
    );


I'm not sure if this can be fixed in Bug 300008 or not.

Comment 19

[moz_bug_r_a4]

Attachment: arbitrary code execution with Adblock (eval)

This works on:
Firefox/1.0.4
Firefox/1.0.5 2005-07-09-04-aviary1.0.1
Firefox/1.0+  2005-07-08-18-trunk
Mozilla/1.7.8
Mozilla/1.7.9 2005-07-09-09-1.7

Comment 20

[Brendan Eich [:brendan]]

I think the burden is on adblock to avoid passing content-controlled strings to
setTimeout and similar functions.

/be

Comment 21

[Chris Beard]

so i've reached out to the adblock development team to understand who we should
be talking to, assuming no else had already done that.  i'm told that the group
is rather informal, but technical contacts are [e-mail] and [e-mail]. 
they hang out on #adblock on irc.m.o.

dveditz, i haven't told them anything, i'm only making the connection.  they
seemed quite receptive to doing whatever is needed.

Comment 22

[Jay Patel [:jay]]

Adding AdBlock developer [e-mail] so he can take a look at this.

Comment 23

[Ben Karel [eschew]]

Actually, Adblock 0.6 (unreleased) already stores per-window data in properties
(in content window objects) with names randomly-generated on browser launch.
That particular masking measure was designed merely to hide Adblock's traces
from anti-adblocking scripts, but it should also make this somewhat harder to
exploit.

Yeah, using random properties doesn't really fix this bug. A sophisticated
script could store a list of all properties for the window object as defined by
the DOM, and then simply inspect those properties that aren't in the predefined
list. Ideally, there would be a way for scripts to mark properties with the
dontenumerate flag, but AFAIK that's not in the standard.

From the Location bar in DPA2, trying to access the .__parent__ properties of
the 0.6 equivalents of the ._AdblockData and ._AdblockFiltered properties throws
a "Permission denied to get property Object.__parent__" exception. 

Calling .eval() on the eqivalents does give access to the Adblock object, but
trying to access Components.stack results in an exception, as does attempting to
use the file/directory_service to instantiate a nsIFile.

The call to eval() from within that particular function has been made obsolete
in 0.6, but that doesn't really seem to matter much, since any and all of the
Adblock object's properties and functions could be subject to modification. That
is to say: I'm not seeing any priviledge-escalation here, but an attacker could
screw up Adblock (and, from there, the rest of the browser-session -- replacing
the [nsIContentPolicy] shouldLoad() function with function() { throw ""; } means
no more content loading until restart).

If moving storage of per-window data out of content windows is the only secure
solution, as it looks to be, we could probably set up a system that uses a
randomly- (or, I suppose, sequentially-)generated number as a unique per-window
identifying key into a hash stored within the Adblock object in the hidden
window.... That would reduce Adblock's content-window profile to a single
randomly-named property with an integer value, and I don't see how that could be
exploitable. It'll be some work, but I don't really see any way around it at
this point.

Thoughts?

Comment 24

[Brendan Eich [:brendan]]

(In reply to comment #23)
> Ideally, there would be a way for scripts to mark properties with the
> dontenumerate flag, but AFAIK that's not in the standard.

Not in ECMA-262, but even then, a script with some knowledge of your PRNG could
probe quickly.  Storing adblock info on content-accessible DOM wrapped-native JS
objects is not a good plan, period, full stop.

You really don't want to mix adblock chrome-parented objects with content.  If
you can use the xpcnativewrappers=yes option in your chrome manifest for 1.1,
and fix adblock to cope with the XPCNativeWrapper automation that results from
that wise default (getting the .wrappedJSObject from the XPCNativeWrapper only
when you *really* need to see the content DOM node as it is, not operate on it
via the well-known DOM interfaces), then adblock will be secure.

As it is, saying that content could mess up adblock but not exploit it is making
a large bet, and from our experience with the same bet for other chrome, not a
good bet.

/be

Comment 25

[Brendan Eich [:brendan]]

So bug 281988 was where the XPCNativeWrapper automation work finally occurred.

The doc for XPCNativeWrapper automation is
http://developer-test.mozilla.org/en/docs/Safely_accessing_content_DOM_from_chrome

/be

Comment 26

[Boris Zbarsky [:bzbarsky]]

Ben, does adblock intend to continue storing data on the content nodes?  That
is, do you need a way to do so safely, or do you plan to move to some other system?

Comment 27

[rue]

Per clarity: when xpcnativewrappers=yes: 
.
a.) properties set by chrome on content are set on a wrapper? 
b.) the properties cannot be accessed by content?
c.) the wrapper is persisted for the native's life? 
d.) all subsequent chrome-calls see the exact same wrapper?
.
On that last point (d), I've seen mention that wrapper-sharing is "per script
filename". Adblock sets things from component.js, but later accesses them from
filterall.js. Would xpcnativewrappers=yes break this?

Comment 28

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #27)
> a.) properties set by chrome on content are set on a wrapper? 

No, properties cannot be set on a wrapper at all at the moment, hence my 
question.  See bug 300325.

> b.) the properties cannot be accessed by content?

If they were set on a wrapper this would be true.

> c.) the wrapper is persisted for the native's life? 

See bug 295937.  This is a non-issue as long as you can't set properties at 
all, but if we do allow property sets we'll also make sure this works.

> d.) all subsequent chrome-calls see the exact same wrapper?

Given (c) this would be correct.

> On that last point (d), I've seen mention that wrapper-sharing is "per script
> filename".

I'm not aware of this... if you could point me to said mention, I would 
appreciate it.

Comment 29

[Daniel Veditz [:dveditz]]

Adding distributors

Comment 30

[rue]

bz:
The third paragraph in comment 24 concludes: "..then adblock will be secure."
But, its proposal would only solve things if wrapper-retention were implemented.
Which is why I asked.
.
The mention of filename-based wrapper-sharing is here -- ironically, under
'Resolved Issues': http://wiki.mozilla.org/User:Brendan


Regarding deliberations on whether or not to add wrapper+prop retention: Adblock
might be the only one. Anticipating no further support/progress, I've come up
with an alternate solution, extension-side -- reflecting shaver's Sandbox out
through a dummy-module, then instantiating an array/object-generating function
inside its "clean" environment:
.
new Script("var generator=function(){return []}").exec(mySandbox)
mySandbox.generator()
.
Not as simple as the manifest-flag, but it works. Ideally, Sandbox would be frozen.

Comment 31

[Brendan Eich [:brendan]]

(In reply to comment #30)

> The mention of filename-based wrapper-sharing is here -- ironically, under
> 'Resolved Issues': http://wiki.mozilla.org/User:Brendan

You're misreading that -- it says nothing about wrapper identity/lifetime being
scoped by source files.  It says that deciding whether to share wrappers, or use
automated XPCNativeWrapper protection, must be configurable by source file, or
at least by "content package" in the chrome sense.

> Regarding deliberations on whether or not to add wrapper+prop retention: 
> Adblock might be the only one. Anticipating no further support/progress, 

Why did you jump to that conclusion?

> I've come up with an alternate solution, extension-side -- reflecting shaver's
> Sandbox out through a dummy-module, then instantiating an
> array/object-generating function inside its "clean" environment:
> .
> new Script("var generator=function(){return []}").exec(mySandbox)
> mySandbox.generator()
> .
> Not as simple as the manifest-flag, but it works. Ideally, Sandbox would be 
> frozen.

Please don't do this if you can use xpcnativewrappers=yes.  We are not going to
freeze Sandbox because you assumed no progress on the questions you raised.  We
are not going to support other extensions rolling their own solutions when the
general, long-standing patterns can be made to work in most cases with the
XPCNativeWrapper automation.

/be

Comment 32

[Boris Zbarsky [:bzbarsky]]

rue, Ben: we're just trying to get a sense of what usage patterns extensions
(like adblock) have so we can decide what to do with bug 300325.  The goal here
is to have maximum usability while remaining usefully secure.  We're not out to
screw extensions over, by any means!

Comment 33

[Brendan Eich [:brendan]]

bz's more diplomatic than I am. ;-)

Rue, Ben: we are here to help.  Adblock is not alone.  The shared-DOM (with
protection from JS overrides) model you enjoy today is something we want to
support, not break.  Let's work together.

I think bug 300025 should be fixed, not WONTFIXed.  jst may be convinced too, by
your testimony here.

/be

Comment 34

[Ben Karel [eschew]]

Well, ideally, usage patterns should be reflective of requirements, so...

Currently, in order for filterall to work properly, we need a way of uniquely
associating a |window| object with a list of nodes that Adblock has procesed
from that window. Currently, it's done implicitly, by storing objects as
properties of each window; it could be done with a single integer, but even
integer literals have the .eval() method, making them insecure (what if we
clobbered .eval on whatever we store in content?).

More to the point, I'm not currently aware of any way to differentiate between
window objects (URL alone is not enough) without marking them in some way. Any
suggestions?

If not, then yes, we'd pretty much require the ability to set new properties on
wrappers / continue storing data in content.

(We also, in 0.6, tag nodes that pass through shouldLoad(), but we could move
back to the 0.5 model of storing such data in a separate array indexed by URL.
Again, identifying windows uniquely is the key.)

And /be is of course right about access-exploitability, since arbitrary code
could be injected into, say, shouldLoad(), which runs with full chrome privs IIRC.

Comment 35

[Boris Zbarsky [:bzbarsky]]

What sort of differentiate?  You can tell them apart via == of course, but if
you're trying to match up a window object with some random data then you need to
either have the data on the object or the data/object pair somewhere (and do a
search through the somewhere for the right pair).

Comment 36

[Brendan Eich [:brendan]]

(In reply to comment #34)
> Well, ideally, usage patterns should be reflective of requirements, so...

Even better if we didn't break any existing patterns of usage, based on whatever
requirements existed when an extension was written.

The hard case with split wrappers (xpcnativewrappers=yes automation) is when
chrome wants to get or set a JS property that content sets or gets -- not a DOM
attribute mapped to a property.  That can be handled by using .wrappedJSObject
carefully.
 
> Currently, in order for filterall to work properly, we need a way of uniquely
> associating a |window| object with a list of nodes that Adblock has procesed
> from that window. Currently, it's done implicitly, by storing objects as
> properties of each window; it could be done with a single integer, but even
> integer literals have the .eval() method, making them insecure (what if we
> clobbered .eval on whatever we store in content?).

It sounds like there's some misunderstanding of the security hazards here.

First, what do you mean by "single integer"?  JS does not create an object for
each primitive-typed value, so 1.eval evals against a temporary Number instance
created on the fly.  There's no persistent harm, and the eval runs in the scope
of its caller, so if content calls it there is no cross-trust-domain scripting hole.

What's more, with bug 300008, the eval native method does its own checks and
should not be usable to attack chrome, unless chrome blindly calls a method on a
content DOM node.  That's why we recommend xpcnativewrappers=yes.  You can't be
tricked if your chrome uses its own wrappers for the underlying DOM natives.
 
> More to the point, I'm not currently aware of any way to differentiate between
> window objects (URL alone is not enough) without marking them in some way. Any
> suggestions?

Object identity, as bz pointed out; or just decorate the chrome wrapper for the
window object with ad-hoc JS properties, with xpcnativewrappers=yes.  Those JS
properties won't be seen by content, and can't be spoofed by content.

> If not, then yes, we'd pretty much require the ability to set new properties
> on wrappers

That should be possible, I agree.

> / continue storing data in content.

That doesn't follow.  If you can store your data on chrome wrappers, use them to
safely call into the content DOM, and not be tricked by content manipulations of
its wrappers, everyone wins.  To actually block ads, you'll need to do some
mutating operations, but these are DOM ops -- not ad-hoc JS property gets or
sets on the content wrappers, right?

> (We also, in 0.6, tag nodes that pass through shouldLoad(), but we could move
> back to the 0.5 model of storing such data in a separate array indexed by URL.
> Again, identifying windows uniquely is the key.)

I don't see a problem here in the xpcnativewrappers=yes model, unless content
scripts are *supposed* to see your ad-hoc properties.  But that's not safe, so
it can't be the case.

If the devmo docs on XPCNativeWrapper are unclear, we should fix them -- please
list any questions left unanswered, so we can get this sorted out for you guys
and for others.  Thanks for your help,

/be

Comment 37

[Brendan Eich [:brendan]]

> each primitive-typed value, so 1.eval evals against a temporary Number instance

I mean (1).eval(program) or 1..eval(program), of course (think about precedence
of the lexical grammar production for floating point literal vs. syntactic
production for the dot operator.

/be

Comment 38

[Ben Karel [eschew]]

bz: Differentiate meaning to "match a window object with some random data".
Right -- currently, we store data on the window, but unless/until bug 300325 is
fixed, that won't be secure. 
|
If I understand you correctly, the "other" way of doing it would involve keeping
a referenced copy of *every* window object in a given browser session. According
to rue, from his experience with SessionSaver, there's no non-hackish and
-fragile way to detect tab closings, which means that this method would
effectively involve leaking every single window. In general, it just seems like
an awfully unintuitive and "wrong" solution.

/be: 
Wait, now I'm somewhat confused:

>just decorate the chrome wrapper for the
>window object with ad-hoc JS properties, with xpcnativewrappers=yes.  Those JS
>properties won't be seen by content, and can't be spoofed by content.

Unless there is a persistent one-to-one mapping between a window object and its
chrome wrapper, as fetched from XPCNativeWrapper(), this doesn't work, as it's
the window object that we are given to differentiate, not the chrome wrapper.

You seem to be implying that after the following theoretical code is executed,

var wrapper = new XPCNativeWrapper(window, 'myProperty');
wrapper.myProperty = 'myValue';
wrapper = new XPCNativeWrapper(window, 'myProperty');

then wrapper.myProperty should equal 'myValue', and
window.eval('alert(typeof(window.myProperty))') should alert "undefined"?
Looking through the devmo docs, I don't see any mention of the consequences of
passing non-DOM-properties as arguments. This (the fact that it doesn't work)
seems like bug 300325, or is that something else?

As noted earlier, it would be ideal to have an "invisible" profile from content
JS, so that Adblock isn't (directly) detectable by anti-adblocking scripts.

If it's OK security-wise to mark the content window object with an integer, then
that's good, but it still leaves the issue of leaks and how to know when to
dispose of the obsolete data now being kept in chrome, which was automatically
handled when data was stored within the content windows.

Re: blocking ads: in 0.6, all display mutations are done by setting a given
value on a (randomly-chosen-on-first-run-after-install) attribute on a given
node; code injected into userContent.css does the rest.

Comment 39

[Brendan Eich [:brendan]]

(In reply to comment #38)
> /be: 
> Wait, now I'm somewhat confused:
> 
> >just decorate the chrome wrapper for the
> >window object with ad-hoc JS properties, with xpcnativewrappers=yes.  Those
> >JS properties won't be seen by content, and can't be spoofed by content.
> 
> Unless there is a persistent one-to-one mapping between a window object and
> its chrome wrapper, as fetched from XPCNativeWrapper(), this doesn't work, as
> it's the window object that we are given to differentiate, not the chrome
> wrapper.

Who gives you (you == chrome script) the window object?  Isn't really adblock's
chrome scripts/functions that inspect content window objects?  If so, then by
setting xpcnativewrappers=yes, those scripts will get their own persistently
associated XPCNativeWrappers for the content DOM objects.

> You seem to be implying that after the following theoretical code is executed,
> 
> var wrapper = new XPCNativeWrapper(window, 'myProperty');
> wrapper.myProperty = 'myValue';
> wrapper = new XPCNativeWrapper(window, 'myProperty');
> 
> then wrapper.myProperty should equal 'myValue', and
> window.eval('alert(typeof(window.myProperty))') should alert "undefined"?
> Looking through the devmo docs, I don't see any mention of the consequences of
> passing non-DOM-properties as arguments. This (the fact that it doesn't work)
> seems like bug 300325, or is that something else?

That bug will be fixed, but even then, if you set xpcnativewrappers=yes, you
don't have to explicitly construct XPCNativeWrappers.  Your chrome script will
get an XPCNativeWrapper automatically created, one for one, with the underlying
wrapped native.

> As noted earlier, it would be ideal to have an "invisible" profile from
> content JS, so that Adblock isn't (directly) detectable by anti-adblocking 
> scripts.

It won't be, once the JS properties it sets go on its own automatically created
XPCNativeWrappers, where content scripts cannot see them.

> If it's OK security-wise to mark the content window object with an integer,

Mark the chrome XPCNativeWrapper for the content window *native*.

I think I see the confusion.  Here's some ASCII art (and I love the tall
textareas bookmarklet!):

  +----------------+        +----------------+        *-----------------*
  |XPCNativeWrapper|========|XPCWrappedNative|*~~~~~~~|native DOM window|
  +-----+----------+        +----------------+|       *-----------------*
        V                  / *----------------*
        |_wrappedJSObject_/

+---+ borders bound JS objects; *---* borders bound native XPCOM objects.

The XPCNativeWrapper is a JS object; so is the wrapped native whose private data
is of type XPCWrappedNative (these names are obscurely different; history, and a
reminder to think clearly about which is which).  That XPCOM object is mapped
via a hashtable per "scope" (window JS object, essentially) to the root XPCOM
interface pointer of the native DOM window instance.

So when you use xpcnativewrappers=yes in your chrome manifest, you get automatic
wrapping of the JS object the content sees (the XPCWrappedNative's JS object),
namely the XPCNativeWrapper on the left.  If you set properties on that object
(once bug 300325 is fixed), content scripts can't see them -- there is no path
available to scripts from right to left above.  The only path available, and
only to chrome scripts, is from the XPCNativeWrapper to the XPCWrappedNative's
JS object, the property arc labeled wrappedJSObject above.

> then
> that's good, but it still leaves the issue of leaks and how to know when to
> dispose of the obsolete data now being kept in chrome, which was automatically
> handled when data was stored within the content windows.

There's no leak issue.  The XPCNativeWrappers created automatically, and any
properties you set on them, will live as long as their XPCWrappedNatives live
(bz, remind me if there's a bug to fix here still).

> Re: blocking ads: in 0.6, all display mutations are done by setting a given
> value on a (randomly-chosen-on-first-run-after-install) attribute on a given
> node; code injected into userContent.css does the rest.

Do you mean DOM attribute, or JS property not named in any w3c or Mozilla DOM IDL?

/be

Comment 40

[Brendan Eich [:brendan]]

Just to be super clear:

> namely the XPCNativeWrapper on the left.  If you set properties on that object
> (once bug 300325 is fixed), content scripts can't see them -- ....

I'm talking about JS properties, not DOM attributes.  So when you use an
XPCNativeWrapper to operate on the native XPCOM object underlying the
XPCWrappedNative, by getting or setting attributes, or calling methods, you
indeed affect the underlying DOM or other data -- which is exactly what it
sounds as if Adblock needs to do so the userContent.css rules can block certain
ad content nodes.

I hope there's no case where Adblock needs to set non-DOM-attribute ("ad-hoc")
JS properties on content DOM nodes.

/be

Comment 41

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #38)
> In general, it just seems like an awfully unintuitive and "wrong" solution.

Agreed.  That's why I filed bug 300325...  ;)

If adblock is aiming to affect styles, it would be calling setAttribute(), I
assume -- setting a random property on a content node won't create a
corresponding attribute that can be styled.  So that part sounds like it should
be ok.

Comment 42

[Brendan Eich [:brendan]]

Noting new dependencies.  Sort of morphing this bug, hope no one minds.

/be

Comment 43

[Ben Karel [eschew]]

/be:

For better or worse, any solution implemented to fix this issue *must* be
backwards compatible to at least Mozilla 1.3.1, which is the highest-versioned
Gecko application available under Mac OS 9, which happens to be rue's platform
of choice. As far as I can tell, no force on this earth short of Jesus Christ
Himself is going to convince rue to change platforms (and even then he might
hold out for the real deal).

Anyways, said solution doesn't technically have to be *secure* on pre-1.1
platforms, though that would be an obvious plus. That's why I'd only mentioned
explicitly-created XPCNativeWrappers -- consistency of behavior for 1.1 *and*
older builds.

====================
From what I've seen, I think the most obvious "correct" course of action is
this: have bug 300325 fixed [edit: marked fixed while writing], and Adblock will
use xpcnativewrappers=yes and not explicitly construct wrappers. This will leave
1.1 users secure, while maintaining full functionality -- if not full security
-- on older builds. Which is, incidentally, your original recommendation, no? :)
====================

(We can try experimenting with things like clobbering .eval on our properties
and other such tricks, I guess, to try to make exploits as difficult as possible
to discover on older builds... which somewhat reminds me -- will this bug be
un-sensitivised in the future? The vulnerabilities, after all, still exist in
older builds with Adblock installed.)

When you say "I hope there's no case where Adblock needs to set
non-DOM-attribute ("ad-hoc") JS properties on content DOM nodes.", I assume you
mean "set JS properties that are meant to be seen by content"? That is correct.

>It won't be, once the JS properties it sets go on its own automatically
>created XPCNativeWrappers, where content scripts cannot see them.

But bug 300325 is about the fact that that will not work in pre-1.1 builds,
right? -- and that would be an issue if we tried using XPCNativeWrappers
explicitly in all builds, as noted above.

The hypothetical "leak" issue would be because we'd (have been) caching the
window objects -- XPCWrappedNatives, if you will -- so as to retain
backwards-compatibility and consistency of behavior across all Mozilla platforms.

re: the code example in comment 38 -- I assume the answer is "undefined"?

bz (and /be): "attribute" implies DOM attribute, property implies JS property.

Comment 44

[Brendan Eich [:brendan]]

(In reply to comment #43)

Everything sounds cool -- just wanted clarity on this item:

> >It won't be, once the JS properties it sets go on its own automatically
> >created XPCNativeWrappers, where content scripts cannot see them.
> 
> But bug 300325 is about the fact that that will not work in pre-1.1 builds,
> right?

No, because before 1.1, XPCNativeWrapper was implemented in JS, and you could
add "expandos" to it.

> -- and that would be an issue if we tried using XPCNativeWrappers
> explicitly in all builds, as noted above.

No, you could do that.  But I agree it's better to use xpcnativewrappers=yes and
implicit wrappers.

/be

Comment 45

[Boris Zbarsky [:bzbarsky]]

> That's why I'd only mentioned explicitly-created XPCNativeWrappers --
> consistency of behavior for 1.1 *and* older builds.

That got me thinking... What if, in 1.1, explicit XPCNativeWrapper constructors
that are:

1)  Called from code that uses xpcnativewrappers=yes
2)  Called on an XPCNativeWrapper (usual, given #1)

just return the XPCNativeWrapper they're called on?  I can't think of reasons
why such code would want to create explicit wrappers, really; if it does, it can
do so by getting the wrappedJSObject first and calling new XPCNativeWrapper() on
that...

That would let code be safe in both 1.1 and pre-1.1 releases and all that good
stuff.  jst, shaver, brendan, what do you think?

Comment 46

[Boris Zbarsky [:bzbarsky]]

Actually, comment 45 makes no sense -- in pre-1.8 (Gecko) builds you couldn't
set props on an XPCNativeWrapper and expect them to stick around, so that
wouldn't be a viable solution anyway....

Comment 47

[Brendan Eich [:brendan]]

(In reply to comment #46)
> Actually, comment 45 makes no sense -- in pre-1.8 (Gecko) builds you couldn't
> set props on an XPCNativeWrapper and expect them to stick around, so that
> wouldn't be a viable solution anyway....

You mean because XPCNativeWrapper creation before 1.8 (1.8b2, IIRC) was
explicit. But an explicit 'new XPCNativeWrapper(...)' could be kept alive via
any strong ref (a global var, e.g.) that the chrome script maintained.

But in any case, the point I thought you were making is that an explicitly
created XPCNativeWrapper that is given another XPCNW should not double-wrap. 
Once we fix the implicit-lifetime bug.

/be

Comment 48

[Boris Zbarsky [:bzbarsky]]

That was sorta the point I was making, but the point was based on trying to
solve a specific problem, and it doesn't solve it.  At the moment it doesn't
double-wrap -- it just unwraps and rewraps, which is fine and gives very
consistent behavior for explicit wrappers.

Comment 49

[moz_bug_r_a4]

.eval exploit has been blocked by the fix for Bug 300867.
Content window can no longer execute eval using the scope of the hidden window.


I've tested on:
Firefox 1.0.6  2005-07-17-05
Firefox 1.0+   2005-07-17-07
Mozilla 1.7.10 2005-07-17-09

Exploit (attachment 188804 [details]) failed with this error:
Error: function eval must be called directly, and not by way of a function of
another name

Comment 50

[Brendan Eich [:brendan]]

Fixed, again.  Any similar recurrence should be reported under a new bug.

/be