1010434 - NeckoParent matches a wrong app for the request of a new RemoteOpenFile.

Status: RESOLVED FIXED

(Core :: Networking: File, defect)

Description

[Thinker Li [:sinker]]

NeckoParent matches random one of apps in the same content process for checking the permission of opening a pkg file.  It causes fault alarms of permission error.

Comment 1

[Thinker Li [:sinker]]

see https://bugzilla.mozilla.org/show_bug.cgi?id=1002897#c39
and https://bugzilla.mozilla.org/show_bug.cgi?id=1002897#c44

Comment 2

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: try matching manifestURLs of all PBrowserParents

Comment 3

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Activity apps are grouped only in tarako.

Comment 4

[[:fabrice] Fabrice Desré]

Comment on attachment 8423029 [details] [diff] [review]
Patch: try matching manifestURLs of all PBrowserParents

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

I'm not convinced this is the correct fix.
Let's say we have two apps in the same process: app://app1/manifest.webapp and app://app2/manifest.webapp

If app1 that has no webapps-manage permission tries to access app://app2/script.js this patch lets that happen.

But I'm not sure we can fix that without actually keeping track of which app initiated the request. It looks like all the security checks in AllocPRemoteOpenFileParent are bogus when running several apps in the same process.

::: netwerk/ipc/NeckoParent.cpp
@@ +569,5 @@
> +          return nullptr;
> +        }
> +        nsPrintfCString urlToMatch("%s/%s/application.zip",
> +                                   NS_LossyConvertUTF16toASCII(basePath).get(),
> +                                   NS_LossyConvertUTF16toASCII(uuid).get());

nit: s/urlToMatch/pathToMatch

