Remove domain policy goop from CAPS

RESOLVED FIXED in mozilla29

Status

()

Core
Security: CAPS
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Depends on: 1 bug, {dev-doc-complete, site-compat})

unspecified
mozilla29
dev-doc-complete, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 31+)

Details

Attachments

(6 attachments)

(Assignee)

Description

4 years ago
jst and I spoke about this a few weeks ago, and we think it can go away.
(Assignee)

Comment 3

4 years ago
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.
Depends on: 840488
(Assignee)

Comment 4

4 years ago
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.
(Assignee)

Comment 6

4 years ago
Created attachment 8345330 [details] [diff] [review]
Part 1 - Remove custom privilege setup for cut/copy/paste. v1
Attachment #8345330 - Flags: review?(mrbkap)
(Assignee)

Comment 7

4 years ago
Created attachment 8345332 [details] [diff] [review]
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.
Attachment #8345332 - Flags: review?(mrbkap)
(Assignee)

Comment 8

4 years ago
Created attachment 8345333 [details] [diff] [review]
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.
(Assignee)

Comment 9

4 years ago
Created attachment 8345334 [details] [diff] [review]
Part 4 - Remove now-unused policy machinery. v1
Attachment #8345334 - Flags: review?(mrbkap)
(Assignee)

Comment 10

4 years ago
Created attachment 8345335 [details] [diff] [review]
Part 5 - Remove EnabledID() machinery. v1
Attachment #8345335 - Flags: review?(mrbkap)
(Assignee)

Comment 11

4 years ago
Created attachment 8345336 [details] [diff] [review]
Part 6 - Remove namespace mozilla::hotness. v1
Attachment #8345336 - Flags: review?(mrbkap)
(Assignee)

Comment 12

4 years ago
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.

Updated

4 years ago
Attachment #8345330 - Flags: review?(mrbkap) → review+

Updated

4 years ago
Attachment #8345332 - Flags: review?(mrbkap) → review+
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?
Attachment #8345333 - Flags: review+
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

;-)
Attachment #8345334 - Flags: review?(mrbkap) → review+

Updated

4 years ago
Attachment #8345335 - Flags: review?(mrbkap) → review+
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...
Attachment #8345336 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 16

4 years ago
(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?
(Assignee)

Comment 17

4 years ago
(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

Updated

4 years ago
Depends on: 951575

Updated

4 years ago
Depends on: 995943
So before this patch, did things like:

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

Still work?
(Assignee)

Comment 21

4 years ago
(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).
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?
(Assignee)

Comment 23

4 years ago
(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.
(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).
(Assignee)

Comment 25

4 years ago
(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. ;-)
I'll add this to the site compat doc.
relnote-firefox: --- → ?
Keywords: dev-doc-needed
OS: Mac OS X → All
Hardware: x86 → All
Corrections welcome!
https://developer.mozilla.org/en-US/Firefox/Releases/29/Site_Compatibility#Security
Keywords: dev-doc-needed → dev-doc-complete, site-compat

Updated

4 years ago
Depends on: 1004260
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.
relnote-firefox: ? → -
> 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.
(Assignee)

Comment 30

4 years ago
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.
relnote-firefox: - → ?
Bobby, could you propose a wording for the release notes? Thanks
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 32

4 years ago
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.
Flags: needinfo?(bobbyholley)
Added to the release notes. Thanks
relnote-firefox: ? → 31+

Updated

3 years ago
Depends on: 1061128

Updated

3 years ago
Depends on: 1061136

Updated

3 years ago
No longer depends on: 1061128
You need to log in before you can comment on or make changes to this bug.