STP #403

jumde · 2018-09-05T19:57:57Z

fix brave/brave-browser#803
fix brave/brave-browser#3025
related brave/brave-browser#3888

Description

Smart Tracking Protection aims to block trackers that use storage in the first party context to track users (see: https://webkit.org/blog/8311/intelligent-tracking-prevention-2-0/ - First Party Bounce Trackers).

How it works:

Tracker list (csv) will be generated by an offline tracker that @snyderp worked on.
This list will be available as part of the Tracking Protection component. (Issue 2980: Creating an initial list of storage trackers for STP tracking-protection#26)
STP will monitor redirect chains, and on an automated redirect (no user interaction) if tracking protection is enabled , access to storage apis (cookie, local storage, session storage, indexedDB, WebSQL) will be blocked.

For more info refer to:
Spec - https://docs.google.com/document/d/1jolVinFcN4sl0z1zdIHyGdV594Yz6RY4zssnm_VJOYg/edit#

Slide deck -
https://docs.google.com/presentation/d/1eykguzVpzAUSUWibabXEwUYsxES44eN61phTQ0fsgL8/edit#slide=id.p

Please feel free to DM/email me if you have any additional questions

Submitter Checklist:

Test Plan:

Navigate to Brave's data directory and create a file: StorageTrackingProtection.dat in afalakplffnnnlkncjhbmahjfjhmlkal/1.0.9/1

$ cat StorageTrackingProtection.dat
browsertesting.orgfree.com,

Test Domains

browsertesting.freevar.com/redirect_stp.html -> browsertesting.orgfree.com/stp.html -> browsertesting.ueuo.com/stp.html -> browsertesting.freevar.com/final.html

Repro Steps

For Devs - Enable STP runtime and buildflags

In brave_browser/lib/config.js add an arg brave_stp_enabled: true
In brave_browser/package.json add --enable-smart-tracking-protection to the start script to enable stp

PR to add runtime switch to brave-browser: brave/brave-browser#3026

Feature test:

Open Brave and navigate to http://browsertesting.freevar.com/redirect_stp.html
The navigation has a 5 second time window (for demo purposes)
Once the navigation is complete. Open http://browsertesting.orgfree.com/.
Navigate to Developer Tools > Application. Verify none of the storage entries (localStorage, sessionStorage, IndexedDB, WebSQL, Cookies) are populated
Open http://browsertesting.ueuo.com
Navigate to Developer Tools > Application. All of the storage entries should be populated.
Navigate to http://browsertesting.freevar.com/. Disable Ads and Trackers blocked in shields
Navigate to http://browsertesting.freevar.com/redirect_stp.php. Storage entries should be available for orgfree.com

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

bridiver · 2018-09-18T19:08:28Z

components/brave_shields/browser/brave_resource_dispatcher_host_delegate.cc

@@ -30,6 +38,31 @@ void BraveResourceDispatcherHostDelegate::AppendStandardResourceThrottles(
    content::ResourceContext* resource_context,
    ResourceType resource_type,
    std::vector<std::unique_ptr<content::ResourceThrottle>>* throttles) {
+
+  CHECK(g_brave_browser_process->tracking_protection_service()->IsInitialized());


wy are we doing this in AppendStandardResourceThrottles? This code isn't doing anything with resource throttles

bridiver · 2018-09-18T19:08:40Z

components/brave_shields/browser/tracking_protection_service.cc

@@ -54,6 +54,14 @@ void TrackingProtectionService::Cleanup() {
  tracking_protection_client_.reset();
 }

+std::vector<std::string> TrackingProtectionService::addFirstPartyTrackers() {


Method names should be capitalized
These two methods don't really make sense to me. Why do we have add/remove instead of just providing a list?

bridiver · 2018-09-18T19:09:29Z

components/brave_shields/browser/brave_resource_dispatcher_host_delegate.cc

+  HostContentSettingsMap* contentSettingsMap = io_data->GetHostContentSettingsMap();
+
+  if (!g_brave_browser_process->tracking_protection_service()->addFirstPartyTrackers().empty()) {
+    for(std::string tracker : g_brave_browser_process->tracking_protection_service()->addFirstPartyTrackers()) {


I don't think these should go into user content settings. That will potentially change or wipe out user specified settings

these need to come from a separate content settings provider as we discussed in slack

bridiver

see comments

Comments addressed

bridiver · 2018-09-26T21:17:27Z

browser/renderer_host/brave_render_message_filter.cc

+	CHECK(g_brave_browser_process->tracking_protection_service()->IsInitialized());
+
+	bool allow_brave_shields = brave_shields::IsAllowContentSetting(
+      host_content_settings_map_, top_origin_url, top_origin_url, CONTENT_SETTINGS_TYPE_PLUGINS,


shouldn't this use the primary and secondary url for the check?

bridiver · 2018-09-26T21:19:49Z

browser/renderer_host/brave_render_message_filter.cc

+  return ChromeRenderMessageFilter::OnMessageReceived(message);
+}
+
+bool BraveRenderMessageFilter::ShouldStoreState(const GURL& origin_url, 


why are there two separate checks? ShouldStoreState in BraveRenderMessageFilter and then ShouldStoreState in TrackingProtectionService? I think TrackingProtectionService should encapsulate the whole check

bridiver · 2018-09-26T23:08:15Z

patches/chrome-browser-chrome_content_browser_client.cc.patch

+     service_manager::mojom::ServiceRequest* service_request) {
+   int id = host->GetID();
+   Profile* profile = Profile::FromBrowserContext(host->GetBrowserContext());
+-  host->AddFilter(new ChromeRenderMessageFilter(id, profile));


patching is not necessary for this, you can do a chromium src redefine

bridiver · 2018-09-26T23:14:11Z

browser/renderer_host/brave_render_message_filter.cc

+               bool* allowed) {
+	*allowed = ShouldStoreState(origin_url, top_origin_url);
+
+  	BrowserThread::PostTask(


we should call the superclass method here and just pass it a different bool if we don't care about the result. We don't want to have to keep this in sync with upstream changes

jumde · 2018-10-08T20:06:25Z

components/brave_shields/browser/tracking_protection_service.cc

+  const GURL& origin_url) {
+
+  if (!first_party_storage_trackers_initailized_) {
+    LOG(INFO) << "First party storage trackers not initialized";


INFO till the PR in tracking-protection repo is merged.

darkdh

I don't enumerate all the lints that requires <=80 chars, too many of them.
Also see other comments

chromium_src/chrome/browser/chrome_content_browser_client.cc

components/brave_shields/browser/tracking_protection_service.cc

components/brave_shields/browser/brave_shields_util.cc

components/brave_shields/browser/tracking_protection_service.cc

iefremov · 2019-04-11T16:39:55Z

components/brave_shields/browser/tracking_protection_helper.cc

+    base::PostTaskWithTraits(
+        FROM_HERE, {BrowserThread::IO},
+        base::Bind(&TrackingProtectionHelper::SetStartingSiteForRenderFrame,
+                   base::Unretained(this), handle->GetURL(),


Unretained is not allowed here, you want weak pointers

iefremov · 2019-04-11T16:45:16Z

components/brave_shields/browser/tracking_protection_helper.cc

+  }
+}
+
+void TrackingProtectionHelper::DeleteRenderFrameKey(int render_process_id,


This and other IO functions now can be free functions, not methods.

components/brave_shields/browser/tracking_protection_service.cc

bsclifton · 2019-04-16T21:31:52Z

@jumde does this need an official security review? I didn't see one. I know @diracdeltas and likely others are aware of the change, but wanted to see if we could capture something formally

bsclifton

Ran through test plan - works great! Verified it builds + all tests pass (updated PR template). Let comment about security review - let's create one if needed (even if it's just a formality). Great work 😄

@bridiver

1. Parse the FirstPartyTracker list provided by the `tracking-protection` extension. 2. On any request to Store data (Cookies, localStorage, sessionStorage, WebSQL, indexedDB) 1. Check if the Shield Settings and tracker blocking is allowed for the site that initiated the redirects 2. Check if the site is in the Storage trackers list. 3. Deny if the URL is found in the tracker list. This feature is currently gated behind a BUILD flag and a runtime flag. The flags are disabled by default auditors: @bridiver, @iefremov

bsclifton · 2019-04-18T07:12:14Z

components/brave_shields/browser/tracking_protection_service.cc

-  char* thirdPartyHosts =
-    tracking_protection_client_->findFirstPartyHosts(base_host.c_str());
+  std::string thirdPartyHosts =
+      tracking_protection_client_->findFirstPartyHosts(base_host.c_str());


this is returning char* that was allocated on heap... it gets leaked, because it's never cleaned up. Delete should still be called

A work-around for this (if you wanted to use std::string) could be:

char* thirdPartyHosts = tracking_protection_client_->findFirstPartyHosts(base_host.c_str()); std::string thirdPartyHostsStr = std::string(thirdPartyHosts); delete []thirdPartyHosts; // use thirdPartyHostsStr now instead

jumde added the pr/work-in-progress label Sep 5, 2018

jumde force-pushed the stp branch from 3e324df to ea00781 Compare September 14, 2018 00:32

jumde changed the title ~~WIP: Preventing data storage from first parties using ObservableProvider~~ WIP: STP Sep 17, 2018

bridiver reviewed Sep 18, 2018

View reviewed changes

bridiver previously requested changes Sep 18, 2018

View reviewed changes

jumde force-pushed the stp branch 5 times, most recently from 89a7aa4 to 05198eb Compare September 26, 2018 00:13

jumde force-pushed the stp branch from 05198eb to 01e7476 Compare September 26, 2018 00:26

bridiver reviewed Sep 26, 2018

View reviewed changes

jumde force-pushed the stp branch from 64c97b1 to 0577615 Compare September 27, 2018 01:27

jumde force-pushed the stp branch 2 times, most recently from 0bd9e45 to 69e39e8 Compare October 8, 2018 19:59

jumde changed the title ~~WIP: STP~~ STP Oct 8, 2018

jumde commented Oct 8, 2018

View reviewed changes

jumde force-pushed the stp branch from 69e39e8 to 14c1549 Compare October 8, 2018 20:16

jumde removed the pr/work-in-progress label Oct 8, 2018

jumde force-pushed the stp branch 2 times, most recently from f974c1b to f06f1c4 Compare October 10, 2018 16:48

bridiver requested a review from darkdh October 10, 2018 16:53

jumde force-pushed the stp branch from f06f1c4 to 9ff2eec Compare October 15, 2018 17:07

darkdh previously requested changes Oct 15, 2018

View reviewed changes

jumde force-pushed the stp branch 4 times, most recently from 945732d to 35e3501 Compare April 11, 2019 03:52

iefremov reviewed Apr 11, 2019

View reviewed changes

bridiver reviewed Apr 11, 2019

View reviewed changes

components/brave_shields/browser/tracking_protection_service.cc Show resolved Hide resolved

jumde force-pushed the stp branch from 35e3501 to fdba64c Compare April 11, 2019 20:18

iefremov self-requested a review April 11, 2019 21:11

iefremov previously approved these changes Apr 11, 2019

View reviewed changes

jumde dismissed iefremov’s stale review via fb76a8f April 14, 2019 20:29

jumde force-pushed the stp branch 4 times, most recently from 9499fae to c46cf91 Compare April 16, 2019 17:58

bsclifton previously approved these changes Apr 16, 2019

View reviewed changes

jumde dismissed bsclifton’s stale review via 2b1efd2 April 16, 2019 22:27

jumde force-pushed the stp branch 2 times, most recently from 2b1efd2 to b65057d Compare April 17, 2019 05:47

jumde added 2 commits April 17, 2019 00:06

fix lint

8375203

jumde force-pushed the stp branch from b65057d to 8375203 Compare April 17, 2019 07:07

iefremov self-requested a review April 18, 2019 05:10

iefremov approved these changes Apr 18, 2019

View reviewed changes

jumde merged commit 7cdda52 into master Apr 18, 2019

jumde added this to the 0.65.x - Nightly milestone Apr 18, 2019

bsclifton reviewed Apr 18, 2019

View reviewed changes

jumde mentioned this pull request Apr 18, 2019

Issue 4152: findFirstPartyHosts returns a value on the heap. Deleting it to avoid leaks #2262

Merged

28 tasks

NejcZdovc deleted the stp branch April 18, 2019 10:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

STP #403

STP #403

jumde commented Sep 5, 2018 •

edited by bsclifton

Loading

bridiver Sep 18, 2018

bridiver Sep 18, 2018

bridiver Sep 18, 2018

bridiver Sep 18, 2018

bridiver left a comment

bridiver Sep 26, 2018

bridiver Sep 26, 2018

bridiver Sep 26, 2018

bridiver Sep 26, 2018

jumde Oct 8, 2018

darkdh left a comment

iefremov Apr 11, 2019

iefremov Apr 11, 2019

bsclifton commented Apr 16, 2019

bsclifton left a comment

bsclifton Apr 18, 2019 •

edited

Loading

STP #403

STP #403

Conversation

jumde commented Sep 5, 2018 • edited by bsclifton Loading

Description

Submitter Checklist:

Test Plan:

Test Domains

Repro Steps

For Devs - Enable STP runtime and buildflags

Feature test:

Reviewer Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton commented Apr 16, 2019

bsclifton left a comment

Choose a reason for hiding this comment

bsclifton Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

jumde commented Sep 5, 2018 •

edited by bsclifton

Loading

bsclifton Apr 18, 2019 •

edited

Loading