The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla17

Status

()

Core
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

(Depends on: 2 bugs, Blocks: 2 bugs)

Trunk
mozilla17
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-kilimanjaro:+, blocking-basecamp:+)

Details

(Whiteboard: [qa-])

Attachments

(7 attachments, 17 obsolete attachments)

4.70 KB, patch
mounir
: checkin+
Details | Diff | Splinter Review
8.29 KB, patch
sicking
: review+
mounir
: checkin+
Details | Diff | Splinter Review
29.28 KB, patch
bholley
: review+
sicking
: superreview+
Details | Diff | Splinter Review
3.73 KB, patch
Details | Diff | Splinter Review
10.14 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
12.24 KB, patch
sicking
: review+
Details | Diff | Splinter Review
1.09 KB, patch
sicking
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
blocking-kilimanjaro: --- → ?
blocking-basecamp: --- → +
blocking-kilimanjaro: ? → +
Assignee: nobody → mounir
(Assignee)

Updated

5 years ago
Assignee: mounir → nobody
Component: DOM → General
QA Contact: general → general
Summary: Add a |jarIdentifier| attribute to nsIPrincipal for jar isolations → Add a dataId attribute to nsIPrincipal for app isolation
(Assignee)

Updated

5 years ago
Assignee: nobody → mounir
(Assignee)

Updated

5 years ago
Duplicate of this bug: 764204
(Assignee)

Updated

5 years ago
Summary: Add a dataId attribute to nsIPrincipal for app isolation → Add dataId and appStatus attributes to nsIPrincipal for app isolation
(Assignee)

Comment 2

5 years ago
Created attachment 639023 [details] [diff] [review]
Part 1 - Generate the data identifier using SHA1
Attachment #639023 - Flags: review?(jonas)
(Assignee)

Comment 3

5 years ago
Created attachment 639024 [details] [diff] [review]
Part 2 - add attributes to nsIPrincipal
Attachment #639024 - Flags: superreview?(jonas)
Attachment #639024 - Flags: review?(mrbkap)
(Assignee)

Comment 4

5 years ago
Created attachment 639026 [details] [diff] [review]
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?
Attachment #639026 - Flags: review?(mrbkap)
(Assignee)

Comment 5

5 years ago
Created attachment 639027 [details] [diff] [review]
Patch 4 - Pass the dataId to nsPrincipal::Init()
Attachment #639027 - Flags: review?(mrbkap)
(Assignee)

Comment 6

5 years ago
Created attachment 639029 [details] [diff] [review]
Part 5 - Update CreateCodeBasePrincipal()

We might need to audit some calls that are getting default values later.
Attachment #639029 - Flags: review?(jonas)
(Assignee)

Comment 7

5 years ago
Created attachment 639030 [details] [diff] [review]
Part 6 - Update GetChannelPrincipal()

Ditto, we might need to audit some calls getting default values.
Attachment #639030 - Flags: review?(jonas)
(Assignee)

Comment 8

5 years ago
Created attachment 639031 [details] [diff] [review]
Part 7 - Update GetCodeBasePrincipal()

We will have to audit some calls getting default values. Jonas volunteered to do that ;)
Attachment #639031 - Flags: review?(jonas)
(Assignee)

Updated

5 years ago
Depends on: 770831
(Assignee)

Comment 9

5 years ago
Created attachment 639042 [details] [diff] [review]
Part 8 - Use DocShell to get the document's principal

This part is dependant on bug 770831 and bug 770832.
Attachment #639042 - Flags: review?(jonas)
(Assignee)

Updated

5 years ago
Blocks: 770532
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);
Attachment #639023 - Flags: review?(jonas) → review+
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.
Attachment #639030 - Flags: review?(jonas) → review-
Blocks: 773373

Updated

5 years ago
Attachment #639024 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 12

5 years ago
Created attachment 641891 [details] [diff] [review]
Part 0 - Init tests
Attachment #641891 - Flags: review?(jonas)
(Assignee)

Comment 13

5 years ago
Created attachment 641893 [details] [diff] [review]
Part 1 - Generate the data identifier using SHA1. r=sicking

r=sicking
Attachment #639023 - Attachment is obsolete: true
Attachment #641893 - Flags: review?
(Assignee)

Comment 14

5 years ago
Created attachment 641894 [details] [diff] [review]
Part 2 - Add attributes to nsIPrincipal. r=mrbkap

r=mrbkap

I thought maybe we could name this "siloId" instead. Might be clearer.
Attachment #639024 - Attachment is obsolete: true
Attachment #639024 - Flags: superreview?(jonas)
Attachment #641894 - Flags: superreview?
(Assignee)

Updated

5 years ago
Attachment #641893 - Flags: review? → review?(jonas)
(Assignee)

Updated

5 years ago
Attachment #641894 - Flags: superreview? → superreview?(jonas)
(Assignee)

Comment 15

5 years ago
Created attachment 641895 [details] [diff] [review]
Part 3 - Pass the dataId to nsPrincipal::Init()
Attachment #639026 - Attachment is obsolete: true
Attachment #639027 - Attachment is obsolete: true
Attachment #639026 - Flags: review?(mrbkap)
Attachment #639027 - Flags: review?(mrbkap)
Attachment #641895 - Flags: review?(mrbkap)
(Assignee)

Comment 16

5 years ago
Created attachment 641896 [details] [diff] [review]
Part 4 - Update CreateCodeBasePrincipal()
Attachment #639029 - Attachment is obsolete: true
Attachment #639029 - Flags: review?(jonas)
Attachment #641896 - Flags: review?(jonas)
(Assignee)

Comment 17

5 years ago
Created attachment 641897 [details] [diff] [review]
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.
Attachment #639030 - Attachment is obsolete: true
Attachment #641897 - Flags: review?(jonas)
(Assignee)

Comment 18

5 years ago
Created attachment 641898 [details] [diff] [review]
Part 6 - Update GetCodeBasePrincipal()
Attachment #639031 - Attachment is obsolete: true
Attachment #639031 - Flags: review?(jonas)
Attachment #641898 - Flags: review?(jonas)
(Assignee)

Comment 19

5 years ago
Created attachment 641900 [details] [diff] [review]
Part 7 - Use DocShell to get the document's principal
Attachment #641900 - Flags: review?(jonas)
(Assignee)

Updated

5 years ago
Attachment #639042 - Attachment is obsolete: true
Attachment #639042 - Flags: review?(jonas)
(Assignee)

Updated

5 years ago
Attachment #641893 - Flags: review?(jonas)
Attachment #641891 - Flags: review?(jonas) → review+
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 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.
Attachment #641896 - Flags: review?(jonas) → review+
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.
Attachment #641897 - Flags: review?(jonas) → review-
(Assignee)

Updated

5 years ago
Depends on: 770894
(Assignee)

Comment 23

5 years ago
Created attachment 642791 [details] [diff] [review]
Part 0 - Init tests. r=sicking
Attachment #641891 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Summary: Add dataId and appStatus attributes to nsIPrincipal for app isolation → Add extendedOrigin, appd and appStatus attributes to nsIPrincipal for app isolation
(Assignee)

Comment 24

5 years ago
Created attachment 642800 [details] [diff] [review]
Part 1 - GetExtendedOrigin()
Attachment #641893 - Attachment is obsolete: true
Attachment #642800 - Flags: review?(jonas)
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.
Attachment #642800 - Flags: review?(jonas) → review+
(Assignee)

Comment 26

5 years ago
Created attachment 642844 [details] [diff] [review]
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.
Attachment #641894 - Attachment is obsolete: true
Attachment #641894 - Flags: superreview?(jonas)
Attachment #642844 - Flags: superreview?(jonas)
Attachment #642844 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 27

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

That parts has no meaning anymore.
Attachment #641895 - Attachment is obsolete: true
Attachment #641895 - Flags: review?(mrbkap)
(Assignee)

Updated

5 years ago
Depends on: 327244
(Assignee)

Comment 28

5 years ago
Created attachment 642869 [details] [diff] [review]
Part 3 - Update CreateCodeBasePrincipal(). r=sicking
Attachment #641896 - Attachment is obsolete: true
(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...
(Assignee)

Comment 30

5 years ago
(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...
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 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.
Attachment #642844 - Flags: review?(bobbyholley+bmo) → review+
Oh, one other question - is this stuff supposed to affect C++ security decisions when using these principals? If so, this patch is insufficient.
(Assignee)

Comment 34

5 years ago
(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?
(Assignee)

Updated

5 years ago
Attachment #641897 - Attachment is obsolete: true
(Assignee)

Comment 35

5 years ago
Created attachment 643163 [details] [diff] [review]
Part 4 - GetAppCodebasePrincipal / GetNoAppCodebasePrincipal / GetSimpleCodebasePrincipal
Attachment #641898 - Attachment is obsolete: true
Attachment #641898 - Flags: review?(jonas)
Attachment #643163 - Flags: superreview?(jonas)
Attachment #643163 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 36

5 years ago
For part 4, callers will be fixed by Jonas in bug 774585.
(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?
(Assignee)

Updated

5 years ago
Attachment #641900 - Flags: review?(jonas)
(Assignee)

Comment 38

5 years ago
(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 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.
Attachment #642844 - Flags: superreview?(jonas) → superreview+
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.
Attachment #643163 - Flags: superreview?(jonas) → superreview+
(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?
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.
(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 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.
(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.
(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.
(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.
(Assignee)

Updated

5 years ago
Depends on: 775354
> > 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?
(Assignee)

Updated

5 years ago
Whiteboard: [leave open]
(Assignee)

Updated

5 years ago
Attachment #642791 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #642800 - Flags: checkin+
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.
(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.
Sorry, I had missed that part of the comment. Thanks for the clarification!
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
Attachment #642791 - Flags: checkin+
Attachment #642800 - Flags: checkin+
(Assignee)

Comment 53

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

Has been pushed again.
Attachment #642791 - Flags: checkin+
(Assignee)

Comment 54

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

ditto
Attachment #642800 - Flags: checkin+
(Assignee)

Updated

5 years ago
Blocks: 775713
(Assignee)

Comment 55

5 years ago
Created attachment 644089 [details] [diff] [review]
Part 4 - GetAppCodebasePrincipal / GetNoAppCodebasePrincipal

There is no long GetSimpleCodebasePrincipal actualy...
Attachment #643163 - Attachment is obsolete: true
Attachment #643163 - Flags: review?(bobbyholley+bmo)
Attachment #644089 - Flags: review?(mrbkap)
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.
Attachment #644089 - Flags: review?(mrbkap) → review+
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
Followup:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe71c51ee0b9
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
Mounir: can this be marked fixed?
(Assignee)

Updated

5 years ago
Attachment #641900 - Attachment is obsolete: true
(Assignee)

Comment 61

5 years ago
(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.
(Assignee)

Updated

5 years ago
Depends on: 776296
(Assignee)

Updated

5 years ago
Depends on: 776297
(Assignee)

Comment 62

5 years ago
Created attachment 644696 [details] [diff] [review]
Tests
Attachment #644696 - Flags: review?(jonas)
(Assignee)

Comment 63

5 years ago
Created attachment 644697 [details] [diff] [review]
Part 5 - Change the extendedOrigin generation.
Attachment #644697 - Flags: review?(jonas)
Attachment #644696 - Flags: review?(jonas) → review+
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')
Attachment #644697 - Flags: review?(jonas) → review+
(Assignee)

Updated

5 years ago
Flags: in-testsuite+
Target Milestone: --- → mozilla17
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/b3db4a4b4003
https://hg.mozilla.org/mozilla-central/rev/a2c2baca43d2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Depends on: 776824
Depends on: 777582

Updated

5 years ago
Whiteboard: [qa-]
Blocks: 789710
You need to log in before you can comment on or make changes to this bug.