Closed Bug 1121045 Opened 9 years ago Closed 9 years ago

[b2g] we need root access on the b2g builds ( 3.0 & 2.2) for flame-kk

Categories

(Release Engineering :: Applications: MozharnessCore, defect, P2)

Product:
Release Engineering
Component:
Applications: MozharnessCore
Platform:
ARM
Gonk (Firefox OS)
Type:
defect

Tracking

(blocking-b2g:2.2+, firefox38 wontfix, firefox39 wontfix, firefox40 fixed, b2g-v2.2 verified, b2g-master verified)

Status:
VERIFIED FIXED
Project Flags:
blocking-b2g 2.2+
Tracking Flags:
Tracking Status
firefox38 --- wontfix
firefox39 --- wontfix
firefox40 --- fixed
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: pbylenga, Assigned: nhirata)

References

Details

(Keywords: qablocker)

Attachments

(1 file, 1 obsolete file)

flame-kk_userdebug.patch
492 bytes, patch
nthomas
: review+
Details | Diff | Splinter Review 
+++ This bug was initially created as a clone of Bug #1104338 +++

1. flash the 3.0 build with a full flash
2. adb shell

Expected : root access
Actual: non root access


as in Bug 1104338 we also need VARIANT=userdebug for 3.0 builds.

In the meantime we will use the workaround by switching in the boot.img from the Engineering builds.
:nthomas, do you think you could help with this, please?
Flags: needinfo?(nthomas)
Are you asking for additional builds which are VARIANT=userdebug, or modifying the existing VARIANT=user builds ?
Flags: needinfo?(nthomas)
:nhirata, could you please clarify?
Flags: needinfo?(nhirata.bugzilla)
Using replacing the VARIANT=user build with using VARIANT=userdebug instead.

VARIANT=user will cause the boot.img to have a production; which will not allow us to have root on the device

with VARIANT=userdebug, it will allow us to adb root which will allow us to get root on the device.  We need root in order to modify certain files or even have access to get to the crash ids.
Flags: needinfo?(nhirata.bugzilla)
[Blocking Requested - why for this release]:
This is a qablocker, swapping in the boot.img from engineering isn't a good recovery due to bug 1124248.
blocking-b2g: --- → 3.0?
Keywords: qablocker
:nthomas, do you think you could help with this, please?
Flags: needinfo?(nthomas)
Is this for device builds only ? All of them or just some ? It would help if these requests were as specific as possible.
Flags: needinfo?(nthomas)
Sorry, we need this for flame-kk builds for 3.0 and 2.2 for test support.

mozilla-central-flame-kk/
mozilla-b2g37_v2_2-flame-kk/
blocking-b2g: 3.0? → 2.2?
status-b2g-v2.2: --- → affected
status-b2g-master: --- → affected
Flags: needinfo?(nthomas)
Summary: [b2g] we need root access on the b2g builds ( 3.0) → [b2g] we need root access on the b2g builds ( 3.0 & 2.2) for flame-kk
I'll need to dig into our code & configuration to find out how to limit this to just flame-kk opt builds on the two branches requested.
Assignee: nobody → nthomas
Flags: needinfo?(nthomas)
Priority: -- → P2
FYI, bug 1121577 - userdebug builds are dangerous to use on the open web.
Actually we'll have to block on that, as more than QA uses these builds (eg dogfooders).
Depends on: 1121577
Modifies these builders as requested:
  b2g_mozilla-b2g37_v2_2_flame-kk_nightly
  b2g_mozilla-b2g37_v2_2_flame-kk_periodic
  b2g_mozilla-central_flame-kk_nightly
  b2g_mozilla-central_flame-kk_periodic
and also these integration branches:
  b2g_mozilla-inbound_flame-kk_periodic
  b2g_b2g-inbound_flame-kk_periodic
  b2g_fx-team_flame-kk_periodic
