2032672 - PHal handlers in HalParent grant all content processes unfiltered access to device APIs, ignoring per-feature prefs

Status: NEW

(Core :: Hardware Abstraction Layer (HAL), defect)

Description

[pakhunov.anton.n]

The PHal protocol used by content processes to reach device HAL (hal/sandbox/SandboxHal.cpp)
predates the per-feature prefs that gate several of these APIs in WebIDL. Several of its
Recv handlers in HalParent carry comments admitting as much:

RecvVibrate                         "We give all content vibration permission."
RecvEnableSensorNotifications       "We currently allow any content to register
                                     device-sensor listeners."
RecvGetCurrentBatteryInformation    "We give all content battery-status permission."
RecvEnableBatteryNotifications      "We give all content battery-status permission."
RecvEnableNetworkNotifications      "We give all content access to this network-status
                                     information."
RecvGetCurrentNetworkInformation    (same intent)
RecvModifyWakeLock                  "We allow arbitrary content to use wake locks."
RecvLockScreenOrientation           FIXME/bug 777980 already acknowledges this one.

Meanwhile the WebIDL side gates these features behind per-feature prefs that default to
false on release builds:

dom.vibrator.enabled                    Navigator.vibrate
device.sensors.ambientLight.enabled     DeviceLightEvent
device.sensors.proximity.enabled        DeviceProximityEvent
dom.netinfo.enabled                     NetworkInformation API

The pref enforcement lives only in the WebIDL binding on the child side. A compromised
content process that issues raw PHal IPC goes around the WebIDL layer entirely, and the
HalParent handlers hand out the feature without looking at the pref. Concretely the
renderer gets:

• Vibrate on Android without a user gesture, with the Vibration API pref off.
• Ambient-light and proximity sensor events. These have been pref-gated for a reason
  (environment/physical-state fingerprinting) and the gate is bypassed. On platforms
  with the sensor actually wired up, the renderer receives continuous readings.
• Battery level and charging status (the Battery Status API was removed from the web
  platform in 2016 over fingerprinting, and the fact that the pref is off in release
  reflects that - but the underlying IPC still serves the same data).
• Network type, wifi state, default gateway. Useful for mobile-carrier fingerprinting
  and for probing local network layout.
• Wake lock on any topic, with no limit, keeping the device awake. Battery drain and a
  side-channel into the power-management state.
• Screen orientation lock without being in fullscreen, which is bug 777980 in disguise -
  the FIXME in the source actually says the check is missing because "we don't have that
  information currently".

Root cause is that HalParent was written before per-feature prefs existed and never
updated when those prefs were added. Fix is per-handler, small: each handler gains a
StaticPrefs check for its corresponding pref, and RecvEnableSensorNotifications switches
on the SensorType and checks the matching pref for ambient-light and proximity.

IPCResult RecvVibrate(...) {
  if (!StaticPrefs::dom_vibrator_enabled()) return IPC_OK();
  hal::Vibrate(pattern, WindowIdentifier(std::move(id), nullptr));
  return IPC_OK();
}

IPCResult RecvEnableSensorNotifications(const SensorType& aSensor) {
  if (aSensor == SENSOR_AMBIENT_LIGHT &&
      !StaticPrefs::device_sensors_ambientLight_enabled()) return IPC_OK();
  if (aSensor == SENSOR_PROXIMITY &&
      !StaticPrefs::device_sensors_proximity_enabled()) return IPC_OK();
  hal::RegisterSensorObserver(aSensor, this);
  return IPC_OK();
}

and so on for the battery/network/netinfo/wakelock handlers. The screen-orientation one
is already tracked as bug 777980.

Verified on mozilla-central tip via hg-edge raw-file of hal/sandbox/SandboxHal.cpp. The
comments quoted above are in the source at tip.

Comment 1

[Andrew McCreight [:mccr8]]

This would be nice to fix, but I don't think we're really trying to guarantee resistance from fingerprinting in the face of arbitrary code execution in a content process.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:gsvelto, could you have a look please?

For more information, please visit BugBot documentation.

Comment 3

[Gabriele Svelto [:gsvelto]]

I'm not really sure how bad this would be in practice, Andrew do you think this is an S3 or is it worthy of an S2 status?

Comment 4

[Andrew McCreight [:mccr8]]

I think S3 is fine but as comment 0 it wouldn't be too hard to fix (though it would be better to make failure pref checks do IPC_FAIL instead of IPC_OK). In my experience with adding pref checks to Recv methods, the main hassle is tests that flip prefs can be a bit fussy.