1121577 - The pref security.turn_off_all_security_so_that_viruses_can_take_over_this_computer is enabled in "eng" and "userdebug" builds

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect)

Description

[Julien Wajsberg [:julienw]]

I noticed that the pref "security.turn_off_all_security_so_that_viruses_can_take_over_this_computer" is enabled in "eng" builds for Flame that are on pvtbuilds.

It's not enabled in "user" builds though. I couldn't look at "userdebug" builds for Flame but it seems enabled on the Open C builds I got from http://builds.firefoxos.mozfr.org/, that I was told are "userdebug".

Comment 1

[Julien Wajsberg [:julienw]]

One lead is that marionette-server.js sets this pref in [1], and marionette is enabled on eng builds.

It's acceptable when we launch a specific Firefox build to use with marionette, but not having it all the time on a device. Instead, I think our testing scripts should restart B2G with this pref to use marionette (I think that they restart B2G anyway), and reset it to false afterwards.

[1] http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-server.js#29

Comment 2

[Daniel Veditz [:dveditz]]

This pref does what it says, and if any developer runs these builds on the open net they could be easily hacked. Thankfully this isn't in user builds, but who knows what an attacker could get by targeting insiders who run eng builds.

Comment 3

[Bobby Holley (:bholley)]

This pref should only be set in automation. If we're making builds that people are actually using, that's a huge problem.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Jonathan, can we just enable the pref dynamically when we're actually using marionette? Or can we get rid of marionette's dependency on it entirely?

Comment 5

[Jonathan Griffin (:jgriffin)]

This was enabled in bug 1038844 by bholley; it looks like we need it in order to use specialpowers.  One fix might be to only toggle the pref when a test requests specialpowers access, and then disable it when the session is closed.  Would that be enough?

Comment 6

[Bobby Holley (:bholley)]