@@ +575,5 @@
> +          matchManifestURL = true;
> +          break;
> +        } else {
> +          mustMatch.Append(urlToMatch);
> +          mustMatch.AppendLiteral(", ");

add a comment explaining that we do that only to print the error.

Comment 5

[[:fabrice] Fabrice Desré]

Jason, how do you think we should fix that? It seems we need to get the app id from the channel owner and propagate that back to AllocPRemoteOpenFileParent instead of relying of the appId of the TabParent.

Comment 6

[Jason Duell]

> Let's say we have two apps in the same process: app://app1/manifest.webapp and
> app://app2/manifest.webapp If app1 that has no webapps-manage permission tries
> to access app://app2/script.js this patch lets that happen.  But I'm not sure
> we can fix that without actually keeping track of which app initiated the
> request.

Well, the problem here IIUC is that we have no reliable way in the parent (that I know of, at least) to distinguish between "this request came from app1 in process foo" vs "this request is being forged by app2 to look like it came from app1, which lives in the same process".  The security is per-process, as I understand it.

bent: is there a way to get true security/isolation here, i.e. to guarantee which app in a multi-app process is making an IPDL connection/request?

We should at least implement something which doesn't allow naive attempts by app1 to open app2's files. That could rely on the appID passed via the SerializedLoadContext (i.e. it would trust the self-reported appID from the child).  We'd need to make sure the self-reported appID is one of the appIDs in the ManagedPBrowserParent list, of course.

Comment 7

[[:fabrice] Fabrice Desré]

Hm, actually we get the app uri as a parameter to AllocPRemoteOpenFileParent. So we could check which tabparent matches this app uri and do our checks against that tab.

Does that sound right?

Comment 8

[Jason Duell]

Fabrice: that provides an alternate way to figure out which appID is requesting the file (by assuming that only app1 will ask for app://app1 URIs).  It doesn't address the security issue of app2 asking for app:/app1 URIs, but if we have no way to protect against that, then I guess it's fine. I slightly prefer checking the SerializedLoadContext just because it has an explicit appId field, but if that's harder to write it's no big deal.

Comment 9

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

If there are two apps in a single process, and we don't have a reliable way to tell which app is requesting the file, the protection will be based on what child told parent. So could passing PBrowser as a parameter to AllocPRemoteOpenFileParent be a right way to implement this?

Comment 10

[Thinker Li [:sinker]]

(In reply to Fabrice Desré [:fabrice] from comment #7)
> Hm, actually we get the app uri as a parameter to
> AllocPRemoteOpenFileParent. So we could check which tabparent matches this
> app uri and do our checks against that tab.

Since this uri comes from a content process, I have security concern.  I think it even more worse than checking all associated PBrowserParents.

Comment 11

[Thinker Li [:sinker]]

Yes, we could match the parameter with tabparents.  So, forget my previous comment.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

If we ever end up running more than one app in a single process then we will not be able to reliably distinguish requests between the two apps. Any app in such a process could forge a request that looks like it came from another app and the parent cannot say with certainty whether or not the request is valid. This is why so far we have resisted allowing more than one app per process.

The activity processes in the tarako case are "ok" because activity apps are all certified-only at this point. We are implicitly trusting that our certified activity apps will not to try to impersonate one another.

However as this bug shows we have never fully tested this scenario. We need to be careful here.

When a PBrowser actor is available we should use it to uniquely distinguish apps. Chrome won't have one so we need to make sure that that case works too.

Comment 13

[Jason Duell]

> When a PBrowser actor is available we should use it to uniquely distinguish
> apps. Chrome won't have one so we need to make sure that that case works too.

Chrome is always the parent process?  If so we're fine since this is only e10s logic.

The way we've been getting PBrowsers/appIDs in the child in general with necko channels is to check the channel.notificationCallsbacks and see if they support nsILoadContext.  We could do the same thing here, but I'm not sure if all clients of RemoteOpenFile set the callbacks.  Given that we have no security re: spoofing between multiple apps in the child, why don't we get the appId from the app:// URI (I'm not sure how to get from the URI to the appId, but sounds like there's a way) and then just verify that it's one of the appIds in the ManagedPBrowserParent list.  That skips the issue of making sure we set notificationCallbacks at all call sites.

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Jason Duell (:jduell) from comment #13)
We discussed offline, going to try just fixing the loop for tarako.

Comment 15

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: try matching manifestURLs of all PBrowserParents v2

Address comments from previous review.

Comment 16

[[:fabrice] Fabrice Desré]

Comment on attachment 8427540 [details] [diff] [review]
Patch: try matching manifestURLs of all PBrowserParents v2

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

lgtm, but we also need bent and/or jduell to review.

Comment 17

[Hema Koka [:hema]]

Please help review the changes, we have another dependency (https://bugzilla.mozilla.org/show_bug.cgi?id=1002897) that needs to land soon.

Thanks
Hema

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

The needinfo flag does not increase the amount of bandwidth that I have to do reviews.

Comment 19

[Jason Duell]

Comment on attachment 8427540 [details] [diff] [review]
Patch: try matching manifestURLs of all PBrowserParents v2

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

After talking to bent I think we want to use the same approach that's used for all other necko channels--required a LoadContext be set for the .notificationCallbacks in the child, and get the appId from that.  I'll knock off a patch. Sorry Patrick!

Comment 20

[Jason Duell]

Attachment: v3: use serializedLoadContext/TabChild to verify appID

Patrick: I don't have a B2G build handy to test this.  It passes the one remoteFileOpen xpcshell test we have, but that's not testing the code that's changed (xpcshell bypasses checks and doesn't have multiple apps in one process).

Could you run it and see if it work?  If so, and if the code looks good to you, ask bent for a review.  

Much appreciated--thanks!

Comment 21

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch on 1.3t

Rebased on 1.3t for testing on tarako.

Comment 22

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Comment on attachment 8430480 [details] [diff] [review]
v3: use serializedLoadContext/TabChild to verify appID

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

Yes, this fixes the problem on tarako. Thanks!

::: netwerk/ipc/NeckoParent.cpp
@@ +499,5 @@
>    return true;
>  }
>  
>  PRemoteOpenFileParent*
> +NeckoParent::AllocPRemoteOpenFileParent(PBrowserParent* aBrowser,

aBrowser is not used in functions.

Comment 23

[Jason Duell]

Comment on attachment 8430480 [details] [diff] [review]
v3: use serializedLoadContext/TabChild to verify appID

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

> aBrowser is not used in functions.

Yes, that's true.  I'm just cooking up a patch now that gets rid of it.

bent: are you good reviewing this as is now, with assumption that final patch won't pass the TabChild (it turns out we don't need it since we iterate over the tabchildren in the process--all we need is the loadContext to say which appID is being asked for).

OTOH I should have the new patch very very soon :)

Comment 24

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I can wait :)

Comment 25

[Jason Duell]

Attachment: v4 for mozilla-central

This is ready for review.  Tarako-rebased version coming next, but won't require review if this is good.

Comment 26

[Jason Duell]

Attachment: v4 for tarako (1.3t)

Just bitrot fixes.

Comment 27

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8431160 [details] [diff] [review]
v4 for mozilla-central

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

I think this looks pretty good! r=me with these addressed:

::: modules/libjar/nsJARChannel.cpp
@@ +376,5 @@
>              // Open file on parent: OnRemoteFileOpenComplete called when done
>              nsCOMPtr<nsITabChild> tabChild;
> +            NS_QueryNotificationCallbacks(this, tabChild);
> +            nsCOMPtr<nsILoadContext> loadContext;
> +            NS_QueryNotificationCallbacks(this, loadContext);

I actually don't know this well enough but it's ok to switch from:

  NS_QueryNotificationCallbacks(mCallbacks, mLoadGroup, tabChild);

to:

  NS_QueryNotificationCallbacks(this, tabChild);

?

And then using the latter pattern for loadContext?

@@ +378,5 @@
> +            NS_QueryNotificationCallbacks(this, tabChild);
> +            nsCOMPtr<nsILoadContext> loadContext;
> +            NS_QueryNotificationCallbacks(this, loadContext);
> +            rv = remoteFile->AsyncRemoteFileOpen(PR_RDONLY, this, tabChild.get(),
> +                                                 loadContext.get());

Just curious, are the two |.get()| really needed here?

::: netwerk/ipc/NeckoParent.cpp
@@ +531,5 @@
> +        nsresult rv = appsService->GetAppByLocalId(appId, getter_AddRefs(mozApp));
> +        if (NS_FAILED(rv) || !mozApp) {
> +          break;
> +        }
> +        hasManage = false;

Nit: this |= false| should be unnecessary.

::: netwerk/ipc/PNecko.ipdl
@@ +68,5 @@
>  
>    PDNSRequest(nsCString hostName, uint32_t flags);
>  
> +  PRemoteOpenFile(SerializedLoadContext loadContext,
> +                  URIParams fileuri, OptionalURIParams appuri);

Nit: let's put that param on its own line.

Comment 28

[Jason Duell]

Attachment: v5-mozilla-central

>  NS_QueryNotificationCallbacks(this, tabChild);

Yes, this form is fine (and we should probably clean the tree of the old version)

> are the two |.get()| really needed here?

No, they weren't--thanks.

> Nit: this |= false| should be unnecessary.

removed

> Nit: let's put that param on its own line.

done.

Comment 29

[Jason Duell]

Attachment: 1010434-tarako.v5

Comment 30

[Jason Duell]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8164fe57ac92

Fabrice: I'm not sure if this needs to get landed directly on the 1.3t branch, or on 1.3 in general.  Also, are there other branches this needs to land on?

Comment 31

[[:fabrice] Fabrice Desré]

(In reply to Jason Duell (:jduell) from comment #30)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/8164fe57ac92
> 
> Fabrice: I'm not sure if this needs to get landed directly on the 1.3t
> branch, or on 1.3 in general.  Also, are there other branches this needs to
> land on?

We want to land on m-c and 1.3t

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ae055051998

https://tbpl.mozilla.org/php/getParsedLog.php?id=40752611&tree=Mozilla-Inbound

Comment 33

[Jason Duell]

Bah--compile error that didn't happen on my build box.  Here's a try run with a fix.  I'll land if/when it's green:

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

Comment 34

[Jason Duell]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed3a15901349
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/d20fd3b8a257

Comment 35

[Phil Ringnalda (:philor)]

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