Closed Bug 1100964 Opened 6 years ago Closed 6 years ago

Expose a way to enable system app debugging

Categories

(Firefox OS Graveyard :: Developer Tools, defect)

defect
Not set
normal

Tracking

(b2g-v2.2 fixed)

RESOLVED FIXED
2.2 S2 (19dec)
Tracking Status
b2g-v2.2 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 1 obsolete file)

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.
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.
Paul, Did you had any discussion with FTU owners and reached an agreement about introducing new UI in the FTU??
Flags: needinfo?(ptheriault)
 
(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.
Flags: needinfo?(ptheriault)
(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?
Flags: needinfo?(poirot.alex)
Attached patch gecko patch v1 (obsolete) — Splinter Review
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".
(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.
Flags: needinfo?(poirot.alex)
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.
Attachment #8525388 - Flags: feedback?(fabrice)
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 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 ;)
Attachment #8525388 - Flags: feedback?(fabrice) → feedback-
Attached patch gecko patch v2Splinter Review
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
Attachment #8525388 - Attachment is obsolete: true
Attachment #8529080 - Flags: review?(fabrice)
(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 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?
Attachment #8529080 - Flags: review?(fabrice) → review+
Checkin needed for the gecko patch.
Keywords: checkin-needed
needs dom peer review for MozPowerManager.webidl
Flags: needinfo?(poirot.alex)
Keywords: checkin-needed
Comment on attachment 8529080 [details] [diff] [review]
gecko patch v2

Hey Jonas, could you review the API change made to mozPower.
Flags: needinfo?(poirot.alex)
Attachment #8529080 - Flags: review?(jonas)
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.
Attachment #8524635 - Flags: review?(arthur.chen)
Assignee: nobody → poirot.alex
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.
Attachment #8529080 - Flags: review?(jonas) → feedback+
I think we are good to go for the gecko patch.
Keywords: checkin-needed
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.
Attachment #8524635 - Flags: review?(arthur.chen) → feedback+
Comment on attachment 8524635 [details] [review]
Introduce a wipe button in settings

Arthur, I rebased, fixed the locales and switched to DialogService
Attachment #8524635 - Flags: review?(arthur.chen)
Comment on attachment 8524635 [details] [review]
Introduce a wipe button in settings

Great work! r=me with the comments addressed, thanks.
Attachment #8524635 - Flags: review?(arthur.chen) → review+
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.
Checking needed for the gaia patch, there is just a calendar intermittent.
Master: https://github.com/mozilla-b2g/gaia/commit/9cc3f74a4abfc071b0359568e4cf488e05e10b8c
Status: NEW → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S2 (19dec)
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
Thanks jryans!

Cmills, is there anywhere else this should be documented?
Flags: needinfo?(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?
Flags: needinfo?(cmills)
(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.
Cool, thanks for the explanation. I've added a note to the WebIDE page to explain this and link back to the developer setting.
You need to log in before you can comment on or make changes to this bug.