(In reply to Jonathan Griffin (:jgriffin) from comment #5)
> This was enabled in bug 1038844 by bholley; it looks like we need it in
> order to use specialpowers.  One fix might be to only toggle the pref when a
> test requests specialpowers access, and then disable it when the session is
> closed.  Would that be enough?

Yes, I think so.

Comment 7

[Julien Wajsberg [:julienw]]

v2.0 is gecko 32, so because bug 1038844 landed in gecko 33 I think it's not affected.
v2.1 and v2.2 definitely are, I checked it myself.

Comment 8

[Jonathan Griffin (:jgriffin)]

Attachment: MozReview Request: bz://1121577/jgriffin

Comment 9

[Jonathan Griffin (:jgriffin)]

/r/2637 - Bug 1121577 - Only toggle security pref during a session, r=AutomatedTester

Pull down this commit:

hg pull review -r 5f3fff34e8ac19fbe5cb75e782202d01ed2ff0dd

Comment 10

[Jonathan Griffin (:jgriffin)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1c7ecd1b933

Comment 11

[David Burns :automatedtester]

Retriggered try jobs because of infra issues when the tests ran, will check back later

Comment 12

[David Burns :automatedtester]

looks like something must still be trying to get that pref and causing an assertion failure.

Comment 13

[Jonathan Griffin (:jgriffin)]

(In reply to David Burns :automatedtester from comment #12)
> looks like something must still be trying to get that pref and causing an
> assertion failure.

That "something" is probably SpecialPowers itself; let met change where we're unsetting the pref and try again.

Comment 14

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8550598 [details]
MozReview Request: bz://1121577/jgriffin

/r/2637 - Bug 1121577 - Only toggle security pref during a session, r=AutomatedTester

Pull down this commit:

hg pull review -r ec2cfc892129ff047541a33aab76db8b6e0d3a16

Comment 15

[Jonathan Griffin (:jgriffin)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=70e4daf78b2b

Comment 16

[Jonathan Griffin (:jgriffin)]

same problem.  This patch doesn't turn off the pref, but only turns it on when someone starts Marionette.  Thus the security hole would only exist for people who have run a Marionette test on their phone and not rebooted it afterwards:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=27e1f0e417db

Comment 17

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8550598 [details]
MozReview Request: bz://1121577/jgriffin

/r/2637 - Bug 1121577 - Only toggle security pref during a session, r=AutomatedTester

Pull down this commit:

hg pull review -r 7abb73deb86adda1c631fbc03339c3fc485060dd

Comment 18

[Jonathan Griffin (:jgriffin)]

(In reply to Jonathan Griffin (:jgriffin) from comment #16)
> same problem.  This patch doesn't turn off the pref, but only turns it on
> when someone starts Marionette.  Thus the security hole would only exist for
> people who have run a Marionette test on their phone and not rebooted it
> afterwards:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=27e1f0e417db

This is green.  We should land it regardless of whether we decide to try and figure out how to unset the pref after a session is finished.

Comment 19

[David Burns :automatedtester]

https://reviewboard.mozilla.org/r/2637/#review2059

Ship It!

Comment 20

[Jonathan Griffin (:jgriffin)]

One more try run, for good measure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e702fb2812db

Comment 21

[David Burns :automatedtester]

Comment on attachment 8550598 [details]
MozReview Request: bz://1121577/jgriffin

https://reviewboard.mozilla.org/r/2635/#review2113

Ship It!

Comment 22

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b166a3cda8f6

Comment 23

[Jonathan Griffin (:jgriffin)]

Now the question is, is this enough?  The hole is opened only when you actually run a Marionette test, but then remains open until B2G is restarted.  Is this OK, or do we want to do additional work to try and close the hole after the test has finished?

Comment 24

[Bobby Holley (:bholley)]

(In reply to Jonathan Griffin (:jgriffin) from comment #23)
> Now the question is, is this enough?  The hole is opened only when you
> actually run a Marionette test, but then remains open until B2G is
> restarted.  Is this OK, or do we want to do additional work to try and close
> the hole after the test has finished?

I think we should do that, yes.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G cpptest startup crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b29617a738f

https://treeherder.mozilla.org/logviewer.html#?job_id=5796167&repo=mozilla-inbound

Comment 26

[Ryan VanderMeulen [:RyanVM]]

This looks like yours too:
https://treeherder.mozilla.org/logviewer.html#?job_id=5797465&repo=mozilla-inbound

Comment 27

[Julien Wajsberg [:julienw]]

So, there are phones outthere that have this pref (the builds at http://builds.firefoxos.mozfr.org/ are using the "userbuild" variang); updates don't remove the prefs, I guess, so how can we remove them in an update? Alexandre, any possibility in the update script?

Comment 28

[:gerard-majax]

It depends, where and how is it set when we build userdebug builds ? I'm even wondering if it should be done in userdebug builds.

Comment 29

[Julien Wajsberg [:julienw]]

Alex, as said earlier, it's being set when marionette starts up. With the patch here we don't do this anymore but for the builds where the pref was set in previous builds, how can we remove the pref?

Comment 30

[:gerard-majax]

Julien thanks, I missed this point. But sadly, the issue is not only for builds from french community, but I think all dogfooding flame builds updates delivered by aus4.mozilla.org are userdebug, hence they are also concerned.

I looked in my profile's backups, and this started between mid june and end of july. So a lot of people dogfooding Flame are also concerned. The pref is turned to true in user's prefs.js. I don't see a nice ending to this.

For users that receives FOTA, we can hack in Edify, maybe simply running a shell script would do the trick. But what about users doing an OTA?

Comment 31

[:gerard-majax]

Ben and Asa, you may want to be aware of this, too.

Comment 32

[Gabriele Svelto [:gsvelto]]

I'm not aware of a way to run a script or do something similar during OTA. Since this pref must be explicitly set only in a certain scenario why don't we just modify Gecko to reset it every time at startup?

Comment 33

[Julien Wajsberg [:julienw]]

(In reply to Gabriele Svelto [:gsvelto] from comment #32)
> I'm not aware of a way to run a script or do something similar during OTA.
> Since this pref must be explicitly set only in a certain scenario why don't
> we just modify Gecko to reset it every time at startup?

Related, I have a feeling that the current fix is too "magic"; in my opinion we should never change this pref in the marionette server, and only change it from the marionette client. The client would stop gecko, change the pref, start gecko, run scenarios, stop gecko, reset the pref, restart gecko. But with this behavior we could not reset the pref at Gecko startup obviously.

Stopping/starting Gecko is what the marionette-js framework is doing for some time, I think marionette-python is doing the same, and I think the same could be easily done for the Firefox harness too because we run a new Firefox everytime.

Would this break compatibility with any 3rd-party tool?

Comment 34

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to Alexandre LISSY :gerard-majax from comment #30)
> Julien thanks, I missed this point. But sadly, the issue is not only for
> builds from french community, but I think all dogfooding flame builds
> updates delivered by aus4.mozilla.org are userdebug, hence they are also
> concerned.
> 
> I looked in my profile's backups, and this started between mid june and end
> of july. So a lot of people dogfooding Flame are also concerned. The pref is
> turned to true in user's prefs.js. I don't see a nice ending to this.

That's odd, because bug 1121045 is asking us to switch to userdebug for some flame builds.

Comment 35

[Nick Thomas [:nthomas] (UTC+12)]

Any objections to moving ahead with bug 1121045 ? Anyone who flashes a flame 3.0 or 2.2 build from pvtbuilds will get a userdebug build after that is done, instead of user.

Comment 36

[Julien Wajsberg [:julienw]]

Nick, with the patch that landed already, this pref is not set anymore unless someone runs marionette scenarios.

What is left to address here is:
* disable the pref after a marionette scenario has been run
* make an update remove the pref if it exists for existing eng/userbuild users

Hey Paul, just to let you know of this important issue, maybe you can help make it move forward and keep it active?

Comment 37

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

On reflection, this is in some publicly used builds so this really should be private even if its late to be private.

Comment 38

[Jonathan Griffin (:jgriffin)]

I have a patch which fixes the problem that caused the original backout, see https://treeherder.mozilla.org/#/jobs?repo=try&revision=593e5f75bc2c&exclusion_state=all. 

But unfortunately, it breaks Marionette on B2G emulators, which generate the following mysterious error on newSession:

{u'message': u'"toString" is read-only', u'error': u"error occurred while processing 'newSession"}

Am building an emulator so I can try to see what's going on.

Comment 39

[:gerard-majax]

(In reply to Jonathan Griffin (:jgriffin) from comment #38)
> I have a patch which fixes the problem that caused the original backout, see
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=593e5f75bc2c&exclusion_state=all. 
> 
> But unfortunately, it breaks Marionette on B2G emulators, which generate the
> following mysterious error on newSession:
> 
> {u'message': u'"toString" is read-only', u'error': u"error occurred while
> processing 'newSession"}
> 
> Am building an emulator so I can try to see what's going on.

Are you sure that this error is because of your patch ? It's funny, because I started to see something similar in bug 1130723.

Comment 40

[Jonathan Griffin (:jgriffin)]

Ok, I have a patch working locally, will run it on try.  The culprit was https://dxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/SpecialPowersObserverAPI.js#24 

SpecialPowersException.prototype.toString = function() {
  return this.name + ': "' + this.message + '"';
};

I have no idea why this started generating this message when I simply changed when specialpowers was loaded.  The fix was to morph this to:

SpecialPowersException.prototype = {
  toString: function SPE_toString() {
    return this.name + ': "' + this.message + '"';
  }
};

Comment 41

[Jonathan Griffin (:jgriffin)]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6da4bb63dc29

Comment 42

[Jonathan Griffin (:jgriffin)]

(In reply to Jonathan Griffin (:jgriffin) from comment #41)
> try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6da4bb63dc29

This looks good; will land the patch after the tree re-opens.

Comment 43

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/91c1bd885ad9

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for Gip failures on B2G OSX Desktop in https://hg.mozilla.org/integration/mozilla-inbound/rev/18ae6f259c06

https://treeherder.mozilla.org/logviewer.html#?job_id=6477291&repo=mozilla-inbound

Comment 45

[Jonathan Griffin (:jgriffin)]

*sigh*  building b2gdesktop so I can investigate

Comment 46

[Jonathan Griffin (:jgriffin)]

Here's a new green, try run:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=825d2f301617&exclusion_state=all

Turns out that the problem was that we were missing a chrome event by loading SpecialPowers later, and an observer waiting for that event was responsible for registering message listeners.

Will land as soon as m-i re-opens.

Comment 47

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/584bbe0e1cd8

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/584bbe0e1cd8

Comment 49

[Julien Wajsberg [:julienw]]

Jonathan, can you please give an outline of what your patch does here?

Is it reseting the pref after the marionette session ends?

Thanks!

Comment 50

[Jonathan Griffin (:jgriffin)]

(In reply to Julien Wajsberg [:julienw] from comment #49)
> Jonathan, can you please give an outline of what your patch does here?
> 
> Is it reseting the pref after the marionette session ends?
> 
> Thanks!

No, this patch only delays enabling the pref until a Marionette session is actually started, so the pref is never set if you never run a Marionette test.

Unsetting the pref after a test currently breaks a bunch of stuff, probably because pieces of SpecialPowers are still loaded.  I need to figure out how to safely unload all of that before I can unset the pref.  Will work on that next.

Comment 51

[Jonathan Griffin (:jgriffin)]

So cleaning this up is pretty tricky.  What happens is that after turning the pref off, we crash when the browser loses focus.  This happens 100% of the time on b2gdesktop builds, and rarely on desktop Firefox builds.

My guess is SpecialPowers is setting up some listener that gets fired when this happens, but I haven't been able to track it down so far.

cc'ing Ted in case he has any ideas what's going on here.

I'll be traveling this week so will unlikely have time to dig into this more until next.

Comment 52

[Andrew McCreight [:mccr8]]

I'm reducing this to sec-high because it sounds like this only affects you once you run a marionette session, which will be less common.

Comment 53

[Jonathan Griffin (:jgriffin)]

(In reply to Andrew McCreight [:mccr8] from comment #52)
> I'm reducing this to sec-high because it sounds like this only affects you
> once you run a marionette session, which will be less common.

Yes, that's correct.

Comment 54

[Julien Wajsberg [:julienw]]

(In reply to Andrew McCreight [:mccr8] from comment #52)
> I'm reducing this to sec-high because it sounds like this only affects you
> once you run a marionette session, which will be less common.

But all builds that are already outthere still have the pref because we still didn't clean this up. I think this should still be sec-critical until we have the logic to clean the pref.

Comment 55

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Julien Wajsberg [:julienw] (PTO March 7th -> 15th) from comment #54)
> But all builds that are already outthere still have the pref because we
> still didn't clean this up. I think this should still be sec-critical until
> we have the logic to clean the pref.

I agree! Is there a followup for doing that?

Comment 56

[Julien Wajsberg [:julienw]]

Not yet, I initially thought we could do all this in one patch, but we already started landing a first part here.

Comment 57

[Andrew McCreight [:mccr8]]

(In reply to Julien Wajsberg [:julienw] (PTO March 7th -> 15th) from comment #54)
> But all builds that are already outthere still have the pref because we
> still didn't clean this up. I think this should still be sec-critical until
> we have the logic to clean the pref.

Sounds reasonable.

Comment 58

[Matt Wobensmith [:mwobensmith][:matt:]]

Hi Jonathan, any update?

Question for anyone: 
Do we have an idea of how many users out there are actually affected by this issue? Hundreds? Thousands?

Comment 59

[Julien Wajsberg [:julienw]]

I'd lean towards "hundreds".

Comment 60

[Jonathan Griffin (:jgriffin)]

I have no idea; I would guess that the number of people dogfooding with engineering builds _and_ running Marionette tests on those builds is quite small (smaller than "hundreds"), but I don't think there's any way to know for sure.

It's suboptimal to run tests on a real dogfooding phone because the tests often delete/add data in ways that make using it not fun.

Comment 61

[Matt Wobensmith [:mwobensmith][:matt:]]

Jonathan, based on comment 51, I was wondering if there had been any progress here.

Comment 62

[Jonathan Griffin (:jgriffin)]

The current status is show by this try run:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=55975ef15ac6

The patch there disables SpecialPowers and unsets the pref at the end of a Marionette session.  On desktop Firefox, this exposes some racy condition that seems to be a moving target; on b2gdesktop, it fails 100% of the time during harness initialization.

Is there an easy way to figure out what JS is causing us to hit  xpc::CrashIfNotInAutomation(), or am I pretty much stuck with dump() debugging?

Comment 63

[Boris Zbarsky [:bzbarsky]]

Have CrashIfNotInAutomation dump a JS stack?

Comment 64

[Julien Wajsberg [:julienw]]

(In reply to Jonathan Griffin (:jgriffin) from comment #60)
> I have no idea; I would guess that the number of people dogfooding with
> engineering builds

Remember "userbuild" builds are also affected.

> _and_ running Marionette tests on those builds is quite
> small (smaller than "hundreds"), but I don't think there's any way to know
> for sure.

That's only for new builds containing your patch. All older users already have the pref set, even if they never run marionette.

> 
> It's suboptimal to run tests on a real dogfooding phone because the tests
> often delete/add data in ways that make using it not fun.

For sure :)

Comment 65

[Jonathan Griffin (:jgriffin)]

(In reply to Not doing reviews right now from comment #63)
> Have CrashIfNotInAutomation dump a JS stack?

I did that here:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1b86b088f02

I get several different stack traces, so tracking this down should be fun.  :)

Comment 66

[Jonathan Griffin (:jgriffin)]

I have a patch to disable the pref, and a clean try run that's quite small:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=89fe2fc8cc18

I'll schedule a full try run and if the green persists, will put up a patch for review.

Comment 67

[Jonathan Griffin (:jgriffin)]

Attachment: Unset pref at delete_session,

try run looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=096462d3f23e&exclusion_profile=false

The failed builds are due to an old mozharness pointer in my push; not related to this change.

Comment 68

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/979433554c81

Comment 69

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8580902 [details] [diff] [review]
Unset pref at delete_session,

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

::: testing/marionette/marionette-server.js
@@ +2611,5 @@
> +
> +    let prefTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +    prefTimer.initWithCallback(function() {
> +      prefTimer.cancel();
> +      if (this.enabled_security_pref) {

Are we sure this is the right "this"?

I tried using nsITimer this way in xpcshell, and the callback was run with "this" bound to the callback function itself, which I'd expect would cause this condition to always be false.

Comment 70

[Jonathan Griffin (:jgriffin)]

Oops you're right.

Comment 71

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/979433554c81

Comment 72

[Jonathan Griffin (:jgriffin)]

(In reply to Jonathan Griffin (:jgriffin) from comment #70)
> Oops you're right.

Fixing this gives this try run:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e28389e6d8a

Overall it looks good, except for the Gip failures.  Those are all the same, with the stack trace:

deleting pref!
0 BrowserElementParent.prototype._createEvent(evtName = "loadprogresschanged", detail = [object Object], cancelable = false) ["file:///builds/slave/test/build/application/b2g/components/BrowserElementParent.js":48]
    this = [object Object]
1 BrowserElementParent.prototype._fireEventFromMsg(data = [object Object]) ["file:///builds/slave/test/build/application/b2g/components/BrowserElementParent.js":39]
    this = [object Object]
2 BrowserElementParent.prototype._setupMessageListener/<(aMsg = [object Object]) ["file:///builds/slave/test/build/application/b2g/components/BrowserElementParent.js":25]
    this = [object ChromeMessageSender]

This is a little confusing as it has nothing to do with Marionette.

Comment 73

[Julien Wajsberg [:julienw]]

I see a crash in the logs:

20:31:44    ERROR -  PROCESS-CRASH | runner.py | application crashed [@ xpc::CrashIfNotInAutomation()]

Is it possible that this function make the process crash if the pref is not set?

Comment 74

[Jonathan Griffin (:jgriffin)]

Yes, the crash is a side effect of not having the pref set; the stack trace in comment #72 is the JS that was being executed that caused us to hit the assertion that crashes.

Comment 75

[Jonathan Griffin (:jgriffin)]

I wonder if we should consider removing SpecialPowers from Marionette altogether?  The main reason it exists there, AFAICR, is to support the WebAPI tests, which operate in content space but want to tickle a few things in chrome space sometimes, ala https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/tests/marionette/head.js#88

We could possibly special case those somehow so that SpecialPowers isn't enabled at all in regular Marionette tests.  David, thoughts?

Comment 76

[David Burns :automatedtester]

+1 on removing special powers if it is not required. It would allow us to simplify executeScript code

Comment 77

[Jonathan Griffin (:jgriffin)]

(In reply to David Burns :automatedtester from comment #76)
> +1 on removing special powers if it is not required. It would allow us to
> simplify executeScript code

I did some investigation around this and it's possible and clearly the way to go.

Comment 78

[Jonathan Griffin (:jgriffin)]

Tracking the work to remove SpecialPowers from Marionette in bug 1149618.

Comment 79

[Matt Wobensmith [:mwobensmith][:matt:]]

It appears that there could be two issues here - what to do for currently affected users, and getting the fix out there for people to use in the future.

The latter seems to have some movement, but this is still a critical bug. If something merits a sec-critical rating, it should have immediate priority.

Seeing as this type of thing will happen more in the future - phones w/o latest updates - can this problem be addressed in a different way? Perhaps via developer evangelism? We are not talking about normal end users. We could reach out to Flame users via a public document to explain how to turn off this pref or otherwise fix their device so that they are not vulnerable.

In any case, in absence of a purely technical solution in the short term, we should do something to protect these people. 

Paul, your thoughts?

Comment 80

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

Matt, this bug has been partially mitigated by a change that pref is now OFF by default, and only enabled when marionette is run for the first time. It's currently left on after marionette runs, unless the user turns it off. 

So this is bug is now focused on that latter part I think. OR if i understand that later comments in https://bugzilla.mozilla.org/show_bug.cgi?id=1121045#c57 and comment 78 in this bug, it sounds like we are going to resolve this by removing special powers all together.

Given that the initial issue that is the title of this bug is resolved, I've downgraded the risk here.

Comment 81

[Julien Wajsberg [:julienw]]

TBH I'm especially concerned about users of the community build which is a "userbuild" variant build, and thus has this pref enabled. These users are using these builds on the wild for some time now and they're not aware of the issue at all.

Comment 82

[Bobby Holley (:bholley)]

(In reply to Julien Wajsberg [:julienw] from comment #81)
> TBH I'm especially concerned about users of the community build which is a
> "userbuild" variant build, and thus has this pref enabled. These users are
> using these builds on the wild for some time now and they're not aware of
> the issue at all.

It's only a problem for them if they run marionette tests, right?

Comment 83

[Julien Wajsberg [:julienw]]

(In reply to Bobby Holley (:bholley) from comment #82)
> (In reply to Julien Wajsberg [:julienw] from comment #81)
> > TBH I'm especially concerned about users of the community build which is a
> > "userbuild" variant build, and thus has this pref enabled. These users are
> > using these builds on the wild for some time now and they're not aware of
> > the issue at all.
> 
> It's only a problem for them if they run marionette tests, right?

No, because before patch in comment 48 the pref was set at boot time, even if the user never runs marionette tests.

Jonathan please correct if I'm wrong here.

Comment 84

[(not currently active) Ted Mielczarek]

bholley: what would you think about making this pref be checked early in startup and the value stored, and then having that code unset it so it's not saved for future runs? AFAICT it's only actually checked in two places so it shouldn't be hard to change that logic:
https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#2250
https://dxr.mozilla.org/mozilla-central/source/js/xpconnect/src/xpcprivate.h#3767

That way test harnesses could set it in the profile they're using to enable it for the current run, but it wouldn't persist for restarts beyond that. Additionally, users who ran a build that had the pref enabled would get it disabled when they updated to a build with this fix.

Comment 85

[Jonathan Griffin (:jgriffin)]

(In reply to Julien Wajsberg [:julienw] from comment #83)
> (In reply to Bobby Holley (:bholley) from comment #82)
> > (In reply to Julien Wajsberg [:julienw] from comment #81)
> > > TBH I'm especially concerned about users of the community build which is a
> > > "userbuild" variant build, and thus has this pref enabled. These users are
> > > using these builds on the wild for some time now and they're not aware of
> > > the issue at all.
> > 
> > It's only a problem for them if they run marionette tests, right?
> 
> No, because before patch in comment 48 the pref was set at boot time, even
> if the user never runs marionette tests.
> 
> Jonathan please correct if I'm wrong here.

You're not wrong.  The patches which have already landed fix this problem, but for users who are still using older builds and haven't updated, they will still be vulnerable whether or not they've ever run a Marionette test.  I'm not sure what the solution is here, except outreach.

And I assume you meant "userdebug" builds, since Marionette is not enabled on "user" builds.

Comment 86

[Julien Wajsberg [:julienw]]

(In reply to Jonathan Griffin (:jgriffin) from comment #85)

> And I assume you meant "userdebug" builds, since Marionette is not enabled
> on "user" builds.

Yes ! I keep saying "userbuild" instead of "userdebug" ;)

Comment 87

[Bobby Holley (:bholley)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #84)
> bholley: what would you think about making this pref be checked early in
> startup and the value stored, and then having that code unset it so it's not
> saved for future runs? AFAICT it's only actually checked in two places so it
> shouldn't be hard to change that logic:
> https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.
> cpp#2250
> https://dxr.mozilla.org/mozilla-central/source/js/xpconnect/src/xpcprivate.
> h#3767
> 
> That way test harnesses could set it in the profile they're using to enable
> it for the current run, but it wouldn't persist for restarts beyond that.
> Additionally, users who ran a build that had the pref enabled would get it
> disabled when they updated to a build with this fix.

Fine by me.

Comment 88

[Jonathan Griffin (:jgriffin)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #84)
> bholley: what would you think about making this pref be checked early in
> startup and the value stored, and then having that code unset it so it's not
> saved for future runs? AFAICT it's only actually checked in two places so it
> shouldn't be hard to change that logic:
> https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.
> cpp#2250
> https://dxr.mozilla.org/mozilla-central/source/js/xpconnect/src/xpcprivate.
> h#3767
> 
> That way test harnesses could set it in the profile they're using to enable
> it for the current run, but it wouldn't persist for restarts beyond that.
> Additionally, users who ran a build that had the pref enabled would get it
> disabled when they updated to a build with this fix.

Could we modify this so that pref is checked dynamically, but unset immediately if it is set so it doesn't persist?  If we check it only at boot, it means we'll have to restart B2G to run tests, and on the emulator on a test slave, this is an expensive process...it will add a few minutes to each test chunk.

Comment 89

[(not currently active) Ted Mielczarek]

(In reply to Jonathan Griffin (:jgriffin) from comment #88)
> Could we modify this so that pref is checked dynamically, but unset
> immediately if it is set so it doesn't persist?  If we check it only at
> boot, it means we'll have to restart B2G to run tests, and on the emulator
> on a test slave, this is an expensive process...it will add a few minutes to
> each test chunk.

That's a little trickier to get right, but I think we could do it. You'd have a pref observer that sets a global boolean and then unsets the pref, but you'd have to be careful for the observer to not retrigger itself.

Honestly writing that out it sounds like a pref is not what you want here, you'd rather just have an observer service topic or something like "enable-whatever", but that's almost worse because we'd have to make other precautions so that random addons don't enable it.

Comment 90

[Jonathan Griffin (:jgriffin)]

SpecialPowers has been removed from Marionette on trunk in bug 1149618.  As of now, running Marionette tests on an eng/userdebug build will not set this pref.

I'm unassigning myself, since all the engineering work is done, but there may be additional work needed in the sense of outreach, etc, for users with older builds.

Comment 91

[Julien Wajsberg [:julienw]]

Jonathan, do you think it's possible to add code to automatically remove the pref ? Or should we just try to reach the users and tell them to remove it?

Comment 92

[Jonathan Griffin (:jgriffin)]

Do you mean Ted's idea from comment #89?  Probably, although note that we're still dependent on this pref for running mochitests (and that isn't going to change), so we'd have to try to implement the trickier dynamic option.

Comment 93

[Ryan VanderMeulen [:RyanVM]]

It's pretty unclear what needs to be done here still. Can somebody please clarify?

Comment 94

[Julien Wajsberg [:julienw]]

Still: removing the pref from existing profile that use the pref -- likely the case in profiles used by community builds fpr Open C, but possibly others too.

Comment 95

[Ryan VanderMeulen [:RyanVM]]

[Blocking Requested - why for this release]: v2.1 is EOL and v2.2 is already on its way out. Re-nominated for v2.5 blocking status.

Comment 96

[Dave Hylands [:dhylands]]

Just thought I'd mention that the z3c (aries) phones are all running userdebug builds.

Comment 97

[Julien Wajsberg [:julienw]]

I think the build on aries phone doesn't have the issue to begin with. Current userdebug builds don't have the pref set. The remaining issue is "only" cleaning up old profiles.

Of course the issue will go away naturally eventually. But I know there are still users of Open C, and there are also users who backup/restore their profiles and thus retain their prefs too.

Comment 98

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

(In reply to Julien Wajsberg [:julienw] from comment #97)
> I think the build on aries phone doesn't have the issue to begin with.
> Current userdebug builds don't have the pref set. The remaining issue is
> "only" cleaning up old profiles.
> 
> Of course the issue will go away naturally eventually. But I know there are
> still users of Open C, and there are also users who backup/restore their
> profiles and thus retain their prefs too.

This issue does not affect 2.5, removing the blocking flag.

Comment 99

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Can we unrestrict this bug yet?  It's a good example of why “reduce security for testing” settings need to be handled carefully, and how several individually reasonable-seeming changes can combine to produce unpleasant surprises.  But I can't tell if there might still be affected users.  Comment #94 and comment #97 are a little worrying, but it's two years later and both FxOS and B2G are, to be blunt, dead.

Comment 100

[Julien Wajsberg [:julienw]]

I'm pretty sure some people are still using old Firefox OS builds on their phones -- albeit very few. I think they're aware that using these builds put them at risk, even without this.

Comment 101

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.