Opened 5 weeks ago

Last modified 5 weeks ago

#32002 new task

Double-check Storage Access API for disk leaks and 3rd party cookie blocking adherence

Reported by: gk Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-disk-leak, TorBrowserTeam201910, ff68-esr
Cc: Actual Points: 0.5
Parent ID: Points: 0.2
Reviewer: Sponsor:

Description

The Storage Access API is a means to prevent trackers across origins yet to not break authentication flows and other benign 3rd party identifier usage.

We should make sure it's not using the disk in our default Tor Browser mode.

Moreover, we disable third-party cookies outright, mainly because we have not looked at the first-party isolation yet (#21905), and we should make sure this is not bypassed in non-private-browsing-mode contexts.

Child Tickets

Change History (4)

comment:1 Changed 5 weeks ago by gk

Okay, here comes the Private Browsing Mode part. The API got enabled in https://bugzilla.mozilla.org/show_bug.cgi?id=1513021 (it's desktop only for now, the mobile bug is https://bugzilla.mozilla.org/show_bug.cgi?id=1543720) but the bulk of the implementation got done in https://bugzilla.mozilla.org/show_bug.cgi?id=1469714.

The relevant method here is Document::RequestStorageAccess() (https://searchfox.org/mozilla-esr68/source/dom/base/Document.cpp#12711). It explicitly checks for Private Browsing Mode and rejects access in that case:

  if (nsContentUtils::IsInPrivateBrowsing(this)) {
    // If the document is in PB mode, it doesn't have access to its persistent
    // cookie jar, so reject the promise here.
    promise->MaybeRejectWithUndefined();
    return promise.forget();
  }

(https://searchfox.org/mozilla-esr68/source/dom/base/Document.cpp#12790ff.)

So, we are good from that point of view.

comment:2 Changed 5 weeks ago by gk

Here comes the second part. Note, we are actually using content protection in the mode where we outright disable third party cookies (see #26345 for what we did).

We are good here, too, as the code is ultimately checking the cookie pref in https://searchfox.org/mozilla-esr68/source/toolkit/components/antitracking/AntiTrackingCommon.cpp in the respective IsFirstPartyStorageAccessGrantedFor() methods and it is bailing out early in case we have network.cookie.cookieBehavior set to 1

  if (behavior == nsICookieService::BEHAVIOR_REJECT_FOREIGN ||
      behavior == nsICookieService::BEHAVIOR_LIMIT_FOREIGN) {
    // XXX For non-cookie forms of storage, we handle BEHAVIOR_LIMIT_FOREIGN by
    // simply rejecting the request to use the storage. In the future, if we
    // change the meaning of BEHAVIOR_LIMIT_FOREIGN to be one which makes sense
    // for non-cookie storage types, this may change.
    LOG(("Nothing more to do due to the behavior code %d", int(behavior)));
    *aRejectedReason = nsIWebProgressListener::STATE_COOKIES_BLOCKED_FOREIGN;
    return false;
  } 

(that's for both versions of this method where either a window or a channel is provided)
IsFirstPartyStorageAccessGrantedFor() is ultimately called from a lot of places responsible for things like LocalStorage/SessionStorage and Cookies.

I am not sure about the principal case here yet.

comment:3 Changed 5 weeks ago by gk

Actual Points: 0.5

comment:4 Changed 5 weeks ago by gk

Keywords: ff68-esr added

Adding ff68-esr keyword

Note: See TracTickets for help on using tickets.