758258 - Add extendedOrigin, appd and appStatus attributes to nsIPrincipal for app isolation

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Mounir Lamouri (:mounir)]

The concept of data jars (bug 756644) will require a way to identify with which jar a principal is associated with. The same way, permissions might need more information than simply origin and we could use the jar identifier here too.

I will attach a very basic patch to get early feedback from Jonas. Goes without saying: the hardest part of the work will be to have that new attribute used in other components.

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - Generate the data identifier using SHA1

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - add attributes to nsIPrincipal

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: Part 3 - Update Principals equality algorithm

I wonder: should we allow installed/trusted/whatever to access non-installed/untursted/whatever content if they are from the same origin?

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Patch 4 - Pass the dataId to nsPrincipal::Init()

Comment 6

[Mounir Lamouri (:mounir)]

Attachment: Part 5 - Update CreateCodeBasePrincipal()

We might need to audit some calls that are getting default values later.

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Part 6 - Update GetChannelPrincipal()

Ditto, we might need to audit some calls getting default values.

Comment 8

[Mounir Lamouri (:mounir)]

Attachment: Part 7 - Update GetCodeBasePrincipal()

We will have to audit some calls getting default values. Jonas volunteered to do that ;)

Comment 9

[Mounir Lamouri (:mounir)]

Attachment: Part 8 - Use DocShell to get the document's principal

This part is dependant on bug 770831 and bug 770832.

Comment 10

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

Comment on attachment 639023 [details] [diff] [review]
Part 1 - Generate the data identifier using SHA1

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

