913734 - Remove domain policy goop from CAPS

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Bobby Holley (:bholley)]

jst and I spoke about this a few weeks ago, and we think it can go away.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[Bobby Holley (:bholley)]

Boris and I decided that we should sort out what the new script enable/disable architecture looks like first, since otherwise these patches will break that stuff.

Comment 4

[Bobby Holley (:bholley)]

Note to self - I added a "mozilla::hotness" namespace in bug 840488 to disambiguate the new DomainPolicy class from the old one. That should be removed in these patches.

Comment 5

[Bobby Holley (:bholley)]

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

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 1 - Remove custom privilege setup for cut/copy/paste. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 2 - Stop storing security policies on XPCWrappedNativeProtos. v1

This is just a cache, so we can safely remove it without impacting correctness.
The rest of this mechanism goes away in subsequent patches.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 3 - Stop consulting domain policies in CAPS. v1

The whole LookupPolicy juggernaut is basically a mechanism for setting custom
per-(protocol, origin, property, action) access control in the preferences
service.

There are two sets of preferences currently in all.js. One of them is set up
for mailnews, for the mailbox:, imap:, and news: protocols. According to jst,
this was designed as a whack-a-mole security mechanism for javascript running
in HTML email. IIUC, we no longer allow JS to run at all in mailnews, so this
is obsolete.

The other mechanism appears to be our old-fashioned implementation of the
same-origin policy, which has been obsoleted by the new compartment
architecture.

In addition, most of this stuff was obsoleted by the new dom bindings, since
these DOM classes no longer go through XPCWrappedNativeJSOps, and thus no
longer trigger these security checks at all.

We stop using the infrastructure in this patch, and rip it out in the next one.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 4 - Remove now-unused policy machinery. v1

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 5 - Remove EnabledID() machinery. v1

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 6 - Remove namespace mozilla::hotness. v1

Comment 12

[Bobby Holley (:bholley)]

Blake, note that I added a new thing called "DomainPolicy" in bug 840488, which basically handles per-domain script blocking (to support things like NoScript). Let me know if you have any questions.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8345333 [details] [diff] [review]
Part 3 - Stop consulting domain policies in CAPS. v1

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

I assume you wanted me to review this too.

