Skip to content
This repository has been archived by the owner on Feb 26, 2022. It is now read-only.

Bug 601295 - Make worker sandboxes non-privileged, but compatible with jQuery and other frameworks without leaking objects to document #171

Merged
merged 8 commits into from Jun 1, 2011

Conversation

ochameau
Copy link
Contributor

Implement Proxy over Xraywrappers in order to properly define and secure content scripts scope:

  • avoid to be tricked by websites by any method/object overload,
  • avoid polluting webpage with content scripts values,
  • remove privileges from sandbox and use website principal.

These wrappers are based on top of XrayWrappers in order to ensure that we call native methods like getElementById, addEventListener, ... and avoid polluting document scope by setting JS values only on these wrappers instead of document objects.
We had to use proxy as XrayWrappers doesn't implement any Javascript feature like:

  • expando attributes like: div.onclick = function () {}
  • document.formName or window.frameName accessors
  • overloading native methods from the content script
    And it allowed us to fix some bugs around wrappers that disallow using some popular JS framework in Firefox 4 (see bugs 658909 and 658560)

test.waitUntilDone();

let page = Page({
onMessage: function(message) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's strange that these two test functions are assigned to the same property, since it means only the second one is being called.

Nevertheless, they test different things and should both be run (and modified to work without access to the unwrapped window object).


return fun.apply(getProxyForObject(this), args);
};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So from what I see here, I can tell that anyone who has access to fun can access __proxy or even swap it. I don't yet know if that's a real issue, but regardless comment explaining that will be appropriate here.

@Gozala
Copy link
Contributor

Gozala commented Jun 3, 2011

Alex, that was a lot of work man, thanks!!

I made lot's of comments, but in summary I think it's just 5 things, from those first 3 are really important IMO:

  1. You should not trust built in methods of objects that you do not create as they may be overridden for diff reasons. Instead you should always use Object.prototype.* or Function.prototype.* etc..
  2. You always need to delegate to the original methods even if you think they are build-in one as they may be overridden:
    Bug 601295 - Make worker sandboxes non-privileged, but compatible with jQuery and other frameworks without leaking objects to document #171 (comment)
  3. You need to make small fixes to make sure your internal properties can't be compromised (overriding __proxy will allow attacker to get access to things you're trying to restrict access to): Bug 601295 - Make worker sandboxes non-privileged, but compatible with jQuery and other frameworks without leaking objects to document #171 (comment)
  4. Diff suggestions to minimize complexity of the code like removing __isWrappedProxy etc..
  5. Suggestions on improving code readability and maintainability.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants