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

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 2

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=ebd91d330d2e

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Introduce an explicit API to do principal checks with document.domain. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Add AccessCheck::subsumesConsideringDomain and clean up other implementations. v1

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.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 3 - Consider document.domain when computing security wrappers. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 4 - Consider document.domain when deciding whether to return contentDocument. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 5 - Consider document.domain when doing Location security checks. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 6 - Consider document.domain for showModalDialog's dialogArguments and returnValue. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 7 - Consider document.domain in wacky postMessage security check. v1

We can clean this stuff up in bug 968460.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 8 - Remove useless security checks in nsDocument. v1

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).

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 9 - Ignore document.domain by default. v1

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 10 - Hoist nsScriptSecurityManager::CheckSameOriginPrincipal into nsPrincipal::EqualsConsideringDomain. v1

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 11 - Remove unused CAPS gunk. v1

Comment 14

[Bobby Holley (:bholley)]

Attachment: Part 12 - Remove usage of explicit *IgnoringDomain variants. v1

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 13 - Remove implementations of *IgnoringDomain variants. v1

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

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?

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 19

[Bobby Holley (:bholley)]

(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. ;-)

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 21

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=20ee7c0578b3

Comment 22

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=1f73309205ad

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Bobby Holley (:bholley)]

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.