Closed Bug 789224 Opened 8 years ago Closed 7 years ago

Remove vestigial capability code

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(7 files, 1 obsolete file)

The capability manager as we knew it is dead, thanks to bug 757046. Now it's time to reap some serious code cleanup.
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
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
Depends on: 789494
Last try push had an out-of-date base. Trying again:

https://tbpl.mozilla.org/?tree=Try&rev=b70b1f1ae770
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
Almost, just one more bug with respect to js:// URIs:

https://tbpl.mozilla.org/?tree=Try&rev=c4e891183f0b
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.
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
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
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.
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.
This stuff is super ugly and confusing. I think we're better off without it.
Blocks: 792695
No longer blocks: 792695
Blocks: 777467
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
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
Success! Looks like that did the trick. Uploading the updated patch, and flagging review on the entire set.
Attachment #664474 - Attachment is obsolete: true
Attachment #664473 - Flags: review?(mrbkap)
Attachment #664927 - Flags: review?(mrbkap)
Attachment #664475 - Flags: review?(mrbkap)
Attachment #664476 - Flags: review?(mrbkap)
Attachment #664477 - Flags: review?(mrbkap)
Attachment #664478 - Flags: review?(mrbkap)
Attachment #664479 - Flags: review?(mrbkap)
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
Attachment #664473 - Flags: review?(mrbkap) → review+
Attachment #664475 - Flags: review?(mrbkap) → review+
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.
Attachment #664476 - Flags: review?(mrbkap) → review+
Attachment #664477 - Flags: review?(mrbkap) → review+
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 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 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 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.
Attachment #664478 - Flags: review?(mrbkap) → review?(dveditz)
Attachment #664479 - Flags: review?(mrbkap) → review+
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.
Attachment #664927 - Flags: review?(mrbkap) → review+
Blocks: 797206
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.
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.
blocking-basecamp: --- → ?
Depends on: 799152
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.
blocking-basecamp: ? → +
No longer depends on: 799152
I pushed the bottom patch because of a dependency by bug 797204.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab15ebccc0c0
Whiteboard: [leave open]
Attachment #664473 - Flags: checkin+
Priority: -- → P1
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
Attachment #664478 - Flags: review?(dveditz) → review+
Blocks: 804174
(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.
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla19
Depends on: 807102
You need to log in before you can comment on or make changes to this bug.