Backport of fix shipped in Firefox 58.0.1?

We could think about backporting the sec-critical fix shipped in Firefox 58.0.1:

https://hg.mozilla.org/releases/mozilla-release/rev/c2db4a50dc5c93b44852d9a5201f7ec062ecc6cb

ESR 52 got audited and this issue was not found there. We could use the backport as a defense-in-depth as it closes out a whole attack vector. The patch is largish, though.

added TorBrowserTeam201804R component::applications/tor browser owner::pospeselr priority::medium resolution::fixed severity::normal status::closed tbb-backported type::task labels

Trac:
Owner: tbb-team to pospeselr
Status: new to assigned

Trac:
0001-Bug-25147-Backport-of-fix-shipped-in-Firefox-58.0.1.patch

Trac:
Status: assigned to needs_review
Keywords: TorBrowserTeam201802 deleted, TorBrowserTeam201802R added

Trac:
Keywords: TorBrowserTeam201802R deleted, TorBrowserTeam201803R added

Trac:
Cc: N/A to mcs, brade

Thanks, looks good to me. I wonder whether we have some means to find out if there are instances of this problem that are solely on the ESR 52 branch which Mozilla did not deem worth enough to write a defense-in-depth for. But anyway, that should give us at least the protections available on -release.

(And lucky me pointing to the patch on the release branch, the thing that landed originally on mozilla-central is slightly different and not complete, which is easy to overlook).

Replying to gk:

Thanks, looks good to me.

Kathy and I also reviewed the backported patch and we think it is okay. We do have a couple of questions:

• Did we look at the "depends on" bug list from https://bugzilla.mozilla.org/show_bug.cgi?id=1432966? Maybe that explains some of the differences between the mozilla-central patch and the release one; for example, I just checked and the fix for https://bugzilla.mozilla.org/show_bug.cgi?id=1433414 is present.
• The changes to devtools/client/responsive.html/components/Browser.js are missing. Do we need them? I guess the equivalent file in ESR52 is browser.js (with a lowercase-B).

I wonder whether we have some means to find out if there are instances of this problem that are solely on the ESR 52 branch which Mozilla did not deem worth enough to write a defense-in-depth for. But anyway, that should give us at least the protections available on -release.

I think the only method is to look at all occurrences of innerHTML =, and that is a painful exercise. Kathy and I started that task and found some things that are in ESR52 but not in mozilla-central. Unfortunately, we had to give up after only getting part way through the huge list of files that need to be examined (we stopped somewhere in the d's, just after 'devtools'). For the record, here are the files we did find that contain innerHTML = statements that look like they should be patched: browser/base/content/newtab/sites.js browser/components/customizableui/CustomizeMode.jsm browser/components/syncedtabs/SyncedTabsDeckView.js

Replying to mcs:

Replying to gk:

Thanks, looks good to me.

Kathy and I also reviewed the backported patch and we think it is okay. We do have a couple of questions:

• Did we look at the "depends on" bug list from https://bugzilla.mozilla.org/show_bug.cgi?id=1432966? Maybe that explains some of the differences between the mozilla-central patch and the release one; for example, I just checked and the fix for https://bugzilla.mozilla.org/show_bug.cgi?id=1433414 is present.

Yes, I did that during the review and I think basically all the differences between the m-c and the m-r patch can be explained that way.

• The changes to devtools/client/responsive.html/components/Browser.js are missing. Do we need them? I guess the equivalent file in ESR52 is browser.js (with a lowercase-B).

Good question and nice catch! I have not checked the source but it does not seem to be unreasonable.

I wonder whether we have some means to find out if there are instances of this problem that are solely on the ESR 52 branch which Mozilla did not deem worth enough to write a defense-in-depth for. But anyway, that should give us at least the protections available on -release.

I think the only method is to look at all occurrences of innerHTML =, and that is a painful exercise. Kathy and I started that task and found some things that are in ESR52 but not in mozilla-central. Unfortunately, we had to give up after only getting part way through the huge list of files that need to be examined (we stopped somewhere in the d's, just after 'devtools'). For the record, here are the files we did find that contain innerHTML = statements that look like they should be patched: browser/base/content/newtab/sites.js browser/components/customizableui/CustomizeMode.jsm browser/components/syncedtabs/SyncedTabsDeckView.js

I could ask one of the Moz engineers whether there is a better way. IIRC there is somewhere a doc where the listed all the things they checked wrt ESR 52.

mcs:

• I did not go down the tree through the dependent bugs, will do that now.
• The Browser.js (and some others) was not included since that file was missing in our source tree. Didn't occur to me that they could have been moved or renamed, so I will go back and check this too.

Okay, I applied the patch as-is to tor-browser-52.6.0esr-8.0-2 (commit 3eb8f10e0c16c52a1d586e190e82009041535503) and after looking at the code a bit more I pushed a fixup (commit b6bc1f1a802dc93620219faeb2f65e2afc78b83c) to take the browser.js changes into account which mcs pointed out.

I leave this ticket open until Richard has checked everything mentioned in comment:8. I think we can file an additional bug for checking whether there are things in esr52 that should be patched as well but are not available anymore on mozilla-central/mozilla-release.

Trac:
Status: needs_review to needs_information
Keywords: TorBrowserTeam201803R deleted, TorBrowserTeam201803 added

Moving our tickets to April.

Trac:
Keywords: TorBrowserTeam201803 deleted, TorBrowserTeam201804 added

So the innerHTML property has been changed such that all existing assignments will automatically sanitize the HTML if it's running within the system context. The new UnsafeSetInnerHTML method that has replaced some of the innerHTML = X statements is meant to circumvent this check for known cases where firefox needs to hand craft some HTML within the system context.

Any issues here with this patch would result in breaking functionality, rather than making system context pages less safe.

I've gone through all the dependent bugs against #1432966 and verified they either don't apply or have already been brought down to our latest branch ( origin/tor-browser-52.7.3esr-8.0-1 )

Trac:
Keywords: TorBrowserTeam201804 deleted, TorBrowserTeam201804R added

So, we are good with this bug then and only #25458 (moved) remains? FWIW: I did not get any reply about specific ESR52 code that would need to get patched which is not in Firefox 58 anymore when asking the Mozilla engineer. I think we should not spend more energy on this defense-in-depth, though, apart from fixing breakage a la #25458 (moved).

Replying to gk:

So, we are good with this bug then and only #25458 (moved) remains? FWIW: I did not get any reply about specific ESR52 code that would need to get patched which is not in Firefox 58 anymore when asking the Mozilla engineer. I think we should not spend more energy on this defense-in-depth, though, apart from fixing breakage a la #25458 (moved).

Looked into #25458 (moved) and sure enough, it's caused by this patch. We didn't catch it (and there's no related bug for it in FF) because the offending calling code no longer exists in FF latest. Will have a patch up shortly once I've verified the fix.

I think we are good here. Giving this another round of testing in the alpha and shipping it to stable in case nothing else explodes.

Trac:
Keywords: N/A deleted, tbb-backport added

Trac:
Resolution: N/A to fixed
Status: needs_information to closed

Backported to tor-browser-52.8.0esr-7.5-1 (commit 8c3c7dcd7e71ae7ca9237fef555efb602ddc7bcc and commit dfc72b77f566b3dd98f08db0e4a8e7bedcf050a1, the latter being the backport of the fixup patch done in #25458 (moved)).

Trac:
Keywords: tbb-backport deleted, tbb-backported added

closed