789224 - Remove vestigial capability code

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect, P1)

Description

[Bobby Holley (:bholley)]

The capability manager as we knew it is dead, thanks to bug 757046. Now it's time to reap some serious code cleanup.

Comment 1

[Bobby Holley (:bholley)]

Not finished with the patches yet, but did an intermediate try push to see if anything's broken so far:

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

Comment 2

[Bobby Holley (:bholley)]

Removed enablePrivilege from a bunch of tests that didn't like the semantic change in nsContentUtils::IsCallerChrome, and repushed to try:

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

Comment 3

[Bobby Holley (:bholley)]

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

Comment 4

[Bobby Holley (:bholley)]

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

Comment 5

[Bobby Holley (:bholley)]

Last try push had an out-of-date base. Trying again:

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

Comment 6

[Bobby Holley (:bholley)]

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

Comment 7

[Bobby Holley (:bholley)]

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

Comment 8

[Bobby Holley (:bholley)]

Arg, certificate principals weren't as unused as I thought they were. It appears that we still need some of that machinery during addon install time. However, it _can_ be separated out of caps. Trying that:

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

Comment 9

[Bobby Holley (:bholley)]

Almost, just one more bug with respect to js:// URIs:

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

Comment 10

[Bobby Holley (:bholley)]

This is green except for a windows-only failures in:

browser/components/tabview/test/browser_tabview_bug663421.js
browser/components/tabview/test/browser_tabview_snapping.js

Which test Tab Preview, a windows-only feature. Investigating.

Comment 11

[Bobby Holley (:bholley)]

Ugh, can't reproduce locally. Trying various things, but in the mean time I'm going to nail down which changeset is causing the issues on try:

https://tbpl.mozilla.org/?tree=Try&rev=6d234b9893d2
https://tbpl.mozilla.org/?tree=Try&rev=dd2ea0ac781d
https://tbpl.mozilla.org/?tree=Try&rev=c60eb57b06e9

Comment 12

[Bobby Holley (:bholley)]

And somehow, I managed to trigger M-5 runs instead of M-o runs:

https://tbpl.mozilla.org/?tree=Try&rev=e81ce372630c
https://tbpl.mozilla.org/?tree=Try&rev=e9f868696570
https://tbpl.mozilla.org/?tree=Try&rev=a1d1474064f6

Comment 13

[Bobby Holley (:bholley)]

Alright, so the second patch, which rejiggers a bunch of privilege checks, is the culprit. Seeing as how I can't reproduce this locally, I'm pushing a bunch of logging code to try:

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

Comment 14

[Bobby Holley (:bholley)]

Attachment: Part 1 - Remove chrome check in xpc_EvalInSandbox. v1

Attaching the patches here for reference.

This function is also called to evaluate javascript:// URIs, at which point the chrome caller assumption isn't actually valid. Let's just do the modern thing here, and fix up the caller that breaks.

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 2 - Remove miscellaneous UniversalXPConnect checks sprinkled throughout gecko. v1

Comment 16

[Bobby Holley (:bholley)]

Attachment: Part 3 - Remove capability manager stuff in nsScriptSecurityManager. v1

Comment 17

[Bobby Holley (:bholley)]

Attachment: Part 4 - Remove principal capability and preference infrastructure. v1

Comment 18

[Bobby Holley (:bholley)]

Attachment: Part 5 - Remove signed script security checks. v1

Comment 19

[Bobby Holley (:bholley)]

Attachment: Part 6 - Separate certificate principals out from CAPS. v1

There's no longer any reason why "certificate principals" need to be principals at all.
I tried to rip them out entirely, but it looks like they're still used vestigially at XPI
install time to display author information. But there's no reason that they have to be
porkbarreled into the security-critical objects that we pass around all over the place.
So let's make them their own deal.

I was tempted to call them "certificate holders", but that would involve renaming methods and
cause more compat fuss than necessary.

Comment 20

[Bobby Holley (:bholley)]

Attachment: Part 7 - Remove CAPS_HACKER debugging instrumentation. v1

This stuff is super ugly and confusing. I think we're better off without it.

Comment 21

[Bobby Holley (:bholley)]

Sigh, the logging wasn't super helpful. I split the offending commit up some more and pushed parts of it one by one. I pushed only the first half of changes. If they're all green, I'll push more:

https://tbpl.mozilla.org/?tree=Try&rev=f75d0fd43b4b
https://tbpl.mozilla.org/?tree=Try&rev=6a03cf6e3dc3
https://tbpl.mozilla.org/?tree=Try&rev=2bfd09e506e6
https://tbpl.mozilla.org/?tree=Try&rev=27b3637aa8cb
https://tbpl.mozilla.org/?tree=Try&rev=56bfa263d5ab
https://tbpl.mozilla.org/?tree=Try&rev=d3b111683de1

Comment 22

[Bobby Holley (:bholley)]

The parts in comment 21 were all green - the plot thickens!

Pushing the rest of the changeset pieces incrementally:

https://tbpl.mozilla.org/?tree=Try&rev=fa261aecc597
https://tbpl.mozilla.org/?tree=Try&rev=0cbc178cc62b
https://tbpl.mozilla.org/?tree=Try&rev=d1bbfd18fe41
https://tbpl.mozilla.org/?tree=Try&rev=ee525ddd42e3
https://tbpl.mozilla.org/?tree=Try&rev=b5218c354c62
https://tbpl.mozilla.org/?tree=Try&rev=ba5cc48c8982

Comment 23

[Bobby Holley (:bholley)]

Ah, nsWindowWatcher! Of course... :\

Looks like I got the logical equivalencies ever-so-slightly wrong while simplifying some voodoo logic. Pushing the updated version for a win32 m-o run and crossing my fingers:

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

Comment 24

[Bobby Holley (:bholley)]

Attachment: Part 2 - Remove miscellaneous UniversalXPConnect checks sprinkled throughout gecko. v2

Success! Looks like that did the trick. Uploading the updated patch, and flagging review on the entire set.

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 664473 [details] [diff] [review]
Part 1 - Remove chrome check in xpc_EvalInSandbox. v1

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

::: dom/src/jsurl/nsJSProtocolHandler.cpp
@@ +290,5 @@
>          NS_ENSURE_SUCCESS(rv, rv);
>  
> +        // The nsXPConnect sandbox API gives us a wrapper to the sandbox for
> +        // our current compartment. Because our current context doesn't necessarily
> +        // subsumes that of the sandbox, we want to unwrap and enter the sandbox's

subsumes -> subsume

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 664476 [details] [diff] [review]
Part 4 - Remove principal capability and preference infrastructure. v1

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

It is really nice to see this code go away.

Comment 27

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 664927 [details] [diff] [review]
Part 2 - Remove miscellaneous UniversalXPConnect checks sprinkled throughout gecko. v2

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

::: content/base/src/nsContentUtils.cpp
@@ +1792,5 @@
>    if (NS_FAILED(rv)) {
>      return false;
>    }
> +  if (is_caller_chrome)
> +      return true;

{}, two-space

@@ +6039,5 @@
>      }
>    }
>  
> +  if (IsCallerChrome()) {
> +      return true;

two-space

::: docshell/base/nsDocShell.cpp
@@ +1668,5 @@
>  {
> +    // We want to bypass this check for chrome callers, but only if there's
> +    // JS on the stack. System callers still need to do it.
> +    if (nsContentUtils::GetCurrentJSContext() && nsContentUtils::IsCallerChrome())
> +      return true;

{}, and I think 4-space

@@ +1684,1 @@
>      rv = originDocument->NodePrincipal()->

nsresult rv = ...

::: dom/base/nsDOMClassInfo.h
@@ +552,1 @@
>    }

Inline this into the one caller instead (and remove the nsContentUtils.h include)

Comment 28

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 664477 [details] [diff] [review]
Part 5 - Remove signed script security checks. v1

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

::: content/xbl/src/nsXBLBinding.cpp
@@ +1507,5 @@
>    if (NS_FAILED(rv) || !canExecute) {
>      return false;
>    }
>  
> +  return true;

'return NS_SUCCEEDED(rv) && canExecute;'?

Comment 29

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 664478 [details] [diff] [review]
Part 6 - Separate certificate principals out from CAPS. v1

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

::: security/manager/ssl/src/nsCertificatePrincipal.cpp
@@ +31,5 @@
> +NS_IMETHODIMP
> +nsCertificatePrincipal::GetCertificate(nsISupports** aCert)
> +{
> +  nsCOMPtr<nsISupports> cert = mCert;
> +  *aCert = cert.forget().get();

cert.forget(aCert);

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 664478 [details] [diff] [review]
Part 6 - Separate certificate principals out from CAPS. v1

I don't understand the InstallTrigger stuff well enough to review this. The patch looks good to me though.

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 664927 [details] [diff] [review]
Part 2 - Remove miscellaneous UniversalXPConnect checks sprinkled throughout gecko. v2

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

::: content/xbl/src/nsXBLContentSink.cpp
@@ +730,5 @@
>        // have a concept of enabling capabilities on a per-principal basis,
>        // but only on a per-principal-and-JS-stackframe basis!  So for now
>        // this is basically equivalent to testing that we have the system
>        // principal, since there is no JS stackframe in sight here...
> +      if (nsContentUtils::IsSystemPrincipal(mDocument->NodePrincipal())) {

The XXX comment above this can change a little now.

Comment 32

[Bobby Holley (:bholley)]

dveditz - is this a review you can easily do? If not, can you suggest someone else? The patch is mostly code removal, and I'd really like to land this before the branch.

Comment 33

[Mounir Lamouri (:mounir)]

Bug 777467 is a basecamp blocker and depends on this bug. I think it would be nice to have it marked as a blocker to focus attention and have that last review done.

Comment 34

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

I'm a little bit worried about blocking on this one. If it turns out to be a bigger amount of work that is remaining than expected, we should reevaluate.

Comment 35

[Bobby Holley (:bholley)]

I pushed the bottom patch because of a dependency by bug 797204.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab15ebccc0c0

Comment 36

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ab15ebccc0c0

Comment 37

[Daniel Veditz [:dveditz]]

Comment on attachment 664478 [details] [diff] [review]
Part 6 - Separate certificate principals out from CAPS. v1

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

r=dveditz

::: caps/idl/nsIPrincipal.idl
@@ +95,1 @@
>       * XXXbz except see bug 147145!

You can zap this XXX note too -- bug 147145 (Freeze nsIPrincipal) is WONTFIX

Comment 38

[Bobby Holley (:bholley)]

Thanks Dan!

Updated and pushed to inbound:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c0dc5df119
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/77749ba52bbf
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/50d1235983d3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6980e4e0ceec
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/42b11a0fe323
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a578debf08b9

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d7c0dc5df119
https://hg.mozilla.org/mozilla-central/rev/77749ba52bbf
https://hg.mozilla.org/mozilla-central/rev/50d1235983d3
https://hg.mozilla.org/mozilla-central/rev/6980e4e0ceec
https://hg.mozilla.org/mozilla-central/rev/42b11a0fe323
https://hg.mozilla.org/mozilla-central/rev/a578debf08b9

Comment 40

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Bobby Holley (:bholley) from comment #8)
> Arg, certificate principals weren't as unused as I thought they were. It
> appears that we still need some of that machinery during addon install time.
> However, it _can_ be separated out of caps. Trying that:

All of that can be replaced by a new nsIZipReader::AreAllContentsOfThisJARSignedByTheSameTrustedCertificate() method that returns true/false. I am working on that now (as well as making this disk- and/or network- I/O-causing task asynchronous), as part of implementing signed package apps.

Comment 41

[Mounir Lamouri (:mounir)]

Bobby, my feeling is that bug should be marked FIXED. I think the [leave open] wasn't removed after the first patch landing.

Also, could you push those patches to mozilla-aurora? a=blocking-basecamp. I would need that to push bug 777467 to aurora.

Comment 42

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/713b35535da4
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/9ffb80107340
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/ee28bae3cd54
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/82a70417b502
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/78cc1d3e1d5a
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/d3453d0161c5
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/4d04726fe87b