1100964 - Expose a way to enable system app debugging

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Developer Tools, defect)

Description

[Alexandre Poirot [:ochameau]]

To followup the various discussions on the ML:
  Old first discussion about devtools and security limitations:
    https://groups.google.com/d/msg/mozilla.dev.b2g/TgKygGrmsVo/bwofq8KhtqwJ
  The recent one, justifying this bug:
    https://groups.google.com/d/msg/mozilla.dev.b2g/skYzCcgdMVk/mpQaihO_2UsJ

We would start by adding an item in the developer menu, in order to reset the phone with the unrestricted devtools mode, where you are not limited to debug just the app you pushed, but also certified and pre-installed apps as well as chrome of apps and parent process. This mode allows hacking on Gaia instead of just single apps.

Then, we would try to introduce an item into FTU, in order to enable this mode while suggesting the developer to keep a pincode on his phone.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Introduce a wipe button in settings

I started looking at how to wipe the phone.
navigator.mozPower.factoryReset has two modes "normal" and "wipe",
but, unfortunately, both of them do not show the FTU after the reset.

If we want to just introduce a "Wipe and enable unrestricted mode" button,
we would most likely have to add a new mode to factoryReset,
but I'm not sure it is reasonable?

Otherwise, we can fix wipe or normal, in order to ensure that we show the FTU on reset and then somehow expose the unrestricted toggle in some pages of the FTU.

Comment 2

[Alexandre Poirot [:ochameau]]

Paul, Did you had any discussion with FTU owners and reached an agreement about introducing new UI in the FTU??

Comment 3

