Opened 6 years ago

Last modified 4 months ago

#12683 new defect

Permissions in nsIPermissionManager aren't cleared with TorButton's "New Identity"

Reported by: isis Owned by: tbb-team
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-linkability, tbb-newnym, tbb-torbutton, gitlab-tb-torbutton
Cc: isis, mikeperry, gk Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


When TorButton's "New Identity" button is pressed, the permissions stored with nsIPermissionManager aren't cleared, even though nsIPermissionManager.removeAll() is called.

From torbutton_do_new_identity() in src/chrome/content/torbutton.js:

  torbutton_log(3, "New Identity: Clearing permissions");
  let pm = Cc[";1"].

  torbutton_log(3, "New Identity: Sending NEWNYM");

There's a ton of info stored in this thing, including how many time the site has been visited, if popups are allowed, if a site can access offline storage, etc. For me, several dozen sites are listed after clicking "New Identity". It seems to have been keeping these permissions for quite a while, as some of my sites are reported to have hundreds of visits.

To reproduce, do some stuff in TorBrowser for a while, then click "TorButton > New Identity", then navigate to about:permissions.

Child Tickets

Change History (7)

comment:1 Changed 6 years ago by mikeperry

Keywords: tbb-newnym added
Priority: normalmajor

comment:2 in reply to:  description Changed 6 years ago by gk

Component: TorbuttonTorBirdy
Owner: set to ioerror

Replying to isis:

To reproduce, do some stuff in TorBrowser for a while, then click "TorButton > New Identity", then navigate to about:permissions.

I think I followed these instructions closely but was not able to reproduce your problem. :) Do you have some, well, more fain-grained steps for me?

comment:3 Changed 6 years ago by gk

Component: TorBirdyTorBrowserButton
Owner: changed from ioerror to mikeperry

comment:4 Changed 6 years ago by erinn

Component: TorBrowserButtonTor Browser
Keywords: tbb-torbutton added
Owner: changed from mikeperry to tbb-team

comment:5 Changed 6 years ago by isis

Hmm. I tested on two machines, both of which were able to reproduce this issue, and both of which had:

Preference Name Value
extensions.torbutton.block_disk false
extensions.torbutton.saved.disk_cache true

Then I tested on a third machine with:

Preference Name Value
extensions.torbutton.block_disk true
extensions.torbutton.saved.disk_cache true

So it seems like, perhaps, if TorButton's block_disk patches are disabled, then FF writes the nsIPermissionsManager preferences to disk, and TorButton isn't able to clear them from disk, even when calling Ci.nsIPermissionManager.removeAll(). Perhaps Ci.nsIPermissionManager.removeAll() is broken?

Additionally, and I believe this supports my theory that nsIPermissionManager.removeAll() is broken, TorButton sets

Preference Name Value
permissions.memory_only true


Preference Name Value
extensions.torbutton.block_disk true

and likewise false when the other is set to false.

There is another method, RemoveAllInternal in the nsPermissionManager implementation:

899 nsresult
900 nsPermissionManager::RemoveAllInternal(bool aNotifyObservers)
901 {
902   // Remove from memory and notify immediately. Since the in-memory
903   // database is authoritative, we do not need confirmation from the
904   // on-disk database to notify observers.
905   RemoveAllFromMemory();
906   if (aNotifyObservers) {
907     NotifyObservers(nullptr, NS_LITERAL_STRING("cleared").get());
908   }
910   // clear the db
911   if (mDBConn) {
912     nsCOMPtr<mozIStorageAsyncStatement> removeStmt;
913     nsresult rv = mDBConn->
914       CreateAsyncStatement(NS_LITERAL_CSTRING(
915          "DELETE FROM moz_hosts"
916       ), getter_AddRefs(removeStmt));
918     if (!removeStmt) {
919       return NS_ERROR_UNEXPECTED;
920     }
921     nsCOMPtr<mozIStoragePendingStatement> pending;
922     mozIStorageStatementCallback* cb = new DeleteFromMozHostListener(this);
923     rv = removeStmt->ExecuteAsync(cb, getter_AddRefs(pending));
926     return rv;
927   }
929   return NS_OK;
930 }

which is called from the nsPermissionManager::RemoveAll method of the nsIPermissionManager interface. Earlier in the same implementation, there are some notes in the docs mentioning a reference cycle:

211  * Note: Beware that, if you hold onto a |CloseDatabaseListener| from a
212  * |nsPermissionManager|, this will create a cycle.

So, my best understanding so far of this bug is that TorButton sets permissions.memory_only=false if extensions.torbutton.block_disk=false, which seems to cause nsPermissionManager to try to remove the preferences from the database, but nsPermissionManager::RemoveAllInternal doesn't raise any sort of error if it can't scrub and re-initialise the database, and so it only successfully runs the RemoveAllFromMemory() method, leaving the databases as they were when the preferences were written to disk.

Perhaps because the database access is async, when nsPermissionManager::RemoveAll is called, it calls nsPermissionManager::RemoveAllFromMemory, which sets the pointer for the nsPermissionManager to nullptr, and then calls nsPermissionManager::RemoveAllInternal which tries to handle updating the database asynchronously, and in the meantime TorButton is closing all the windows (a.k.a. "closing the browser") which might trigger the CloseDatabaseListener to fire, causing the database writes to stop, leaving the changes erased from memory and not from disk when "New Identity" is clicked.

Last edited 6 years ago by isis (previous) (diff)

comment:6 Changed 5 years ago by bugzilla

Severity: Normal

about:permissions was removed from FF45 ESR: (Remove about:permissions from Firefox)

Last edited 4 years ago by bugzilla (previous) (diff)

comment:7 Changed 4 months ago by gk

Keywords: gitlab-tb-torbutton added

Add magic gitlab keyword.

Note: See TracTickets for help on using tickets.