821671 - Check alarm API parameters in the parent

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

The three messages used in the Alarm API all involve the child telling the parent their manifest URL. If the child process if compromised, this could be any manifest, and thus the child could get/add/remove alarms for any app. 

I think the solution is for the following message handlers in AlarmService.jsm to verify the manifest supplied matches the sender of the message:

AlarmsManager:GetAll
AlarmsManager:Add
AlarmsManager:Remove

Comment 1

[David Chan [:dchan]]

Gene: How difficult would this fix be? Would it be possible to change the AlarmService code to pull the manifestURL from aMessage.target rather than passing it explicitly in the message?

Comment 2

[Gene Lian (I already quit Mozilla)]

Hi Paul, 

Thanks for pointing this out. Actually, I used to solve a similar issue at:

Bug 777224 - Alarm API - .getAll() and .remove() can only interact with alarms scheduled by the same app

I've already verified the manifest URL somewhere in the parent to decide whether the child has the privilege to .getAll() or .remove() the alarms. Also, we don't need to worry about the .add() because we've already checked its permission before adding the alarm.

Does that solve this issue already? Or are you worrying about the compromised child process can pass the manifest URL that doesn't belong to it? But is that possible?

Comment 3

[Alex Keybl [:akeybl]]

Is this necessary for v1? If so, please nominate for b-b?

Comment 4

[David Chan [:dchan]]

