Closed Bug 1121577 Opened 5 years ago Closed 3 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(blocking-b2g:-, firefox37 wontfix, firefox38 wontfix, firefox39 wontfix, firefox40 wontfix, firefox41 wontfix, firefox42 wontfix, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 wontfix, b2g-v2.1S wontfix, b2g-v2.2 wontfix, b2g-v2.2r wontfix, b2g-master wontfix)

RESOLVED WONTFIX
blocking-b2g -
Tracking Status
firefox37 --- wontfix
firefox38 --- wontfix
firefox39 --- wontfix
firefox40 --- wontfix
firefox41 --- wontfix
firefox42 --- wontfix
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- wontfix
b2g-v2.1S --- wontfix
b2g-v2.2 --- wontfix
b2g-v2.2r --- wontfix
b2g-master --- wontfix

People

(Reporter: julienw, Unassigned)

References

Details

(Keywords: sec-moderate, Whiteboard: "eng" and "userdebug" builds are unsafe to use)

Attachments

(2 files)

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".
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
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.
Keywords: sec-critical
Whiteboard: "eng" and "userdebug" builds are unsafe to use
This pref should only be set in automation. If we're making builds that people are actually using, that's a huge problem.
Flags: needinfo?(jgriffin)
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?
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?
Flags: needinfo?(jgriffin)
(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.
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.
/r/2637 - Bug 1121577 - Only toggle security pref during a session, r=AutomatedTester

Pull down this commit:

hg pull review -r 5f3fff34e8ac19fbe5cb75e782202d01ed2ff0dd
Assignee: nobody → jgriffin
Retriggered try jobs because of infra issues when the tests ran, will check back later
looks like something must still be trying to get that pref and causing an assertion failure.
blocking-b2g: 2.1? → 2.1+
(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 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
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 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
(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 on attachment 8550598 [details]
MozReview Request: bz://1121577/jgriffin

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

Ship It!
Attachment #8550598 - Flags: review?(dburns) → review+
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?
(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.
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?
Flags: needinfo?(lissyx+mozillians)
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.
Flags: needinfo?(lissyx+mozillians)
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?
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?
Flags: needinfo?(gsvelto)
Flags: needinfo?(felash)
Ben and Asa, you may want to be aware of this, too.
Flags: needinfo?(bhearsum)
Flags: needinfo?(asa)
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?
Flags: needinfo?(gsvelto)
(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?
Flags: needinfo?(felash)
Flags: needinfo?(bhearsum)
(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.
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.
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?
Flags: needinfo?(ptheriault)
Summary: The pref security.turn_off_all_security_so_that_viruses_can_take_over_this_computer is enabled in "eng" builds → The pref security.turn_off_all_security_so_that_viruses_can_take_over_this_computer is enabled in "eng" and "userbuild" builds
On reflection, this is in some publicly used builds so this really should be private even if its late to be private.
Group: b2g-core-security
Flags: needinfo?(ptheriault)
Summary: The pref security.turn_off_all_security_so_that_viruses_can_take_over_this_computer is enabled in "eng" and "userbuild" builds → The pref security.turn_off_all_security_so_that_viruses_can_take_over_this_computer is enabled in "eng" and "userdebug" builds
Group: core-security
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.
(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.
Flags: needinfo?(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 + '"';
  }
};
Flags: needinfo?(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.
*sigh*  building b2gdesktop so I can investigate
Flags: needinfo?(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.
Jonathan, can you please give an outline of what your patch does here?

Is it reseting the pref after the marionette session ends?

Thanks!
(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.
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.
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.
Keywords: sec-criticalsec-high
(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.
(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.
Flags: needinfo?(continuation)
(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?
Not yet, I initially thought we could do all this in one patch, but we already started landing a first part here.
(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.
Flags: needinfo?(continuation)
Keywords: sec-highsec-critical
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?
Flags: needinfo?(jgriffin)
I'd lean towards "hundreds".
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.
Flags: needinfo?(jgriffin)
Jonathan, based on comment 51, I was wondering if there had been any progress here.
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?
Have CrashIfNotInAutomation dump a JS stack?
(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 :)
(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.  :)
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.
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.
Attachment #8580902 - Flags: review?(dburns)
Attachment #8580902 - Flags: review?(dburns) → review+
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.
Oops you're right.
https://hg.mozilla.org/mozilla-central/rev/979433554c81
Status: NEW → ASSIGNED
Target Milestone: 2.2 S7 (6mar) → 2.2 S9 (3apr)
(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.
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?
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.
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?
Flags: needinfo?(dburns)
+1 on removing special powers if it is not required. It would allow us to simplify executeScript code
Flags: needinfo?(dburns)
Flags: needinfo?(asa)
Flags: needinfo?
Flags: needinfo?
(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.
Tracking the work to remove SpecialPowers from Marionette in bug 1149618.
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?
Flags: needinfo?(ptheriault)
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.
Flags: needinfo?(ptheriault)
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.
(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?
(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.
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.
Flags: needinfo?(bobbyholley)
(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.
(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" ;)
(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.
Flags: needinfo?(bobbyholley)
(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.
(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.
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.
Assignee: jgriffin → nobody
Status: ASSIGNED → NEW
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?
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.
It's pretty unclear what needs to be done here still. Can somebody please clarify?
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.
[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.
blocking-b2g: 2.1+ → 2.5?
Just thought I'd mention that the z3c (aries) phones are all running userdebug builds.
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.
Group: core-security
(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.
blocking-b2g: 2.5? → -
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
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.
Flags: needinfo?(dveditz)
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.
Group: b2g-core-security
Flags: needinfo?(dveditz)
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.