Opened 6 months ago

Closed 4 months ago

Last modified 8 weeks ago

#25147 closed task (fixed)

Backport of fix shipped in Firefox 58.0.1?

Reported by: gk Owned by: pospeselr
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201804R, tbb-backported
Cc: mcs, brade Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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.

Child Tickets

TicketStatusOwnerSummaryComponent
#25458closedpospeselrUI customization half-broken in Tor Browser 8.0a3Applications/Tor Browser

Attachments (1)

0001-Bug-25147-Backport-of-fix-shipped-in-Firefox-58.0.1.patch (37.0 KB) - added by pospeselr 6 months ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 6 months ago by pospeselr

Owner: changed from tbb-team to pospeselr
Status: newassigned

comment:2 Changed 6 months ago by pospeselr

Keywords: TorBrowserTeam201802R added; TorBrowserTeam201802 removed
Status: assignedneeds_review

comment:3 Changed 6 months ago by pospeselr

Keywords: TorBrowserTeam201803R added; TorBrowserTeam201802R removed

comment:4 Changed 5 months ago by gk

Cc: mcs brade added

comment:5 Changed 5 months ago by gk

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).

Last edited 5 months ago by gk (previous) (diff)

comment:6 in reply to:  5 ; Changed 5 months ago by 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:

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

comment:7 in reply to:  6 Changed 5 months ago by gk

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:

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.

comment:8 Changed 5 months ago by pospeselr

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.

comment:9 Changed 5 months ago by gk

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.

comment:10 Changed 5 months ago by gk

Keywords: TorBrowserTeam201803 added; TorBrowserTeam201803R removed
Status: needs_reviewneeds_information

comment:11 Changed 4 months ago by gk

Keywords: TorBrowserTeam201804 added; TorBrowserTeam201803 removed

Moving our tickets to April.

comment:12 Changed 4 months ago by pospeselr

Keywords: TorBrowserTeam201804R added; TorBrowserTeam201804 removed

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 )

comment:13 Changed 4 months ago by gk

So, we are good with this bug then and only #25458 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.

comment:14 in reply to:  13 Changed 4 months ago by pospeselr

Replying to gk:

So, we are good with this bug then and only #25458 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.

Looked into #25458 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.

comment:15 Changed 4 months ago by gk

Keywords: tbb-backport added

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.

comment:16 Changed 4 months ago by gk

Resolution: fixed
Status: needs_informationclosed

comment:17 Changed 8 weeks ago by gk

Keywords: tbb-backported added; tbb-backport removed

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).

Note: See TracTickets for help on using tickets.