Closed Bug 729994 Opened 12 years ago Closed 12 years ago

Jetpack content script specific wrapper

Categories

(Core :: XPConnect, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: gkrizsanits, Assigned: gkrizsanits)

References

Details

Attachments

(2 files)

Currently none of the existing wrappers provide the features the jetpack content script needs. The requirement is close to an xraywrapper with a couple of differences / extra features. Content script will have the same principal as the content itself, so changing the xraywrapper behavior in this specific case should not result any dangerous security leak, however I will still have to investigate the set of features to be sure about that. (and it still an open question if it's feasible to implement it the way we want or not)

listener expando (should be there already)

overwriting native properties on the wrapper

accessing form props like document.form1 (bases on name attribute)
same for: IMG FORM APPLET EMED OBJECT

window : frames and iframes (name attribute)

window.frames (array of frames)

form tag : can access form elementes (like input)

postMassage : nsGlobalWindow::CallerInnerWindow

history API is broken (maybe by proxy code... bug 679054 )
Assignee: nobody → gkrizsanits
So after some talks with Alex I have a better understanding what these requirements are and I would like to figure out if it makes sense to have them on xraywrapper by default or make these features optional (so only a jetpack specific sandbox would use them, in case of the two compartment have the same principal)

overwriting native properties on the wrapper - this is basically shadowing, so expando properties on the wrapper could hide native methods, but only from chrome side. I think this should be ok for xraywrappers.

The rest are some nsDOMClassInfo magic, like nsHTMLFormElementSH::FindNamedItem and nsHTMLDocumentSH::DocumentAllGetProperty. So in case of the underlying native is some specific DOM element, we would try these custom property look-up methods as well. Now I don't think it will make xraywrapper code any nicer for sure, and I'm not sure if the new dom binding will fix this problem in the near future or not. And the main question is if jetpack sdk needs this feature, can we let xraywrappers work this way by default, or would that expose a security leak in case of one of the two compartments is chrome and the other is not? I assume it would but not sure... Blake, Bobby: any thoughts on this?
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #1)
> So after some talks with Alex I have a better understanding what these
> requirements are and I would like to figure out if it makes sense to have
> them on xraywrapper by default or make these features optional (so only a
> jetpack specific sandbox would use them, in case of the two compartment have
> the same principal)

So, do I understand correctly that jetpack is currently a special case where we have multiple compartments for a single origin?

In a compartment-per-global world (coming very soon, bug 650353), this will change. So this isn't a good way to differentiate jetpack-specific stuff.
 
> overwriting native properties on the wrapper - this is basically shadowing,
> so expando properties on the wrapper could hide native methods, but only
> from chrome side. I think this should be ok for xraywrappers.

I don't understand the security characteristics you're looking for here. Who is doing the shadowing, and who sees it?

If content shadows and you want chrome to see it, chrome should be waiving Xray via .wrappedJSObject.

If you want chrome to be able to shadow properties itself, but have the shadowing visible only to chrome, that could be reasonable. In fact, that _should_ be how things work today. There is one Xray wrapper for the content object in the chrome compartment, and that Xray wrapper has an expando object so that chrome can modify the wrapper, but not the underlying object itself.

This will change with compartment-per-global of course, but I'm curious as to why you'd be seeing it now. Are there multiple chrome compartments involved? Or am I misunderstanding your question entirely?


> The rest are some nsDOMClassInfo magic, like
> nsHTMLFormElementSH::FindNamedItem and
> nsHTMLDocumentSH::DocumentAllGetProperty. So in case of the underlying
> native is some specific DOM element, we would try these custom property
> look-up methods as well.

Wait, I'm confused. Are you saying there there we currently have custom nsDOMClassInfo behavior on the underlying object (like window.frames[0] or document.form1) that isn't visible through Xray wrapper? Or that you want special new behavior on the wrapper but not on the underlying object?

If it's the former, I thought we already did that, but maybe not. I wonder how easy it would be to fix...
> 
> So, do I understand correctly that jetpack is currently a special case where
> we have multiple compartments for a single origin?

Not for long, in fact I think the only reason that this can be still true since there was a weakmap related issue, that I fixed. But let's now suppose we already in the new one global per compartment world from now on, since that will be the case soon.

> If you want chrome to be able to shadow properties itself, but have the
> shadowing visible only to chrome, that could be reasonable. In fact, that
> _should_ be how things work today. There is one Xray wrapper for the content
> object in the chrome compartment, and that Xray wrapper has an expando
> object so that chrome can modify the wrapper, but not the underlying object
> itself.

Exactly what I need, and I think it's reasonable to have this working. It might be fixed already, Alex do you have some test for this?

> 
> This will change with compartment-per-global of course, but I'm curious as
> to why you'd be seeing it now. Are there multiple chrome compartments
> involved? Or am I misunderstanding your question entirely?

Ok. So there are no chrome compartments involved at all. There is a compartment from addon side, and there is the content compartment, the current web page. Both have the same (url based) principal. But we do want some kind of xraywrapper like behavior between them (which is possible to get via sandboxes). The problem is that XrayWrappers have some odd behaviors that causes problems to us, and currently I'm trying to figure out that these issues we have are actualy bug to be fixed on the XrayWrappers or some of those are actual features, to avoid a security leak. So as I said XrayWrappers in this case are not used between chrome and content, but in case where usually same origin policy would be used (transparent wrappers), in worst case, I can implement these custom behaviors as some custom "mode" of XrayWrapper, that does not affect XrayWrappers in any other case. 

> Wait, I'm confused. Are you saying there there we currently have custom
> nsDOMClassInfo behavior on the underlying object (like window.frames[0] or
> document.form1) that isn't visible through Xray wrapper? Or that you want
> special new behavior on the wrapper but not on the underlying object?
> 
> If it's the former, I thought we already did that, but maybe not. I wonder
> how easy it would be to fix...

Well, window.frames[0] works for sure, but I'm not so sure about the rest... I guess the best if we write some tests first...  Alex could you help me with that?
As a second thought I might completely misunderstand the one compartment per global principal. I assumed that if I create a sandbox with let's say a url based principal, then a new global (+ compartment) will be created even if there is already an existing compartment with the same url principal (a tab in the browser for example with the same url). Am I wrong?
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3)
> But let's now suppose
> we already in the new one global per compartment world from now on, since
> that will be the case soon.

