Closed
Bug 956382
Opened 11 years ago
Closed 11 years ago
Make consideration of document.domain the exception, not the rule
Categories
(Core :: Security: CAPS, defect)
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(13 files)
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
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.
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #8371732 -
Flags: review?(mrbkap)
Assignee | ||
Comment 4•11 years ago
|
||
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)
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #8371735 -
Flags: review?(mrbkap)
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #8371736 -
Flags: review?(mrbkap)
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #8371737 -
Flags: review?(mrbkap)
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #8371738 -
Flags: review?(mrbkap)
Assignee | ||
Comment 9•11 years ago
|
||
We can clean this stuff up in bug 968460.
Attachment #8371739 -
Flags: review?(mrbkap)
Assignee | ||
Comment 10•11 years ago
|
||
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)
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #8371742 -
Flags: review?(mrbkap)
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #8371743 -
Flags: review?(mrbkap)
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #8371744 -
Flags: review?(mrbkap)
Assignee | ||
Comment 14•11 years ago
|
||
Attachment #8371745 -
Flags: review?(mrbkap)
Assignee | ||
Comment 15•11 years ago
|
||
Attachment #8371746 -
Flags: review?(mrbkap)
Updated•11 years ago
|
Attachment #8371732 -
Flags: review?(mrbkap) → review+
Updated•11 years ago
|
Attachment #8371733 -
Flags: review?(mrbkap) → review+
Comment 16•11 years ago
|
||
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 17•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #8371737 -
Flags: review?(mrbkap) → review+
Updated•11 years ago
|
Attachment #8371739 -
Flags: review?(mrbkap) → review+
Updated•11 years ago
|
Attachment #8371738 -
Flags: review?(mrbkap) → review+
Comment 18•11 years ago
|
||
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+
Assignee | ||
Comment 19•11 years ago
|
||
(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. ;-)
Updated•11 years ago
|
Attachment #8371741 -
Flags: review?(mrbkap) → review+
Updated•11 years ago
|
Attachment #8371742 -
Flags: review?(mrbkap) → review+
Updated•11 years ago
|
Attachment #8371743 -
Flags: review?(mrbkap) → review+
Comment 20•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #8371745 -
Flags: review?(mrbkap) → review+
Updated•11 years ago
|
Attachment #8371746 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 21•11 years ago
|
||
Assignee | ||
Comment 22•11 years ago
|
||
Comment 23•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d4308d295f7d
https://hg.mozilla.org/mozilla-central/rev/a0fd5acb4ea5
https://hg.mozilla.org/mozilla-central/rev/4d72b6493e1d
https://hg.mozilla.org/mozilla-central/rev/d4c5f52beb20
https://hg.mozilla.org/mozilla-central/rev/5c99be23516c
https://hg.mozilla.org/mozilla-central/rev/5863efef36ed
https://hg.mozilla.org/mozilla-central/rev/3d34cdd10637
https://hg.mozilla.org/mozilla-central/rev/31e3a45440b8
https://hg.mozilla.org/mozilla-central/rev/ce0bc42b933d
https://hg.mozilla.org/mozilla-central/rev/d8de4338ba02
https://hg.mozilla.org/mozilla-central/rev/18d581a3cdda
https://hg.mozilla.org/mozilla-central/rev/5874930ca0f9
https://hg.mozilla.org/mozilla-central/rev/1f73309205ad
Assignee: nobody → bobbyholley
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Assignee | ||
Comment 24•11 years ago
|
||
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+
You need to log in
before you can comment on or make changes to this bug.
Description
•