::: caps/src/nsScriptSecurityManager.cpp
@@ +3605,5 @@
> +nsScriptSecurityManager::GenerateDataId(PRUint32 aAppId,
> +                                        nsIURI* aURI, nsIURI* aBrowserURI,
> +                                        nsACString& aGenerateDataId)
> +{
> +  MOZ_ASSERT(aURI);

Can you also assert that if aAppID == NO_APP_ID then aBrowserURI is null? Maybe even do a runtime abort if that isn't the case.

@@ +3610,5 @@
> +
> +  // Fallback.
> +  if (aAppId == NO_APP_ID && !aBrowserURI) {
> +    nsCAutoString origin;
> +    GetOriginFromURI(aURI, origin);

Move these two lines to outside of the if-branch since you need it either way.

@@ +3627,5 @@
> +
> +
> +  nsCAutoString key;
> +  key.AppendInt(aAppId);
> +  key.Append(origin + browserOrigin);

Hmm.. I think we need a safe separator here so that we can't get collisions using specially crafted schemes or some such.

How about
key.Append(NS_LITERAL_CSTRING(':') + origin +
           NS_LITERAL_CSTRING(':') + browserOrigin);

Comment 11

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

Comment on attachment 639030 [details] [diff] [review]
Part 6 - Update GetChannelPrincipal()

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

I don't think this is entirely correct. We should have different app-ids for "null app" (like in firefox) and "unknown app" (when we don't have enough context to figure out which app a load is coming from). The latter is useful in many situations when the principal won't be used to call into the permissionmanager or access indexedDB/localStorage data.

Comment 12

[Mounir Lamouri (:mounir)]

Attachment: Part 0 - Init tests

Comment 13

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - Generate the data identifier using SHA1. r=sicking

r=sicking

Comment 14

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Add attributes to nsIPrincipal. r=mrbkap

r=mrbkap

I thought maybe we could name this "siloId" instead. Might be clearer.

Comment 15

[Mounir Lamouri (:mounir)]

Attachment: Part 3 - Pass the dataId to nsPrincipal::Init()

Comment 16

[Mounir Lamouri (:mounir)]

Attachment: Part 4 - Update CreateCodeBasePrincipal()

Comment 17

[Mounir Lamouri (:mounir)]

Attachment: Part 5 - Update GetChannelPrincipal()

The UNKNOWN_APP_ID case is handled by nsScriptSecurityManager::GenerateDataId(). We will have to look at each consumer to know what they should pass.

Comment 18

[Mounir Lamouri (:mounir)]

Attachment: Part 6 - Update GetCodeBasePrincipal()

Comment 19

[Mounir Lamouri (:mounir)]

Attachment: Part 7 - Use DocShell to get the document's principal

Comment 20

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

Comment on attachment 641894 [details] [diff] [review]
Part 2 - Add attributes to nsIPrincipal. r=mrbkap

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

I D

::: caps/idl/nsIPrincipal.idl
@@ +220,5 @@
> +     */
> +    readonly attribute AUTF8String dataId;
> +
> +    const short APP_STATUS_NOT_INSTALLED = 0;
> +    const short APP_STATUS_INSTALLED     = 1;

I don't understand these values. Is "NOT_INSTALLED" the same as "in desktop firefox"? If so, could we call them "NO_APP" and "UNKNOWN_APP"?

Comment 21

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

Comment on attachment 641896 [details] [diff] [review]
Part 4 - Update CreateCodeBasePrincipal()

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

r=me with that addressed

::: caps/src/nsScriptSecurityManager.cpp
@@ +2024,5 @@
> +
> +    // An app is installed if it has an app id and it is not inside a mozbrowser.
> +    PRUint16 appStatus =
> +      aAppId != nsIAppsService::NO_APP_ID &&
> +      aAppId != nsIScriptSecurityManager::UNKNOWN_APP_ID &&

Ah, I think I see how this works. I don't think we should have app IDs spread out over multiple interfaces.

@@ +2027,5 @@
> +      aAppId != nsIAppsService::NO_APP_ID &&
> +      aAppId != nsIScriptSecurityManager::UNKNOWN_APP_ID &&
> +      aBrowserOrigin.IsEmpty()
> +        ? nsIPrincipal::APP_STATUS_INSTALLED
> +        : nsIPrincipal::APP_STATUS_NOT_INSTALLED;

Ok, trying to figure out when we need this. Should we maybe have a third state which is APP_STATUS_UNKNOWN? Seems somewhat scary to me to map UNKNOWN_APP_ID to APP_STATUS_NOT_INSTALLED when we don't actually know that that is correct.

Comment 22

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

Comment on attachment 641897 [details] [diff] [review]
Part 5 - Update GetChannelPrincipal()

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

::: caps/src/nsScriptSecurityManager.cpp
@@ +2907,5 @@
>                                                  PRUint32 redirFlags,
>                                                  nsIAsyncVerifyRedirectCallback *cb)
>  {
>      nsCOMPtr<nsIPrincipal> oldPrincipal;
> +    GetChannelPrincipal(oldChannel, nsIAppsService::NO_APP_ID, EmptyCString(),

I think this should be UNKNOWN_APP_ID

::: content/base/src/nsContentUtils.cpp
@@ +5655,5 @@
>      return NS_ERROR_NOT_AVAILABLE;
>  
>    nsCOMPtr<nsIPrincipal> oldPrincipal;
>    nsContentUtils::GetSecurityManager()->
> +    GetChannelPrincipal(aOldChannel, nsIAppsService::NO_APP_ID, EmptyCString(),

UNKNOWN_APP_ID?

::: content/base/src/nsDocument.cpp
@@ +2059,1 @@
>                                             getter_AddRefs(principal));

Shouldn't you be getting the appid and browser origin from the docShell here?

If not, I think this should be UNKNOWN_APP_ID

::: content/base/src/nsScriptLoader.cpp
@@ +1195,5 @@
>    // principal as the origin principal
>    if (aRequest->mCORSMode == CORS_NONE) {
>      rv = nsContentUtils::GetSecurityManager()->
> +      GetChannelPrincipal(channel, nsIAppsService::NO_APP_ID, EmptyCString(),
> +                          getter_AddRefs(aRequest->mOriginPrincipal));

Same here, get the appid/browserorigin from mDocument's docshell, or use UNKNOWN_APP_ID.

Hmm... I wonder if a better solution is to make GetChannelPrincipal internally get the docshell using the loadgroup and get the appid/browserorigin there.

And use UNKNOWN_APP_ID if there is no loadgroup (which should basically never happen).

Stopping review here for further discussion.

Comment 23

[Mounir Lamouri (:mounir)]

Attachment: Part 0 - Init tests. r=sicking

Comment 24

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - GetExtendedOrigin()

Comment 25

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

Comment on attachment 642800 [details] [diff] [review]
Part 1 - GetExtendedOrigin()

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

::: caps/idl/nsIScriptSecurityManager.idl
@@ +266,5 @@
> +     * appId can be nsIAppsService::NO_APP_ID,
> +     * nsIScriptSecurityManager::UNKWOWN_APP_ID or a valid app id.
> +     * inMozBrowser has to be true if the uri is inside a mozbrowser iframe.
> +     */
> +    const unsigned long UNKNOWN_APP_ID = 4294967295; // PR_UINT32_MAX

I don't really care which interface NO_APP_ID and UNKNOWN_APP_ID lives on, but it's very confusing to have them on different interfaces. Put them on the same.

::: caps/src/nsScriptSecurityManager.cpp
@@ +3629,5 @@
> +  // aExtendedOrigin = origin + " " + aAppId + " " + int(aInMozBrowser)
> +  aExtendedOrigin.Assign(origin + NS_LITERAL_CSTRING(" "));
> +  aExtendedOrigin.AppendInt(aAppId);
> +  aExtendedOrigin.AppendLiteral(" ");
> +  aExtendedOrigin.AppendLiteral(aInMozBrowser ? "1" : "0");

Don't use AppendLiteral unless you're passing a raw literal. Here you are passing a char*. You can use Append and pass '0' and '1' instead.

And if you use "t" and "f" instead of 0/1 you don't need the last separator.

Comment 26

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Add attributes to nsIPrincipal.

Part 2 was reviewed by Blake but it has changed quite a lot so it needs another review. Bholley seems to be the new caps/ guy so asking him.

Comment 27

[Mounir Lamouri (:mounir)]

Comment on attachment 641895 [details] [diff] [review]
Part 3 - Pass the dataId to nsPrincipal::Init()

That parts has no meaning anymore.

Comment 28

[Mounir Lamouri (:mounir)]

Attachment: Part 3 - Update CreateCodeBasePrincipal(). r=sicking

Comment 29

[Bobby Holley (:bholley)]

(In reply to Mounir Lamouri (:mounir) from comment #26)

>  Bholley seems to be the new caps/ guy so asking him.

Sort of. I'm touching a lot of code in caps and doing some reviews, but mrbkap is still the owner.

Why do we need the extendedOrigin string? Why can't interested consumers pull the appropriate information off of the other fields? It seems like we should avoid codifying a string-format-y API if we can help it...

Comment 30

[Mounir Lamouri (:mounir)]

(In reply to Bobby Holley (:bholley) from comment #29)
> (In reply to Mounir Lamouri (:mounir) from comment #26)
> 
> >  Bholley seems to be the new caps/ guy so asking him.
> 
> Sort of. I'm touching a lot of code in caps and doing some reviews, but
> mrbkap is still the owner.
> 
> Why do we need the extendedOrigin string? Why can't interested consumers
> pull the appropriate information off of the other fields? It seems like we
> should avoid codifying a string-format-y API if we can help it...

I do not agree. We want consumers of .extendedOrigin to actually use that string as an opaque string to isolate data/permissions using that information instead of .origin. Requesting consumers to recreate the string would be more work and would be error-prone. In addition, this string is for the moment quite human-readable but in future releases it might we might go back to a hashed string (which was the initial idea). This said, we might name it something else...

Comment 31

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

The idea with the string is that it should be treated as an opaque identifier. It allows us to change the strategy for what we consider the boundaries of a "cookie jar". For example, if we change how mozbrowser works such that websites are allowed to be browsers then the mozbrowser information might change from being a true/false flag to being the origin of the page which contains the browser.

Comment 32

[Bobby Holley (:bholley)]

Comment on attachment 642844 [details] [diff] [review]
Part 2 - Add attributes to nsIPrincipal.

>+
>+    /**
>+     * Returns the extended origin of the principal.
>+     * The extended origin is a string that has more information than the origin
>+     * and can be used to isolate data or permissions between different
>+     * principals while taking into account parameters like the app id or the
>+     * fact that the principal is embedded in a mozbrowser.
>+     * Some principals will return the origin for extendedOrigin.
>+     * Some principals will assert if you try to access the extendedOrigin.
>+     */
>+    readonly attribute AUTF8String extendedOrigin;

Per comment 31, can you note that this is intended to be an opaque identifier?

>   nsresult InitFromPersistent(const char* aPrefName,
>                               const nsCString& aFingerprint,
>                               const nsCString& aSubjectName,
>                               const nsACString& aPrettyName,
>                               const char* aGrantedList,
>                               const char* aDeniedList,
>                               nsISupports* aCert,
>                               bool aIsCert,
>-                              bool aTrusted);
>+                              bool aTrusted,
>+                              PRUint32 aAppId,
>+                              bool aInMozBrowser);

rather than adding these parameters and then just passing in the empty defaults, let's just make InitFromPersistent automatically use those empty values in its constructor.

>
> NS_IMETHODIMP
>+nsPrincipal::GetExtendedOrigin(nsACString& aExtendedOrigin)
>+{
>+  if (mAppId == nsIScriptSecurityManager::UNKNOWN_APP_ID) {
>+    MOZ_ASSERT(false);

I don't agree with this pattern. If script shouldn't be able to make it happen, MOZ_ASSERT against it and forget about it. Otherwise, check for it and throw.

>+NS_IMETHODIMP
>+nsPrincipal::GetAppStatus(PRUint16* aAppStatus)
>+{
>+  if (mAppId == nsIScriptSecurityManager::UNKNOWN_APP_ID) {
>+    MOZ_ASSERT(false);

Similarly.


>+NS_IMETHODIMP
>+nsPrincipal::GetAppId(PRUint32* aAppId)
>+{
>+  if (mAppId == nsIScriptSecurityManager::UNKNOWN_APP_ID) {
>+    MOZ_ASSERT(false);

And here.

>+  PRUint32 appId;
>+  rv = aStream->Read32(&appId);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  bool inMozBrowser;
>+  rv = aStream->ReadBoolean(&inMozBrowser);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = Init(fingerprint, subjectName, prettyName, cert, codebase, appId, inMozBrowser);

I don't think we should change the serialization stream unless we can prove with tests that it works. I don't think this stuff is really used much anymore, so I'd just skip this.


>+PRUint16
>+nsPrincipal::GetAppStatus()
>+{
>+  MOZ_ASSERT(mAppId != nsIScriptSecurityManager::UNKNOWN_APP_ID);

Make sure this isn't triggerable from script either.

>+  // aExtendedOrigin = origin + " " + aAppId + " " + int(aInMozBrowser)
>+  aExtendedOrigin.Assign(origin + NS_LITERAL_CSTRING(" "));
>+  aExtendedOrigin.AppendInt(aAppId);
>+  aExtendedOrigin.Append(aInMozBrowser ? NS_LITERAL_CSTRING("t")
>+                                       : NS_LITERAL_CSTRING("f"));

this does not match the format in the comment.

r=bholley with those comments addressed.

Comment 33

[Bobby Holley (:bholley)]

Oh, one other question - is this stuff supposed to affect C++ security decisions when using these principals? If so, this patch is insufficient.

Comment 34

[Mounir Lamouri (:mounir)]

(In reply to Bobby Holley (:bholley) from comment #33)
> Oh, one other question - is this stuff supposed to affect C++ security
> decisions when using these principals? If so, this patch is insufficient.

We are planning to update ::Equals. Is there anything that would be required?

Comment 35

[Mounir Lamouri (:mounir)]

Attachment: Part 4 - GetAppCodebasePrincipal / GetNoAppCodebasePrincipal / GetSimpleCodebasePrincipal

Comment 36

[Mounir Lamouri (:mounir)]

For part 4, callers will be fixed by Jonas in bug 774585.

Comment 37

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

(In reply to Bobby Holley (:bholley) from comment #32)
> > NS_IMETHODIMP
> >+nsPrincipal::GetExtendedOrigin(nsACString& aExtendedOrigin)
> >+{
> >+  if (mAppId == nsIScriptSecurityManager::UNKNOWN_APP_ID) {
> >+    MOZ_ASSERT(false);
> 
> I don't agree with this pattern. If script shouldn't be able to make it
> happen, MOZ_ASSERT against it and forget about it. Otherwise, check for it
> and throw.

The reason we are doing this is to avoid regressions in Firefox. In Firefox, we'll basically always use NO_APP_ID and UNKNOWN_APP_ID principals. By making the two behave the same, except that we assert when UNKNOWN_APP_ID is used, debug builds will help us find bugs in the code, while release builds behave just like they always have. In Firefox this won't be a security problem since we for now just have one "cookie jar" anyway.

In B2G, where this actually matters security-wise, it would make sense to update this to return an error or some such though. But I don't know if it's worth adding a compile-time flag for now?

Comment 38

[Mounir Lamouri (:mounir)]

(In reply to Jonas Sicking (:sicking) from comment #37)
> In B2G, where this actually matters security-wise, it would make sense to
> update this to return an error or some such though. But I don't know if it's
> worth adding a compile-time flag for now?

Could be done in a follow-up. That was my plane actually.

Comment 39

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

Comment on attachment 642844 [details] [diff] [review]
Part 2 - Add attributes to nsIPrincipal.

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

r=me with those things fixed.

::: caps/idl/nsIPrincipal.idl
@@ +224,5 @@
> +     */
> +    readonly attribute AUTF8String extendedOrigin;
> +
> +    const short APP_STATUS_NOT_INSTALLED = 0;
> +    const short APP_STATUS_INSTALLED     = 1;

Add APP_STATUS_TRUSTED and APP_STATUS_CERTIFIED here too to make it more clear what this is.

::: caps/src/nsScriptSecurityManager.cpp
@@ +3617,5 @@
> +  MOZ_ASSERT(aURI);
> +
> +  if (aAppId == nsIScriptSecurityManager::UNKNOWN_APP_ID) {
> +    aExtendedOrigin.Truncate();
> +    return;

Hmm.. Another option would be to have this function treat UNKNOWN_APP_ID as NO_APP_ID after an assertion. That might catch more places where we could have bugs. Either way it doesn't seem right to just silently return an empty string here without at least asserting.

@@ +3623,5 @@
> +
> +  // Fallback.
> +  if (aAppId == nsIScriptSecurityManager::NO_APP_ID && !aInMozBrowser) {
> +    nsCAutoString origin;
> +    aURI->GetOrigin(origin);

Does this return a punycode encoded URL? That's what we should return.

Why the temporary here? You could just call GetOrigin(aExtendedOrigin).

@@ +3630,5 @@
> +    return;
> +  }
> +
> +  nsCAutoString origin;
> +  aURI->GetOrigin(origin);

Actually, make this GetOrigin(aExtendedOrigin) and move it before the if-statement above since both branches do it.

Comment 40

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

Comment on attachment 643163 [details] [diff] [review]
Part 4 - GetAppCodebasePrincipal / GetNoAppCodebasePrincipal / GetSimpleCodebasePrincipal

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

::: caps/src/nsScriptSecurityManager.cpp
@@ +2005,5 @@
> +                                                 bool aInMozBrowser,
> +                                                 nsIPrincipal** aPrincipal)
> +{
> +  NS_ENSURE_TRUE(aAppId != nsIScriptSecurityManager::UNKNOWN_APP_ID,
> +                 NS_ENSURE_INVALID_ARG);

NS_ERROR_INVALID_ARG?

@@ +2008,5 @@
> +  NS_ENSURE_TRUE(aAppId != nsIScriptSecurityManager::UNKNOWN_APP_ID,
> +                 NS_ENSURE_INVALID_ARG);
> +  NS_ENSURE_TRUE(aAppId != nsIScriptSecurityManager::NO_APP_ID ||
> +                 aInMozBrowser,
> +                 NS_ENSURE_INVALID_ARG);

Why shouldn't you be able to call this function with NO_APP/false? Seems annoying for callers to have to check for that combo and call getNoAppCodebasePrincipal.

Comment 41

[Bobby Holley (:bholley)]

(In reply to Jonas Sicking (:sicking) from comment #37)
> The reason we are doing this is to avoid regressions in Firefox. In Firefox,
> we'll basically always use NO_APP_ID and UNKNOWN_APP_ID principals. By
> making the two behave the same, except that we assert when UNKNOWN_APP_ID is
> used, debug builds will help us find bugs in the code, while release builds
> behave just like they always have. In Firefox this won't be a security
> problem since we for now just have one "cookie jar" anyway.

This doesn't make sense to me. Doesn't this just mean that Firefox debug builds will assert?

> In B2G, where this actually matters security-wise, it would make sense to
> update this to return an error or some such though. But I don't know if it's
> worth adding a compile-time flag for now?

If you want this to assert in B2g only, then yes, I think a compile-time flag is warranted. Is there not one already?

Comment 42

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

The goal is to make Firefox debug builds assert when we have a bug in the code which might affect B2G (as well as future versions of firefox which implements cookie jars), while making Firefox release builds work with low risk of regression.

Comment 43

[Bobby Holley (:bholley)]

(In reply to Jonas Sicking (:sicking) from comment #42)
> The goal is to make Firefox debug builds assert when we have a bug in the
> code which might affect B2G (as well as future versions of firefox which
> implements cookie jars), while making Firefox release builds work with low
> risk of regression.

If we're asserting against something in a debug build, then tinderbox will break if we try to land code that violates that condition. If honoring that condition is something we can't control (i.e., could be violated by third-party scripts), then IMO we need to check/throw and not assert, since assert should generally not be triggerable from script (although I know there are exceptions to this).

Put another way - for almost every asserted condition in Gecko, we'd probably rather handle the failure than crash if it were to happen in production. But littering the code with |assert(foo); if (!foo) {...}| has a very high gunkification cost.

So I don't think we should be doing this unless we have a very specific concern. There may be one here, but I haven't heard it articulated.

Comment 44

[Bobby Holley (:bholley)]

Comment on attachment 643163 [details] [diff] [review]
Part 4 - GetAppCodebasePrincipal / GetNoAppCodebasePrincipal / GetSimpleCodebasePrincipal

>     /**
>      * Return a principal that has the same origin as aURI.
>+     * This principals should not be used for any data/permission check, it will
>+     * have appId = UNKNOWN_APP_ID.

Really? That seems like a total footgun to me. Principals are almost always used for security checks. Why do we need this API?


>-    nsIPrincipal getCodebasePrincipal(in nsIURI aURI);
>+    nsIPrincipal getSimpleCodebasePrincipal(in nsIURI aURI);
>+
>+    /**
>+     * Returns a principal that has the given information.
>+     * @param appId is the app id of the principal. It can't be UNKNOWN_APP_ID.
>+     * It can be NO_APP_ID only if inMozBrowser is true.
>+     * @param inMozBrowser is true if the principal has to be considered as
>+     * inside a mozbrowser frame.
>+     */
>+    nsIPrincipal getAppCodeBasePrincipal(in nsIURI uri,
>+                                         in unsigned long appId,
>+                                         in boolean inMozBrowser);
>+
>+    /**
>+     * Returns a principal with that has the same origin as uri and is not part
>+     * of an appliction.
>+     * The returned principal will have appId = NO_APP_ID.
>+     */
>+    nsIPrincipal getNoAppCodeBasePrincipal(in nsIURI uri);

I assume getNoAppCodeBasePrincipal will be used by desktop firefox and such?


>+NS_IMETHODIMP
>+nsScriptSecurityManager::GetNoAppCodebasePrincipal(nsIURI* aURI,
>+                                                   nsIPrincipal** aPrincipal)
>+{
>+  return GetCodebasePrincipal(aURI,  nsIScriptSecurityManager::NO_APP_ID,
>+                              false, aPrincipal);

please add /* aInMozBrowser = */ to the boolean parameter, since it's not otherwise clear from context.


holding off on review until we figure out the getSimpleCodebasePrincipal stuff.

Comment 45

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #32)
> >   nsresult InitFromPersistent(const char* aPrefName,
> >                               const nsCString& aFingerprint,
> >                               const nsCString& aSubjectName,
> >                               const nsACString& aPrettyName,
> >                               const char* aGrantedList,
> >                               const char* aDeniedList,
> >                               nsISupports* aCert,
> >                               bool aIsCert,
> >-                              bool aTrusted);
> >+                              bool aTrusted,
> >+                              PRUint32 aAppId,
> >+                              bool aInMozBrowser);
> 
> rather than adding these parameters and then just passing in the empty
> defaults, let's just make InitFromPersistent automatically use those empty
> values in its constructor.

Scratch that comment - I now see it's used in the later patch.

Comment 46

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

(In reply to Bobby Holley (:bholley) from comment #43)
> If we're asserting against something in a debug build, then tinderbox will
> break if we try to land code that violates that condition.

That's exactly what I want.

> If honoring that
> condition is something we can't control (i.e., could be violated by
> third-party scripts), then IMO we need to check/throw and not assert, since
> assert should generally not be triggerable from script (although I know
> there are exceptions to this).

It's not that we can't control it. It's that it's a bug if we end up in that situation and we need to fix that bug. And it's likely that we have that bug *somewhere* in the code.

> Put another way - for almost every asserted condition in Gecko, we'd
> probably rather handle the failure than crash if it were to happen in
> production. But littering the code with |assert(foo); if (!foo) {...}| has a
> very high gunkification cost.
> So I don't think we should be doing this unless we have a very specific
> concern. There may be one here, but I haven't heard it articulated.

I disagree. Assertions are useful for finding bugs. That is their primary purpose. We don't want to wallpaper over these bugs, we want to find and fix them.

Handling the situation that we are asserting about is useful in situations where we aren't reasonably sure that the assertion won't fire. In this case I would say that we're reasonably sure that it will fire, but the only way to find all those situations is with an assertion.

Maybe one way of thinking about it is that in this case the code flow is complex enough that we don't feel confident that this can't ever happen, hence we want the code to handle it. But it's really bad any time it does happen it's really bad and we need to fix it ASAP, hence we want the assertion so that tinderbox, people doing fuzzing and people running debug builds will help us find it.

Let me put it this way, we have the following facts:

* Any time that we end up getting the extendedOrigin or any other of these new properties on a UNKNOWN_APP principal we have a bug in our code.
* We should always fix that bug, but obviously we need to find the bug in order to fix it.
* Any time we have such a bug it's a really bad bug for B2G, and will likely a bad bug in the future for Firefox (once we start doing cookie jars there).
* However any time we have such a bug it doesn't matter in Firefox today.
* It's unlikely that we have tests for all conditions that can trigger those bugs. I.e. pushing to try won't help us find all the instances of this bug.
* It's highly likely that we have these types of bugs.

With these facts in mind, what do you propose that we do?


I'm happy to make B2G builds use a runtime abort even in release builds once we have done a little more testing. However I'm not ok with doing that in Firefox given that the bug doesn't actually affect firefox.

Comment 47

[Bobby Holley (:bholley)]

(In reply to Jonas Sicking (:sicking) from comment #46)
> (In reply to Bobby Holley (:bholley) from comment #43)
> > If we're asserting against something in a debug build, then tinderbox will
> > break if we try to land code that violates that condition.
> 
> That's exactly what I want.
> 
> > If honoring that
> > condition is something we can't control (i.e., could be violated by
> > third-party scripts), then IMO we need to check/throw and not assert, since
> > assert should generally not be triggerable from script (although I know
> > there are exceptions to this).
> 
> It's not that we can't control it. It's that it's a bug if we end up in that
> situation and we need to fix that bug. And it's likely that we have that bug
> *somewhere* in the code.
> 
> > Put another way - for almost every asserted condition in Gecko, we'd
> > probably rather handle the failure than crash if it were to happen in
> > production. But littering the code with |assert(foo); if (!foo) {...}| has a
> > very high gunkification cost.
> > So I don't think we should be doing this unless we have a very specific
> > concern. There may be one here, but I haven't heard it articulated.
> 
> I disagree. Assertions are useful for finding bugs. That is their primary
> purpose. We don't want to wallpaper over these bugs, we want to find and fix
> them.
> 
> Handling the situation that we are asserting about is useful in situations
> where we aren't reasonably sure that the assertion won't fire. In this case
> I would say that we're reasonably sure that it will fire, but the only way
> to find all those situations is with an assertion.
> 
> Maybe one way of thinking about it is that in this case the code flow is
> complex enough that we don't feel confident that this can't ever happen,
> hence we want the code to handle it. But it's really bad any time it does
> happen it's really bad and we need to fix it ASAP, hence we want the
> assertion so that tinderbox, people doing fuzzing and people running debug
> builds will help us find it.
> 
> Let me put it this way, we have the following facts:
> 
> * Any time that we end up getting the extendedOrigin or any other of these
> new properties on a UNKNOWN_APP principal we have a bug in our code.
> * We should always fix that bug, but obviously we need to find the bug in
> order to fix it.
> * Any time we have such a bug it's a really bad bug for B2G, and will likely
> a bad bug in the future for Firefox (once we start doing cookie jars there).
> * However any time we have such a bug it doesn't matter in Firefox today.
> * It's unlikely that we have tests for all conditions that can trigger those
> bugs. I.e. pushing to try won't help us find all the instances of this bug.
> * It's highly likely that we have these types of bugs.
> 
> With these facts in mind, what do you propose that we do?

Assert (fatally) against it and don't handle it. Maybe make it a runtime abort anywhere security is so entirely compromised that we'd rather crash than continue (and where the check doesn't severely affect perf). If it can make it all the way through 18 weeks on tinderbox without it firing in a debug build, then I don't think release users are going to be hitting it left and right (and as mentioned, if we care enough about it, we can crash when they hit it). I think that handling cases that Really Must Not Happen just delays fixing them.

This is our approach all throughout pretty much all the pieces of code that I'm familiar with (and I work on a lot of security-sensitive code), so I'm only objecting to this on the grounds that I don't see why we should be doing something different here. But you're obviously working on the critical path of B2G here, and this is totally not very important. Do what you like.

Comment 48

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

> > With these facts in mind, what do you propose that we do?
> 
> Assert (fatally) against it and don't handle it.

This runs a significant risk of regressing firefox. What is the advantage of this approach to make that risk worth it?

Also, what do you mean by "don't handle it"? Do you simply mean to not have code for it? As long as we don't runtime abort we will run some code which means that we'll be handling it for some definition of handle.


> Maybe make it a runtime
> abort anywhere security is so entirely compromised that we'd rather crash
> than continue (and where the check doesn't severely affect perf).

This is what I've been saying is the plan to do in a followup patch for B2G. We need build-time flags for this as well as a bit more testing under our belt before I feel comfortable with it.

> If it can
> make it all the way through 18 weeks on tinderbox without it firing in a
> debug build, then I don't think release users are going to be hitting it
> left and right (and as mentioned, if we care enough about it, we can crash
> when they hit it). I think that handling cases that Really Must Not Happen
> just delays fixing them.

I'm not convinced that we're more likely to find these bugs by "not handling it" than by doing what the patch does.

> This is our approach all throughout pretty much all the pieces of code that
> I'm familiar with (and I work on a lot of security-sensitive code), so I'm
> only objecting to this on the grounds that I don't see why we should be
> doing something different here. But you're obviously working on the critical
> path of B2G here, and this is totally not very important. Do what you like.

We quite frequently add assertions without adding any other code to make sure that the error is surfaced in any other way. This will effectively many times result in us "handling it".

It sounds like your whole objection is about four lines of code that we are temporarily adding in order to not disrupt B2G and Firefox development.

I really rather spend my time focusing on more important issues. Can we please just move on?

Comment 49

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

Put it another way:

What makes this situation special is that I'm very worried that we'll hit this assertion in ways that tinderbox won't trigger. So if we cause that to abort or otherwise "fail" in release builds, we'll disrupt the development.

The other thing that makes this special is that it's temporary. Once we feel more certain that the code is correct we should remove the code to handle the error.

Comment 50

[Bobby Holley (:bholley)]

(In reply to Jonas Sicking (:sicking) from comment #48)
> I really rather spend my time focusing on more important issues. Can we
> please just move on?

Yes, by all means! I said so in comment 47. This is totally not worth spending time on.

(In reply to Jonas Sicking (:sicking) from comment #49)
> What makes this situation special is that I'm very worried that we'll hit
> this assertion in ways that tinderbox won't trigger. So if we cause that to
> abort or otherwise "fail" in release builds, we'll disrupt the development.
> 
> The other thing that makes this special is that it's temporary. Once we feel
> more certain that the code is correct we should remove the code to handle
> the error.

That sounds entirely reasonable.

Comment 51

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

Sorry, I had missed that part of the comment. Thanks for the clarification!

Comment 52

[Steve Fink [:sfink] [:s:]]

Pushed with:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1bafff5720a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/afc1cf222996

Backed out for suspected mochitest-other breakage:

http://hg.mozilla.org/integration/mozilla-inbound/rev/d4ce853fa8bf
http://hg.mozilla.org/integration/mozilla-inbound/rev/e852fdc80ca2

220 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | the correct URL should have been loaded - got file:///private/tmp/, expected file:///tmp

Comment 53

[Mounir Lamouri (:mounir)]

Comment on attachment 642791 [details] [diff] [review]
Part 0 - Init tests. r=sicking

Has been pushed again.

Comment 54

[Mounir Lamouri (:mounir)]

Comment on attachment 642800 [details] [diff] [review]
Part 1 - GetExtendedOrigin()

ditto

Comment 55

[Mounir Lamouri (:mounir)]

Attachment: Part 4 - GetAppCodebasePrincipal / GetNoAppCodebasePrincipal

There is no long GetSimpleCodebasePrincipal actualy...

Comment 56

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 644089 [details] [diff] [review]
Part 4 - GetAppCodebasePrincipal / GetNoAppCodebasePrincipal

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

::: caps/src/nsScriptSecurityManager.cpp
@@ +1978,5 @@
>  }
>  
>  NS_IMETHODIMP
> +nsScriptSecurityManager::GetCodebasePrincipal(nsIURI* aURI,
> +                                                    nsIPrincipal** aPrincipal)

Nit: indentation is off here.

Comment 57

[Ed Morley [:emorley]]

Parts 3-5 repushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/04d93909f3de
https://hg.mozilla.org/integration/mozilla-inbound/rev/93ed679edcc9
https://hg.mozilla.org/integration/mozilla-inbound/rev/a91856fdca2f

Comment 58

[Ed Morley [:emorley]]

Followup:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe71c51ee0b9

Comment 59

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f907bf664d47
https://hg.mozilla.org/mozilla-central/rev/c1fc9ea724ac
https://hg.mozilla.org/mozilla-central/rev/04d93909f3de
https://hg.mozilla.org/mozilla-central/rev/93ed679edcc9
https://hg.mozilla.org/mozilla-central/rev/a91856fdca2f

Comment 60

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

Mounir: can this be marked fixed?

Comment 61

[Mounir Lamouri (:mounir)]

(In reply to Jonas Sicking (:sicking) from comment #60)
> Mounir: can this be marked fixed?

I would like to push the tests before marking it fixed.

Comment 62

[Mounir Lamouri (:mounir)]

Attachment: Tests

Comment 63

[Mounir Lamouri (:mounir)]

Attachment: Part 5 - Change the extendedOrigin generation.

Comment 64

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

Comment on attachment 644697 [details] [diff] [review]
Part 5 - Change the extendedOrigin generation.

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

::: caps/src/nsScriptSecurityManager.cpp
@@ +3644,2 @@
>    aExtendedOrigin.AppendInt(aAppId);
> +  aExtendedOrigin.Append(NS_LITERAL_CSTRING("+" ) + origin + NS_LITERAL_CSTRING("+"));

Remove the space after the "+"

@@ +3646,2 @@
>    aExtendedOrigin.Append(aInMozBrowser ? NS_LITERAL_CSTRING("t")
>                                         : NS_LITERAL_CSTRING("f"));

Technically you can put this with the other expression. But if you want to keep it separate for readability, then just do .Append(browser ? 't' : 'f')

Comment 65

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b3db4a4b4003
https://hg.mozilla.org/mozilla-central/rev/a2c2baca43d2