Agreed.
 
> > If you want chrome to be able to shadow properties itself, but have the
> > shadowing visible only to chrome, that could be reasonable. In fact, that
> > _should_ be how things work today. There is one Xray wrapper for the content
> > object in the chrome compartment, and that Xray wrapper has an expando
> > object so that chrome can modify the wrapper, but not the underlying object
> > itself.
> 
> Exactly what I need, and I think it's reasonable to have this working. It
> might be fixed already, Alex do you have some test for this?

I doubt it works in the compartment-per-global world, because the expando object is per-wrapper, which effectively makes it per-compartment. I think it would be reasonable to find some way to make it per-origin, though.

Is it a security risk for the content to see these properties? If so, this might be a bit dubious.

> Ok. So there are no chrome compartments involved at all. There is a
> compartment from addon side, and there is the content compartment, the
> current web page. Both have the same (url based) principal. But we do want
> some kind of xraywrapper like behavior between them (which is possible to
> get via sandboxes).

Oh, I see. Jetpack passes wantXrays=true. Of course.

> The problem is that XrayWrappers have some odd behaviors
> that causes problems to us, and currently I'm trying to figure out that
> these issues we have are actualy bug to be fixed on the XrayWrappers or some
> of those are actual features, to avoid a security leak.

Can you specify what those are? There's a good chance they're bugs.

> Well, window.frames[0] works for sure, but I'm not so sure about the rest...
> I guess the best if we write some tests first

Yeah. If there are specific DOM-y behaviors that are getting screwed up by XrayWrapper, we should fix that.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #4)
> As a second thought I might completely misunderstand the one compartment per
> global principal.

There's no such thing as a global principal. The two situations are compartment-per-principal and compartment-per-global. Or maybe that was a typo? ;-)

