Closed
Bug 821671
Opened 12 years ago
Closed 12 years ago
Check alarm API parameters in the parent
Categories
(Firefox OS Graveyard :: General, defect)
Firefox OS Graveyard
General
Tracking
(blocking-basecamp:+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed)
People
(Reporter: pauljt, Assigned: airpingu)
References
Details
Attachments
(3 files, 9 obsolete files)
(deleted),
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
airpingu
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
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?
Flags: needinfo?(clian)
Assignee | ||
Comment 2•12 years ago
|
||
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?
Flags: needinfo?(clian) → needinfo?(ptheriault)
Comment 3•12 years ago
|
||
Is this necessary for v1? If so, please nominate for b-b?
Comment 4•12 years ago
|
||
(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.
Flags: needinfo?(ptheriault)
Reporter | ||
Comment 5•12 years ago
|
||
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.
Reporter | ||
Comment 6•12 years ago
|
||
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?
blocking-basecamp: --- → ?
Flags: needinfo?
Reporter | ||
Updated•12 years ago
|
Flags: needinfo?(jonas)
(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.
Flags: needinfo?(jonas)
Flags: needinfo?
Assignee: nobody → clian
Updated•12 years ago
|
blocking-basecamp: ? → +
Assignee | ||
Comment 8•12 years ago
|
||
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. ;)
Assignee | ||
Comment 9•12 years ago
|
||
Also, thanks David, Paul and Jonas for the good directions! :)
Assignee | ||
Comment 10•12 years ago
|
||
Bug 820206 and Bug 821607 are B2G C3 bugs so I believe this one should be marked as B2G C3 too.
Target Milestone: --- → B2G C3 (12dec-1jan)
Assignee | ||
Comment 11•12 years ago
|
||
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.
Assignee | ||
Comment 12•12 years ago
|
||
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.
Attachment #694792 -
Attachment is obsolete: true
Attachment #695167 -
Flags: review?(jonas)
Assignee | ||
Comment 13•12 years ago
|
||
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.
Attachment #695168 -
Flags: review?(jonas)
Assignee | ||
Comment 14•12 years ago
|
||
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.
Attachment #695169 -
Flags: review?(jonas)
Assignee | ||
Comment 15•12 years ago
|
||
Fix minor spots. Please see comment #12 for the summary. Thanks!
Attachment #695167 -
Attachment is obsolete: true
Attachment #695167 -
Flags: review?(jonas)
Attachment #695170 -
Flags: review?(jonas)
Assignee | ||
Comment 16•12 years ago
|
||
Fix minor spots. Please see comment #13 for the summary. Sorry for the noise!
Attachment #695168 -
Attachment is obsolete: true
Attachment #695168 -
Flags: review?(jonas)
Attachment #695172 -
Flags: review?(jonas)
Assignee | ||
Comment 17•12 years ago
|
||
Assignee | ||
Comment 18•12 years ago
|
||
Correct minor typos. Please see comment #14 for the summary.
Attachment #695169 -
Attachment is obsolete: true
Attachment #695169 -
Flags: review?(jonas)
Attachment #696002 -
Flags: review?(jonas)
Assignee | ||
Comment 19•12 years ago
|
||
Correct minor typos. Please see comment #14 for the summary.
Attachment #696002 -
Attachment is obsolete: true
Attachment #696002 -
Flags: review?(jonas)
Attachment #696003 -
Flags: review?(jonas)
Assignee | ||
Comment 20•12 years ago
|
||
Rebased. Please see comment #12 for the summary. Thanks!
Attachment #695170 -
Attachment is obsolete: true
Attachment #695170 -
Flags: review?(jonas)
Attachment #696023 -
Flags: review?(jonas)
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.
Attachment #696023 -
Flags: review?(jonas) → review+
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
Attachment #695172 -
Flags: review?(jonas) → review+
Attachment #696003 -
Flags: review?(jonas) → review+
Assignee | ||
Comment 23•12 years ago
|
||
Thanks Jonas for the review on holidays! I'll upload new patches tonight (New York time) as soon as I'm available. ;).
Assignee | ||
Comment 24•12 years ago
|
||
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!
Attachment #696023 -
Attachment is obsolete: true
Attachment #696684 -
Flags: review?(jonas)
Assignee | ||
Comment 25•12 years ago
|
||
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!
Attachment #695172 -
Attachment is obsolete: true
Attachment #696685 -
Flags: review?(jonas)
Assignee | ||
Comment 26•12 years ago
|
||
Updated•12 years ago
|
Target Milestone: B2G C3 (12dec-1jan) → B2G C4 (2jan on)
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);
}
Attachment #696684 -
Flags: review?(jonas) → review+
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.
Attachment #696685 -
Flags: review?(jonas) → review+
Assignee | ||
Comment 29•12 years ago
|
||
Addressing comment #27.
Thanks Jonas for the good point of the usage of template!
Attachment #696684 -
Attachment is obsolete: true
Attachment #698553 -
Flags: review+
Assignee | ||
Comment 30•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Whiteboard: Please check them in by the Part 1,2,3 order.
Comment 31•12 years ago
|
||
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
Keywords: checkin-needed
Updated•12 years ago
|
Whiteboard: Please check them in by the Part 1,2,3 order.
Comment 32•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/275a687a36ce
https://hg.mozilla.org/mozilla-central/rev/eb675de841e9
https://hg.mozilla.org/mozilla-central/rev/ac4b105b9137
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 33•12 years ago
|
||
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
status-b2g18:
--- → fixed
status-firefox19:
--- → wontfix
status-firefox20:
--- → wontfix
status-firefox21:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•