Closed Bug 1522608 Opened 11 months ago Closed 9 months ago

Port bug 1512949: replace use of eval - Total MozMill and Xpcshell debug failure on 2019-01-24 (all platforms): Assertion failure: false (do not use eval with system privileges), at caps/nsScriptSecurityManager.cpp:483

Categories

(Thunderbird :: General, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 67.0

People

(Reporter: jorgk-bmo, Assigned: aceman)

References

Details

(Whiteboard: [Thunderbird-testfailure: X debug all])

Attachments

(5 files, 5 obsolete files)

Assertion failure: false (do not use eval with system privileges), at /builds/worker/workspace/build/src/caps/nsScriptSecurityManager.cpp:483

PROCESS-CRASH | account | application crashed [@ nsScriptSecurityManager::ContentSecurityPolicyPermitsJSAction(JSContext*, JS::Handle<JS::Value>)]

M-C last good: 3e5f4a8daaee1b9ccad2fe88666a3cfadf
M-C first bad: 2c5eb9d8b8e2680e7ff3d8fe2714bfd78e
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3e5f4a8daaee1b9ccad2fe88666a3cfadf&tochange=2c5eb9d8b8e2680e7ff3d8fe2714bfd78e

nsScriptSecurityManager.cpp was last changed more than a week ago
https://hg.mozilla.org/mozilla-central/log/tip/caps/nsScriptSecurityManager.cpp
so this is triggered some other way.

Summary: Total MozMill debug failure on 2019-01-24 (all platforms): Assertion failure: false (do not use eval with system privileges), at /builds/worker/workspace/build/src/caps/nsScriptSecurityManager.cpp:483 → Total MozMill and Xpcshell debug failure on 2019-01-24 (all platforms): Assertion failure: false (do not use eval with system privileges), at caps/nsScriptSecurityManager.cpp:483
Whiteboard: [Thunderbird-testfailure: X debug all]

I'd say a debug build won't even start, Aceman? I'm building one now.

Flags: needinfo?(acelists)

By running an Xpcshell test interactively I got a stack:

Assertion failure: false (do not use eval with system privileges), at c:/mozilla-source/comm-central/caps/nsScriptSecurityManager.cpp:483
#01: js::DirectEval (c:\mozilla-source\comm-central\js\src\builtin\Eval.cpp:438)
#02: Interpret (c:\mozilla-source\comm-central\js\src\vm\Interpreter.cpp:2970)
#03: js::RunScript (c:\mozilla-source\comm-central\js\src\vm\Interpreter.cpp:421)
#04: js::InternalCallOrConstruct (c:\mozilla-source\comm-central\js\src\vm\Interpreter.cpp:561)
#05: InternalCall (c:\mozilla-source\comm-central\js\src\vm\Interpreter.cpp:588)
#06: js::Call (c:\mozilla-source\comm-central\js\src\vm\Interpreter.cpp:604)
#07: JS_CallFunctionValue (c:\mozilla-source\comm-central\js\src\jsapi.cpp:2555)
#08: nsXPCWrappedJSClass::CallMethod (c:\mozilla-source\comm-central\js\xpconnect\src\XPCWrappedJSClass.cpp:1008)
#09: nsXPCWrappedJS::CallMethod (c:\mozilla-source\comm-central\js\xpconnect\src\XPCWrappedJS.cpp:610)
#10: PrepareAndDispatch (c:\mozilla-source\comm-central\xpcom\reflect\xptcall\md\win32\xptcstubs_x86_64.cpp:181)
#11: SharedStub (c:\mozilla-source\comm-central\xpcom\reflect\xptcall\md\win32\xptcstubs_asm_x86_64.asm:61)
#12: mozilla::net::`anonymous namespace'::ServerSocketListenerProxy::OnSocketAcceptedRunnable::Run (c:\mozilla-source\comm-central\netwerk\base\nsServerSocket.cpp:495)
#13: nsThread::ProcessNextEvent (c:\mozilla-source\comm-central\xpcom\threads\nsThread.cpp:1147)
#14: nsThreadManager::SpinEventLoopUntilInternal (c:\mozilla-source\comm-central\xpcom\threads\nsThreadManager.cpp:483)
#15: XPTC__InvokebyIndex (c:\mozilla-source\comm-central\xpcom\reflect\xptcall\md\win32\xptcinvoke_asm_x86_64.asm:99)

In this case the JS stack was:
0 createHandler(d = [object Object]) ["resource://testing-common/mailnews/IMAPpump.js":56:32]
this = [object Object]
1 onSocketAccepted(socket = [xpconnect wrapped nsIServerSocket @ 0x1dce57d2280 (native @ 0x1dcde3047a8)], trans = [xpconnect wrapped (nsISupports, nsISocketTransport, nsITransport, nsIDNSListener, nsIInterfaceRequestor) @ 0x1dce59a8940 (native @ 0x1dce5a56018)]) ["resource://testing-common/mailnews/maild.js":133:18]
this = [object Object]
2 _do_main() ["c:\mozilla-source\comm-central\testing\xpcshell\head.js":224:2]
this = [object BackstagePass @ 0x1dce50295e0 (native @ 0x1dcde31ebc0)]
3 _execute_test() ["c:\mozilla-source\comm-central\testing\xpcshell\head.js":528:4]
this = [object BackstagePass @ 0x1dce50295e0 (native @ 0x1dcde31ebc0)]
4 <TOP LEVEL> ["typein":1:0]

And IMAPpump.js line 56 is, ta, dah: mixinExtension(handler, eval("IMAP_" + part + "_extension"));

So, if eval doesn't work any more, we have a problem :-(

Looking for JS changes in the regression range, I see this:
7259ee92e345 Jan de Mooij — Bug 1522051 - Stop giving singleton types to call objects. r=tcampbell

Flags: needinfo?(jdemooij)

(In reply to Jorg K (GMT+1) from comment #2)

So, if eval doesn't work any more, we have a problem :-(

Looking for JS changes in the regression range, I see this:
7259ee92e345 Jan de Mooij — Bug 1522051 - Stop giving singleton types to call objects. r=tcampbell

Very unlikely to be related. I'd suggest bisecting this.

Flags: needinfo?(jdemooij)

In that range I'd say bug 1512949.

Nope, backout of rev 7259ee92e345 didn't fix it. So next regression candidate, bug 1509441. I'll go to the changeset before, rev 24ad7d6e7775.

Thanks, Jan, how did I miss "eval" in that commit message :-(

Looking at bug 1512949, they are really replacing eval. Hmm, for the time being, I'll fix this with pref security.allow_eval_with_system_principal.

Aceman, can we get rid of the evals?

debug build does start fine.
Should we stop using eval?

Summary: Total MozMill and Xpcshell debug failure on 2019-01-24 (all platforms): Assertion failure: false (do not use eval with system privileges), at caps/nsScriptSecurityManager.cpp:483 → Port bug 1512949: replace use of eval - Total MozMill and Xpcshell debug failure on 2019-01-24 (all platforms): Assertion failure: false (do not use eval with system privileges), at caps/nsScriptSecurityManager.cpp:483

Mine doesn't start, but I have an IMAP account in the profile. But most of MozMill fails.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2774b8292eea
Allow eval with system principal after bug 1512949. rs=bustage-fix

We don't have many evals in application code, but a fair few in testing code which would explain the test failures. I guess the one that killed me was:
comm/common/src/Overlays.jsm
488 this.window.eval(node.textContent);
since my testing profile has some overlay add-ons. I tried to attach the debugger, but it crashed before :-(

Here they all are:
https://dxr.mozilla.org/comm-central/search?q=eval(+path%3Acomm%2F+file%3A*.js&redirect=false

I've made a patch for the IMAP stuff, since I already had it open for other reasons. It should clear up a lot of the XPCShell failures. It's currently on Try.

Don't take this as a sign I'm working on the other failures right now.

Attached patch 1522608.patch (obsolete) — Splinter Review

I tried to fix all occurrences, but skipped some code e.g. in mozmill that seems potentially unused. Let's see the try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=22d7e996451a13684e9f2134fcf91dc7e79ec908

Assignee: nobody → acelists
Flags: needinfo?(acelists)

Geoff had further changes for the IMAP stuff:
https://hg.mozilla.org/try-comm-central/rev/4ce5843cc85d#l2.2

Surely he's the better reviewer for the lot.

It's a different implementation for the same goal. Yes, I think it can be faster (less imports), but has more changes needed.

The try is all green on opt so the replacements seem fine.
On debug mozmill is still broken, the last offender seems to be
https://searchfox.org/comm-central/source/common/src/Overlays.jsm#486
It loads script code (contents of <script> element) and executes it.
Any clever hack around that?
Save the string into a temp file and call Services.scriptloader.loadSubScript() as in the branch when the script is linked via 'src' ? :)

I'm tempted to take it out and say scripts need to be in a file now.

(In reply to :aceman from comment #16)

Save the string into a temp file and call Services.scriptloader.loadSubScript() as in the branch when the script is linked via 'src' ? :)

I've done almost that, but using a data URL instead: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c179d3bed56b18522424dd82b9a0a1321cb0ac61

Thanks, that does work.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0861540fb85919784acb9894d764397216f9cea6

Still crashes opening preferences, and I do not yet see why.

Let's land what we have already working.

This is the part that Darktrojan did.

Attachment #9039329 - Flags: review+
Attached patch 1522608-aceman.patch (obsolete) — Splinter Review

This makes the rest of mozmill work, except some tests that open preferences which still crash, but I do not yet see where 'eval' is called there.

Attachment #9038960 - Attachment is obsolete: true
Attachment #9039330 - Flags: review?(geoff)
Comment on attachment 9039330 [details] [diff] [review]
1522608-aceman.patch

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

::: mail/test/resources/mozmill/mozmill/extension/content/modules/controller.js
@@ -688,5 @@
>  
>    frame.events.pass({'function':'controller.waitFor()'});
>  }
>  
> -MozMillController.prototype.waitForEval = function(expression, timeout, interval, subject) {

I remove this function as it seems unused.
But there is still assertJS() which is used in calendar tests so those should be updated first.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/be6ed62ca1ec
remove uses of eval() in IMAP tests and Overlay.jsm. r=aceman

Keywords: checkin-needed
Comment on attachment 9039330 [details] [diff] [review]
1522608-aceman.patch

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

::: mailnews/base/test/unit/test_nsIMsgFolderListenerLocal.js
@@ +175,3 @@
>    // Create a third folder for more testing.
> +  function addFolder2() { addFolder(gRootFolder, "folder3",
> +                                    function(folder) { gLocalFolder3 = folder; }); },

Can we put the function code on a new line please? This is not pretty. :(

::: mailnews/imap/test/unit/test_nsIMsgFolderListenerIMAP.js
@@ +155,5 @@
>    // Create another folder to move and copy messages around, and force initialization.
> +  function testAddFolder1() { addFolder(gRootFolder, "folder2",
> +                                        function(folder) { gIMAPFolder2 = folder; }) },
> +  function testAddFolder2() { addFolder(gRootFolder, "folder3",
> +                                        function(folder) { gIMAPFolder3 = folder; }) },

Same here.
Attachment #9039330 - Flags: review?(geoff) → review+

(In reply to Geoff Lankow (:darktrojan) from comment #24)

  • function addFolder2() { addFolder(gRootFolder, "folder3",
  •                                function(folder) { gLocalFolder3 = folder; }); },
    

Can we put the function code on a new line please? This is not pretty. :(

Sure, in what way?

function addFolder2() {
  addFolder(gRootFolder, "folder3", function(folder) { gLocalFolder3 = folder; });
},

Ok, thanks.

Attachment #9039330 - Attachment is obsolete: true
Attachment #9039450 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/3728fc9ecdc3
remove most uses of eval() in C-C tests. r=darktrojan

Thanks, how much is missing now?

Still comment 19.

Attachment #9039450 - Attachment description: 1522608.patch → 1522608.patch [landed in comment 28]
Attachment #9039329 - Attachment description: 1522608-darktrojan.patch → 1522608-darktrojan.patch [landed in comment 23]

It still exists and the function causing the assert (AssertEvalNotUsingSystemPrincipal)
only outputs the file chrome://messenger/content/preferences/preferences.xml . There does not seem to be any obvious eval, maybe there is one in the linked scripts. I couldn't dig deeper yet.

(In reply to :aceman from comment #31)

There does not seem to be any obvious eval, maybe there is one in the linked scripts. I couldn't dig deeper yet.

FWIW, I see some "new Function" uses in that file (assuming I looked at the right file) and those are similar to eval.

Yes, there is 'new Function' but that seems to still be used in m-c. Is it really a problem?

Arai helped me to debug this in the JS engine and indeed the 'new Function()' calls are the problem.

We have a bunch of them:
https://dxr.mozilla.org/comm-central/search?q=%22new+Function%22+%2Bpath%3Acomm&redirect=false

This removes one use of 'new Function' used for execution of the onpaneload attribute. This converts it to .addEventListener() call containing the same code, but as a native function. This only handles onpaneload on <prefpane> inside <preftab>.
Those under <prefwindow> needs to be handled too, see bug 1522778 comment 38.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b3a2de20ba8419a4ea9a59e4b64852d07aac2c56

Now this same treatment needs to be done for onsyncfrompreference and onsynctopreference attributes. (Disabling those 2 makes opening the prefs no longer crash in the eval() check).

And possibly in calendar prefpane. Darktrojan, can you continue from here?

Attachment #9041305 - Flags: review?(geoff)
Attachment #9041305 - Flags: feedback?(arai.unmht)
Comment on attachment 9041305 [details] [diff] [review]
1522608-preftab.patch

Good. I never did like the way that worked.
Attachment #9041305 - Flags: review?(geoff) → review+
Flags: needinfo?(geoff)
Comment on attachment 9041305 [details] [diff] [review]
1522608-preftab.patch

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

::: mail/components/preferences/preferences.js
@@ -34,5 @@
>      radio.style.listStyleImage = pane.style.listStyleImage;
>      selector.appendChild(radio);
>  
>      pane.dispatchEvent(new CustomEvent("paneload"));
> -    new Function(pane.getAttribute("onpaneload")).call(pane);

isn't this necessary?
does dispatchEvent properly call the handler?
Attachment #9041305 - Flags: feedback?(arai.unmht) → feedback+

(In reply to Tooru Fujisawa [:arai] from comment #37)

 pane.dispatchEvent(new CustomEvent("paneload"));
  • new Function(pane.getAttribute("onpaneload")).call(pane);
    isn't this necessary?
    does dispatchEvent properly call the handler?

Yes, I tested the code in addEventListener (the various init functions) does run properly.

Keywords: checkin-needed
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk

I guess we'll be done here and I can back out the pref change from https://hg.mozilla.org/comm-central/rev/2774b8292eea.

Flags: needinfo?(geoff)
Keywords: leave-open

Oh, re-read comment #25, there's still more to it.

Flags: needinfo?(geoff)
Keywords: leave-open

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/288ed882534b
remove uses of 'new Function()' in C-C Preferences onpaneload. r=darktrojan

Keywords: checkin-needed

(In reply to :aceman from comment #34)

Arai helped me to debug this in the JS engine and indeed the 'new Function()' calls are the problem.

We have a bunch of them:
https://dxr.mozilla.org/comm-central/search?q=%22new+Function%22+%2Bpath%3Acomm&redirect=false

Mozilla-central also has a lot of them (example), how do they get away with it?

Flags: needinfo?(geoff)
Flags: needinfo?(arai.unmht)
Flags: needinfo?(acelists)

I don't know. Maybe that custom element is not considered using 'system principal' ?
They also do not use 'var {object} = ChromeUtils.import()' that they forced on us at https://searchfox.org/mozilla-central/source/toolkit/content/preferencesBindings.js#12 .

But you can set security.allow_eval_with_system_principal to false in TB and you'll see the prefs crash.

Flags: needinfo?(acelists)
Depends on: 1527770

I'm going to change all the preferences stuff in bug 1527770, as it's too big to piggy-back on this bug.

I also don't know.

Flags: needinfo?(arai.unmht)
Attached patch 1522608-calendar-eval-1.diff (obsolete) — Splinter Review

This gets all the eval out of calendar. With this and bug 1527770 we're good to go here.

Attachment #9044578 - Flags: review?(philipp)
Attachment #9044578 - Flags: review?(philipp) → review+

In case somebody gets to this before I do and wants to complain that it doesn't apply.

Attachment #9044578 - Attachment is obsolete: true
Attachment #9047584 - Flags: review+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/e854574aae9f
Remove all uses of new Function and assertJS in calendar, and remove assertJS from mozmill. r=philipp DONTBUILD

Keywords: checkin-needed
Target Milestone: --- → Thunderbird 67.0
Attached patch 1522608-whitelist-1.diff (obsolete) — Splinter Review

It looks like preferencesBindings.js is whitelisted and that's how it passes the test. Now I'm not quite sure how my previous Try run passed.

This patch adds some files to the whitelist and removes the temporary disabling of the check. Try passes with it (and the other stuff that needs to land).

Attachment #9048658 - Flags: review?(acelists)

m-c cheats and has preferencesBindings.js whitelisted and thus can still use e.g. 'synctopreference' which calls 'new Function()' inside.

So the question is whether we want to migrate away from that sooner than them, or we wait.

Either they'll fix it in a way that doesn't affect us, or we can copy what they do when they do it. I suspect they're probably not in a rush to do anything about it.

Comment on attachment 9048658 [details] [diff] [review]
1522608-whitelist-1.diff

So this is more a technical direction decision now. The patch looks OK if it prefs tab does not crash with it. Does there need to be the trailing comma in the pref value?
Attachment #9048658 - Flags: review?(acelists) → review?(mkmelin+mozilla)

Oh no, that's a screw-up from when I reordered the values. I hope that hasn't affected whether this passes or not, and I suspect it could've.

Okay, leaving the comma behind was a real mistake, no wonder everything was going so smoothly. Instead I've copied the pref from m-c and added the files from c-c that need to be in it. I've also put it in an #ifdef like on m-c, and left the other pref in the file.

Attachment #9048658 - Attachment is obsolete: true
Attachment #9048658 - Flags: review?(mkmelin+mozilla)
Attachment #9048718 - Flags: review?(acelists)
Attachment #9048718 - Flags: review?(acelists) → review?(mkmelin+mozilla)
Comment on attachment 9048718 [details] [diff] [review]
1522608-whitelist-2.diff

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

Better than what we had before. r=mkmelin
Attachment #9048718 - Flags: review?(mkmelin+mozilla) → review+

I'm not sure whether to land this since comment #47 makes reference to bug 1527770.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/f1ef44fd16c8
Whitelist some files that need to use eval with system privileges; r=mkmelin DONTBUILD
Status: NEW → RESOLVED
Closed: 9 months ago
Keywords: leave-open
Resolution: --- → FIXED
Type: defect → task

Comment on attachment 9099264 [details]
Bug 1522608 - Fix include-what-you-use violations in LayerAttributes.h. r=tnikkel

Revision D48372 was moved to bug 1552608. Setting attachment 9099264 [details] to obsolete.

Attachment #9099264 - Attachment is obsolete: true

(Sorry for the noise. I had a wrong bug number in a commit message.)

I was starting to wonder ;-)

You need to log in before you can comment on or make changes to this bug.