The default bug view has changed. See this FAQ.

Apply security checks to PHal

RESOLVED FIXED in mozilla17

Status

()

Core
Hardware Abstraction Layer (HAL)
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

(Depends on: 1 bug)

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

Attachments

(2 attachments)

Some of the interfaces, like setting the system clock, need permissions checks.
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.
Assignee: nobody → jones.chris.g
(Where such permissions exist ...)
Depends on: 777980
Created attachment 646422 [details] [diff] [review]
Check process capabilities in hal
blocking-basecamp: --- → +
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
Attachment #646422 - Flags: review?(justin.lebar+bug)
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?
Attachment #646422 - Flags: review?(justin.lebar+bug) → review+
(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.
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 ...
Created attachment 652334 [details] [diff] [review]
Don't try to manage wake locks in android child processes
Attachment #652334 - Flags: review?(blassey.bugs)
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
Attachment #652334 - Flags: review?(blassey.bugs) → review+
Pushed with nit pick

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e314d3b7120
https://hg.mozilla.org/mozilla-central/rev/9e314d3b7120
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.