776835 - Apply security checks to PHal

Status: RESOLVED FIXED

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

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Some of the interfaces, like setting the system clock, need permissions checks.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This will be very easy to do, but it's going to take me some time to map the Hal interfaces to DOM permissions for the client APIs.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(Where such permissions exist ...)

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Check process capabilities in hal

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 646422 [details] [diff] [review]
Check process capabilities in hal

The checks and Permission Names here were derived from

https://docs.google.com/spreadsheet/ccc?key=0Akyz_Bqjgf5pdENVekxYRjBTX0dCXzItMnRyUU1RQ0E#gid=0

Comment 5

[Justin Lebar (not reading bugmail)]

Comment on attachment 646422 [details] [diff] [review]
Check process capabilities in hal

>   virtual bool
>   RecvSetLight(const LightType& aLight,  const hal::LightConfiguration& aConfig, bool *status) MOZ_OVERRIDE
>   {
>+    // XXX we don't currently expose light control independently, but
>+    // we do set button backlight on/off along with screen on/off.

Nit: Expose light control to what, and independent of what?

>   virtual bool
>   RecvEnableSwitchNotifications(const SwitchDevice& aDevice) MOZ_OVERRIDE
>   {
>+    // Content has no reason to listen to switch events currently.
>+    switch (aDevice) {
>+    SWITCH_HEADPHONES:
>+    SWITCH_USB:
>+      return false;
>+    }
>+
>     hal::RegisterSwitchObserver(aDevice, this);
>     return true;
>   }
>@@ -560,16 +629,23 @@ public:
>   virtual bool
>   RecvGetCurrentSwitchState(const SwitchDevice& aDevice, hal::SwitchState *aState) MOZ_OVERRIDE
>   {
>+    // Content has no reason to listen to switch events currently.
>+    switch (aDevice) {
>+    SWITCH_HEADPHONES:
>+    SWITCH_USB:
>+      return false;
>+    }
>+
>     *aState = hal::GetCurrentSwitchState(aDevice);
>     return true;
>   }
> };

Isn't this the equivalent of making Recv{Enable,Get}CurrentSwitchState return false unconditionally?  (The only other two switch devices are UNKNOWN and NUM_SWITCH_DEVICE.)  If so, can we make this less obfuscated?

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Justin Lebar [:jlebar] from comment #5)
> Comment on attachment 646422 [details] [diff] [review]
> Check process capabilities in hal
> 
> >   virtual bool
> >   RecvSetLight(const LightType& aLight,  const hal::LightConfiguration& aConfig, bool *status) MOZ_OVERRIDE
> >   {
> >+    // XXX we don't currently expose light control independently, but
> >+    // we do set button backlight on/off along with screen on/off.
> 
> Nit: Expose light control to what, and independent of what?
> 

    // XXX currently, the hardware key light and screen backlight are
    // controlled as a unit.  Those are set through the power API, and
    // there's no other way to poke lights currently, so we require
    // "power" privileges here.

> >   virtual bool
> >   RecvEnableSwitchNotifications(const SwitchDevice& aDevice) MOZ_OVERRIDE
> >   {
> >+    // Content has no reason to listen to switch events currently.
> >+    switch (aDevice) {
> >+    SWITCH_HEADPHONES:
> >+    SWITCH_USB:
> >+      return false;
> >+    }
> >+
> >     hal::RegisterSwitchObserver(aDevice, this);
> >     return true;
> >   }
> >@@ -560,16 +629,23 @@ public:
> >   virtual bool
> >   RecvGetCurrentSwitchState(const SwitchDevice& aDevice, hal::SwitchState *aState) MOZ_OVERRIDE
> >   {
> >+    // Content has no reason to listen to switch events currently.
> >+    switch (aDevice) {
> >+    SWITCH_HEADPHONES:
> >+    SWITCH_USB:
> >+      return false;
> >+    }
> >+
> >     *aState = hal::GetCurrentSwitchState(aDevice);
> >     return true;
> >   }
> > };
> 
> Isn't this the equivalent of making Recv{Enable,Get}CurrentSwitchState
> return false unconditionally?  (The only other two switch devices are
> UNKNOWN and NUM_SWITCH_DEVICE.)  If so, can we make this less obfuscated?

Yes.  Sure.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Failing on try

I/Gecko   (10188): Protocol error: Handler for EnableWakeLockNotifications returned error code
I/Gecko   (10188): 
I/Gecko   (10188): ###!!! [Parent][AsyncChannel] Error: Processing error: message was deserialized, but the handler returned false (indicating failure)
I/Gecko   (10188): 

why are we enabling wake lock notifications in content processes on android but not b2g ...

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Don't try to manage wake locks in android child processes

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 652334 [details] [diff] [review]
Don't try to manage wake locks in android child processes

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

::: widget/android/nsAppShell.cpp
@@ +180,5 @@
>  {
>      gAppShell = this;
>      sAfterPaintListener = new AfterPaintListener();
>  
> +    if (XRE_GetProcessType() == GeckoProcessType_Default) {

nit, just return early

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Pushed with nit pick

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e314d3b7120

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9e314d3b7120