Closed Bug 776835 Opened 8 years ago Closed 8 years ago

Apply security checks to PHal

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17
blocking-basecamp +

People

(Reporter: cjones, Assigned: cjones)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

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 ...)
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 ...
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+
https://hg.mozilla.org/mozilla-central/rev/9e314d3b7120
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.