1029352 - Necko dumps all apps in the same process into the same {cookie/cache/etc}jar

Status: RESOLVED WONTFIX

(Core :: Networking, defect)

Description

[Jason Duell]

Attachment: part 1: fix

I just noticed this:  we've been grabbing the first appId we find in ManagedPBrowserParent.   This means that right now all apps in a single child process are sharing the same cookies, http cache, localstorage, etc.

Instead we should be looking at the appId the SerializedLoadContent from the child provides us, and verifying that it's one of the apps in the process.

Comment 1

[Jason Duell]

Attachment: part2: refactor some code

Comment 2

[Jason Duell]

Attachment: 1029352.appID-fix-omnibus

I forget that bent prefers his reviews in one big patch.

Comment 3

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

Comment on attachment 8444963 [details] [diff] [review]
part 1: fix

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

This is close but not quite right I don't think:

::: netwerk/ipc/NeckoParent.cpp
@@ +143,5 @@
> +    // request is not from a different app in that process.
> +    if (appId == aSerialized.mAppId) {
> +      *aAppId = appId;
> +      *aInBrowserElement = aSerialized.IsNotNull() ?
> +        aSerialized.mIsInBrowserElement : tabParent->IsBrowserElement();

I think we can be more strict here wrt the browser flag.

Please see http://mxr.mozilla.org/mozilla-central/source/dom/ipc/AppProcessChecker.cpp#160 because you basically want to do the same comparisons I believe.

Comment 4

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

Comment on attachment 8444966 [details] [diff] [review]
1029352.appID-fix-omnibus

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

The one I reviewed got obsoleted, same comments apply here.

::: netwerk/ipc/NeckoParent.cpp
@@ +534,5 @@
>    // security checks
>    if (UsingNeckoIPCSecurity()) {
> +    uint32_t appId = NECKO_UNKNOWN_APP_ID;
> +    bool inBrowser = false;
> +    if (GetValidatedAppInfo(aSerialized, Manager(), &appId, &inBrowser)) {

Just curious, do we ever use those error strings that get returned here?

Comment 5

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

(In reply to Jason Duell (:jduell) from comment #2)
> I forget that bent prefers his reviews in one big patch.

:)

Comment 6

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

Holy crap!

Can you enumerate exactly which APIs this affects. This might be a serious enough security bug that it's a blocker for Tarako.

Comment 7

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

(In reply to Jonas Sicking (:sicking) from comment #6)
> This might be a serious enough security bug that it's a blocker for Tarako.

All the apps that share a process are currently certified right?

Comment 8

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

Yes. But depending on which APIs this affects, that might be either a security problem or will simply make the apps not work.

Comment 9

[Jason Duell]

APIs affected: Cookies, HTTP cache, auth cache, appcache, localstorage, sessionStorage.

Comment 10

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

This will probably make OAuth work in pretty erratic ways. I.e. sometimes the user will have to log in when they shouldn't need to, and other times the user won't have to log in even when using a new app.

Fortunately this only affects certified apps. And don't know which of them use OAuth. The only one I can think of is the contacts app for google/facebook import. If so, the effect is likely just that you have to log in during import more often than you should need to.

Paul, do you know of other badness here? This only affects certified apps since they are the only ones that we put in the same process.

Comment 11

[Jason Duell]

Attachment: v1->v2 diff

OK, here's a diff just of the fixes.

Comment 12

[Jason Duell]

Comment on attachment 8445496 [details] [diff] [review]
v1->v2 diff

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

::: netwerk/ipc/NeckoParent.cpp
@@ +142,5 @@
>      // in the child process, but there's currently no way to verify the
>      // request is not from a different app in that process.
>      if (appId == aSerialized.mAppId) {
>        *aAppId = appId;
> +      

meh--whitespace.  I'll fix in the actual patch.

@@ +551,5 @@
> +                                            &inBrowser);
> +    if (error) {
> +      printf_stderr("NeckoParent::AllocPRemoteOpenFileParent: "
> +                    "FATAL error: %s: KILLING CHILD PROCESS\n",
> +                    error);

We already use the error string in all other call sites--thanks for pointing this out.

Comment 13

[Jason Duell]

Attachment: 1029352.appId-fix.v2

Comment 14

[Jason Duell]

Comment on attachment 8445497 [details] [diff] [review]
1029352.appId-fix.v2

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

::: netwerk/ipc/NeckoParent.cpp
@@ +127,5 @@
>      }
>      // We may get appID=NO_APP if child frame is neither a browser nor an app
>      if (appId == NECKO_NO_APP_ID) {
>        if (tabParent->HasOwnApp()) {
>          continue;

Bent, do you know if we still need this?

@@ +134,2 @@
>          // <iframe mozbrowser> which doesn't have an <iframe mozapp> above it.
>          // This is not supported now, and we'll need to do a code audit to make

Or this?

Comment 15

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

Comment on attachment 8445497 [details] [diff] [review]
1029352.appId-fix.v2

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

::: netwerk/ipc/NeckoParent.cpp
@@ +148,5 @@
> +        bool inBrowser = aSerialized.mIsInBrowserElement;
> +
> +        // If TabParent says child only runs inBrowserElement content, but it
> +        // passes a serializedLoadContext that says otherwise, child is lying.
> +        if (tabParent->IsBrowserElement() && !inBrowser) {

I don't think this is right still... It's possible to have two TabParents with the same appId where one is a browser element and one is not.

The AssertAppPrincipal code loops through all TabParents and bails out only on a successful match:

- If any TabParent with a matching appId is not a browser element then the we have a match (regardless of the browser flag passed by the child).
- If any TabParent with a matching appId is a browser element *and* the child claims that it is a browser element then we have a match.

So you have to loop through the whole list to know for sure that you can return an error.

Comment 16

[Jason Duell]

Attachment: v3

> If any TabParent with a matching appId is not a browser element then the we
> have a match (regardless of the browser flag passed by the child).  If any
> TabParent with a matching appId is a browser element *and* the child claims
> that it is a browser element then we have a match.

OK, now we use those somewhat confusing heuristics. (If a process contains both the browser app and the content in the browser, does it have separate tabParent entries with the same appID and different inBrowserElement values, or does it just have one with inBrowserElement=false?  If it has 2 then the code here would be a lot easier to understand if we just require that both appIds and inBrowser both match.)

Comment 17

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

Comment on attachment 8445577 [details] [diff] [review]
v3

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

Ok, I think this is right!

The app vs. browser issue confuses me too, but I know AssertAppPrincipal does it correctly, so as long as we follow that this looks right. :)

::: netwerk/ipc/NeckoParent.cpp
@@ +154,5 @@
> +        *aAppId = appId;
> +        // go with what the child says about inBrowser
> +        *aInBrowserElement = inBrowser;
> +        return nullptr;
> +      } else {

Nit: No need for else after a return.

Comment 18

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

Paul, please see comment 10.

Comment 19

[Paul Theriault [:pauljt] (no longer reading bugmail)]

I can't really think of anything.

In 1.3t, the only apps that seem to use oauth are:

- calendar account config
- email account config

Later versions include find my device & ftu. 

But I can't think of any real security abuse scenarios though. (Ultimately, this is only as bad as the existing web security model isn't it?)

I was worried that pdfjs might be a concern. In 1.3 (ie not tarako), pdfjs handles a web activity that loads a URL, and is certified. But still, the worst case attack I can think of is exploiting a CSRF bug in a website (and that would be exploitable on the web anyways, so it's beyond our scope). And it looks like pdfjs has been removed from 1.3t anyways.

Comment 20

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

Ok, marking this as blocking- then. Though feel free to renominate if you think we should reconsider.

Comment 21

[Jason Duell]

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

What branches do we want this on?

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1a2a957f7d3f

Comment 23

[Ed Morley [:emorley]]

Backed out for turning B2G mochitest-4 orange with failures in test_app_update.html:
https://tbpl.mozilla.org/php/getParsedLog.php?id=42582042&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=42589053&tree=Mozilla-Inbound

These were being mis-starred under bug 858772, hence the delay in spotting this.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8e4081ed693d

Comment 24

[Ed Morley [:emorley]]

These were present on the try run, on both the first B2G mochitest-4 orange, and it's retrigger:
https://tbpl.mozilla.org/?rev=7fe40c8e8b7c&tree=Try

https://tbpl.mozilla.org/php/getParsedLog.php?id=42470633&tree=Try
https://tbpl.mozilla.org/php/getParsedLog.php?id=42470633&tree=Try

Also on mochitest-7 on debug (same test, we just chunk differently on debug):
https://tbpl.mozilla.org/php/getParsedLog.php?id=42470964&tree=Try
https://tbpl.mozilla.org/php/getParsedLog.php?id=42474629&tree=Try

Comment 25

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

As many as we can. I'd try to get it on 1.4 and 2.0.

Comment 26

[Jason Duell]

Keeps failing reliably, but only on the ARM B2G emulator.

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

Having Valentin take a look

Comment 27

[Jason Duell]

bent:

So Valentin stuck some printfs in the ARM emulator, and discovered that this patch has uncovered an existing bug:  in the emulator this test has browser appID == 36, while serializedLoadContext shows appID == 1001.

The existing code in the tree doesn't check to see if the value the child provides matches any of the browser.appIDs (i.e. it ignores aSeralizedLoadContext.appId).  It just hands out the first AppId it sees in the list of browsers (36).

The new code checks to make sure browser.appID == SeralizedLoadContext.appId, so it fails.

Do we want to try to debug and fix the emulator in this bug, or disable the test there for now, land this patch, and fork a new bug?   I'd guess the latter, but if you have suggestions for debugging this now, Valentin is up for it.

Comment 28

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

The emulator is the closest thing to what we ship, right? Do you think this is a test problem or a code problem?

Comment 29

[Jason Duell]

The test installs/runs/uninstalls apps, so it's probably doing trickier things that your average test, but I don't see any place where it's trying to manipulate appIDs.  

We're seeing 

   System JS : ERROR resource://gre/modules/Webapps.jsm:97 - NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIXULAppInfo.ID]

when we run--not sure if that might be part of the problem.

Valentin is hopefully going to get an actual phone build working so we can see how things work there.

Comment 30

[Valentin Gosu [:valentin] (he/him)]

I wasn't able to run the mochitest on the phone, but I did some manual tests, and installing/uninstalling apps seems to work on a keon.

Also, concerning the nsIXULAppInfo.ID error, at http://dxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#104 , if I set WEBAPP_RUNTIME to true, it fails after after [12 INFO TEST-PASS | /tests/dom/apps/tests/test_app_update.html | Checking installed app], whereas if I set it to false, it fails earlier, after [4 INFO TEST-PASS | /tests/dom/apps/tests/test_app_update.html | Checking uninstalled app]

If the test is doing weird things, we could just add  SpecialPowers.setBoolPref("network.disable.ipc.security", true) to the test and land this.

Comment 31

[Jason Duell]

bent: what do you think? Should we land this and see if it works OK? The patch doesn't seem to break the things the test is testing (install/uninstall).  Do you know someone who could run a mochitest on the phone and debug it?

Valentin: we could either land this with the test disabled on the emulator (only), or turn off the security checks (which would probably need to be for all platforms).  Neither is a great choice.  Bent, got an opinion on that?

Comment 32

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

We definitely need to write automated tests for this. The way to do it is likely to start two apps in different processes. Have both of them set cookies (different cookie value for the two apps). Then start the two apps in the same process. Check that both apps are seeing the correct cookies.

Fabrice can help with how to launch the two apps in different respectively the same process.

Comment 33

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

We should also do s/cookies/localStorage and IDB/g in the above.

Comment 34

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

What sicking said :)

Comment 35

[Jason Duell]

Fabrice,

Can you tell me and/or point to example of multiprocess mochitests?  Ideally with some sort of timing guarantees between the processes? (i.e. we'll want to have process #1 create a cache entry or cookie, etc, and then make sure that process #2 doesn't see it).

Comment 36

[[:fabrice] Fabrice Desré]

(In reply to Jason Duell (:jduell) (on PTO until July 24th) from comment #35)
> Fabrice,
> 
> Can you tell me and/or point to example of multiprocess mochitests?  Ideally
> with some sort of timing guarantees between the processes? (i.e. we'll want
> to have process #1 create a cache entry or cookie, etc, and then make sure
> that process #2 doesn't see it).

The way we do that usually is simply by adding <iframe remote=true> content dynamically during the test.

There are a lot of oop tests in dom/browser-element/mochitest/ and dom/browser-element/mochitest/priority/

Comment 37

[Valentin Gosu [:valentin] (he/him)]

Attachment: Test

Comment 38

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8457740 [details] [diff] [review]
Test

This test is largely based on the browser-element ones.
The only problem is the test runs with network.disable.ipc.security: true, so the result is the same as before. Would opening a mozapp iframe at app://calendar.gaiamobile.org/manifest.webapp make it use the calendar's appId?

Comment 39

[[:fabrice] Fabrice Desré]

Yes, you need to set the manifest url to an app actually installed. I don't think that http://test1.example.com/manifest.webapp is one. Don't use the gaia ones since they are only installed on b2g.

Comment 40

[Valentin Gosu [:valentin] (he/him)]

Thanks! I'll try to install some as part of the test.

Comment 41

[Valentin Gosu [:valentin] (he/him)]

Attachment: That the cookie jar is separate for different apps

Comment 42

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8458325 [details] [diff] [review]
That the cookie jar is separate for different apps

I changed the test to install and use some arbitrary apps.
However, I was unable to get it to fail _without_ the patch. Paul, can you think of a use case in which it would? Thanks!

Comment 43

[Paul Theriault [:pauljt] (no longer reading bugmail)]

My guess is that starting two apps in the same process is not enough to see the bug here - wouldn't an app1 actually have to attempt to request a cookie for an extended origin of another app in the same process? (i.e. it needs to spoof the appid of another app)

If my reading of the code is correct, then I don't think this is possible unless the child app has been compromised. So I think in order to test this, you will need to add a some function to SpecialPowers or similar to spoof it's appid (simulating a compromise scenario). 

PS I'm guessing a little here, based on my reading of the code. So I'll explain my logic below in case this helps, or so that someone can tell me if my understanding is correct or not:

An app requests a cookie here:
http://dxr.mozilla.org/mozilla-central/source/netwerk/cookie/CookieServiceParent.cpp#74

The key argument is aLoadContext which contains the serialized information about the loading context, including the appID. This is usually set in chrome code in the child, here IIUC:

http://dxr.mozilla.org/mozilla-central/source/docshell/base/SerializedLoadContext.h#75

So in normal operation, the app will always be honest about its appid, since the appid is inserted into the loadcontext in Chrome code. However if there child is compromised it can lie about it's appid, and request cookies from other app's origins.

Comment 44

[Valentin Gosu [:valentin] (he/him)]

Thanks, Paul. That does affect my approach a bit, but your pointers will definitely prove useful!

Comment 45

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8458325 [details] [diff] [review]
That the cookie jar is separate for different apps

Cancelling review for now, until I apply Paul's approach to the test.

Comment 46

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

(In reply to Paul Theriault [:pauljt] from comment #43)
> My guess is that starting two apps in the same process is not enough to see
> the bug here - wouldn't an app1 actually have to attempt to request a cookie
> for an extended origin of another app in the same process? (i.e. it needs to
> spoof the appid of another app)

I don't think that's the case. But Jason would know.

Comment 47

[Jason Duell]

> My guess is that starting two apps in the same process is not enough to see
> the bug here... in normal operation, the app will always be honest about its appid.

It's true that the serializedLoadContext passed in by the child will have an accurate appID unless the child process is compromised, but the bug here is that we ignore the child's stated value (a good idea in theory--why trust the child?): CookieServiceParent::GetAppInfoFromParams() calls into NeckoParent::GetValidatedAppInfo(), and that function is currently blindly returning the first appID (that isn't NO_APP) from the array of browsers returned by aContent->ManagedPBrowserParent():

  http://mxr.mozilla.org/mozilla-central/source/netwerk/ipc/NeckoParent.cpp#132

So as long as we're running two apps in the same process, and those show up as two different BrowserParents in PContentParent->ManagedPBrowserParent(), we should see that the apps wind up writing into the same cookie database (with whichever appID comes first in the list of browsers).

But... Valentin tells me he's only seeing PContentParent->ManagedPBrowserParent() return an array with a single browser in it, even when we're running two apps in the same process.  I.e instead of seeing the same PContentParent, with two apps returned by ManagedPBrowserParent(), we see 2 different PContentParent's, each with a single (different) app.  So the existing code works fine, since we just return the first (only) appId.

jonas: I'm now thoroughly confused.  So when exactly does PContentParent->ManagedPBrowserParent() return more than one PBrowserParent?  (I'd ask :bent but he's on PTO)

Comment 48

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

Sounds as confusing to me as it does to you.

Comment 49

[[:fabrice] Fabrice Desré]

Are we 100% sure to run both apps in the same process?

Comment 50

[Valentin Gosu [:valentin] (he/him)]

(In reply to Fabrice Desré [:fabrice] from comment #49)
> Are we 100% sure to run both apps in the same process?

Yes, I checked, and when testing on linux-desktop, even when the apps were in the same process, there was only one PBrowserParent returned for each app.

However, on the emulator, I'm running into a bug where running apps with IDs 4 and 15 (email and calendar) they both return appID 36. This seems to be the issue behind the failing test - and why the patch got backed out. I'm filing a new bug to track this issue.

Comment 51

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Jason Duell (:jduell) from comment #47)
> > My guess is that starting two apps in the same process is not enough to see
> > the bug here... in normal operation, the app will always be honest about its appid.
> 
> It's true that the serializedLoadContext passed in by the child will have an
> accurate appID unless the child process is compromised, but the bug here is
> that we ignore the child's stated value (a good idea in theory--why trust
> the child?): CookieServiceParent::GetAppInfoFromParams() calls into
> NeckoParent::GetValidatedAppInfo(), and that function is currently blindly
> returning the first appID (that isn't NO_APP) from the array of browsers
> returned by aContent->ManagedPBrowserParent():
> 
>  
> http://mxr.mozilla.org/mozilla-central/source/netwerk/ipc/NeckoParent.cpp#132
> 
> So as long as we're running two apps in the same process, and those show up
> as two different BrowserParents in PContentParent->ManagedPBrowserParent(),
> we should see that the apps wind up writing into the same cookie database
> (with whichever appID comes first in the list of browsers).
> 
> But... Valentin tells me he's only seeing
> PContentParent->ManagedPBrowserParent() return an array with a single
> browser in it, even when we're running two apps in the same process.  I.e
> instead of seeing the same PContentParent, with two apps returned by
> ManagedPBrowserParent(), we see 2 different PContentParent's, each with a
> single (different) app.  So the existing code works fine, since we just
> return the first (only) appId.

Note in bug 1020157 I changed the GetValidatedAppInfo to return the AppID from the ContentParent which should be very accurate.
 
> jonas: I'm now thoroughly confused.  So when exactly does
> PContentParent->ManagedPBrowserParent() return more than one PBrowserParent?
> (I'd ask :bent but he's on PTO)

When the content uses window.open it will create another PBrowser.

Comment 52

[Jason Duell]

> When the content uses window.open it will create another PBrowser.

Does the new window always have the same appID or can it be diferent?  If the former than I'm not sure we have any cases where this "bug" is actually breaking anything.

Comment 53

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Jason Duell (:jduell) from comment #52)
> > When the content uses window.open it will create another PBrowser.
> 
> Does the new window always have the same appID or can it be diferent?  If
> the former than I'm not sure we have any cases where this "bug" is actually
> breaking anything.

It should have the same appID.

I didn't read all the comments in this bug but I guess it's for the case in memory constrained environment like Tarako we added a option to run different apps using the same process. IMO this is not normal and breaking some security assumptions. But in this case they should have different appID.

Comment 54

[Kan-Ru Chen [:kanru] (UTC+9)]

What's blocking us to land this patch? Bug 1020157 now depends on this bug so I want to land them together.

Comment 55

[Jason Duell]

I would love to see this land.

It's currently blocked by bug 1044333, i.e. the emulator is reporting the wrong appID for apps, and we're not sure if that's a bug in the emulator, or something that might actually be happening on the phone (which would be a reasonably serious bug if so).  I haven't gotten any traction on getting this investigated.

OTOH there's some evidence (comment 52 and 53) that this isn't actually breaking anything, so I suspect that's why it's not at the top of anyone's list.  But now that it's blocking a 2.1 blocker I think we should either 1) land this and disable tests on the emulator (ideally after validating that bug 1044333 isn't shipping on phones, or 2) change bug 1020157 so it doesn't depend on this patch.  Fabrice, thoughts?

Comment 56

[Jason Duell]

Ben may have thoughts here too.

Comment 57

[Jason Duell]

s/disable tests on the emulator/disable the specific test that's failing on the emulator/ :)

Comment 58

[Kan-Ru Chen [:kanru] (UTC+9)]

Thanks, I'll take a look of bug 1044333 :)

Comment 59

[[:fabrice] Fabrice Desré]

Since Kan-Ru will investigate the emulator appID issue, I'd rather wait for results in this bug. I'm quite confident that we don't have the issue on devices, but disabling tests is sad...