Closed
Bug 913734
Opened 11 years ago
Closed 10 years ago
Remove domain policy goop from CAPS
Categories
(Core :: Security: CAPS, defect)
Core
Security: CAPS
Tracking
()
RESOLVED
FIXED
mozilla29
Tracking | Status | |
---|---|---|
relnote-firefox | --- | 31+ |
People
(Reporter: bholley, Assigned: bholley)
References
(Depends on 1 open bug)
Details
(Keywords: dev-doc-complete, site-compat)
Attachments
(6 files)
7.78 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
27.07 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
8.35 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
51.50 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
1.78 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
2.91 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
jst and I spoke about this a few weeks ago, and we think it can go away.
Assignee | ||
Comment 1•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=df4714d5daed
Assignee | ||
Comment 2•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=6195607647e1
Assignee | ||
Comment 3•11 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•11 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 5•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=fcbbd8f6b833
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8345330 -
Flags: review?(mrbkap)
Assignee | ||
Comment 7•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
Attachment #8345334 -
Flags: review?(mrbkap)
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8345335 -
Flags: review?(mrbkap)
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8345336 -
Flags: review?(mrbkap)
Assignee | ||
Comment 12•10 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•10 years ago
|
Attachment #8345330 -
Flags: review?(mrbkap) → review+
Updated•10 years ago
|
Attachment #8345332 -
Flags: review?(mrbkap) → review+
Comment 13•10 years ago
|
||
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 14•10 years ago
|
||
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•10 years ago
|
Attachment #8345335 -
Flags: review?(mrbkap) → review+
Comment 15•10 years ago
|
||
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•10 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•10 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
Assignee | ||
Comment 18•10 years ago
|
||
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•10 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment 20•10 years ago
|
||
So before this patch, did things like: user_pref("capability.policy.default.Window.open", "noAccess"); Still work?
Assignee | ||
Comment 21•10 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).
Comment 22•10 years ago
|
||
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•10 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.
Comment 24•10 years ago
|
||
(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•10 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. ;-)
Comment 26•10 years ago
|
||
I'll add this to the site compat doc.
Comment 27•10 years ago
|
||
Corrections welcome! https://developer.mozilla.org/en-US/Firefox/Releases/29/Site_Compatibility#Security
Comment 28•10 years ago
|
||
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•10 years ago
|
||
> 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•10 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.
Comment 31•10 years ago
|
||
Bobby, could you propose a wording for the release notes? Thanks
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 32•10 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)
You need to log in
before you can comment on or make changes to this bug.
Description
•