Stefan Arentz | :st3fan | ⏰ EST | he/him Reporter
	Description • 12 years ago

Applications currently have an all-or-nothing view on settings through the 'settings' permission.

I noticed this because all built-in applications currently have read-only access to the settings, pretty much only to be able to attach an observer navigator.mozSettings to keep track of language.current. (This is through the shared/js/l10n.js module)

This means that an application that we have rated low risk, like the Radio, can actually lookup the WiFi password and the phone's pin code. (I think this actually means we should consider every app high risk now.)

Anyway, this sharing of all settings is not a good design and it should be a high priority to do this in a better way in the next release. Some suggestions:

* Allow certain settings to be public and read-only. For example the locale settings are important to any app (certified, privileged and web) and should be available.
* Some settings are only relevant to a specific app. There is no need at all for the phone's pincode to be shared. Just we should have a way to limit these to just the homescreen and the settings app.

Filed under Gaia::Settings even though this is more an API issue.

	Daniel Veditz [:dveditz]
	Comment 1 • 12 years ago

There's no point in a permission for app-only prefs: apps can already store settings in localStorage or indexedDB without asking. If there's any point to a Settings permission it's to access some kind of shared permissions.

But if every app can read everything just to get one or a small number of common things then maybe that bit of data needs to be available in a different way (e.g. navigator.language), or make a way to segment settings according to category (like those in the Settings UI, network settings, personalization settings, privacy&security settings, etc).

	Daniel Veditz [:dveditz]
	Comment 2 • 12 years ago

navigator.language already exists, btw, and there was some controversy in the past about it leaking the user's chosen locale. It no longer reports the language of the browser UI but rather the value of the accept-language the user is broadcasting to the web anyway.

In keeping with that choice I would NOT expose the phone's locale setting to "web" apps, only privileged and certified ones.

	Stefan Arentz | :st3fan | ⏰ EST | he/him Reporter
	Comment 3 • 12 years ago