I assumed that if I create a sandbox with let's say a url
> based principal, then a new global (+ compartment) will be created even if
> there is already an existing compartment with the same url principal (a tab
> in the browser for example with the same url). Am I wrong?

Sandbox creation currently creates a separate compartment by design, by passing in a separate |identity| pointer to disambiguate it. Soon, this whole mechanism will go away, because _every_ global will have its own compartment.
(In reply to Bobby Holley (:bholley) from comment #5)
> 
> I doubt it works in the compartment-per-global world, because the expando
> object is per-wrapper, which effectively makes it per-compartment. I think
> it would be reasonable to find some way to make it per-origin, though.

We need it the per compartment way, so it's fine. 

> 
> Is it a security risk for the content to see these properties? If so, this
> might be a bit dubious.

The security concern I have is about the custom DOM property getter methods. I'm not sure how safe are those, or if any of them can be abused in some known way. But we'll get back to this later when it's 100% defined what we really need with some tests.

> Oh, I see. Jetpack passes wantXrays=true. Of course.

Exactly.

> Can you specify what those are? There's a good chance they're bugs.

Yeah, I think the best if I write tests for all of them and then we can easily check them one by one.

> 
> Yeah. If there are specific DOM-y behaviors that are getting screwed up by
> XrayWrapper, we should fix that.
>

Alright. I would be happier to fix bugs on XrayWrapper, than implementing our own customized version...
 
> 
> There's no such thing as a global principal. The two situations are
> compartment-per-principal and compartment-per-global. Or maybe that was a
> typo? ;-)

Hehe, what I meant was one compartment per global concept, using the principal word here was quite misleading, sorry. so I get the compartment-per-global part, but my question is do we plan to have compartment-per-pirncipal too? So in the future if I have two sandboxes created with the same URL based principal, will they have the same global too? 

> Sandbox creation currently creates a separate compartment by design, by
> passing in a separate |identity| pointer to disambiguate it. Soon, this
> whole mechanism will go away, because _every_ global will have its own
> compartment.

But how will this be changed? I hope they will still have their own globals (and as a consequence their own compartment...), just we can get rid of that identity object hack. Right?
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #6)
> but my question is do we plan to have
> compartment-per-pirncipal too? So in the future if I have two sandboxes
> created with the same URL based principal, will they have the same global
> too?

Each sandbox has and always will have its own global, since this is crucial for sandboxes to work right.

Each global will (soon) have its own compartment - this is bug 650353, if you're interested in following along. It will no longer be possible for two globals to share a compartment.

> But how will this be changed? I hope they will still have their own globals
> (and as a consequence their own compartment...), just we can get rid of that
> identity object hack. Right?

Correct.
Attached file test
So I wrote some tests and it looks very good so far. The bugs indeed are fixed. There were only 2 cases that failed. First is overwriting some magic DOMClassInfor implemented native property: Components.utils.evalInSandbox("win.frames = 42, win.frames", sandbox) should be 42 but instead the expando is ignored here.

And second, which is unrelated to XrayWrappers, is the postMessage that seems not to work from inside a sandbox (last test: expected message received and test finishes, actual: no message received and test is hanging)
Attached file test entry point
Whoops, I forgot to add the entry point to the test, if anyone wants to try the chrome test, this file should be run.
Depends on: 733035
Comment on attachment 602899 [details]
test

I looks like this test is broken due to this line:
    var sandbox = Components.utils.Sandbox(window, { wantXrays: true });
`window` is a reference to a priviledged document and it looks like Sandbox doesn't create XrayWrappers, even when wantXrays is set to true.

  var sandbox = Components.utils.Sandbox(window, { wantXrays: true });
  sandbox.win = window;

  window.foo = "bar";
  Components.utils.reportError(
    Components.utils.evalInSandbox("win.foo", sandbox));
  // Prints "bar" ...

It is not a big deal as we can pass a non-priviledged document and get a valid test. The main issue is that these tests don't pass in reality. XrayWrappers still have all issues mentioned in bug 738244 comment 2.
Fixes on the xray wrapper for the content-proxy elimination pretty much fixes all these issues hence I close this one.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: