774585 - Update GetCodebasePrincipal callers to use the correct "data jar"

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

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

Right now GetCodebasePrincipal only takes a single argument, the URI to create a principal for.

We're turning this into three separate functions:
GetCodebasePrincipal: Used when we don't know or don't care which data jar is used. These principals are not allowed to be used to access local data.
GetAppCodebasePrincipal: Takes an additional app-id and mozbrowser flag and creates a principal which has full access to local data.
GetNoAppCodebasePrincipal: Same as GetAppCodebasePrincipal but always uses the "null app".

Patches coming up which changes necessary GetCodebasePrincipal callers.

Comment 1

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

Attachment: Update about: redirectors to remove getCodebasePrincipal calls

Currently we are very careful to ensure that when a channel is created for a about: URI, we always call SetOwner with a "about:foo"-uri codebase principal. This to avoid that the channel gets a system principal.

However the only reason that the channel would get a system principal is because the chrome channel calls SetOwner(systemPrincipal). Otherwise we should be getting the correct principal.

So this patch simply makes us call SetOwner(null) so that about: uris that shouldn't have the system principal follows the normal codepaths in GetChannelPrincipal and gets a codebase principal for the original about: URI.

Comment 2

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

Attachment: Make sessionstorage code pass in docshell when creating principal

This makes sessionstorage call GetAppCodebasePrincipal when creating principals. There's also some cleanup to use principal.extendedOrigin rather than using URI.prePath and remove the only getSessionStorageForURI caller.

Comment 3

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

Attachment: Remove unused aTarget argument

Comment 4

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

Attachment: Fix xpc-sandbox creation code

Comment 5

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

Attachment: Fix browser code and tests

Comment 6

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

Attachment: Update about: redirectors to remove getCodebasePrincipal calls

Somehow I missed the mobile about: redirectors. This patch includes them too.

Comment 7

[Mounir Lamouri (:mounir)]

(In reply to Jonas Sicking (:sicking) from comment #6)
> Somehow I missed the mobile about: redirectors. This patch includes them too.

You also missed the reviewer ;)

Comment 8

[Mounir Lamouri (:mounir)]

Could you also change calls from GetCodebasePrincipal() to GetSimpleCodebasePrincipal() so we make sure all consumers know what they are doing, see bug 758258.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 642879 [details] [diff] [review]
Fix xpc-sandbox creation code

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +3407,5 @@
>          do_GetService(kScriptSecurityManagerContractID);
>      NS_ENSURE_TRUE(secman, NS_ERROR_FAILURE);
>  
> +    // It would be cool to allow passing in the app-id and mozbrowser info
> +    // to the sandbox constructor. But right now we won't have that ability.

Isn't this use-case taken care of by passing in a window instead of a string?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 643102 [details] [diff] [review]
Update about: redirectors to remove getCodebasePrincipal calls

Please add some comments explaining why setting null will do the right thing, ok?

r=me

Comment 11

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

Comment on attachment 643102 [details] [diff] [review]
Update about: redirectors to remove getCodebasePrincipal calls

https://hg.mozilla.org/integration/mozilla-inbound/rev/096a755c48ea

Comment 12

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

Comment on attachment 642878 [details] [diff] [review]
Remove unused aTarget argument

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

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/096a755c48ea
https://hg.mozilla.org/mozilla-central/rev/ebfa4531ca57

Comment 14

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

I must have forgotten to hit save when adding [leave open] to the whiteboard.

Comment 15

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

Attachment: Make sessionStorage/localStorage code create the correct principals

Comment 16

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

Attachment: Fix browser code and tests

Comment 17

[Mounir Lamouri (:mounir)]

Comment on attachment 643682 [details] [diff] [review]
Make sessionStorage/localStorage code create the correct principals

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

r=me with the comments applied.

::: browser/components/sessionstore/src/SessionStorage.jsm
@@ +56,2 @@
>  
> +      if (principal) {

As long as you are here:
if (!principal) {
  return data;
}

@@ +140,5 @@
>     * Returns a given history entry's URI.
>     * @param aHistory
>     *        That tab's session history
>     * @param aIndex
>     *        The history entry's index

Update the doc.

Comment 18

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

Attachment: Add GetDocShellCodebasePrincipal

I wish we could have called this method GetAppCodebasePrincipal, but XPCOM idl doesn't allow overloads :(

Comment 19

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

Attachment: Make getChannelPrincipal get the correct app principal

Comment 20

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

Comment on attachment 644199 [details] [diff] [review]
Make getChannelPrincipal get the correct app principal

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

the test changes here are by Mounir and are rs=me

Comment 21

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

Attachment: Make sessionStorage code create the correct principals

Actually passes tests now

Comment 22

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

Attachment: Rename getCodebasePrincipal to getSimpleCodebasePrincipal

Comment 23

[Mounir Lamouri (:mounir)]

Comment on attachment 644220 [details] [diff] [review]
Rename getCodebasePrincipal to getSimpleCodebasePrincipal

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

You should probably update nsIScriptSecurityManager uuid. Not a big deal given that it has been updated many times for a lot of related patches.

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2bac914080d
https://hg.mozilla.org/integration/mozilla-inbound/rev/365d6054c8b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/8906997e57e6

Plus followup:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f976bd5e456

Comment 25

[Ed Morley [:emorley]]

Even after a landing, backout, relanding, multiple followups, disable test_principal_extendedorigin_appid_appstatus.html still unfortunately failed, so I've just disabled the whole test for now:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7ff9297eedf

I realise the current Try situation is dire (bug 772458), but inbound has now been busted for 40 consecutive pushes (~90 changesets worth); and pretty much all due to this bug and those related to it. This causes delays for all other devs, means some people start dumping on mozilla-central - and so when I finally get green and merge the ~140 outstanding inbound changesets to m-c, I'm much more likely to get conflicts (in areas of the codebase I have no idea about). We'll also likely have more bustage later today, as everyone who has been delayed sees the tree is open and dumps on it straight away.

I really do feel your pain about Try (and the urgency surrounding B2G goals), which is why I've tried to be more flexible in this instance (eg international calls to mounir to wake him up rather than just back everything out), but I don't see this as being very sustainable moving forwards.

The best course of action would be for you to both ask your manager to put pressure on those responsible for bug 772458 - I've tried to help out on it in my spare time, but I'm not really that familiar with the build/infra stuff, so it needs some #build/#it/#???? loving.

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d2bac914080d
https://hg.mozilla.org/mozilla-central/rev/365d6054c8b6
https://hg.mozilla.org/mozilla-central/rev/8906997e57e6
https://hg.mozilla.org/mozilla-central/rev/d7ff9297eedf

Comment 27

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2328647c5d6d
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3972de3f4a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/224e9f2148b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/24d60aa478bb

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/24d60aa478bb
https://hg.mozilla.org/mozilla-central/rev/224e9f2148b2
https://hg.mozilla.org/mozilla-central/rev/c3972de3f4a5
https://hg.mozilla.org/mozilla-central/rev/2328647c5d6d

Comment 29

[Adam Kowalczyk]

I've just tripped over this change in my extension. I don't know how common it is for extensions to use this interface (probably less than it should...) but I'm gonna mark it addon-compat.

Comment 30

[Ian Nartowicz]

This breaks addons.  I don't know how many, but one of mine at least.  Renaming the deprecated method (is it going to go completely one day?) with no transition period is not exactly helpful to people who need to provide support to older versions while making an addon work with the latest version.

Comment 31

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

The reason we removed the existing functions was that enough of the security model changed enough that we couldn't keep both models around, and we wanted everyone to make sure that all code call the appropriate function.

However the new model has turned out to be more B2G specific than we intended. The plan is to fix that as soon as we have time (and as soon as Firefox intends to take advantage of cookie jars).

I'd be fine with naming back getNoAppCodebasePrincipal to getCodebasePrincipal if it's really needed. But if it's not affecting too many addons I'd prefer to keep things as-is.

Comment 32

[Jorge Villalobos [:jorgev] (he/him)]

getCodebasePrincipal is used by at least a couple dozen add-ons. Does naming the function back as getCodebasePrincipal fixes the compatibility issue, though? Didn't its signature change in any way?

Comment 33

[Boris Zbarsky [:bzbarsky]]

All that changed was the name, not the signature.

I do think we should change the name back....

Comment 34

[Alex Keybl [:akeybl]]

(In reply to Jonas Sicking (:sicking) from comment #31)
> I'd be fine with naming back getNoAppCodebasePrincipal to
> getCodebasePrincipal if it's really needed. But if it's not affecting too
> many addons I'd prefer to keep things as-is.

Let's track this in bug 806587.