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

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sinker, Assigned: jduell.mcbugs)

Tracking

unspecified
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.3T+, b2g-v1.3T fixed)

Details

(Whiteboard: [tarako_only])

Attachments

(2 attachments, 6 obsolete attachments)

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.
Activity apps are grouped only in tarako.
blocking-b2g: --- → 1.3T?
Whiteboard: [tarako_only]
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.
Attachment #8423029 - Flags: review?(fabrice)
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.
> 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.
Flags: needinfo?(bent.mozilla)
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?
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.
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?
(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.
Yes, we could match the parameter with tabparents.  So, forget my previous comment.
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.
Flags: needinfo?(bent.mozilla)
> 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.
Flags: needinfo?(bent.mozilla)
(In reply to Jason Duell (:jduell) from comment #13)
We discussed offline, going to try just fixing the loop for tarako.
Flags: needinfo?(bent.mozilla)
blocking-b2g: 1.3T? → 1.3T+
Assignee: nobody → pwang
Address comments from previous review.
Attachment #8423029 - Attachment is obsolete: true
Attachment #8427540 - Flags: review?(fabrice)
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.
Attachment #8427540 - Flags: review?(jduell.mcbugs)
Attachment #8427540 - Flags: review?(fabrice)
Attachment #8427540 - Flags: review?(bent.mozilla)
Attachment #8427540 - Flags: review+
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
Flags: needinfo?(jduell.mcbugs)
Flags: needinfo?(bent.mozilla)
The needinfo flag does not increase the amount of bandwidth that I have to do reviews.
Flags: needinfo?(bent.mozilla)
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!
Attachment #8427540 - Flags: review?(jduell.mcbugs)
Attachment #8427540 - Flags: review?(bent.mozilla)
Attachment #8427540 - Flags: feedback-
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!
Assignee: pwang → jduell.mcbugs
Attachment #8427540 - Attachment is obsolete: true
Attachment #8430480 - Flags: feedback?(pwang)
Flags: needinfo?(jduell.mcbugs)
Posted patch Patch on 1.3t (obsolete) — Splinter Review
Rebased on 1.3t for testing on tarako.
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.
Attachment #8430480 - Flags: feedback?(pwang) → feedback+
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 :)
Attachment #8430480 - Flags: review?(bent.mozilla)
Posted patch v4 for mozilla-central (obsolete) — Splinter Review
This is ready for review.  Tarako-rebased version coming next, but won't require review if this is good.
Attachment #8430480 - Attachment is obsolete: true
Attachment #8430480 - Flags: review?(bent.mozilla)
Attachment #8431160 - Flags: review?(bent.mozilla)
Posted patch v4 for tarako (1.3t) (obsolete) — Splinter Review
Just bitrot fixes.
Attachment #8430603 - Attachment is obsolete: true
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.
Attachment #8431160 - Flags: review?(bent.mozilla) → review+
>  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.
Attachment #8431160 - Attachment is obsolete: true
Attachment #8431754 - Flags: review+
Attachment #8431164 - Attachment is obsolete: true
Attachment #8431796 - Flags: review+
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?
Flags: needinfo?(fabrice)
(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
Flags: needinfo?(fabrice)
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
https://hg.mozilla.org/mozilla-central/rev/ed3a15901349
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.