(In reply to Daniel Veditz [:dveditz] from comment #2)
> In keeping with that choice I would NOT expose the phone's locale setting to
> "web" apps, only privileged and certified ones.

I think that makes sense.

% ./fxos-repl.py connect app://hackme.gaiamobile.org/index.html 
Connected to app://hackme.gaiamobile.org/index.html
>>> navigator.language
en-US

I'll file a bug for making gaia/shared/js/l10n.js depend on navigator.language instead. If that is done then I think pretty much all apps can stop using the settings permission.

	Stefan Arentz | :st3fan | ⏰ EST | he/him Reporter
	Comment 4 • 12 years ago

See also bug 841196

	Paul Theriault [:pauljt] (no longer reading bugmail)
	Comment 5 • 12 years ago

(In reply to Stefan Arentz [:st3fan] from comment #0)
> Applications currently have an all-or-nothing view on settings through the
> 'settings' permission.
> 
> I noticed this because all built-in applications currently have read-only
> access to the settings, pretty much only to be able to attach an observer
> navigator.mozSettings to keep track of language.current. (This is through
> the shared/js/l10n.js module)
> 
> This means that an application that we have rated low risk, like the Radio,
> can actually lookup the WiFi password and the phone's pin code. (I think
> this actually means we should consider every app high risk now.)

This is exactly right. There was discussion of segregating settings into 'high risk' and 'low risk. 

So I dont see this so much as a bug, but really a feature request, to have different levels of "settings" and have seperate "access" levels on the 'settings' permission.

> 
> Anyway, this sharing of all settings is not a good design and it should be a
> high priority to do this in a better way in the next release. Some
> suggestions:
> 
> * Allow certain settings to be public and read-only. For example the locale
> settings are important to any app (certified, privileged and web) and should
> be available.
> * Some settings are only relevant to a specific app. There is no need at all
> for the phone's pincode to be shared. Just we should have a way to limit
> these to just the homescreen and the settings app.

Again this is somewhat historical - the exact segregation of apps wasn't really clear until we actually built Gaia. And I am expecting to see lots of changes in the future. So whatever segregation we do introduce will need to be flexible enough to support all use cases. I am also wary of creating complexity, when the current intention for the settings API is that it will never be exposed to third-party apps. 

I like the idea of public/read-only access to certain settings - I think we would always want permission declaration to access even low risk settings, since there is a privacy aspect to even low risk settings.

I dont think we need settings per app - since that is what indexeddb/localstorage etc already provides. But maybe hive off "system" settings or something, and restrict them to only the System and Settings APp. (I would deliberately EXCLUDE homescreen from this group, since homescreen has significant larger attack surface - everything.me, shortcuts, lots of user interaction etc)

> 
> Filed under Gaia::Settings even though this is more an API issue.

	Jason Smith [:jsmith]
	Comment 6 • 12 years ago

Stefan - Do you think this is worth tracking for v1? Important enough to stop ship?

	Naoki Hirata :nhirata (please use needinfo instead of cc)
	Comment 7 • 12 years ago

Does this mean that a person can write a webapp and get the pin, wifi security password, and other setting information from settings?

	Stefan Arentz | :st3fan | ⏰ EST | he/him Reporter
	Comment 8 • 12 years ago

No, only certified applications can use the settings api. So the attack surface is limited but it is still not good that for example an app like the Radio or Camera can access the passcode.

	Paul Theriault [:pauljt] (no longer reading bugmail)
	Comment 9 • 12 years ago

Note, that this is pretty much a dupe of 763965 but there is better discussion here so I am leaving this open (is this ok)?

	Paul Theriault [:pauljt] (no longer reading bugmail)
	Comment 11 • 12 years ago

This is not blocking, but is something we should aim for as it would provide significant risk mitigation in the future. At last count there are 8 apps with read-write access to the settings API, which means 8 apps which basically have full access to the phone. 

An exercise that would be a good first step would be to make a list of all the settings, and try to separate between innocuous settings like wallpaper, and more critical settings like pin code. It would be good to come up with a simple classification system as well that will work for settings added in the future.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 12 • 12 years ago

As-filed, I think this bug is WONTFIX. Like dveditz points out in comment 1, if an application wants a non-global setting, they can just use IndexedDB. The purpose of the settings API was specifically to support system-wide settings.

However we should support granting access to settings on a per-setting basis. So that we can enumerate in the permissions section of the manifest enumerate that an application has read-only access to settings A and B, and read-write access to settings C and D.

But please lets not morph this bug into that since morphed bugs usually get very confused.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 13 • 12 years ago

Resolving this as WONTFIX per comment 13. If we want to have multiple permission levels for settings, or have the ability to whitelist which settings are granted access to, (which I think we should), then that's a separate bug.

	Stefan Arentz | :st3fan | ⏰ EST | he/him Reporter
	Comment 14 • 12 years ago

You can ignore the recommendations/ideas that were posted in this bug. Although that is fun brainstorming, it is not relevant.

The gist of this security bug is:

"Applications currently have an all-or-nothing view on settings through the 'settings' permission."

And that situation has not changed. So I would like to keep this bug alive and under the security reviews until work has done to improve this.

We can let the bugs for the actual implementation block this one.

See it more as a tracking bug for the sec team than a todo bug for the b2g team.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 15 • 12 years ago

(In reply to Stefan Arentz [:st3fan] from comment #15)
> The gist of this security bug is:
> 
> "Applications currently have an all-or-nothing view on settings through the
> 'settings' permission."

That's not the issue that's described in the bug summary and that's only partially what comment 0 talks about.

The actual change that we should make is to make the settings permission grantable on a per-setting basis.

Morphing this bug into covering that solution is only going to make discussions in this bug more confused and the bug is going to be less effective. It would be more effective to file a separate bug on the actual change that we want to make.

Morphing bugs are always a bad idea and despite this it constantly happens, and it almost always leads to wasted time and lots of confusion.

	Paul Theriault [:pauljt] (no longer reading bugmail)
	Comment 16 • 12 years ago

Raised 846200 to develop the fix for this bug.