Closed Bug 814222 Opened 7 years ago Closed 7 years ago

Need additional security checks for the "networkstats-manage" permission

Categories

(Core :: DOM: Device Interfaces, defect, P1)

defect

Tracking

()

RESOLVED FIXED
B2G C4 (2jan on)
blocking-basecamp +
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed

People

(Reporter: bent.mozilla, Assigned: albert)

References

Details

Attachments

(3 files, 3 obsolete files)

It looks like the "networkstats-manage" is not properly checked.

We check the permission in the child process when creating the 'navigator.mozNetworkStats' object. If the permission doesn't exist we still create the object rather than returning null. Then when any method is called on it we throw an exception.

The permission is never checked in the parent so a hacked child can read network stats.
blocking-basecamp: ? → +
Assignee: nobody → acperez
Attached patch Fix permissions management (obsolete) — Splinter Review
Attachment #686062 - Flags: review?(philipp)
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)
No longer blocks: 820202
Comment on attachment 686062 [details] [diff] [review]
Fix permissions management

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

Good catch! This is a crucial fix. However, there are some parts of this patch that are unrelated. Also, can you please add a basic test case to make sure the permission really is enforced. Thanks!

::: dom/network/src/NetworkStatsManager.js
@@ -14,5 @@
>  Cu.import("resource://gre/modules/DOMRequestHelper.jsm");
>  
> -XPCOMUtils.defineLazyServiceGetter(this, "cpmm",
> -                                   "@mozilla.org/childprocessmessagemanager;1",
> -                                   "nsISyncMessageSender");

Why this change? It is unrelated to the actual bug. Please revert this.

@@ +96,5 @@
>    __proto__: DOMRequestIpcHelper.prototype,
>  
>    checkPrivileges: function checkPrivileges() {
>      if (!this.hasPrivileges) {
> +      throw new Error("No permission in this context.\n");

Can you explain this change? I agree throwing a result code isn't a good idea. Throwing a Components.Exception instance would be better IMHO:

  throw Components.Exception("Permission denied", Cr.NS_ERROR_FAILURE);

@@ +209,5 @@
>      }
> +
> +    if (!this.hasPrivileges) {
> +      return null;
> +    }

Good catch!

@@ +212,5 @@
> +      return null;
> +    }
> +
> +    this.cpmm = Cc["@mozilla.org/childprocessmessagemanager;1"]
> +                 .getService(Ci.nsISyncMessageSender);

Please revert this (see above.)
Attachment #686062 - Flags: review?(philipp) → feedback+
Applied comments
Attachment #686062 - Attachment is obsolete: true
Attached patch New tests for permissions (obsolete) — Splinter Review
Added tests for permissions
Attachment #692873 - Flags: review?(philipp)
Tests are ok except for 'test_networkstats_basics.html'

The problem is that parent considers that child process doesn't have permission and throws:

I/Gecko   (  420): Security problem: Content process does not have `networkstats-manage' permission.  It will be killed.
I/Gecko   (  420): [Parent 420] WARNING: pipe error (98): Connection reset by peer: file /home/acperez/OWD/B2G/gecko/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 431
E/GeckoConsole(  420): [JavaScript Error: "this._permissionList is null" {file: "jar:file:///system/b2g/omni.ja!/components/RadioInterfaceLayer.js" line: 689}]

I added the permission with the SpecialPowers API. It can be a bug of the SpecialPowers.addPermission method?
Comment on attachment 692873 [details] [diff] [review]
New tests for permissions

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

I think it would be much clearer to test the different cases in the same file.

::: dom/network/tests/test_networkstats_basics.html
@@ +27,2 @@
>      SimpleTest.finish();
>    }

Seems to me this `if` Block is missing a `return` statement at the end. But with the code you're adding below to always enable this pref, why do we have this `if` block around in the first place? Let's just remove it.

::: dom/network/tests/test_networkstats_default.html
@@ +11,5 @@
> +</div>
> +<pre id="test">
> +<script type="application/javascript">
> +
> +/** Test to ensure NetworkStats is enabled by default **/

Huh? It's not necessarily enabled by default (e.g. on Firefox). So are we really testing that there's no permission here? Or are we testing that it's not enabled?

::: dom/network/tests/test_networkstats_enabled_no_perm.html
@@ +13,5 @@
> +<script type="application/javascript">
> +
> +/** Test to ensure NetworkStats is enabled but mozNetworkStats.connectionTypes
> +    does not work in content.
> +**/

Please use // comments (here and everywhere else).

@@ +22,5 @@
> +
> +try {
> +  navigator.mozNetworkStats.connectionTypes;
> +} catch (e) {
> +  ok(true, "navigator.mozNetworkStats.connectionTypes should raise for content that does not have the networkstats-manage permission");

This test won't fail if accessing `navigator.mozNetworkStats.connectionTypes;` doesn't throw. Better:

  let error;
  try {
    navigator.mozNetworkStats.connectionTypes;
    ok(false, "Accessing navigator.mozNetworkStats.connectionTypes should have thrown!");
  catch (ex) {
    error = ex;
  }
  ok(error, "Got an exception accessing navigator.mozNetworkStats.connectionTypes");

For some extra credit, check that `error` is actually the right exception!
Attachment #692873 - Flags: review?(philipp) → review-
Comment on attachment 692872 [details] [diff] [review]
Fix permissions management

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

Thanks for updating the patch!

(Note: please ask for review again if you haven't gotten an r+. A feedback+ does not count.)
Attachment #692872 - Flags: review+
(In reply to Philipp von Weitershausen [:philikon] from comment #8)
> Comment on attachment 692873 [details] [diff] [review]
> New tests for permissions
> 
> Review of attachment 692873 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think it would be much clearer to test the different cases in the same
> file.

I can't put all tests in the same file because the constructor of navigator.mozNetworkStats is called only once and changing permission and "dom.mozNetworkStats.enabled" values does not make sense.

> ::: dom/network/tests/test_networkstats_basics.html
> @@ +27,2 @@
> >      SimpleTest.finish();
> >    }
> 
> Seems to me this `if` Block is missing a `return` statement at the end. But
> with the code you're adding below to always enable this pref, why do we have
> this `if` block around in the first place? Let's just remove it.

You are right, it is not necessary.

> ::: dom/network/tests/test_networkstats_default.html
> @@ +11,5 @@
> > +</div>
> > +<pre id="test">
> > +<script type="application/javascript">
> > +
> > +/** Test to ensure NetworkStats is enabled by default **/
> 
> Huh? It's not necessarily enabled by default (e.g. on Firefox). So are we
> really testing that there's no permission here? Or are we testing that it's
> not enabled?

Ok, this test is not necessary.


Otherwise, I'm still having the problem that parent considers that child process doesn't have permission.

I/Gecko   (  420): Security problem: Content process does not have `networkstats-manage' permission.  It will be killed.

