Re: [Maildev] Thunderbird Nightly broken recently (and fixed) - Resending, this time to Tom who is not on the list
(Resending with correct From line so it won't get blocked by Mailman.)
(Moving discussion to maildev from drivers as requested by Jörg.)
-
The sugar.js module in my add-on doesn't have the word "eval"
anywhere in it, so I don't know how to identify where eval is ostensibly
being called so that I can fix it. -
SugarJS is a third-party library, and a pretty complex one at that,
so I changing it to eliminate the use of eval is not something that can
necessarily be done "in short." It might be quite complicated to do, and
it's not my code, so it's something I'd really prefer not to dive into. -
Is Thunderbird going to turn crash-on-eval back on any time soon, or
isn't it?
jik
On 8/3/19 1:50 AM, Jörg Knobloch wrote:
On 03 Aug 2019 05:34, Jonathan Kamens wrote:
I'm really not sure what "in the System Principal context" means, so
even if I did need to do something about this I don't really have any
idea what to do.
Hi Jonathan, in short, you need to eliminate use of "eval()", see
https://bugzilla.mozilla.org/show_bug.cgi?id=1473549. Jörg.
P.S.: Let's move the discussion to MailDev.
On 03 Aug 2019 15:57, Jonathan Kamens wrote:
-
The sugar.js module in my add-on doesn't have the word "eval"
anywhere in it, so I don't know how to identify where eval is
ostensibly being called so that I can fix it. -
SugarJS is a third-party library, and a pretty complex one at that,
so I changing it to eliminate the use of eval is not something that
can necessarily be done "in short." It might be quite complicated to
do, and it's not my code, so it's something I'd really prefer not to
dive into.
Hi there,
this is a real problem.
I know of two add-ons using 3rd party JS packages (jQuery and Ace) which
used assignments to innerHTML in chrome code which was disabled on the
Mozilla platform somewhere between 60 and 68 (details on request). In
the case of Ace, I reported it and they changed their code
(https://github.com/ajaxorg/ace-builds/issues/147), I don't know what
happened to the other add-on and jQuery (but I could go digging on request).
Seems like SugarJS now has the same challenge. I have only followed the
de-eval bugs from afar (like I follow everything), but I think that "new
Function(arg1, ..., argN, "body")" has the same issue, see
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function
where a comparison with eval is made.
My recommendation would be to report it to SugarJS, hopefully they are
as good as Ace.
Jörg.
-
I have now asked twice for confirmation of whether it is intended
that Thunderbird will eventually flip the flag that prevents eval from
being invoked in add-ons, and the question still hasn't been answered.
Jörg, if you don't think you're the right person to answer that
question, could you at least acknowledge the question and perhaps
suggest who would be able to better answer it, so that I don't feel like
I'm being ignored? -
Just to confirm that I'm understanding you properly, are you saying
that even though what you emailed me about was the use of eval, in fact
what the problem with SugarJS is probably the Function constructor, not
eval, since the former just invokes the latter under the covers? -
The reason why SugarJS in my add-on is using Function is specifically
to get access to the global context so that SugarJS can extend native
classes. It's using the var global = Function('return this')();
technique described here
https://stackoverflow.com/questions/3277182/how-to-get-the-global-object-in-javascript/3277192#3277192.
This became necessary when this was changed from a BackstagePass to a
NonSyntacticVariablesObject. I understand that the whole point of that
change was to hide the global context from JavaScript code, and that
what SugarJS doing is arguably one of the things that disallowing eval
is intended to prevent. But kind of the whole point of SugarJS is
extending native classes, so I'm a bit stymied about what I could
suggest to the author of SugarJS to do instead of using Function.
In the current version of SugarJS, the author is using a different
technique
https://github.com/andrewplummer/Sugar/blob/master/lib/core.js#L50 for
getting access to the global context, but that doesn't appear to be
working either. It seems that neither global nor window is defined and
an Object when a JavaScript module is being loaded with
ChromeUtils.import. I'm really not sure how to proceed here.
Thanks,
jik
On 8/4/19 7:25 AM, Jörg Knobloch wrote:
On 03 Aug 2019 15:57, Jonathan Kamens wrote:
-
The sugar.js module in my add-on doesn't have the word "eval"
anywhere in it, so I don't know how to identify where eval is
ostensibly being called so that I can fix it. -
SugarJS is a third-party library, and a pretty complex one at
that, so I changing it to eliminate the use of eval is not something
that can necessarily be done "in short." It might be quite
complicated to do, and it's not my code, so it's something I'd really
prefer not to dive into.
Hi there,
this is a real problem.
I know of two add-ons using 3rd party JS packages (jQuery and Ace)
which used assignments to innerHTML in chrome code which was disabled
on the Mozilla platform somewhere between 60 and 68 (details on
request). In the case of Ace, I reported it and they changed their
code (https://github.com/ajaxorg/ace-builds/issues/147), I don't know
what happened to the other add-on and jQuery (but I could go digging
on request).
Seems like SugarJS now has the same challenge. I have only followed
the de-eval bugs from afar (like I follow everything), but I think
that "new Function(arg1, ..., argN, "body")" has the same issue, see
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function
where a comparison with eval is made.
My recommendation would be to report it to SugarJS, hopefully they are
as good as Ace.
Jörg.
Hi Jonathan, hi all,
I didn't want to ignore your question, I just wanted to avoid having to
dig through a lot of stuff to answer it. I had already given you the
link to M-C's meta bug
https://bugzilla.mozilla.org/show_bug.cgi?id=1473549. BTW, a few depends
there are about removing "new Function".
We first got in contact with this in
https://bugzilla.mozilla.org/show_bug.cgi?id=1522608 which came in as a
bustage. There we set pref("security.allow_eval_with_system_principal",
true); as immediate measure, but later removed lots of evals from our code.
As part of the work, we also modified M-C's whitelist in
security.uris_using_eval_with_system_principal, which we had to follow
up on in
https://bugzilla.mozilla.org/show_bug.cgi?id=1535096
https://bugzilla.mozilla.org/show_bug.cgi?id=1545788
There were more removals in
https://bugzilla.mozilla.org/show_bug.cgi?id=1552004 and finally in
Calendar in https://bugzilla.mozilla.org/show_bug.cgi?id=1558986, unless
I missed something. That bug also removed our manipulation of the whitelist:
https://hg.mozilla.org/comm-central/rev/d72b6ee0f40f#l3.12
That closed the chapter for us.
Looks like M-C has now finally removed that pref as well since I can't
find it any more. It's still in the beta:
https://searchfox.org/mozilla-beta/rev/384ef1e91e03429ff24ff29586193e65872f2815/modules/libpref/init/all.js#2518
Looks like this changeset is relevant, it took out the pref and put back
some other scheme:
https://hg.mozilla.org/mozilla-central/rev/63ef0618ec9a07c438701e0357ef0d37abea0dd8
- Bug 1567499
I'm not an expert here, but I think they will eventually put the pref
back and undo the other scheme, like here:
https://hg.mozilla.org/mozilla-central/rev/eb8976f0aa75 - Bug 1564527
With a pref, your add-on could add that library, couldn't it?
I really don't understand which pref Tom was referring to here in the
original tb-drivers post, included below (quote): I'm going to disable
this preference for Thunderbird, because I think this might have been
the tip of the iceberg as far as 'stuff that uses eval with the System
Principal'. (end quote). I'm not aware that TB code was touched in that
action. Tom, can you please elaborate? What did you disable where?
Coming to your question 2: Although I don't know who calls whom with
respect to eval and "new Function", they are both in the same category
and need to go.
I can't speak to question 3 since I'm (sadly) not a JS expert. However,
does this help?
https://searchfox.org/mozilla-central/search?q=getGlobalForObject&case=false®exp=false&path=
Jörg.
Tom's original message below:
Hey all,
In https://bugzilla.mozilla.org/show_bug.cgi?id=1564527 I shipped a
hardening measure to Nightly that would crash if eval() was used in
the System Principal context. We thought we had figured all those out;
but we had not.
We disabled it after observing crashes in
https://bugzilla.mozilla.org/show_bug.cgi?id=1567499 - that bug is
private because it points to (or did point to) crashes that had user
profile paths. We cleaned that data up.
We observed crashes from a few sources:
- Mozilla Test Pilot experiments
- userChromeJS which is a Firefox hack that kinda-enables old-style
legacy extensions and basically executes random user-provided JS in
the system principal context - And one specific type of Thunderbird crash reporting the source
file of resource://sendlater3/sugar.js
I'm going to disable this preference for Thunderbird, because I think
this might have been the tip of the iceberg as far as 'stuff that uses
eval with the System Principal'.
Tom Prince retriggered Thunderbird Nightlies with the fix last friday
a few hours after we determined the cause and fix; and they shipped,
but I wanted to give an after-action report of what happened and why
it won't happen again. (Firstly because I disabled the pref for
thunderbird, Secondly because I learned my lesson about weird stuff
users in the wild do, and Thirdly because I learned that Thunderbird
Nightly exists and how to avoid changing its behavior.)
-tom
PS: Thanks for continuing work and keeping Thunderbird alive.
On Sun, Aug 4, 2019 at 2:08 PM Jörg Knobloch jorgk@jorgk.com wrote:
Looks like M-C has now finally removed that pref as well since I can't
find it any more. It's still in the beta:
Pref is still in central; it just moved to StaticPrefList.yaml
Looks like this changeset is relevant, it took out the pref and put back
some other scheme:
https://hg.mozilla.org/mozilla-central/rev/63ef0618ec9a07c438701e0357ef0d37abea0dd8
- Bug 1567499
My inclusion and subsequent removal from all.js was in error - the
StaticPrefList.yaml definition of the pref is the correct one.
I really don't understand which pref Tom was referring to here in the
original tb-drivers post, included below (quote): I'm going to disable
this preference for Thunderbird, because I think this might have been
the tip of the iceberg as far as 'stuff that uses eval with the System
Principal'. (end quote). I'm not aware that TB code was touched in that
action. Tom, can you please elaborate? What did you disable where?
In https://bugzilla.mozilla.org/show_bug.cgi?id=1567623 I landed code
that turns the pref back on for firefox (but not thunderbird). However
instead of crashing or even preventing the eval() usage, it just sends
us Telemetry.
In https://bugzilla.mozilla.org/show_bug.cgi?id=1570681 I will (at
some point) have it block the usage of eval. This won't crash the
browser but could lead to unexpected behavior. Of course we don't want
that; we want to enumerate and either fix or allowlist all usage of
eval().
-tom
On 05 Aug 2019 07:38, Tom Ritter wrote:
Inhttps://bugzilla.mozilla.org/show_bug.cgi?id=1567623 I landed code
that turns the pref back on for firefox (but not thunderbird). However
instead of crashing or even preventing the eval() usage, it just sends
us Telemetry.
Inhttps://bugzilla.mozilla.org/show_bug.cgi?id=1570681 I will (at
some point) have it block the usage of eval. This won't crash the
browser but could lead to unexpected behavior. Of course we don't want
that; we want to enumerate and either fix or allowlist all usage of
eval().
Thanks for the answer, Tom, but sadly I'm still confused. Let me try to
summarise.
We're talking about two prefs here,
security.allow_eval_with_system_principal and the white/allowlist
security.uris_using_eval_with_system_principal.
Looking at the codebase at time of writing I see:
Disabled by default so it doesn't affect Thunderbird/SeaMonkey, but
enabled in firefox.js
- name: security.allow_eval_with_system_principal
type: bool
value: true
mirror: always
That's already confusing since the comment doesn't match the code. But
it is enabled, right?
https://searchfox.org/mozilla-central/rev/0ffa9e372df56c95547fed9c3433ddec4fbf6f11/browser/app/profile/firefox.js#505
#ifdef NIGHTLY_BUILD
// allow_eval_with_system_principal is enabled on Firefox Desktop only
at this
// point in time
pref("security.allow_eval_with_system_principal", false);
Unless I'm missing something, again, the comment doesn't match the code.
In any case it's disabled but remains enabled for TB.
The net effect is that on trunk 70, eval is allowed for TB but not FF.
Right so far? I can't see security.uris_using_eval_with_system_principal
in a search, so that's gone.
So the plan is to disallow the eval again with
security.allow_eval_with_system_principal and bring the white/allowlist
back?
For beta 69 the situation seems to be like this:
https://searchfox.org/mozilla-beta/rev/384ef1e91e03429ff24ff29586193e65872f2815/modules/libpref/init/all.js#2511
#if defined(DEBUG) && !defined(ANDROID)
pref("security.allow_eval_with_system_principal", false);
pref("security.uris_using_eval_with_system_principal",
"autocomplete.xml,redux.js,react-redux.js, ... and more.
TB doesn't modify these settings. For ESR 68 the situation is similar,
only that TB adds to the allow/whitelist.
And here's another confusing thing: All this is done in DEBUG mode only
which doesn't apply to the shipped installers. So I assume that on 69
and 68 ESR in opt/release builds, those evals still work as they always
have, right?
I think Jonathan is interested in the medium to long term future and
that appears to be to disallow eval and "new Function" in general, but
have a allow/whitelist where he could list the sugarJS he uses?
Jörg.
On 8/4/19 3:08 PM, Jörg Knobloch wrote:
I can't speak to question 3 since I'm (sadly) not a JS expert.
However, does this help?
https://searchfox.org/mozilla-central/search?q=getGlobalForObject&case=false®exp=false&path=
Yes! Thank you for the pointer. I was able to get getGlobalForObject to
work in this context, so for me at least the issue is now resolved.
jik
On Mon, Aug 5, 2019 at 2:22 AM Jörg Knobloch jorgk@jorgk.com wrote:
On 05 Aug 2019 07:38, Tom Ritter wrote:
Inhttps://bugzilla.mozilla.org/show_bug.cgi?id=1567623 I landed code
that turns the pref back on for firefox (but not thunderbird). However
instead of crashing or even preventing the eval() usage, it just sends
us Telemetry.
Inhttps://bugzilla.mozilla.org/show_bug.cgi?id=1570681 I will (at
some point) have it block the usage of eval. This won't crash the
browser but could lead to unexpected behavior. Of course we don't want
that; we want to enumerate and either fix or allowlist all usage of
eval().
Thanks for the answer, Tom, but sadly I'm still confused. Let me try to
summarise.
We're talking about two prefs here,
security.allow_eval_with_system_principal and the white/allowlist
security.uris_using_eval_with_system_principal.
OH! I had forgotten about the allowlist pref.
Looking at the codebase at time of writing I see:
Disabled by default so it doesn't affect Thunderbird/SeaMonkey, but
enabled in firefox.js
- name: security.allow_eval_with_system_principal
type: bool
value: true
mirror: always
That's already confusing since the comment doesn't match the code. But
it is enabled, right?
It's confusing. (My defense is I didn't name it, but I also didn't
rename it to make it easier.)
True - means 'allow eval' meaning security is not enforced.
False - means 'block eval' meaning security IS enforced.
https://searchfox.org/mozilla-central/rev/0ffa9e372df56c95547fed9c3433ddec4fbf6f11/browser/app/profile/firefox.js#505
#ifdef NIGHTLY_BUILD
// allow_eval_with_system_principal is enabled on Firefox Desktop only
at this
// point in time
pref("security.allow_eval_with_system_principal", false);
Unless I'm missing something, again, the comment doesn't match the code.
In any case it's disabled but remains enabled for TB.
Correct, I need to fix the comment.
The net effect is that on trunk 70, eval is allowed for TB but not FF.
Correct.
Right so far? I can't see security.uris_using_eval_with_system_principal
in a search, so that's gone.
Correct, this pref was removed from -central in favor of a hardcoded
allowlist in the .cpp file.
So the plan is to disallow the eval again with
security.allow_eval_with_system_principal and bring the white/allowlist
back?
We have no plans to turn the allowlist back into a pref, we intend to
keep it hardcoded in the cpp file.
For beta 69 the situation seems to be like this:
https://searchfox.org/mozilla-beta/rev/384ef1e91e03429ff24ff29586193e65872f2815/modules/libpref/init/all.js#2511
#if defined(DEBUG) && !defined(ANDROID)
pref("security.allow_eval_with_system_principal", false);
pref("security.uris_using_eval_with_system_principal",
"autocomplete.xml,redux.js,react-redux.js, ... and more.
Correct
TB doesn't modify these settings. For ESR 68 the situation is similar,
only that TB adds to the allow/whitelist.
And here's another confusing thing: All this is done in DEBUG mode only
which doesn't apply to the shipped installers. So I assume that on 69
and 68 ESR in opt/release builds, those evals still work as they always
have, right?
Correct, opt/release builds don't enforce anything.
I think Jonathan is interested in the medium to long term future and
that appears to be to disallow eval and "new Function" in general, but
have a allow/whitelist where he could list the sugarJS he uses?
Correct. In 70 or 71 I want to actually block things in release
builds; preventing the execution of eval and logging an error to the
console. Firefox has Telemetry reporting usage of eval so we can
review and allowlist and tweak as necessary. I don't know if TB has
Telemetry, Telemetry Events, or how to look at it.
If the usage is 'new Function("return this")' or "eval('this')" - both
common idioms - they will continue to work. Otherwise TB could add to
the hardcoded allowlist.
If neither of those is acceptable, we could consider bringing back a
pref-based allowlist.
-tom
On 05 Aug 2019 18:46, Tom Ritter wrote:
In 70 or 71 I want to actually block things in release
builds; preventing the execution of eval and logging an error to the
console. Firefox has Telemetry reporting usage of eval so we can
review and allowlist and tweak as necessary. I don't know if TB has
Telemetry, Telemetry Events, or how to look at it.
If the usage is 'new Function("return this")' or "eval('this')" - both
common idioms - they will continue to work. Otherwise TB could add to
the hardcoded allowlist.
If neither of those is acceptable, we could consider bringing back a
pref-based allowlist.
Hi Tom,
thanks for the comments.
Generally, eval and 'new Function' will not work any more, but some
special cases quoted above will, see:
https://hg.mozilla.org/mozilla-central/rev/e76733f6668b#l4.23
I believe we removed all eval and 'new Function' from Thunderbird and
Calendar code and removed our modification of the pref here:
https://hg.mozilla.org/comm-central/rev/d72b6ee0f40f#l3.12, so I guess
we're prepared for whatever the Mozilla platform decides to do.
Add-ons are a different story, but the next major version TB 76 is still
a year away and add-ons can adjust.
What happened to the issue from your first post to tb-drivers? (quote)
We observed crashes from a few sources:
- Mozilla Test Pilot experiments
- userChromeJS which is a Firefox hack that kinda-enables old-style
legacy extensions and basically executes random user-provided JS in
the system principal context - And one specific type of Thunderbird crash reporting the source
file of resource://sendlater3/sugar.js
Does a userChromeJS add-on still exist for FF? There is one for TB
(which I use myself).
Jörg.
On Mon, Aug 5, 2019 at 9:34 PM Jörg Knobloch jorgk@jorgk.com wrote:
I believe we removed all eval and 'new Function' from Thunderbird and
Calendar code and removed our modification of the pref here:
https://hg.mozilla.org/comm-central/rev/d72b6ee0f40f#l3.12, so I guess
we're prepared for whatever the Mozilla platform decides to do.
Presently, the pref is enabled only for Firefox Desktop. If you want
to turn it on for Thunderbird, I'll leave it to you to edit the
thunderbird.js file to enable it.
Add-ons are a different story, but the next major version TB 76 is still
a year away and add-ons can adjust.
What happened to the issue from your first post to tb-drivers? (quote)
We observed crashes from a few sources:
- Mozilla Test Pilot experiments
- userChromeJS which is a Firefox hack that kinda-enables old-style
legacy extensions and basically executes random user-provided JS in
the system principal context - And one specific type of Thunderbird crash reporting the source
file of resource://sendlater3/sugar.js
Does a userChromeJS add-on still exist for FF? There is one for TB
(which I use myself).
There are people using it; yes.
-tom