[Paul Theriault [:pauljt] (no longer reading bugmail)]

 
(In reply to Alexandre Poirot [:ochameau] from comment #1)
> Created attachment 8524635 [details] [review]
> Introduce a wipe button in settings

There is already a wipe but under More Information -> Reset Phone. I assume the point of this patch is that _this_ developer wipe button would also toggle the preference unrestricted devtools access?



> 
> I started looking at how to wipe the phone.
> navigator.mozPower.factoryReset has two modes "normal" and "wipe",
> but, unfortunately, both of them do not show the FTU after the reset.

I've talked about this approach with Fabrice before, and the challenge is getting something to persist after factory reset, since that it wipes everything. But then I don't understand why FTU isn't shown (how is this state persisted?

> 
> If we want to just introduce a "Wipe and enable unrestricted mode" button,
> we would most likely have to add a new mode to factoryReset,
> but I'm not sure it is reasonable?
> 
> Otherwise, we can fix wipe or normal, in order to ensure that we show the
> FTU on reset and then somehow expose the unrestricted toggle in some pages
> of the FTU.

Either approach is fine with me. 

(In reply to Alexandre Poirot [:ochameau] from comment #2)
> Paul, Did you had any discussion with FTU owners and reached an agreement
> about introducing new UI in the FTU??

No, not yet - adding FTU module owner to this bug. I sent an email to UX too in case they need to be involved.

Comment 4

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Alexandre Poirot [:ochameau] from comment #1)
> Created attachment 8524635 [details] [review]
> Introduce a wipe button in settings
> 
> I started looking at how to wipe the phone.
> navigator.mozPower.factoryReset has two modes "normal" and "wipe",
> but, unfortunately, both of them do not show the FTU after the reset.

Are you sure? I just tested this on 2.1,using the existing button in the settings app, and I was shown FTU after factory reset?

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: gecko patch v1

I'm all but sure it is a reasonable way to implement
a "reset and activate unrestricted devtools" feature on gecko side.

As I wasn't sure it was possible to introduce a new mode to mozPower.factoryReset,
I worked around that by using a setting to pass additional commands
to execute after reset reboot.

But that works...
Also here, I'm using "wipe" feature, that also deletes sdcard.
I'm not sure we want to wipe, but just do the "regular reset".

Comment 6

[Alexandre Poirot [:ochameau]]

(In reply to Paul Theriault [:pauljt] from comment #4)
> (In reply to Alexandre Poirot [:ochameau] from comment #1)
> > Created attachment 8524635 [details] [review]
> > Introduce a wipe button in settings
> > 
> > I started looking at how to wipe the phone.
> > navigator.mozPower.factoryReset has two modes "normal" and "wipe",
> > but, unfortunately, both of them do not show the FTU after the reset.
> 
> Are you sure? I just tested this on 2.1,using the existing button in the
> settings app, and I was shown FTU after factory reset?

That may be because of engineering build or something... I don't know why I don't get the FTU again.

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 8525388 [details] [diff] [review]
gecko patch v1

Fabrice, any feeling to share about this patch?
I'm trying to implement a "reset and activate devtools unrestricted mode" button in the settings apps.
This patch is a workaround to prevent introducing a new mode in mozPower.factoryReset. I can also just introduce mozPower.factoryReset("root"), or something, if you think that's better option.

Comment 8

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Comment on attachment 8525388 [details] [diff] [review]
gecko patch v1

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

When does the preference change happen  ProcessGlobal.js? Before or after the wipe - after would be better I think? Keep in mind that an attacker can sideload an app with the power permission, and call factoryReset too. This is fine, as long as the wipe happens first (otherwise the attacker could initiate the wipe, then pull the battery to flip the pref without doing it.)

::: b2g/components/RecoveryService.js
@@ +98,5 @@
> +      try {
> +        let cmds = Services.prefs.getCharPref("b2g.wipe.additional-commands");
> +        log("Pass additional commands: " + cmds);
> +        text += cmds;
> +      } catch(e) {}

Just be careful here - hijacking recovery service is pretty common way to root a device. This looks ok at a glance though (the commands are coded as a preference, and without root you can't change it right?)

Comment 9

[[:fabrice] Fabrice Desré]

Comment on attachment 8525388 [details] [diff] [review]
gecko patch v1

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

I don't think you should piggyback on 'wipe', because it's erasing the sd-card and you will make many people unhappy... I don't see any special issue with using a new mode instead. I'll let you bikeshed the naming though ;)

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: gecko patch v2

Ok, so what about this?
  
  navigator.mozPower.factoryReset('root');

I'm no longer hacking the 'wipe' command, but I'm still using the same post-reset file helper.


https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0bc0270c6855

Comment 11

[Alexandre Poirot [:ochameau]]

(In reply to Paul Theriault [:pauljt] from comment #8)
> Comment on attachment 8525388 [details] [diff] [review]
> gecko patch v1
> 
> Review of attachment 8525388 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> When does the preference change happen  ProcessGlobal.js? Before or after
> the wipe - after would be better I think? Keep in mind that an attacker can
> sideload an app with the power permission, and call factoryReset too. This
> is fine, as long as the wipe happens first (otherwise the attacker could
> initiate the wipe, then pull the battery to flip the pref without doing it.)

Reset happen first, we read the file on the first launch after reset.

Comment 12

[Alexandre Poirot [:ochameau]]

Comment on attachment 8524635 [details] [review]
Introduce a wipe button in settings

I've updated the gaia patch accordingly.
The existing button in Settings > More information isn't enough, as we need a dedicated one, just for devtools, that would reset AND root the phone.

Any idea who should review this?

Comment 13

[Alexandre Poirot [:ochameau]]

Checkin needed for the gecko patch.

Comment 14

[Carsten Book [:Tomcat]]

needs dom peer review for MozPowerManager.webidl

Comment 15

[Alexandre Poirot [:ochameau]]

Comment on attachment 8529080 [details] [diff] [review]
gecko patch v2

Hey Jonas, could you review the API change made to mozPower.

Comment 16

[Alexandre Poirot [:ochameau]]

Comment on attachment 8524635 [details] [review]
Introduce a wipe button in settings

Hi Arthur,

I'm adding a new item in the developer menu, a button to reset AND unlock devtools privileges. It is different from the one to reset the phone in "More information" menu.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8529080 [details] [diff] [review]
gecko patch v2

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

I don't know the code well enough to really mark r+. But the approach seems great so thumbs up from me.

Comment 18

[Alexandre Poirot [:ochameau]]

I think we are good to go for the gecko patch.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/2e4cbe93b9b0

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2e4cbe93b9b0

Comment 21

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8524635 [details] [review]
Introduce a wipe button in settings

Overall it looks good, f=me. Suggest to display the dialog using the DialogService module.

Comment 22

[Alexandre Poirot [:ochameau]]

Comment on attachment 8524635 [details] [review]
Introduce a wipe button in settings

Arthur, I rebased, fixed the locales and switched to DialogService

Comment 23

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8524635 [details] [review]
Introduce a wipe button in settings

Great work! r=me with the comments addressed, thanks.

Comment 24

[Alexandre Poirot [:ochameau]]

Oh I understand now why we have all these tv/tablet specific strings! (I was really wondering why while writing the patch...)

Patch updated, waiting for try results.

Comment 25

[Alexandre Poirot [:ochameau]]

Checking needed for the gaia patch, there is just a calendar intermittent.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/9cc3f74a4abfc071b0359568e4cf488e05e10b8c

Comment 27

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

This option has been documented[1] on the Developer Settings page, but there may be other places that it should be included as well.

[1]: https://developer.mozilla.org/en-US/Firefox_OS/Debugging/Developer_settings#Reset_and_enable_full_DevTools

Comment 28

[Dietrich Ayala (:dietrich)]

Thanks jryans!

Cmills, is there anywhere else this should be documented?

Comment 29

[Chris Mills [:cmills]]

I think jryans has got the settings page entry perfect, although I think the linked section needs some updates:

https://developer.mozilla.org/en-US/docs/Tools/WebIDE#Unrestricted_app_debugging_%28including_certified_apps.2C_main_process.2C_etc.%29

When you select "Reset and enable full DevTools", how does this relate to the linked section? Does it do the same thing as the WebIDE "request higher privileges" setting?

Comment 30

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #29)
> I think jryans has got the settings page entry perfect, although I think the
> linked section needs some updates:
> 
> https://developer.mozilla.org/en-US/docs/Tools/
> WebIDE#Unrestricted_app_debugging_%28including_certified_apps.
> 2C_main_process.2C_etc.%29
> 
> When you select "Reset and enable full DevTools", how does this relate to
> the linked section? Does it do the same thing as the WebIDE "request higher
> privileges" setting?

They both flip the same pref to enable full DevTools access.  WebIDE does not reset the device (delete all user data) because the device must already be rooted to use the WebIDE button successfully (so it can't be used by most with production devices, since they are not rooted).

The Settings app button "Reset and enable full DevTools" will work for any user, whether they have a rooted device or not, but it also wipes user data to ensure it can't be accessed by attacker trivially flipping a toggle switch.

Comment 31

[Chris Mills [:cmills]]

Cool, thanks for the explanation. I've added a note to the WebIDE page to explain this and link back to the developer setting.