(In reply to Gene Lian [:gene] from comment #2)
> 
> Does that solve this issue already? Or are you worrying about the
> compromised child process can pass the manifest URL that doesn't belong to
> it? But is that possible?

The concern is with the latter threat where a compromised child process with the alarm permission could potentially spoof the manifestURL of another app which also has the alarm permission. 

The risk is very dependent on the handling of the alarm message by the other app. The different scenarios I see are

1. well-behaving app is compromised and spoofs another well-behaving app
2. malicious app is compromised and spoofs a well-behaving app
3. malicious app is compromised and spoofs another malicious app
4. well-behaving app is compromised and spoofs a malicious app
5. app is compromised and spoofs manifestURL to get/set/remove other app alarms

I think scenario 5 is the one I am most concerned about. Scenario 1+2 launch a well-behaving app which shouldn't cause any troubles. Scenario 3+4 seem moot to me since the user has already installed a malicious app. Is it possible to launch an arbitrary app through the alarm api or can it only launch apps that have alarm?

Scenario 5 would essentially undo the fix in bug 777224.

Comment 5

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

I think the key question here is how can the parent securely work out which App sent a message, and hence the manifestURL of the sender of a message, without relying on any of the contents of that message.

Comment 6

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

re: Should this be blocking, I am not sure. We are not going to have full OS level child process sandboxing for version 1 (see bugs like 776648) so even if we fix this there will be escalation vectors available. My initial guess is that we should block for this bug since the ability to set alarms for other apps essentially allows calling functions in other applications in a fairly trivial attack, but it depends how difficult it is to fix. Jonas or Cjones - thoughts?

Comment 7

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

(In reply to Gene Lian [:gene] from comment #2)
> Or are you worrying about the compromised child process can pass the manifest
> URL that doesn't belong to it? But is that possible?

That is exactly the concern yes. There's nothing preventing the child process from sending any manifestURL right now, is there?


We've blocked on this type of issue for other APIs. Not sure what bug 776648 is about as we should have a fairly strong sandbox.

Unfortunately I can't see how you in JS could check that the child process contains the app for a particular manifestURL.

What we probably need to do is to add a function on nsIPermissionChecker called something like assertContainsApp(manifestURL). The implementation would look a lot like assertPermission except that it wouldn't check in the permission database but rather check the url of the containing app.

Comment 8

[Gene Lian (I already quit Mozilla)]

Just leaving a note: I'm going to be on vacation so I might probably have delayed responses. However, I'll still try to fix that when I'm available. No worries. ;)

Comment 9

[Gene Lian (I already quit Mozilla)]

Also, thanks David, Paul and Jonas for the good directions! :)

Comment 10

[Gene Lian (I already quit Mozilla)]

Bug 820206 and Bug 821607 are B2G C3 bugs so I believe this one should be marked as B2G C3 too.

Comment 11

[Gene Lian (I already quit Mozilla)]

Attachment: WIP Patch

This is just a WIP but it's working well. I'd prefer combining and refactoring some common files/functions first before asking a formal review.

Comment 12

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, Provide AssertAppProcess() with Different Types

Hi Jonas,

May I invite you to review this bug please? Please feel free to let me know if I need to find someone proper or you don't have enough bandwidth. Thanks! :) This part contains the following changes:

1. Provide a new generic .AssertAppProcess(..., AssertAppProcessType aType, ...) which can eat different types as below:

  enum AssertAppProcessType {
    ASSERT_APP_PROCESS_PERMISSION = 0,
    ASSERT_APP_PROCESS_MANIFEST_URL = 1
  };

2. s/AppProcessPermissions.h/AppProcessChecker.h since we can now not only check the process's permission but also its manifest URL, so let's make it a more generic file name.

3. Following #1 and #2, the rest is the changes for the callers.

Comment 13

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, Provide assertContainApp() for Checking Manifest URL

Hi Jonas,

This patch is summarized as below:

1. Following the part 1, I provide the following new function to let nsFrameMessageManager be able to check if the process contains the app with the valid manifest URL.

  boolean assertContainApp(in DOMString aManifestURL);

2. s/nsIPermissionChecker/nsIProcessChecker with a more generic IDL name since it can now check both permission and manifest URL.

3. Provide MessageManagerCallback::CheckManifestURL() which follows .CheckPermission(). Also, add .CheckManifestURL() into all the overriders.

4. Provide nsFrameMessageManager::AssertProcessInternal() to refactor the common part of .AssertPermission() and .AssertContainApp() which implement the nsIProcessChecker IDL.

Comment 14

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, Alarm API

Hi Jonas,

This part is trivial. For the security issue (the hacked child process could probably send the messages with the fake manifest URL), we need to recheck the child process's manifest URL by calling the new .assertContainApp() supported by the part-1 and part-2 patches.

Comment 15

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, Provide AssertAppProcess() with Different Types, V1.1

Fix minor spots. Please see comment #12 for the summary. Thanks!

Comment 16

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, Provide assertContainApp() for Checking Manifest URL, V1.1

Fix minor spots. Please see comment #13 for the summary. Sorry for the noise!

Comment 17

[Gene Lian (I already quit Mozilla)]

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

Comment 18

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, Alarm API, V1.1

Correct minor typos. Please see comment #14 for the summary.

Comment 19

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, Alarm API, V1.1

Correct minor typos. Please see comment #14 for the summary.

Comment 20

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, Provide AssertAppProcess() with Different Types, V1.2

Rebased. Please see comment #12 for the summary. Thanks!

Comment 21

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

Comment on attachment 696023 [details] [diff] [review]
Part 1, Provide AssertAppProcess() with Different Types, V1.2

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

r=me with those changes, though I wouldn't mind taking a look once there is a new patch.

::: dom/ipc/AppProcessChecker.cpp
@@ +47,5 @@
> +        if (appId != nsIScriptSecurityManager::NO_APP_ID) {
> +          nsCOMPtr<nsIAppsService> appsService = do_GetService("@mozilla.org/AppsService;1");
> +          if (appsService) {
> +            nsString manifestURL = EmptyString();
> +            appsService->GetManifestURLByLocalId(appId, manifestURL);

No need to do all of this. Just call app->GetManifestURL(manifestURL)

::: dom/ipc/AppProcessChecker.h
@@ +20,5 @@
> +}
> +
> +enum AssertAppProcessType {
> +  ASSERT_APP_PROCESS_PERMISSION = 0,
> +  ASSERT_APP_PROCESS_MANIFEST_URL = 1

Nit: I personally prefer to not define the numerical values of enums unless you are relying on them to have particular values. That makes it clear that the numerical values are unimportant if someone wants to add additional entries to the enum.

Not a big deal if you prefer to keep the values though.

@@ +29,5 @@
> + * If this returns false, the browser didn't have the property and
> + * will be killed.
> + */
> +bool
> +AssertAppProcess(mozilla::dom::PBrowserParent* aActor,

I think I'd prefer to keep the old AssertAppProcessPermission function though. But simply make it an inline function like

bool
AssertAppProcessPermission(mozilla::dom::PBrowserParent* aActor, const char* aPermission) {
  return AssertAppProcess(aActor, ASSERT_APP_PROCESS_PERMISSION, aPermission);
}

And also add

bool
AssertAppProcessManifestURL(mozilla::dom::PBrowserParent* aActor, const char* aManifestURL) {
  return AssertAppProcess(aActor, ASSERT_APP_PROCESS_MANIFEST_URL, aManifestURL);
}

That will make callsites a lot easier to read.

Comment 22

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

Comment on attachment 695172 [details] [diff] [review]
Part 2, Provide assertContainApp() for Checking Manifest URL, V1.1

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

::: content/base/src/nsFrameLoader.cpp
@@ +2308,5 @@
> +bool
> +nsFrameLoader::CheckManifestURL(const nsAString& aManifestURL)
> +{
> +  return AssertAppProcess(GetRemoteBrowser(),
> +                          ASSERT_APP_PROCESS_MANIFEST_URL,

Change this to use AssertAppProcessManifestURL

Comment 23

[Gene Lian (I already quit Mozilla)]

Thanks Jonas for the review on holidays! I'll upload new patches tonight (New York time) as soon as I'm available. ;).

Comment 24

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, Provide AssertAppProcess() with Different Types, V2

Hi Jonas,

Applied your advice at comment #21. Also, did the following extra changes:

1. We have to use the keyword "inline" to specify the functions declared in the headers; otherwise, we'll get compiling errors due to multiple definitions.

2. s/aPropertyName/aCapability, which sounds more comfortable because we're hoping to describe the capabilities of a process.

That will be highly appreciated if you could please double-check the new patches. Thanks Jonas!

Comment 25

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, Provide assertContainApp() for Checking Manifest URL, V2

Hi Jonas,

Applied your comment #22. Also, similarly removed the numbers for:

  enum ProcessCheckerType {
    PROCESS_CHECKER_PERMISSION,
    PROCESS_CHECKER_MANIFEST_URL
  };

Would you mind taking the second review? Thanks a lot!

Comment 26

[Gene Lian (I already quit Mozilla)]

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

Comment 27

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

Comment on attachment 696684 [details] [diff] [review]
Part 1, Provide AssertAppProcess() with Different Types, V2

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

r=me with that fixed (no need to ask for re-review)

::: dom/ipc/AppProcessChecker.h
@@ +61,5 @@
> + * Inline functions for asserting the process's permission.
> + */
> +inline bool
> +AssertAppProcessPermission(mozilla::dom::PBrowserParent* aActor,
> +                           const char* aPermission) {

Instead of having triplicates of this function and AssertAppProcessManifestURL, you could just do:

template<classname T*>
inline bool
AssertAppProcessPermission(T* aActor, const char* aPermission) {
  return AssertAppProcess(aActor,
                          ASSERT_APP_PROCESS_PERMISSION,
                          aPermission);
}

Comment 28

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

Comment on attachment 696685 [details] [diff] [review]
Part 2, Provide assertContainApp() for Checking Manifest URL, V2

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

For the record, if you are just addressing review comments, and the reviewer flagged the earlier patch with r+, then you don't need to re-ask for review.

If you want to, you are of course always welcome to though. Better ask too much than too little, so always ask if you are unsure.

Comment 29

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, Provide AssertAppProcess() with Different Types, V2.1

Addressing comment #27.

Thanks Jonas for the good point of the usage of template!

Comment 30

[Gene Lian (I already quit Mozilla)]

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

Comment 31

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/275a687a36ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb675de841e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac4b105b9137

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/275a687a36ce
https://hg.mozilla.org/mozilla-central/rev/eb675de841e9
https://hg.mozilla.org/mozilla-central/rev/ac4b105b9137

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/24b846686767
https://hg.mozilla.org/releases/mozilla-b2g18/rev/86d6fde323dc
https://hg.mozilla.org/releases/mozilla-b2g18/rev/b4a8874431bc