::: caps/src/nsScriptSecurityManager.cpp
@@ +641,5 @@
> +    // Note that while it would be nice to just rely on aClassName here, it
> +    // isn't set reliably by callers. :-(
> +    const char *className = aClassName;
> +    if (!className && jsObject) {
> +        className = JS_GetClass(jsObject)->name;

Nit (here and below): { on its own line in caps.

@@ +660,5 @@
> +    if (aCallContext && !classInfoData.IsDOMClass()) {
> +        rv = NS_ERROR_DOM_PROP_ACCESS_DENIED;
> +    } else {
> +        nsCOMPtr<nsIPrincipal> principalHolder;
> +        if (jsObject)

Nit: I'd reverse the sense of this if/else:

if (exceptional thing)
{
    NS_ERROR(...);
    return NS_ERROR_BLAH;
}
...

@@ +1558,5 @@
>      JS::RootedObject global(cx, js::UncheckedUnwrap(aGlobal, /* stopAtOuter = */ false));
>  
>      // Check the bits on the compartment private.
>      xpc::Scriptability& scriptability = xpc::Scriptability::Get(aGlobal);
> +    return scriptability.Allowed();

Any reason for the single-use variable here?

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8345334 [details] [diff] [review]
Part 4 - Remove now-unused policy machinery. v1

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

Nice to see all of this code go.

::: caps/idl/nsIPrincipal.idl
@@ -64,5 @@
> -    // write garbage?  If we need to give someone other than the security
> -    // manager a way to set this (which I question, since it can increase the
> -    // permissions of a page) it should be a |void clearSecurityPolicy()|
> -    // method.
> -    [noscript] attribute voidPtr securityPolicy;

Need to bump the IID here.

::: modules/libpref/src/init/all.js
@@ -663,5 @@
>  pref("editor.positioning.offset",            0);
>  
> -
> -// Default Capability Preferences: Security-Critical! 
> -// Editing these may create a security risk - be sure you know what you're doing

;-)

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8345336 [details] [diff] [review]
Part 6 - Remove namespace mozilla::hotness. v1

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

I dunno, the new stuff is pretty hot...

Comment 16

[Bobby Holley (:bholley)]

(In reply to Blake Kaplan (:mrbkap) from comment #13)
> ::: caps/src/nsScriptSecurityManager.cpp
> @@ +641,5 @@
> > +    // Note that while it would be nice to just rely on aClassName here, it
> > +    // isn't set reliably by callers. :-(
> > +    const char *className = aClassName;
> > +    if (!className && jsObject) {
> > +        className = JS_GetClass(jsObject)->name;
> 
> Nit (here and below): { on its own line in caps.

This does not seem consistently enforced at all. In fact, it seems mostly prevalent in the oldest code, which is going away. Do you think we should really keep doing that?

Comment 17

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #16)
> This does not seem consistently enforced at all. In fact, it seems mostly
> prevalent in the oldest code, which is going away. Do you think we should
> really keep doing that?

mrbkap and I decided on IRC that we shouldn't keep doing this going forward.

Addressed other review feedback. One final try push on all platforms:

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

Comment 18

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/26190b7a0b35
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/22929b380e84
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6c4dd8d796c6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4df87ccf0f5d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4dcc22e19dee
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/093a4a3a68ca

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/26190b7a0b35
https://hg.mozilla.org/mozilla-central/rev/22929b380e84
https://hg.mozilla.org/mozilla-central/rev/6c4dd8d796c6
https://hg.mozilla.org/mozilla-central/rev/4df87ccf0f5d
https://hg.mozilla.org/mozilla-central/rev/4dcc22e19dee
https://hg.mozilla.org/mozilla-central/rev/093a4a3a68ca

Comment 20

[Mike Kaply [:mkaply]]

So before this patch, did things like:

   user_pref("capability.policy.default.Window.open", "noAccess");

Still work?

Comment 21

[Bobby Holley (:bholley)]

(In reply to Mike Kaply (:mkaply) from comment #20)
> So before this patch, did things like:
> 
>    user_pref("capability.policy.default.Window.open", "noAccess");
> 
> Still work?

On Window, yes. But not on, say, Document, because that stuff was moved over to the WebIDL bindings (see comment 8), which have no way of hooking into CAPS prefs. So even if we hadn't landed this bug, it still would have stopped working with bug 789261 (which is landing this week).

Comment 22

[Mike Kaply [:mkaply]]

But there's no suitable replacement for any of this, right? Web page clipboard access for instance?

How is it we made all these changes and nothing made it into the Firefox 29 release notes?

Comment 23

[Bobby Holley (:bholley)]

(In reply to Mike Kaply (:mkaply) from comment #22)
> But there's no suitable replacement for any of this

What is 'this'? Granting special permissions to web pages, or turning off random web features?

> Web page clipboard access for instance?

That was a pretty odd one-off feature that I explicitly removed in part 1, with Blake's approval. Thankfully, nobody's complained about that one yet. :-)
 
> How is it we made all these changes and nothing made it into the Firefox 29
> release notes?

We definitely messed up not flagging this bug. But note that in general, it's pretty hard as platform developer to know what old Gecko-proprietary features are widely-enough used to justify keeping them around at the expense of the Open Web.

Comment 24

[Mike Kaply [:mkaply]]

(In reply to Bobby Holley (:bholley) from comment #23)
> (In reply to Mike Kaply (:mkaply) from comment #22)
> > But there's no suitable replacement for any of this
> 
> What is 'this'? Granting special permissions to web pages, or turning off
> random web features?

Granting special permissions to web pages I think is the most important part (based on what we're seeing).

Obviously turning off random web features is a side effect.

> > How is it we made all these changes and nothing made it into the Firefox 29
> > release notes?
> 
> We definitely messed up not flagging this bug. But note that in general,
> it's pretty hard as platform developer to know what old Gecko-proprietary
> features are widely-enough used to justify keeping them around at the
> expense of the Open Web.

I think we can pretty much assume that there is at least someone using everything we remove :)

But in all seriousness, the capability API is definitely an enterprise type feature that I know folks were using. The primary purose is to give sites special privileges for intranet applications. We should probably at least try to get something written up on the FF29 page (especially so that it makes it to the ESR 31 release notes).

Comment 25

[Bobby Holley (:bholley)]

(In reply to Mike Kaply (:mkaply) from comment #24)
> But in all seriousness, the capability API is definitely an enterprise type
> feature that I know folks were using. The primary purose is to give sites
> special privileges for intranet applications. We should probably at least
> try to get something written up on the FF29 page (especially so that it
> makes it to the ESR 31 release notes).

Sounds good. Can you oversee this? You definitely know a lot more about this than I do. ;-)

Comment 26

[Kohei Yoshino [:kohei]]

I'll add this to the site compat doc.

Comment 27

[Kohei Yoshino [:kohei]]

Corrections welcome!
https://developer.mozilla.org/en-US/Firefox/Releases/29/Site_Compatibility#Security

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

This was resolved in FF29 and was not notable enough to be in the release notes - it will show up in the changes between ESR24->ESR31 but as the release notes for ESR31 are merely a mirror of the general notes for FF31 and this change was not made to that version, there is nothing to note here.  Thanks Kohei for updating the site compat docs.  If this is a huge concern to the ESR deployments then perhaps calling attention to it on the mailing list will help keep people appraised of this specific change/removal.

Comment 29

[Mike Kaply [:mkaply]]

> This was resolved in FF29 and was not notable enough to be in the release notes 

I completely disagree. It wasn't that it wasn't notable enough, it was that it was forgotten.

This should be in the ESR release notes, if only because of the removal of clipboard CAPS.

Comment 30

[Bobby Holley (:bholley)]

Yeah, I think I agree. This generated a lot of pain on ff29 (see bug 995943), and ESR users are disproportionately affected. I think we should stick this in there.

Comment 31

[Sylvestre Ledru [:Sylvestre]]

Bobby, could you propose a wording for the release notes? Thanks

Comment 32

[Bobby Holley (:bholley)]

The CAPS infrastructure for specifying site-specific permissions (via capability.policy.* preferences) has been removed. Most notably, attempts to use this functionality to grant access to the clipboard will no longer work. The sole exception is the checkloaduri permission, which may still be used as before to allow sites to load file:// URIs.

Comment 33

[Sylvestre Ledru [:Sylvestre]]

Added to the release notes. Thanks