Can it be a bug of the SpecialPowers.addPermission method?
(In reply to Albert from comment #10)
> > I think it would be much clearer to test the different cases in the same
> > file.
> 
> I can't put all tests in the same file because the constructor of
> navigator.mozNetworkStats is called only once and changing permission and
> "dom.mozNetworkStats.enabled" values does not make sense.

Ah good point.

> Otherwise, I'm still having the problem that parent considers that child
> process doesn't have permission.
> 
> I/Gecko   (  420): Security problem: Content process does not have
> `networkstats-manage' permission.  It will be killed.
> 
> Can it be a bug of the SpecialPowers.addPermission method?

For which test? And how are you running them? In a B2G desktop build or a Firefox build?
(In reply to Philipp von Weitershausen [:philikon] from comment #11)
> (In reply to Albert from comment #10)
>
> > Otherwise, I'm still having the problem that parent considers that child
> > process doesn't have permission.
> > 
> > I/Gecko   (  420): Security problem: Content process does not have
> > `networkstats-manage' permission.  It will be killed.
> > 
> > Can it be a bug of the SpecialPowers.addPermission method?
> 
> For which test? And how are you running them? In a B2G desktop build or a
> Firefox build?

For test_networkstats_basics.html, when networkstats is enabled, permission is granted and I try to get navigator.mozNetworkStats.connectionTypes. NetworkStatsManager works as expected, but in NetworkStatsService 'aMessage.target.assertPermission("networkstats-manage")' return false.

I'm running tests on unagi device.
(In reply to Albert from comment #12)
> For test_networkstats_basics.html, when networkstats is enabled, permission
> is granted and I try to get navigator.mozNetworkStats.connectionTypes.
> NetworkStatsManager works as expected, but in NetworkStatsService
> 'aMessage.target.assertPermission("networkstats-manage")' return false.
> 
> I'm running tests on unagi device.

How are you running those tests? Are other tests that test similar APIs also failing? E.g. dom/settings/tests/test_settings_basics.html. Are they failing when you're running them on a Firefox build?
What's next - does this need a new patch?
(In reply to Dietrich Ayala (:dietrich) from comment #14)
> What's next - does this need a new patch?

Well, there's a few nits I'd like to see addressed in the tests patch. Regarding the tests not working, I'm not sure what Albert is seeing. They pass for me in a Firefox build. I kicked off a try build to see if we can land them (with nits addressed, of course): https://tbpl.mozilla.org/?tree=Try&rev=2a30b59145e4
Try build is green. Looks like we can land this as soon as the nits are addressed.
(In reply to Philipp von Weitershausen [:philikon] from comment #16)
> Try build is green. Looks like we can land this as soon as the nits are
> addressed.

Never mind, I wasn't building with --enable-b2g-ril. Investigating...
Attached patch New tests for permissions (obsolete) — Splinter Review
Changes from comment 8
Attachment #692873 - Attachment is obsolete: true
Attachment #696957 - Flags: review?(philipp)
Target Milestone: B2G C3 (12dec-1jan) → B2G C4 (2jan on)
Fixed syntax errors
Attachment #696957 - Attachment is obsolete: true
Attachment #696957 - Flags: review?(philipp)
Attachment #698695 - Flags: review?(philipp)
Comment on attachment 698695 [details] [diff] [review]
New tests for permissions

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

This is ok, apart from the fact that the tests don't pass ;). I have a patch to fix this so I'm just going to r+ this and attach my patch.
Attachment #698695 - Flags: review?(philipp) → review+
Attached patch Make tests passSplinter Review
The tests weren't passing because the messages sent by NetworkStatsManager weren't received at all, because NetworkStatsService wasn't loaded on Firefox (B2G loads it explicitly in shell.js). This patch fixes that.

On top of that, NetworkStatsManager wasn't wrapping its return values at all, so none of the values actually made it across. I wonder if this API had ever been tested at all. Yay automated tests!

On that note, we should probably always include NetworkStats in the build, but pref it off on Firefox. That way we can run the tests all the time, not just when compiling with --enable-b2g-ril (which the tinderboxes never do).
Attachment #698886 - Flags: review?(fabrice)
Attachment #698886 - Flags: review?(fabrice) → review+
Depends on: 849642
You need to log in before you can comment on or make changes to this bug.