Opened 9 months ago

Last modified 3 weeks ago

#23745 needs_review defect

Tab crashes when using Tor Browser to access Google Drive

Reported by: angelotheram2 Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Major Keywords: tbb-crash, tbb-7.0-issues, tbb-regression, tbb-e10s, TorBrowserTeam201806R
Cc: mcs, brade, arthuredelstein Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Using the Linux 64-bit Tor Browser, or the Tor Browser in Tails 3.0, 3.1 or 3.2, I log in to Google mail. After logged in I go to drive.google.com. The Google Drive page starts to render, then I get the message

Gah. Your tab just crashed. We can help you!

Choose Restore This Tab to reload page content.

Child Tickets

Change History (24)

comment:1 Changed 9 months ago by gk

Keywords: tbb-crash added; Tor Browser removed
Milestone: Tor: unspecified
Priority: MediumHigh
Severity: NormalMajor
Status: newneeds_information

Is that reproducible? Does it go away if you set browser.tabs.remote.autostart.2 to false in your about:config and try again after restart? Does it happen with a vanilla Firefox 52 ESR as well? (see https://www.mozilla.org/en-US/firefox/organizations/all/ for a download option)

comment:2 in reply to:  1 Changed 9 months ago by gk

Cc: mcs brade arthuredelstein added
Status: needs_informationassigned

Replying to gk:

Is that reproducible? Does it go away if you set browser.tabs.remote.autostart.2 to false in your about:config and try again after restart? Does it happen with a vanilla Firefox 52 ESR as well? (see https://www.mozilla.org/en-US/firefox/organizations/all/ for a download option)

Without e10s the crash does not happen and Firefox is not affected. mcs, brade, arthur: Can anybody of you take a look at that and figure out what is going on?

comment:3 Changed 9 months ago by gk

Keywords: tbb-e10s TorBrowserTeam201710 added

comment:4 Changed 9 months ago by angelotheram2

It does go away when I set browser.tabs.remote.autostart.2 to false and restart the browser.
I then set browser.tabs.remote.autostart.2 to back to true and the problem returns.
This setting is a clue and a workaround, thanks.

comment:5 Changed 9 months ago by arthuredelstein

I was able to reliably reproduce this crash and by bisecting our patches I tracked it down to our indexedDB patch:
https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-52.4.0esr-7.5-1&id=31348e47a340494c4002b43d8fb509689f8f7e63
The work for this patch and its predecessor are at #21308 and #16528 respectively.

I also confirmed that if I set "dom.indexedDB.enabled" to true, the browser no longer crashes. (Flipping this pref is not recommended for users, because of privacy/tracking implications. I mention it only for debugging purposes.)

gk observed that the crash is being caused by a block of code in ActorsParent.cpp:

if (principalInfo.type() != PrincipalInfo::TSystemPrincipalInfo &&
    NS_WARN_IF(!Preferences::GetBool(kPrefIndexedDBEnabled, false))) {
  if (aContentParent) {
    // The DOM in the other process should have kept us from receiving any
    // indexedDB messages so assume that the child is misbehaving.
    aContentParent->KillHard("IndexedDB CheckPermission 1");
  }
  return NS_ERROR_DOM_INDEXEDDB_NOT_ALLOWED_ERR;
}

But this does not happen if "dom.indexedDB.enabled" is true. I have yet to investigate why.

In the meantime, I am trying to work out what our strategy should be for indexedDB. Update: I have learned two of the following items are no longer true:

  • indexedDB is a supercookie vector and has not yet been patched to respect first-party isolation in Tor Browser or Firefox. (Actually patched as early as ESR52.)
  • There is a Mozilla ticket for enabling a memory-only indexeddb in Firefox but this has not been resolved. In other words, indexeddb always writes to disk.
  • As mikperry pointed out, there wasn't a good way to programmatically clear indexedDB databases. That issue may have been recently fixed in Firefox and we should investigate if we can use this in New Identity. (In fact, torbutton already apparently clears indexedDBs.)
  • In PBM, indexedDB is already effectively disabled, even when "dom.indexedDB.enabled" is true.
  • Modernizr was updated a year ago to correctly detect the absence of indexedDB in Firefox PBM.

So here are my thoughts. We should of course continue to disable indexedDB in PBM until it can be memory-only, programmatically cleared, and isolated by first party.

There was a concern that some users might disable PBM and then New Identity would fail to wipe indexedDB. Do we actually want to claim to protect users who turn off PBM? That seems possibly out of scope to me. If that's the case, it seems to me we can rely on indexedDB being disabled in PBM by Firefox's built-in mechanism and drop the #21308 patch?

But if we want to protect non-PBM users, then we should investigate whether New Identity can clear indexedDB (#23768). That ticket would be useful in the future if we wanted to enable indexedDB in PBM as well.

If instead we want to ensure we have disabled indexedDB in non-PBM windows as well, then I would suggest writing a new patch that simply stops exposing indexedDB to content by modifying dom/webidl/WindowOrWorkGlobalScope.webidl, so indexedDB is not exposed when the pref "dom.indexedDB.enabled" is false.

Last edited 9 months ago by arthuredelstein (previous) (diff)

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

Replying to arthuredelstein:

There was a concern that some users might disable PBM and then New Identity would fail to wipe indexedDB. Do we actually want to claim to protect users who turn off PBM? That seems possibly out of scope to me. If that's the case, it seems to me we can rely on indexedDB being disabled in PBM by Firefox's built-in mechanism and drop the #21308 patch?

The point is that New Identity is not bound to PBM. In other words: we don't have a menu item saying "New Identity (if you are in Private Browsing Mode)" but rather "New Identity". It is supposed to (or I bet at least perceived as) give/giving you a New Identity no matter what. And I think that is good. Getting a clean slate should not depend on whether you are in PBM or are allowing disk records for some reason.

comment:7 Changed 9 months ago by gk

Keywords: tbb-7.0-issues tbb-regression added

comment:8 in reply to:  5 ; Changed 9 months ago by arthuredelstein

Replying to gk:

And I think that is good. Getting a clean slate should not depend on whether you are in PBM or are allowing disk records for some reason.

OK, I'm persuaded it's a good idea. Thanks!

Replying to arthuredelstein:

  • indexedDB is a supercookie vector and has not yet been patched to respect first-party isolation in Tor Browser or Firefox.
  • As mikperry pointed out, there wasn't a good way to programmatically clear indexedDB databases. That issue may have been recently fixed in Firefox and we should investigate if we can use this in New Identity.

I was behind the curve and learned in fact both these problems have already been solved:

  • New Identity does appear to wipe indexedDB.
  • indexedDB is first-party isolated in ESR52 (and TBB/ESR52).

To confirm these two assertions, I did the following experiment:

  1. I set "browser.privatebrowsing.autostart" to false and "dom.indexedDB.enabled" to true in Tor Browser 7.0.6.
  2. I opened a New non-PB window and visited https://codepen.io/caraya/pen/xVrXrG which allows one to make entries in an indexedDB database. I manually entered 3 entries.
  3. I saw in tor-browser_en-US/Browser/TorBrowser/Data/Browser/profile.default/storage/default that there was a directory called https+++s.codepen.io^firstPartyDomain=codepen.io/.
  4. I restarted Tor Browser, visited the site again and confirmed that my database entries were still present.
  5. I clicked "New Identity", visited site once more and saw the the database was now reported empty.
  6. I noted that tor-browser_en-US/Browser/TorBrowser/Data/Browser/profile.default/storage/ had been deleted, including its subdirectories default/https+++s.codepen.io^firstPartyDomain=codepen.io/

FPI of indexedDB was also confirmed to me by a few Mozillians. And I noticed that code in torbutton here:
https://gitweb.torproject.org/torbutton.git/tree/src/chrome/content/torbutton.js?id=b3ff9863db338b2bd612f109e8bbce4c4af7cbd0#n1151 (Corrected link)
is likely where the indexedDB wiping is happening. (It might be good to update that code to better resemble this Mozilla patch here:
https://hg.mozilla.org/mozilla-central/rev/0fbe00ad0203#l1.42
because it doesn't require turning on the "dom.quotaManager.testing" pref; I will make a note in #23768.)

So to me it looks like FPI and clearing on New Identity mean that non-PBM indexedDB functionality is already the way we want it in Tor Browser, even when dom.indexedDB.enabled = true. And of course PBM windows already disabled indexedDB. So I'm inclined to do a little more testing to confirm FPI of indexedDB. If nothing bad is found, I would suggest we simply revert our #21308 patch, flip the "dom.indexedDB.enabled" pref to true and call that a fix for this ticket. gk, what do you think?

Last edited 9 months ago by arthuredelstein (previous) (diff)

comment:9 in reply to:  8 Changed 9 months ago by gk

Replying to arthuredelstein:

So to me it looks like FPI and clearing on New Identity mean that non-PBM indexedDB functionality is already the way we want it in Tor Browser, even when dom.indexedDB.enabled = true. And of course PBM windows already disabled indexedDB. So I'm inclined to do a little more testing to confirm FPI of indexedDB. If nothing bad is found, I would suggest we simply revert our #21308 patch, flip the "dom.indexedDB.enabled" pref to true and call that a fix for this ticket. gk, what do you think?

Sounds like a plan. Thanks for digging up all that useful info.

comment:10 Changed 9 months ago by gk

Another thought I had: it might be a good idea to take the old Tor Browser version Mike used back then for testing where he realized that IndexedDB related data is not getting deleted on New Identity. Just to be sure we are testing the same thing and it is indeed fixed now but not in the old Tor Browser.

comment:11 in reply to:  10 Changed 8 months ago by arthuredelstein

Replying to gk:

Another thought I had: it might be a good idea to take the old Tor Browser version Mike used back then for testing where he realized that IndexedDB related data is not getting deleted on New Identity. Just to be sure we are testing the same thing and it is indeed fixed now but not in the old Tor Browser.

Good idea. I tested the indexedDB demo at
https://codepen.io/caraya/pen/xVrXrG
with "browser.privatebrowsing.autostart" set to false
using our build at
https://archive.torproject.org/tor-package-archive/torbrowser/5.0a3/tor-browser-linux64-5.0a3_en-US.tar.xz
which is the same TBB version that was available when Mike posted ticket:16528#comment:11.

I confirmed that New Identity does not clear the indexedDB database in that version.

I also re-ran the experiment in comment:8 and confirmed that New Identity clears indexedDB in the latest Tor Browser.

So here are two patches for review: https://github.com/arthuredelstein/tor-browser/commits/23745

Although I could see evidence of FPI in comment:8, I haven't directly observed it. I will write code to test that tomorrow. I'm pretty confident FPI is happening, though, given the filenames I observed and also by confirmation from Mozilla folks, so if there isn't time to wait for that test, I would suggest trying it in the alpha anyway.

comment:12 Changed 8 months ago by arthuredelstein

Keywords: TorBrowserTeam201710R added; TorBrowserTeam201710 removed
Status: assignedneeds_review

comment:13 Changed 8 months ago by gk

Looks good to me. Applied to tor-browser-52.4.0esr-7.5-1 (commits 03d6c603319adbf80c6a9badc0922fd339fb80cc and b4fc15a8986a8d2c8267df281b526eafb58bf6d7).

The patches are straight forward so I applied them as well to tor-browser-52.4.0esr-7.0-1 to get the crash fixed in 7.0.7 as well (commits d3222cf5469b28bfb2b018055693d94254df9cd2 and 00c8f9b86215e26a2a9785be4a45e8a2d652e224).

Please test the FPI, though. We might have time to revert things in the unlikely case FPI is broken. Leaving the ticket open for that part.

comment:14 Changed 8 months ago by gk

Keywords: TorBrowserTeam201711R added; TorBrowserTeam201710R removed

Moving review to November

comment:15 Changed 7 months ago by angelotheram2

All,
I just downloaded and ran Tails 3.3, with Tor Browser 7.0.10, and Google Drive is now working for me without any about:config changes in Firefox. Thank you for the fix.

comment:16 Changed 7 months ago by gk

Keywords: TorBrowserTeam201712R added; TorBrowserTeam201711R removed

Moving review tickets over to December

comment:17 Changed 5 months ago by gk

Moving review tickets to 2018

comment:18 Changed 5 months ago by gk

Keywords: TorBrowserTeam201801R added; TorBrowserTeam201712R removed

Moving review tickets for real.

comment:19 Changed 5 months ago by gk

Keywords: TorBrowserTeam201802R added; TorBrowserTeam201801R removed

Moving reviews to February.

comment:20 Changed 4 months ago by gk

Keywords: TorBrowserTeam201803R added; TorBrowserTeam201802R removed

Moving our reviews to March 2018

comment:21 Changed 3 months ago by gk

Keywords: TorBrowserTeam201804R added; TorBrowserTeam201803R removed

Moving reviews to April 2018

comment:22 Changed 2 months ago by gk

Priority: HighMedium

comment:23 Changed 7 weeks ago by gk

Keywords: TorBrowserTeam201805R added; TorBrowserTeam201804R removed

Moving review tickets to May.

comment:24 Changed 3 weeks ago by gk

Keywords: TorBrowserTeam201806R added; TorBrowserTeam201805R removed

Moving review tickets to June.

Note: See TracTickets for help on using tickets.