Make consideration of document.domain the exception, not the rule

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla30
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(13 attachments)

11.02 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
3.36 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
2.48 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
1.81 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
892 bytes, patch
mrbkap
: review+
Details | Diff | Splinter Review
892 bytes, patch
mrbkap
: review+
Details | Diff | Splinter Review
1.43 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.01 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.36 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
5.05 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
3.05 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
11.13 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
12.62 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
At our security meetup in SF a few weeks ago, Hixie, abarth, and I spent a while complaining about document.domain and how it makes the spec much more complicated than it could otherwise be.

Hixie says his approach at present is to lock-down the number of existing cases where the spec uses Effective Script Origin (rather than Origin) to those strictly required for web-compat, and not use it in any new features.

At present, the list of cases that use Effective Script Origin is quite small - mostly for cross-global property access. In theory, if we explicitly handled document.domain in the computation of our security wrappers, almost everything else could start using EqualsIgnoringDomain and SubsumesIgnoringDomain. And then we could flip the default (with EqualsConsideringDomain and SubsumesConsideringDomain). This is a worthwhile thing for us to do, so that we don't accidentally introduce dependence on document.domain for new web features.

The first step would be to go through all of the existing uses of Subsumes and Equals, and converting as many of them to the IgnoringDomain variants as possible. Theoretically this should be around 95% of them, but if we find others, we should get the spec updated to reflect that.

I'm unlikely to have time for this any time soon, but I figured it was worth getting a bug on file.
As long as we make sure that cross-global property access works the way it should, I strongly suspect simply switching everything else to ignoring document.domain would be safe. Auditing is certainly nice, but I'd rather flip the switch than have nothing happen because no-one has time to do the Auditing.
Blocks: 968460
We now assert that we have a principal when we enter the wrap callback, and we
now have a convenient overload defined in nsIPrincipal.idl.
Attachment #8371733 - Flags: review?(mrbkap)
We can clean this stuff up in bug 968460.
Attachment #8371739 - Flags: review?(mrbkap)
In an o-cap world, we'll generally not get references to cross-origin elements.
And even if we do, we'll fail at the binding layer (when we try to CheckedUnwrap
the security wrapper).
Attachment #8371741 - Flags: review?(mrbkap)
Attachment #8371732 - Flags: review?(mrbkap) → review+
Attachment #8371733 - Flags: review?(mrbkap) → review+
Comment on attachment 8371735 [details] [diff] [review]
Part 3 - Consider document.domain when computing security wrappers. v1

Review of attachment 8371735 [details] [diff] [review]:
-----------------------------------------------------------------

This works and is safe because we recompute security wrappers when document.domain changes, right?
Attachment #8371735 - Flags: review?(mrbkap) → review+
Comment on attachment 8371736 [details] [diff] [review]
Part 4 - Consider document.domain when deciding whether to return contentDocument. v1

Review of attachment 8371736 [details] [diff] [review]:
-----------------------------------------------------------------

What's the advantage of explicitly considering here? I would actually expect this code to ignore document.domain so that either you always get the document or never get it for a given pair of documents.
Attachment #8371736 - Flags: review?(mrbkap)
Attachment #8371737 - Flags: review?(mrbkap) → review+
Attachment #8371739 - Flags: review?(mrbkap) → review+
Attachment #8371738 - Flags: review?(mrbkap) → review+
Comment on attachment 8371736 [details] [diff] [review]
Part 4 - Consider document.domain when deciding whether to return contentDocument. v1

Review of attachment 8371736 [details] [diff] [review]:
-----------------------------------------------------------------

bholley tells me on IRC that this is per spec.
Attachment #8371736 - Flags: review+
(In reply to Blake Kaplan (:mrbkap) from comment #16)
> This works and is safe because we recompute security wrappers when
> document.domain changes, right?

That's correct. Though TBH, if we didn't do that, we'd just end up with the same behavior as Blink. ;-)
Attachment #8371741 - Flags: review?(mrbkap) → review+
Attachment #8371742 - Flags: review?(mrbkap) → review+
Attachment #8371743 - Flags: review?(mrbkap) → review+
Comment on attachment 8371744 [details] [diff] [review]
Part 11 - Remove unused CAPS gunk. v1

Review of attachment 8371744 [details] [diff] [review]:
-----------------------------------------------------------------

It is really nice to see CheckPropertyAccessImpl go away.
Attachment #8371744 - Flags: review?(mrbkap) → review+
Attachment #8371745 - Flags: review?(mrbkap) → review+
Attachment #8371746 - Flags: review?(mrbkap) → review+
This is a spec alignment issue. Our existing test suite does a good job of verifying that document.domain is considered in the cases that we care about. This change flips the default so that we don't accidentally consider it in cases where we shouldn't.
Flags: in-testsuite? → in-testsuite+
Depends on: 1157127
Depends on: 1315146
You need to log in before you can comment on or make changes to this bug.