Ben Turner (not reading bugmail, use the needinfo flag!) Reporter
	Description • 12 years ago

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.

	Andrew Overholt [:overholt]
	Comment 2 • 12 years ago

Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.

	Alex Keybl [:akeybl]
	Comment 3 • 12 years ago

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.

	Philipp von Weitershausen [:philikon]
	Comment 4 • 12 years ago

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.)

	Albert [:albert] Assignee
	Comment 5 • 12 years ago

Applied comments

	Albert [:albert] Assignee
	Comment 6 • 12 years ago

Added tests for permissions

	Albert [:albert] Assignee
	Comment 7 • 12 years ago

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?

	Philipp von Weitershausen [:philikon]
	Comment 8 • 12 years ago

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!

	Philipp von Weitershausen [:philikon]
	Comment 9 • 12 years ago

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.)

	Albert [:albert] Assignee
	Comment 10 • 12 years ago

(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?

	Philipp von Weitershausen [:philikon]
	Comment 11 • 12 years ago

(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?

	Albert [:albert] Assignee
	Comment 12 • 12 years ago

(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.

	Philipp von Weitershausen [:philikon]
	Comment 13 • 12 years ago

(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?

	Dietrich Ayala (:dietrich)
	Comment 14 • 12 years ago

What's next - does this need a new patch?

	Philipp von Weitershausen [:philikon]
	Comment 15 • 12 years ago

(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

	Philipp von Weitershausen [:philikon]
	Comment 16 • 12 years ago

Try build is green. Looks like we can land this as soon as the nits are addressed.

	Philipp von Weitershausen [:philikon]
	Comment 17 • 12 years ago

(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...

	Albert [:albert] Assignee
	Comment 18 • 12 years ago

Changes from comment 8

	Albert [:albert] Assignee
	Comment 19 • 12 years ago

Fixed syntax errors

	Philipp von Weitershausen [:philikon]
	Comment 20 • 12 years ago

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.

	Philipp von Weitershausen [:philikon]
	Comment 21 • 12 years ago

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).

	Philipp von Weitershausen [:philikon]
	Comment 22 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/238d7f44bf2d
https://hg.mozilla.org/integration/mozilla-inbound/rev/a599d0887c6c
https://hg.mozilla.org/integration/mozilla-inbound/rev/ced43cbd739d

	Ed Morley [:emorley]
	Comment 23 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/238d7f44bf2d
https://hg.mozilla.org/mozilla-central/rev/a599d0887c6c
https://hg.mozilla.org/mozilla-central/rev/ced43cbd739d

	Ryan VanderMeulen [:RyanVM]
	Comment 24 • 12 years ago

https://hg.mozilla.org/releases/mozilla-b2g18/rev/76bae53a0a97
https://hg.mozilla.org/releases/mozilla-b2g18/rev/5c270eb78011
https://hg.mozilla.org/releases/mozilla-b2g18/rev/70e314f15bae