813994 - Alarm API - Need additional security checks for the "alarms" permission

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

Currently it looks like we only check the "alarms" permission in the child process for DOM access. A hacked child can send commands to the parent to cancel or add alarms.

Also, it looks like one process can modify or cancel another's alarms since appId is not stored along with alarmId. I'm happy to file another bug on that if needed.

Comment 1

[Gene Lian (I already quit Mozilla)]

(In reply to ben turner [:bent] from comment #0)
> Currently it looks like we only check the "alarms" permission in the child
> process for DOM access. A hacked child can send commands to the parent to
> cancel or add alarms.

Thanks Ben for pointing this out! I'll try to refine that.

> 
> Also, it looks like one process can modify or cancel another's alarms since
> appId is not stored along with alarmId. I'm happy to file another bug on
> that if needed.

Actually, I've already examined it in the parent process to ensure .getAll() and .remove() can only interact with alarms scheduled by the same app (Bug 777224).

Comment 2

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

(In reply to Gene Lian [:gene] from comment #1)
> Actually, I've already examined it in the parent process to ensure .getAll()
> and .remove() can only interact with alarms scheduled by the same app (Bug
> 777224).

Awesome! Sorry about that, I went on vacation and didn't look to see if anything had changed after I got back :-/

Comment 3

[Gene Lian (I already quit Mozilla)]

Attachment: Patch

Hi Bent,

Changes are trivial: just adding an |assertPermission()| check in the parent. The remaining changes are minor codes clean-ups for the ugly codes designed when I was still newbie, including:

  1. // [Upper-case-letter]...[.]
  2. Rephrase some comments.
  3. Remove trailing spaces.
  4. Wrap long line into 80-char ruler.\
  5. Adding braces for one line if-block.
  6. Alignment.

Thanks for your review! Please let me know if you have any other suggestions.

Comment 4

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

Comment on attachment 685569 [details] [diff] [review]
Patch

Thanks, this is probably right, but it's generally not a good idea to mix lots of code cleanup into bug fixes (it makes chasing blame much harder and can sometimes introduce additional bugs). Can you please separate the changes and file a followup for the cleanup?

Comment 5

[Gene Lian (I already quit Mozilla)]

Attachment: Patch V2

Hi Bent,

You're right I shouldn't be too aggressive. Let's simply deal with this issue in this patch. :)

Btw, I clean up |secMan| and |this.hasPrivileges| since they're no longer used in the current codes, which are also permission-related. Please feel free to let me know if you don't like them present in this patch.

Thanks for the review!

Comment 6

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

Comment on attachment 686046 [details] [diff] [review]
Patch V2

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

Looks great! Thanks!

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/23363f443325

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/23363f443325

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d265118050c8
https://hg.mozilla.org/releases/mozilla-beta/rev/73f5df67aa49