and these disposable branches (some of which may go in bug 1121495):
  b2g_ash_flame-kk_periodic
  b2g_cedar_flame-kk_periodic
  b2g_cypress_flame-kk_periodic
  b2g_gum_flame-kk_periodic
  b2g_jamun_flame-kk_periodic
  b2g_maple_flame-kk_periodic
b2g_oak_flame-kk_periodic
b2g_pine_flame-kk_periodic

eg:
 MockCommand {'command': ['scripts/scripts/b2g_build.py',  '--target',  'flame-kk',  '--config',  'b2g/releng-private-updates.py',  '--gaia-languages-file',  'locales/languages_all.json',  '--gecko-languages-file',  'gecko/b2g/locales/all-locales',  '--config',  'balrog/production.py',  '--variant',  'userdebug']
with the new argument at the end.

I want to do some builds to make sure this works, and to see if the package size changes (which might impact free space and therefore updates).
Technically we can't distribute the pvtbuilds images.
We can only distribute the gecko/gaia parts, I thought?

pvtbuilds aren't distributed on the open web?  Maybe I'm mistaken...
Can we change <$project_dir>/gecko/b2g/config/flame-kk/config.json and change :  "VARIANT": "user" 
to "userdebug"  ?

Would that work?  or is there something wrong with doing it that way?
(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #14)
> Technically we can't distribute the pvtbuilds images.
> We can only distribute the gecko/gaia parts, I thought?
> 
> pvtbuilds aren't distributed on the open web?  Maybe I'm mistaken...

I mean browsing the web, per bug 1121577 comment #2 and 3.

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #15)
> Can we change <$project_dir>/gecko/b2g/config/flame-kk/config.json and
> change :  "VARIANT": "user" 
> to "userdebug"  ?
> 
> Would that work?  or is there something wrong with doing it that way?

There are several config files that get merged together, we'd have to check the order that is done to see if it works. I agree it's a little more transparent to modify that file.
(In reply to Nick Thomas [:nthomas] from comment #16)
> (In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from
> comment #14)
> > Technically we can't distribute the pvtbuilds images.
> > We can only distribute the gecko/gaia parts, I thought?
> > 
> > pvtbuilds aren't distributed on the open web?  Maybe I'm mistaken...
> 
> I mean browsing the web, per bug 1121577 comment #2 and 3.

Ah. Marionette being up and running by default.  We don't do that on the user builds as mentioned in the bugs.  I see why there's a concern for engineers though...  That bug would be something that our automation team should be aware of.  Thanks for making that concern noted.


> (In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from
> comment #15)
> > Can we change <$project_dir>/gecko/b2g/config/flame-kk/config.json and
> > change :  "VARIANT": "user" 
> > to "userdebug"  ?
> > 
> > Would that work?  or is there something wrong with doing it that way?
> 
> There are several config files that get merged together, we'd have to check
> the order that is done to see if it works. I agree it's a little more
> transparent to modify that file.

Just wondering, who is taking point to verify this?  ie did you want me to look into it via just making the change and seeing if it works?
Oh, Also to note, I don't think the userdebug version has marionette running by default.  I can double check that sometime this week.
Flags: needinfo?(nthomas)
(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #17)
> Ah. Marionette being up and running by default.  We don't do that on the
> user builds as mentioned in the bugs.  I see why there's a concern for
> engineers though...  That bug would be something that our automation team
> should be aware of.  Thanks for making that concern noted.

And for anyone that flashes flame from pvtbuilds once this lands.
 
(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #18)
> Oh, Also to note, I don't think the userdebug version has marionette running
> by default.  I can double check that sometime this week.

Please do.
Flags: needinfo?(nthomas)
> And for anyone that flashes flame from pvtbuilds once this lands.

true.
  
> (In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from
> comment #18)
> > Oh, Also to note, I don't think the userdebug version has marionette running
> > by default.  I can double check that sometime this week.
> 
> Please do.

userdebug does not seem to have the marionette process running nor is the setting for adb turned on by default.  Having said that I can adb shell into the device without the debugging usb turned on and can adb root into the phone.  By default userdebug does not give you root via adb shell.

Hrm, I think there's a security issue in that with the userdebug you can still adb root with the screenlock code on.  This may cause a problem.  We should probably ping the dev to see what they think.
Julien, what do you think about : https://bugzilla.mozilla.org/show_bug.cgi?id=1121045#c20 ?
Flags: needinfo?(felash)
Fabrice, do you have any concerns about using VARIANT=userdebug?
Flags: needinfo?(fabrice)
This is more a concern to our security team, let's see what Paul thinks.

> Ah. Marionette being up and running by default.
The issue here was that merely having marionette enabled got the bad pref on. It didn't need to be "running". This was changed in bug 1121577.
Flags: needinfo?(felash) → needinfo?(ptheriault)
(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #22)
> Fabrice, do you have any concerns about using VARIANT=userdebug?

If it's for internal usage I have no concerns.
Flags: needinfo?(fabrice)
Patch to switch to userdebug.
Attachment #8560193 - Flags: review?(nthomas)
Attachment #8560193 - Flags: review?(catlee)
(In reply to Julien Wajsberg [:julienw] from comment #23)
> This is more a concern to our security team, let's see what Paul thinks.
> 
> > Ah. Marionette being up and running by default.
> The issue here was that merely having marionette enabled got the bad pref
> on. It didn't need to be "running". This was changed in bug 1121577.

Whats the context here? What does "internal usage" mean? The pref [1] needs to be off though on any build that there is any chance that it will be used to browse the web. 

To explain, this preference allows web pages to completely compromise the host OS, so its safe only for things like automated testing builds where we control exactly what pages are viewed. If we are talking about the engineering builds that developers flash onto their devices for development and testing purposes, I would argue that this preference needs to be disabled. Even if the device itself has no confidential information you are basically introducing a compromised device onto whatever network you connect them too. And also consider the the very real possibility that developers enter their ldap credentials into their devices.


[1] security.turn_off_all_security_so_that_viruses_can_take_over_this_computer
Flags: needinfo?(ptheriault)
Of course, all of that said [1] says the pref isnt set unless the marionette is run. So I guess that we should create a bug to stop marionette server flipping this pref as advised in [2] _before_ we make the switch to userdebug (Does userdebug enable marrionette server, or does it still need to be enabled - either way it sounds fragile and we shouldn't expose our developers/testers to this risk I think)

Im making lots of assumptions here - its been a while since I've looked at marionette so let me know if im missing something here.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1121577#c36
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1121577#c33
Comment on attachment 8560193 [details] [diff] [review]
flame-kk_userdebug.patch

Was some testing done with this ?
Is there a way to run the patch locally to test?  I wasn't aware of any method.
I can't push to try unfortunately, and even then I'm not sure try allows for building on flame...  does it?
Comment on attachment 8560193 [details] [diff] [review]
flame-kk_userdebug.patch

I think this will also change eng builds.
Attachment #8560193 - Flags: review?(nthomas) → review-
Naoki, I'm going to assign this bug to you because I don't want to be a blocker, and have no skin in the game. Attachment 8556044 [details] [diff] will modify the user builds to userdebug as you request, assuming that all other issues are resolved. You could ask for review from catlee, rail, or jlund.
Assignee: nthomas → nhirata.bugzilla
triage: blocking per comment 5 and comment 8
blocking-b2g: 2.2? → 2.2+
Attachment #8560193 - Flags: review?(rail)
Attachment #8560193 - Flags: review?(jlund)
Comment on attachment 8560193 [details] [diff] [review]
flame-kk_userdebug.patch

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

Sorry, I'm not familiar with that config, esp the variable.
Attachment #8560193 - Flags: review?(rail)
Comment on attachment 8560193 [details] [diff] [review]
flame-kk_userdebug.patch

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

I think this patch was r- in https://bugzilla.mozilla.org/show_bug.cgi?id=1121045#c30 but I like rail, am fairly unfamiliar with the consequences of this.

I don't think we can test this on try as try doesn't know about flame-kk builders. I took a look and it seems we run flame-kk builds on m-c and on cedar[1].

Cedar is a project branch used for testing. So we could push a patch to cedar, and see the effect against flame-kk. I can assist on doing such a test if needed. Otherwise, let's see what catlee has to say.

[1] https://wiki.mozilla.org/ReleaseEngineering/SpecialBranches#Cedar
Attachment #8560193 - Flags: review?(jlund) → review-
jlund, hrm.  that's a good point.  If I patch to cedar, can we give it a whirl there?
Flags: needinfo?(jlund)
Comment on attachment 8560193 [details] [diff] [review]
flame-kk_userdebug.patch

Pushed to cedar at Naoki's request - https://hg.mozilla.org/projects/cedar/rev/3f228d2096e5
It appears that there are some more differences between userdebug and user with the FTP and the bluetooth drivers, etc.

This would mean we would still have to test the user build to lessen the likelihood of an uncaught bug due to the difference between the variants.

Changing the gonk layer may also affect vendors, so it might not be the best course of action.  I'm at a loss here.  mwu, do you have any suggestions as we want to avoid making a franken build (ie flash eng. boot image over the production version ) as much as possible.
Flags: needinfo?(mwu)
see https://etherpad.mozilla.org/Smoketest-Root-Build-Issues

I am investigating the bluetooth portion to make sure that the gonk blob is up to date.
Flags: needinfo?(jlund)
I tested cedar with today's build and didn't see the issue.  This is after building my own userdebug version on both MC and cedar and finding that they both work.

Assets for FTU was the other thing that people noticed was different in the previous build.  That seems to be there.

I'll ask for a retest on today's cedar build.

Note: we made the change to userdebug on nexus-5-l and no one seemed to have said anything about that.
There was a patch in bug 1121577, where it doesn't turn on unless Marionette runs and it's already pushed on central.  Pinged julienw to see how to verify.
IMO the fix is not complete yet:
* it sets the pref as soon as marionette is run, and then never removes it.
* it doesn't handle the upgrade use case (which is admittedly less important for this bug)

Jonathan Griffin is still working on the first point but I think he's also busy on other matters.
Understandably it's not completely turned off.  Having said that, what's the risk of community members turning on marionette?  I think we can turn it off via changing the user pref right?  Perhaps, maybe if the switch for ADB+Devtools is turned off, we can have some code to change the userpref to turn that setting off?

This is blocking QA from having a build that isn't a franken build using the boot.img from an engineering build and we need to resolve this asap.
Flags: needinfo?(felash)
> Having said that, what's the risk of community members turning on marionette? 

Well, virtually all gaia developers run marionette at one point...
Flags: needinfo?(felash)
You also have to have an engineering build or userdebug build in order to run Marionette.  Since Dev also use an engineering build, I don't think the reasoning you use should block user/userdebug builds.
Flags: needinfo?(felash)
We discussed on IRC.

Bottom line is: there is a security issue that users of this build are not necessarily aware. But the same security issue is already there for "eng" builds anyway.
Flags: needinfo?(felash)
Paul, understanding that there is still the underlying issue, would it be ok to enable userdebug instead of user for the pvtbuilds?  You would have to turn on marionette for the underlying bug to occur, which people would purposely have to do.

This is a QAblocker as we are currently running on a frankenbuild of engineering boot.img on top of the user build in order to gain access to modify/view information we need to do qa.
Flags: needinfo?(ptheriault)
Just for reference, the difference between eng, userdebug and user variant.
( I found a read me that said that same in the tree somewhere: )
http://www.kandroid.org/online-pdk/guide/build_system.html#androidBuildVariants

( from android: )
http://source.android.com/source/building-running.html#choose-a-target
Nick, the main thing blocking is the security bug, the security bug got a patch where you would have to turn on marionette ( aka shoot yourself in the foot) and these builds don't get shipped outside of mozilla; I think we should push the fix.

ni? bajaj as she mentioned this bug in the stand up meeting.
Flags: needinfo?(nthomas)
Flags: needinfo?(bbajaj)
I can't make a call on the security issue, sorry. Have you explored other avenues to get what you need from existing user builds ? What is that exactly ?
Flags: needinfo?(nthomas)
What my point in the security issue is that it has been set so that you have to shoot yourself in the foot to be affected by it.  ie run marionette on the userdebug build.  There's a patch already that's been pushed to only run the security issue when marionette runs.  Otherwise it's turned off.  QA has verified this portion.

Yes, Marionette can run on the userdebug provided that a fresh gaia be installed over top and the tweak of running adb root on top.  You have to jump through hoops in order to do this; and not only that pvtbuilds are only available to internal mozilla employees.

We already are running a work around with the engineering boot.img on top; having said that the configuration for the engineering build versus the user builds may be off so we are running into issues with having a franken build.  Not only which, it posses even more of a threat to have this as you don't have to jump through as many hoops in order to run a Marionette.  ie you don't have to use adb root to get Marionette working.

We have no other options as we need the certain flags on the device turned on. see comment 48.

Also to note, we already have done this on tarako and nexus 5 L.  So I'm not sure exactly what the hang up here is considering 
1) there is forward movement on security bug in separate bugs.
2) we have already done this in the past
3) the userdebug is more secure than the engineering boot images that we have to use in order to work with the device that the public will end up getting anyhow if we dont' have this option because you can't adb push any gecko/gaia builds on top otherwise
4) pvtbuilds are private builds anyhow.  
5) the security risk of the engineering boot image is already being served by T2M because we didn't have this bug pushed.

I will take the forefront blame for this patch in if that's what your concern is about.
Flags: needinfo?(nthomas)
I have no concern's going forward on this mainly because of all the reasons naoki answered in comment #51. I'll email Paul to help clear the NI here so we are clear from all sides.
Flags: needinfo?(bbajaj)
Comment on attachment 8560193 [details] [diff] [review]
flame-kk_userdebug.patch

I'll reverse my review on this patch, since it clearly works on cedar. It seems that the config file we use for both eng builds (mozharness/configs/b2g/releng-otoro-eng.py) takes precendence over the in-tree b2g/config/flame-kk/config.json.

FTR, I still think it's a bad idea to leave a security problem present, even if it only manifests when Marionette is started. It's the hidden nature of it; AFAIK there is no warning given when Marionette is first started. But I'm going to defer to ptheriault on the final call here.
Flags: needinfo?(nthomas)
Attachment #8560193 - Flags: review?(catlee)
Attachment #8560193 - Flags: review-
Attachment #8560193 - Flags: review+
Attachment #8556044 - Attachment is obsolete: true
Understood.  Thanks for the reversal.

FWIW, We're not leaving it alone; there is forward progress on it in other bugs; would it be better to 
1) have a ui dialog for it when setting the tool and launching Marionette?
2) turning the pref off when adb&devtools is deselected?

We can probably have bugs filed to do those if you think that might help the situation.

I guess we need Paul's input before moving forward as the final approval.
Flags: needinfo?(nthomas)
I'm not close enough to b2g to have any opinion there.
Flags: needinfo?(nthomas)
Just make sure I understand clearly here:

- The pref security.turn_off_security... will be false by default in all builds 
- If the user runs marionette, it changes this pref to true  (or do you need to set it manually?)
- Currently there is no automatic setting of the pref back to false after marionette finishes running, but this is planned (bug 1121577)

Is that correct? If yes, then that sounds OK to me, so long as we warn consumers of pvt builds perhaps (email b2g-internal I mean, this would cover all users wouldnt it?) and that we make sure the follow-up fix is applied asap.

Nick has a point in comment 53 though - is there a reason why marionette flips the preference? What if we just left for the user to change as part of a step to enable marionette so that they at least would be aware of it? At least then the user knows about it and can set it back? Just a thought - I could probably live with either way, assuming we are confident that an email to b2g-internal will at least warn all pvt users not to user these builds on the web if they use marionette.
Flags: needinfo?(ptheriault)
(In reply to Paul Theriault [:pauljt] from comment #56)
> Just make sure I understand clearly here:
> 
> - The pref security.turn_off_security... will be false by default in all
> builds 
> - If the user runs marionette, it changes this pref to true  (or do you need
> to set it manually?)
> - Currently there is no automatic setting of the pref back to false after
> marionette finishes running, but this is planned (bug 1121577)

You got it right.

> 
> Is that correct? If yes, then that sounds OK to me, so long as we warn
> consumers of pvt builds perhaps (email b2g-internal I mean, this would cover
> all users wouldnt it?) and that we make sure the follow-up fix is applied
> asap.
> 
> Nick has a point in comment 53 though - is there a reason why marionette
> flips the preference?

That's a question for jgriffin.

> What if we just left for the user to change as part of
> a step to enable marionette so that they at least would be aware of it? At
> least then the user knows about it and can set it back? Just a thought - I
> could probably live with either way, assuming we are confident that an email
> to b2g-internal will at least warn all pvt users not to user these builds on
> the web if they use marionette.
Marionette flips the pref because that pref is required for SpecialPowers, and SP has become pretty entangled with Marionette.  B2G WebAPI tests use it, for example.

I'm going to untangle the two, although that's going to be complicated and won't happen quickly.

As far as the suggestion of making it required that the user change the pref, that would be fine as long as that mechanism could also be employed via some automated step; otherwise the Flame automation that the B2G team has set up would cease functioning.
It could be that this security pref is going to be removed soon, see bug 984012.
jgriffin, do you have a bug in regards to that work?  This way QA can track it along with bug 984012.

Thanks, Paul.  Sounds like I have the ok to land.  :)
(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #60)
> jgriffin, do you have a bug in regards to that work?  This way QA can track
> it along with bug 984012.
> 
Just filed bug 1149618.
Thanks, jgriffin!

Landed patch on b2g-inbound : http://hg.mozilla.org/integration/b2g-inbound/rev/f60b580a06db

First patch!
https://hg.mozilla.org/mozilla-central/rev/f60b580a06db
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Verified Fixed on Flame 3.0
Builds now have root functionality.

Environmental Variables:
Device: Flame 3.0
Build ID: 20150403010203
Gaia: 7969b367a7da62877c3a24a26d3cb5fda89d766c
Gecko: 70a113676b21
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0
Status: RESOLVED → VERIFIED
status-b2g-master: affected → verified
Flags: needinfo?(pbylenga)
Flags: needinfo?(pbylenga)
Clearing mwu as this is resolved.
Flags: needinfo?(mwu)
I came to realize, I can't ask for 2.2? for the patch since it's in releng not firefox os in bugzilla.
Asking bajaj in comment here if I can land though because apparantly I need a= in my comment for the push.
Flags: needinfo?(bbajaj)
Since this is verified on 3.0, a=bajaj to uplift on b2g37. Naoki confirmed the patch applies cleanly.
Flags: needinfo?(bbajaj)
Adding qawanted to verify this for 2.2 when available.
Keywords: qawanted, verifyme
Keywords: qaurgent
Verified Fixed on Flame 2.2
Builds now have root functionality.

Environmental Variables:
Device: Flame 2.2 (319MB)(Full Flash)
Build ID: 20150408002503
Gaia: ea735c21bfb0d78333213ff0376fce1eac89ead6
Gecko: 43041c78052b
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 37.0 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
status-b2g-v2.2: fixed → verified
Flags: needinfo?(ktucker)
Keywords: qaurgent, qawanted, verifyme
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: