Closed Bug 1359076 Opened 7 years ago Closed 6 years ago

Disable devicelight, deviceproximity and userproximity events

Categories

(Core :: DOM: Device Interfaces, enhancement, P3)

Product:
Core
Component:
DOM: Device Interfaces
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: jkt, Assigned: jkt)

References

(Blocks 3 open bugs)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(1 file, 1 obsolete file)

Bug 1359076: Disable all Device Sensor APIs except orientation by default
59 bytes, text/x-review-board-request
bkelly
: review+
Details 
The Ambient Light API is enabled by default for Firefox, this API seems to only fire on Android from what I can see.

The API uses the same security model as generic sensor which requires a secure context:
https://www.w3.org/TR/generic-sensor/#secure-context

We should restrict these APIs to not be accessible over insecure connections.

This includes:
- https://w3c.github.io/proximity/#security-and-privacy
- https://www.w3.org/TR/orientation-event/#security-and-privacy
- https://www.w3.org/TR/ambient-light/#security-and-privacy

Any others that we may be planning to implement:

https://duckduckgo.com/?q=site%3Ahttps%3A%2F%2Fwww.w3.org%2F+%22Generic-Sensor%22&t=ffab&ia=web

See discussion thread:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/QI2-SO-1jxY
Component: DOM: Security → DOM: Device Interfaces
Assignee: nobody → jkt
Added PR to see how complex tearing these APIs would be. Will wait for the resolution of the dev-platform thread.
Comment on attachment 8861034 [details]
Bug 1359076: Disable all Device Sensor APIs except orientation by default

The PR should move to nsDeviceSensors.cpp instead of placing the removal everywhere.

Removal of Gijs to review.
Attachment #8861034 - Flags: review?(gijskruitbosch+bugs)
See Also: → 1359124
Rather than just patch the current implementation based on an old obsoleted version of the spec, you probably want to instead implement the latest privacy-aware Ambient Light Sensor spec (https://w3c.github.io/ambient-light/) and deprecate the current old implementation that has known issues.

The new spec is reworked to be based on the Generic Sensor API (https://w3c.github.io/sensors/) and it not just requires the use of a secure context but also defines the Permissions API hooks, means to configure (and for the implementation to cap) the polling frequency, among other privacy-inspired features. This is to ensure implementers can now address the concerns raised by Lukasz in his paper (referred to in https://bugzilla.mozilla.org/show_bug.cgi?id=1359124).

The latest spec is reviewed by Lukasz himself who's an active member of the W3C group responsible for the generic sensor-based API specs to ensure the latest spec passes the strictest security and privacy scrutiny.

Feel free to create a new issue to track the implementation of the new Ambient Light Sensor spec and cc me. On a related note, Chromium already implements the new Ambient Light Sensor spec behind a flag and is in process of advancing to an Origin Trial next. It would be a particularly good time for Mozilla to update its implementation now to ensure also Mozilla's implementation feedback could be considered before the spec is considered "done".
Just to set the expectations here:
It doesn't look like there's enough interest in Mozilla to spend a lot of time on this. The thread on dev-platform has not seen a lot of participants.

I think we can find someone to remove the implementation or make it "not that horrible" (reduce frequency and granularity of the read-outs). If you think it's not useful to have an outdated version implemented, we'd rather go for the former.

It sounds unrealistic to find someone that'd want to rewrite our implementation.
Unassigning myself as not working on this right now.
Assignee: jkt → nobody
Priority: -- → P3
Yeah, I think we should just remove our existing implementation. Given that we just dispatch events and don't have any objects, it's not entirely clear we need telemetry to move ahead and per bug 1359124 telemetry might be hard to gather.
Summary: Remove generic sensors over insecure request → Remove generic sensor APIs
Blocks: 754199, 876496, 1057185
Depends on: 1359124
Comment on attachment 8861034 [details]
Bug 1359076: Disable all Device Sensor APIs except orientation by default

Please take a look
Attachment #8861034 - Flags: feedback?(bkelly)
Comment on attachment 8861034 [details]
Bug 1359076: Disable all Device Sensor APIs except orientation by default

https://reviewboard.mozilla.org/r/133034/#review165744

Overall looks reasonable.  Marked r- because it seems we are still marking things as enabled even when we bail out here.

::: dom/base/nsGlobalWindow.cpp:13323
(Diff revision 1)
>      return;
>    }
>  
>    nsCOMPtr<nsIDeviceSensors> ac = do_GetService(NS_DEVICE_SENSORS_CONTRACTID);
>    if (ac) {
> +    nsCOMPtr<nsIDocument> doc = this->GetDoc();

Please move this into the block where its used.  No need to do the AddRef() here if we're not reporting to the console.

::: dom/base/nsGlobalWindow.cpp:13329
(Diff revision 1)
> +    if (!this->IsSecureContextIfOpenerIgnored()) {
> +        nsContentUtils::ReportToConsole(nsIScriptError::errorFlag,
> +                                        NS_LITERAL_CSTRING("DOM"), doc,
> +                                        nsContentUtils::eDOM_PROPERTIES,
> +                                        "GenericSensorInsecureRequestIsForbidden");
> +        return;

It seems like we should bail out before the `mEnabledSensors.AppendElement(aType);` line above.  Otherwise we are setting state like we are enabled when we are in fact not enabled.

Maybe just move the `if (!this->IsSecureContextIfOpenerInored())` check to the top of the method.
Attachment #8861034 - Flags: review-
Attachment #8861034 - Attachment is obsolete: true
Attachment #8861034 - Flags: feedback?(bkelly)
Comment on attachment 8889328 [details]
Bug 1359076: Disable all Device Sensor APIs except orientation and ambient light by default

Sorry, I flagged the wrong attachment *facepalm*.
Marked as obsolete now.

This patch here is implementing a pref observer to move the enabled/disabled check into runtime. This will be required for the test, once device sensors are disabled by default.
The pref observer will also be able hold a method per-Sensor, so we can disable all but Orientation.
Attachment #8889328 - Flags: feedback?(bkelly)
Comment on attachment 8889328 [details]
Bug 1359076: Disable all Device Sensor APIs except orientation and ambient light by default

https://reviewboard.mozilla.org/r/160400/#review165810

::: dom/events/test/test_bug742376.html:19
(Diff revision 2)
>  <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=742376">Mozilla Bug 742376</a>
>  <script class="testbody" type="text/javascript">
>  
>  /** Test for Bug 742376 **/
>  
> +SpecialPowers.setBoolPref("device.sensors.enabled", true);

This will leave the preference enabled after the test.  Please use `SpecialPowers.pushPrefEnv()` instead.  Also, you should provide a callback so that the test only proceeds after the pref has been enabled.

::: dom/system/nsDeviceSensors.cpp:46
(Diff revision 2)
> +
> +namespace {
> +  StaticRefPtr<DeviceSensorPrefManager> gPrefManager;
> +}
> +
> +class DeviceSensorPrefManager final : public nsIObserver

I think you can replace this entire class with a `Preferences::AddBoolVarCache()` call.  You just need to specify a static boolean variable for it to update.

For example:

http://searchfox.org/mozilla-central/source/dom/base/TimeoutManager.cpp#475
Attachment #8889328 - Flags: review-
Attachment #8889328 - Flags: feedback?(bkelly)
my very latest revision looks good on try https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbbb9a6959c6dbdd20e693e97c17f42483bae5be&selectedJob=117997416
Summary: Remove generic sensor APIs → Remove generic sensor APIs (except orientation/acceleration)
Comment on attachment 8889328 [details]
Bug 1359076: Disable all Device Sensor APIs except orientation and ambient light by default

https://reviewboard.mozilla.org/r/160400/#review166792

::: dom/system/nsDeviceSensors.cpp:589
(Diff revision 3)
>    mLastAcceleration.reset();
>    mLastDOMMotionEventTime = TimeStamp::Now();
>  }
>  
>  bool
> -nsDeviceSensors::AreSensorEventsDisabled(nsIDOMWindow* aWindow)
> +nsDeviceSensors::AreSensorEventsDisabled(uint32_t aType, nsIDOMWindow* aWindow)

I personally find it a bit confusing to have preferences that are `.enabled` and then map those to a method that is `IsDisabled()`.

This is fine I guess, but maybe consider switching the logic of the method around to match the preferences.  Less mental gymnastics for readers in the future.
Attachment #8889328 - Flags: review?(bkelly) → review+
Thank you for the speedy review.
There's already a IsSensorEnabled function [1] that checks for active event listeners, which we are at risk being confusing with.
I could switch it to something like checkEnabledByPref or so, though?


[1] http://searchfox.org/mozilla-central/source/dom/system/nsDeviceSensors.h#73
(In reply to Frederik Braun [:freddyb] from comment #22)
> Thank you for the speedy review.
> There's already a IsSensorEnabled function [1] that checks for active event
> listeners, which we are at risk being confusing with.
> I could switch it to something like checkEnabledByPref or so, though?

Oh, yea, then that is extra confusing even with your current patch.  Maybe avoid the word "enabled" at all.  Something like:

  IsSensorAllowedByPref()

WDYT?
Agreed. I'm on my way out, but I'll post a follow-up soon!
Firefox for Android uses the light sensor to determine if it should display reader mode articles in light mode or dark mode. 

In Google maps street view the gyroscope is used to allow the user to look around without touching the screen. The same is true of Facebook 360 photos. https://facebook360.fb.com/360-photos/
Gyroscope is in Orientation/Acceleration, which I explicitly leave enabled in this patch.
Reader mode is privileged JS, so I suggest we leave Light Sensor in but disable it for content?
(In reply to Frederik Braun [:freddyb] from comment #27)
> Reader mode is privileged JS, so I suggest we leave Light Sensor in but
> disable it for content?

Trying to clarify here because "content" is ambiguous: It's used on content windows (see AboutReader.jsm, where this._win is going to be the content's about:reader window), though, so we'd need to make it a chrome only event or something like that, rather than disabling it on type=content (or otherwise unprivileged) docshell frames which would still break this usecase.
This bug was originally created because of some security concerns for ambient light, but due to Reader Mode using it, I propose we go ahead carefully, exempting the disablement of ambient light form this patch and finding a way to make this a feature for reader-mode only in a follow up.

Ben, can you review again, so we get to have per-sensor prefs and proximity disabled?
(In reply to :Gijs from comment #28)
(In reply to Frederik Braun [:freddyb] from comment #30)

Is there a strightforward way in our WebIDL etc, to turn off the Web APIs for web contant, but keeping them accessible from about:reader? Please let me know, if we need non-trivial engineering on JS front-end (passing message from chrome-to-content process, for example.)
Comment on attachment 8889328 [details]
Bug 1359076: Disable all Device Sensor APIs except orientation and ambient light by default

Ben, can you look again?
Main goal of this bug is changed. Now we want to have per-sensor prefs and proximity disabled.
Attachment #8889328 - Flags: review+ → review?(bkelly)
Comment on attachment 8889328 [details]
Bug 1359076: Disable all Device Sensor APIs except orientation and ambient light by default

https://reviewboard.mozilla.org/r/160400/#review168398

Overall looks good, but I think we should have tests verifying the new prefs work as intended.

::: dom/system/nsDeviceSensors.cpp:39
(Diff revision 5)
>  
>  #undef near
>  
>  #define DEFAULT_SENSOR_POLL 100
>  
> +static bool gPrefSensorsEnabled= false;

nit: Missing space before the `=`.

::: dom/system/nsDeviceSensors.cpp:123
(Diff revision 5)
> +                              true);
> +  Preferences::AddBoolVarCache(&gPrefProximitySensorEnabled,
> +                              "device.sensors.proximity.enabled",
> +                              false);
> +  Preferences::AddBoolVarCache(&gPrefAmbientLightSensorEnabled,
> +                              "device.sensors.ambient_lightenabled",

I think this is missing a `.` before `enabled`?  It doesn't match the prefs file changed here.

This kind of raises the question, do we have tests verifying the proximity and ambient_light prefs work?  Can you add those?

::: dom/system/nsDeviceSensors.cpp:595
(Diff revision 5)
>  {
> -  if (!mEnabled) {
> -    return true;
> +  // checks "device.sensors.enabled" master pref
> +  if (!gPrefSensorsEnabled) {
> +    return false;
> +  }
> +  switch (aType) {

Can you add a `default:` case that does `MOZ_ASSERT_UNREACHED()` in debug builds and returns false.

::: dom/system/nsDeviceSensors.cpp:619
(Diff revision 5)
>    }
>  
>    nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(aWindow);
>  
>    if (!window) {
> -    return false;
> +    return true;

Why does this default to allowing sensors instead of disallowing?  This seems a change from the old code.  I would think the conservative position is not to allow these sensors to be used while a window is not present.
Attachment #8889328 - Flags: review?(bkelly) → review-
Comment on attachment 8889328 [details]
Bug 1359076: Disable all Device Sensor APIs except orientation and ambient light by default

https://reviewboard.mozilla.org/r/160400/#review168398

Thank you again for the speedy review, I've addressed your issues below.
It looks to me that we have no working tests for many of these features even when they are shipping right now. Note that those things are also incompliant to any specification out there.
I'll see what I can do though :/

> Can you add a `default:` case that does `MOZ_ASSERT_UNREACHED()` in debug builds and returns false.

Done, thanks!

> Why does this default to allowing sensors instead of disallowing?  This seems a change from the old code.  I would think the conservative position is not to allow these sensors to be used while a window is not present.

As suggested earlier by you, we switched the logic of this function from "do we resist fingerprinting" to "are we allowed by pref" (since we're now checking for more than just one pref). That means we'd also switch the default logic, of course.
(In reply to Frederik Braun [:freddyb] from comment #34)
> It looks to me that we have no working tests for many of these features even
> when they are shipping right now. Note that those things are also
> non-compliant to any specification out there.

What do you suggest we do here, Ben?
Flags: needinfo?(bkelly)
Add tests or remove the non-standard APIs?
Flags: needinfo?(bkelly)
The goal is to remove, but to make removal less risky, hence we disable by pref instead.
Does that not qualify? :/
Flags: needinfo?(bkelly)
Sure.  I think what I was asking for in my review was some test that the prefs worked.  But its fine I guess.
Flags: needinfo?(bkelly)
I'll be taking this over the line... hopefully.

https://groups.google.com/forum/#!topic/mozilla.dev.platform/DcSi_wLG4fc


Merry Y'allhands
Assignee: nobody → jkt
The title of this bug is misleading.

I suggest you rename this bug to: "Disable obsoleted devicelight, deviceproximity and userproximity events"

Specifically, this bug has nothing to do with the Generic Sensor API.

You may also want to update the corresponding MDN pages when this is FIXED:

https://developer.mozilla.org/en-US/docs/Web/API/Ambient_Light_Events
https://developer.mozilla.org/en-US/docs/Web/API/Proximity_Events
Confirm, sorry I should have spotted that before now.
The dev-doc-needed flag should take care of MDN when this is landed.
Summary: Remove generic sensor APIs (except orientation/acceleration) → Disable devicelight, deviceproximity and userproximity events
Attachment #8889328 - Attachment is obsolete: true
To note in the latest patch I have disabled the Event interfaces with the prefs also. This hopefully should disable any content code that is feature testing for this too to minimise breakage etc.

I added in the testing that you wanted too.

I also added deprecation warnings for all of these events when they are enabled including orientation.
Flags: needinfo?(bkelly)
Comment on attachment 8861034 [details]
Bug 1359076: Disable all Device Sensor APIs except orientation by default

https://reviewboard.mozilla.org/r/133034/#review229944

::: dom/webidl/DeviceOrientationEvent.webidl:8
(Diff revision 4)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/.
>   */
>  
> -[Constructor(DOMString type, optional DeviceOrientationEventInit eventInitDict), LegacyEventInit]
> +[Pref="device.sensors.orientation.enabled", Func="nsGlobalWindowInner::DeviceSensorsEnabled", Constructor(DOMString type, optional DeviceOrientationEventInit eventInitDict), LegacyEventInit]
>  interface DeviceOrientationEvent : Event

Why are the event constructors based on the top level "dev.sensors.enabled" pref, but we actually control whether we fire events based on more refined prefs like "device.sensors.orientation.enabled"?  Shouldn't these align with one another?
Attachment #8861034 - Flags: review?(bkelly) → review-
Oh, I misread that.  It has a [Pref=] and [Func=].
Flags: needinfo?(bkelly)
> Why are the event constructors based on the top level "dev.sensors.enabled" pref, but we actually control whether we fire events based on more refined prefs like "device.sensors.orientation.enabled"?  Shouldn't these align with one another?

It seems webidl can't take more than one Pref= which required me to make a function too.

I didn't want to define 4 separate functions for both the pref and top level. I also wanted the top level disable to apply to these constructors too like it always should have.
Flags: needinfo?(bkelly)
Comment on attachment 8861034 [details]
Bug 1359076: Disable all Device Sensor APIs except orientation by default

https://reviewboard.mozilla.org/r/133034/#review229950

I think you need to update test_interfaces.js to properly check for the event interfaces when on the release channel.  For example:

https://searchfox.org/mozilla-central/rev/14d933246211b02f5be21d2e730a57cf087c6606/dom/tests/mochitest/general/test_interfaces.js#288

I think adding `release: true` there should work.  And for the ambient light stuff.
(In reply to Jonathan Kingston [:jkt] from comment #49)
> > Why are the event constructors based on the top level "dev.sensors.enabled" pref, but we actually control whether we fire events based on more refined prefs like "device.sensors.orientation.enabled"?  Shouldn't these align with one another?
> 
> It seems webidl can't take more than one Pref= which required me to make a
> function too.
> 
> I didn't want to define 4 separate functions for both the pref and top
> level. I also wanted the top level disable to apply to these constructors
> too like it always should have.

Yea, I missed the combined Pref and Func.  I think the only issue I see now is that test_interfaces.js needs to be updated.  (Or maybe it was and I just don't see in mozreview.)
Flags: needinfo?(bkelly)
Comment on attachment 8861034 [details]
Bug 1359076: Disable all Device Sensor APIs except orientation by default

https://reviewboard.mozilla.org/r/133034/#review229974

::: dom/tests/mochitest/general/test_interfaces.js:286
(Diff revision 5)
>  // IMPORTANT: Do not change this list without review from a DOM peer!
> -    {name: "DeviceLightEvent", insecureContext: true},
> +    {name: "DeviceLightEvent", insecureContext: true, release: true},
>  // IMPORTANT: Do not change this list without review from a DOM peer!
> -    {name: "DeviceMotionEvent", insecureContext: true},
> +    {name: "DeviceMotionEvent", insecureContext: true, release: true},
>  // IMPORTANT: Do not change this list without review from a DOM peer!
> -    {name: "DeviceOrientationEvent", insecureContext: true},
> +    {name: "DeviceOrientationEvent", insecureContext: true, release: true},

The prefs don't disable orientation and motion event on nightly, right?  I think you only want `release:true` for `DeviceLightEvent` and `DeviceProximityEvent`.
Attachment #8861034 - Flags: review?(bkelly) → review-
Comment on attachment 8861034 [details]
Bug 1359076: Disable all Device Sensor APIs except orientation by default

https://reviewboard.mozilla.org/r/133034/#review229994
Attachment #8861034 - Flags: review?(bkelly) → review+
Looks like this is getting a clean test run, should we land the patch or wait for the next release?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e9ae9f47d3c4f2fac1de304fefd7c23a18288c3&selectedJob=165002506
Flags: needinfo?(lhenry)
Flags: needinfo?(bkelly)
I'd say land it now for 60 - good idea to get it in before the merge tomorrow morning.
After that we try to limit the amount of API changes or major feature changes in Nightly.
Flags: needinfo?(lhenry) → needinfo?(jcristau)
Confirmed with :lizzard on irc that this is OK to land for this release before the soft freeze.
Flags: needinfo?(jcristau)
Flags: needinfo?(bkelly)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 10d54bb19c90924aa92cb5285ccda2d85e941430 -d 940b93518f53: rebasing 449509:10d54bb19c90 "Bug 1359076: Disable all Device Sensor APIs except orientation by default r=bkelly" (tip)
merging dom/base/nsDeprecatedOperationList.h
merging dom/base/nsGlobalWindowInner.cpp
merging dom/base/nsGlobalWindowInner.h
merging dom/events/test/mochitest.ini
merging dom/locales/en-US/chrome/dom/dom.properties
merging dom/tests/mochitest/general/test_interfaces.js
merging modules/libpref/init/all.js
warning: conflicts while merging dom/base/nsDeprecatedOperationList.h! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/locales/en-US/chrome/dom/dom.properties! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by jkingston@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/313ea836726d
Disable all Device Sensor APIs except orientation by default r=bkelly
https://hg.mozilla.org/mozilla-central/rev/313ea836726d
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1442417
LGTM, thanks for your hard work Chris!
Flags: needinfo?(jkt)
Jonathan, what is the timeline for disabling the ambient light sensor API in the Firefox Release channel? The API pref is currently only disabled #ifdef EARLY_BETA_OR_EARLIER.

Bug 1359124 comment 17 says we plan to disable the API without telemetry.
Flags: needinfo?(jkt)
From the Intent to Unship "My plan is to deprecate light and proximity sensors in Stable Firefox in version 62 if no issues arise."

There has been no reported issues. I just hadn't gotten to removing it yet. Raising a bug now to remove :).

Thanks
Flags: needinfo?(jkt)
Also I have been firing DEPRECATED telemetry, the numbers for both of these sensors don't even seem to feature in telemetry due to being so low. Where as MotionEvent is around 0.33% usage it seems.
Thanks! Sorry I didn't see the intent to unship. Here is a link to that thread for posterity:

https://groups.google.com/d/msg/mozilla.dev.platform/DcSi_wLG4fc/iNUKIq9SAAAJ
Blocks: 1462308
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: