1544204 - Consider further #ifdef DEBUG or prefs to avoid MOZ_ASSERTS in Thunderbird testing when accessing http(s) resources - Was: Clicking on "Get Add-ons" crashes in debug builds

Status: NEW

(Core :: DOM: Security, defect, P3)

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1513445 +++

We from Thunderbird have a lot of trouble with the changes from bug 1513445 since our test suite in debug mode accesses http(s) resources, see our bug 1543567.

However, FF itself is affected. A local debug build or a debug build from automation crashes when clicking on "Get Add-ons" in the add-ons manager:

Assertion failure: false (SystemPrincipal must not load remote documents.), at z:/build/build/src/dom/security/nsContentSecurityManager.cpp:790

While I'm here, in bug 1543567 we're trying to trigger this code here that only runs when the environment variable MOZ_DISABLE_NONLOCAL_CONNECTIONS is set:
https://hg.mozilla.org/mozilla-central/rev/18f074af5d93#l1.56

So in other words, pref security.disallow_non_local_systemprincipal_in_tests is only checked if MOZ_DISABLE_NONLOCAL_CONNECTIONS is set. That doesn't appear right. Why do we have to disallow non-local connections to get it to check the pref?

Comment 1

[Frederik Braun [:freddy]]

Yes, this was indeed going to be fixed in bug 1544008, but that was unfortunately backed out. I just requested it for landing.

So, I agree that I should have explained the patch and its goals better.
My aim is to disallow document/subdocument loads from the SystemPrincipal, so we do not give chrome privileges to remote resources inadvertently.

The first draft of this patch broke all tests. E.g., all of our mochitests run on a mock example.com, which is seemingly a remote document. But during tests, these are not really remote. IN fact, we can check that they are not with xpc::AreNonLocalConnectionsDisabled().
If this is the case, we want to raise an NS_ASSERTION(), which just reports the failure. This one we can observe within the mochitest for our patch specifically. But we do not want to assert for all of our mochitests, so we only do this when the security.disallow_non_local_systemprincipal_in_tests is set, which defaults to false. This pref is just for the mochitest to switch the MOZ_ASSERT into an NS_ASSERTION. I apologize if the naming confused you.

If we are not in mochitests (xpc::AreNonLocalConnectionsDisabled() is false), we raise an actual assertion with MOZ_ASSERT() that crashes the process in debug builds.

Indeed, there might be some other breakage besides about:addons, and we intend to this patch as a means to identify and migrate them to a better code pattern iteratively. If migration is hard, we'll add a temporary hotfix and file a follow-up bug. E.g., bug 1544011.

I realize that Thunderbird is quite a different beast and you have lots of other features that interact with web content, so you I understand you might not want to benefit from this re-architecturing work. I also understand that just not taking this patch into comm-central would make your live harder as the DoContentSecurityChecks() function might diverge from the mozilla-central version in the long run.

Maybe there is some middle-ground that would help you move along without making either of our goals significantly harder. Besides the current #ifdef DEBUG, would it help you if I introduced another pref that bypasses all of these security checks?

Comment 2

[Jorg K (CEST = GMT+2)]

Thanks for the detailed reply.

So you're saying that MOZ_DISABLE_NONLOCAL_CONNECTIONS is set for your Mochitests, which are (mildly) similar to our Mozmill tests since the exercise the UI of FF or TB receptively.

So we're somewhat on the right track then setting MOZ_DISABLE_NONLOCAL_CONNECTIONS for part of out Mozmill suite, in fact one directory of tests. Sadly it turns out that TB causes a lot of web access via various services, so we're in the process of disabling them all, as you can see here:
https://hg.mozilla.org/try-comm-central/rev/6dcd3a153bccb16ddc5c1f3808fe31b80765b5a4

Aceman is working on that one, I think he's got it almost done, in his latest patch the system is still "phoning home" to aus5.mozilla.org ;-)
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d9d1be7a363ff5d86f3d19820584130ceab17bc6&selectedJob=240228111

We'll consider your offer of further #ifdef DEBUG or prefs in due course.

Let's relabel this bug for this purpose since you have the "Get add-ons" thing covered in bug 1544008.

Comment 3

[:aceman]

Yes, if we could stop all those remote connections done from m-c maybe we will not need any special ifdef. It seems we do not need those remote services in our test suite (like updates, telemetry, location services). It seems m-c tests also disable many of them for tests like xpcshell and mochitests. We just need to add those prefs to mozmill as that one doesn't inherit the settings from m-c (m-c does not have mozmill).
We just need to find them all. Any idea what else can ping to aus5.mozilla.org besides what I have already disabled in the linked patch?