Closed
Bug 821607
Opened 12 years ago
Closed 12 years ago
Add checks in the parent to ensure system messages are registered for the correct application.
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
(1 file, 1 obsolete file)
1.67 KB,
patch
|
airpingu
:
review+
|
Details | Diff | Splinter Review |
I'm not 100% sure about this issue, but I am raising it for further investigation. My concern relates to the registration for system messages here[1].
The child(SystemMessageManager.js) tells the parent (SystemMessageInternal.js) which manifest and innerWindowID should register for a certain message. If the child process is compromised, I am concerned that the child could fake the manifestURL and then recieve messages intended for other applications. There is a permissions check [2] but this is done based on the manifestURI and the target pageURI, not the InnerWindowID. (e.g what happens if child said its manifest was the the URL of the SMS app, would it receive SMS related system messages?)
My thought was that there needs to a check in the parent that the manifest is the correct one for this app. Or don't even supply the manifest from the child, just let the parent get it from the message?
[1] http://mxr.mozilla.org/mozilla-central/source/dom/messages/SystemMessageManager.js#210
[2]http://mxr.mozilla.org/mozilla-central/source/dom/messages/SystemMessageInternal.js#371
Reporter | ||
Comment 1•12 years ago
|
||
Likewise, there may be other risks here such as with |SystemMessageManager:GetPendingMessages|. The type, URI and manifest could be spoofed, to get pending messages for any page. And perhaps combined with |SystemMessageManager:Unregister| the child could guarantee that message would be waiting to be delivered. If these are actually issues, then all of the messages in the API will need to be reviewed to ensure they validate parameters from the child properly.
But as I said, I am a little vague on the exact use of this code - needs info from someone who knows this code better.
Flags: needinfo?
Assignee | ||
Updated•12 years ago
|
Blocks: system-message-api
Gene: This sounds like a bug for you :)
Assignee: nobody → clian
blocking-basecamp: --- → ?
Updated•12 years ago
|
blocking-basecamp: ? → +
Updated•12 years ago
|
Target Milestone: --- → B2G C3 (12dec-1jan)
Assignee | ||
Comment 4•12 years ago
|
||
Very glad to take it but I'm afraid I won't be able to have 100% full time to fix that because I'm going to be on a vacation (12/24~1/3). I'll still try to get it fixed before C3 as possible as I can. No worries!
Assignee | ||
Comment 5•12 years ago
|
||
This bug can be solved based on the support of Bug 821671, which provides a new utility function |aMessage.target.assertContainApp(aMessage.json.manifestURL)| to check if the message carries the right manifest URL belonging to that child.
Assignee | ||
Comment 6•12 years ago
|
||
Note that this bug depends on the support of a new utility function .AssertContainApp() at Bug 821671. We need to check if the message carries the correct manifest URL that exactly belongs to the child process.
Attachment #696007 -
Flags: review?(jonas)
Attachment #696007 -
Flags: review?(jonas) → review+
Comment 7•12 years ago
|
||
Any reason not to land this?
Assignee | ||
Comment 8•12 years ago
|
||
(In reply to JP Rosevear [:jpr] from comment #7)
> Any reason not to land this?
This patch is actually on top of bug 821671, so it'd be better to check that in first.
Updated•12 years ago
|
Target Milestone: B2G C3 (12dec-1jan) → B2G C4 (2jan on)
Assignee | ||
Comment 9•12 years ago
|
||
r=sicking
Attachment #696007 -
Attachment is obsolete: true
Attachment #698554 -
Flags: review+
Assignee | ||
Comment 10•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Whiteboard: Please check this in *after* bug 821671.
Comment 11•12 years ago
|
||
Keywords: checkin-needed
Whiteboard: Please check this in *after* bug 821671.
Comment 12•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 13•12 years ago
|
||
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
•