Closed Bug 814157 Opened 7 years ago Closed 7 years ago

Need additional security checks for the "desktop-notification" permission

Categories

(Core :: DOM: Core & HTML, defect, P1)

defect

Tracking

()

RESOLVED FIXED
B2G C3 (12dec-1jan)
blocking-basecamp +
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: bent.mozilla, Assigned: gwagner)

References

Details

Attachments

(1 file, 1 obsolete file)

From discussion with dougt, the "desktop-notification" permission is using nsIContentPermissionPrompt and thereafter able to do whatever it wants. A hacked child could thus send notifications any time and we should add some security checks in the parent process.

We could also sanitize url against sms/tel/etc in case that causes problems.
discussed this issue at triage this morning and we agreed that we should block on these sorts of permission bugs for now.

gregor, can you take a look?
Assignee: nobody → anygregor
blocking-basecamp: ? → +
Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.
Priority: -- → P1
Mass Modify: All un-milestoned, unresolved blocking-basecamp+ bugs are being moved into the C3 milestone. Note that the target milestone does not mean that these bugs can't be resolved prior to 12/10, rather C2 bugs should be prioritized ahead of C3 bugs.
Target Milestone: --- → B2G C3 (12dec-1jan)
Been nearly 10 days since an update. Gregor, can you update the bug with your status?
I confirmed with Gregor on IRC that this is next in his list to fix.
Attached patch patch (obsolete) — Splinter Review
Comment on attachment 693184 [details] [diff] [review]
patch

Haven't tested on the phone yet (forgot them at home) but it would be good to know if this is the right direction.
Attachment #693184 - Flags: review?(fabrice)
Andrea, would this work with the patch from bug 811026?
Comment on attachment 693184 [details] [diff] [review]
patch

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

::: b2g/chrome/content/shell.js
@@ +741,5 @@
>                            uid, name, null);
>    },
>  
> +  receiveMessage: function alert_receiveMessage(aMessage) {
> +    if (!this.assertPermission(aMessage, "desktop-notification")) {

[JavaScript Error: "this.assertPermission is not a function" {file: "chrome://browser/content/shell.js" line: 739}]

If I change it to aMessage.target.assertPermission("desktop-notification") this works. Does it look ok to you?
Attachment #693184 - Flags: review?(fabrice) → review-
(In reply to Fabrice Desré [:fabrice] from comment #9)
> Comment on attachment 693184 [details] [diff] [review]
> patch
> 
> Review of attachment 693184 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/chrome/content/shell.js
> @@ +741,5 @@
> >                            uid, name, null);
> >    },
> >  
> > +  receiveMessage: function alert_receiveMessage(aMessage) {
> > +    if (!this.assertPermission(aMessage, "desktop-notification")) {
> 
> [JavaScript Error: "this.assertPermission is not a function" {file:
> "chrome://browser/content/shell.js" line: 739}]
> 
> If I change it to aMessage.target.assertPermission("desktop-notification")
> this works. Does it look ok to you?

Yes that's the right function. I shouldn't post untested code. Sorry for that.
Attached patch patchSplinter Review
Attachment #693184 - Attachment is obsolete: true
(In reply to Gregor Wagner [:gwagner] from comment #8)
> Andrea, would this work with the patch from bug 811026?

Yes. It should work.
Attachment #693205 - Flags: review?(fabrice)
Attachment #693205 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/mozilla-central/rev/4925a90ee7ff
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.