Last Comment Bug 776835 - Apply security checks to PHal
: Apply security checks to PHal
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Hardware Abstraction Layer (HAL) (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
:
Mentors:
Depends on: 777980
Blocks: 776652
  Show dependency treegraph
 
Reported: 2012-07-23 23:48 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-08-17 05:29 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Check process capabilities in hal (12.77 KB, patch)
2012-07-26 17:21 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
justin.lebar+bug: review+
Details | Diff | Review
Don't try to manage wake locks in android child processes (1.41 KB, patch)
2012-08-15 23:26 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
blassey.bugs: review+
Details | Diff | Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-23 23:48:48 PDT
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 2012-07-24 02:07:15 PDT
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 2012-07-24 02:07:29 PDT
(Where such permissions exist ...)
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-26 17:21:07 PDT
Created attachment 646422 [details] [diff] [review]
Check process capabilities in hal
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-27 13:49:02 PDT
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) 2012-07-30 10:40:08 PDT
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 2012-08-07 13:20:07 PDT
(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 2012-08-15 23:07:32 PDT
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 2012-08-15 23:26:33 PDT
Created attachment 652334 [details] [diff] [review]
Don't try to manage wake locks in android child processes
Comment 9 Brad Lassey [:blassey] (use needinfo?) 2012-08-16 12:06:01 PDT
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 2012-08-16 12:36:00 PDT
Pushed with nit pick

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e314d3b7120
Comment 11 Ed Morley [:emorley] 2012-08-17 05:29:45 PDT
https://hg.mozilla.org/mozilla-central/rev/9e314d3b7120

Note You need to log in before you can comment on or make changes to this bug.