Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue 3419: Mitigate HSTS fingerprinting #1744

Merged
merged 2 commits into from
Mar 1, 2019
Merged

Issue 3419: Mitigate HSTS fingerprinting #1744

merged 2 commits into from
Mar 1, 2019

Conversation

jumde
Copy link
Contributor

@jumde jumde commented Feb 20, 2019 •
edited
Loading

fixes brave/brave-browser#3419

Description

HSTS super-cookies are used as a fingerprinting vector. This change disallow's third parties from setting trackable security headers like "Strict-Transport-Security", "Expect-CT", "Public-Key-Pins", "Public-Key-Pins-Report-Only"

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

  1. Navigate to https://avatars2.githubusercontent.com and disable Connections Encrypted
  2. Navigate to https://jsfiddle.net and disable Connections Encrypted (see: Disabling global HTTPS Everywhere setting does not disable HTTPSEverywhere brave-browser#3424)
  3. Clear Browsing Data
  4. Navigate to https://jsfiddle.net/x5mqb807/ and verify that Connections Encrypted is disabled
  5. Verify that content from avatars2.githubusercontent.com loads with a 301 redirect
  6. Navigate to http://avatars2.githubusercontent.com, reload http://avatars2.githubusercontent.com (not https) and verify it loads with a 307 redirect
  7. Reload https://jsfiddle.net/x5mqb807/ in the first tab and verify the image is loaded with a 307 redirect.

Before navigating to avatars2.githubusercontent.com:

screen shot 2019-02-20 at 2 07 33 pm

After navigating to avatars2.githubusercontent.com:

screen shot 2019-02-20 at 2 04 04 pm

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

@jumde jumde self-assigned this Feb 20, 2019
@jumde jumde mentioned this pull request Feb 21, 2019
Closed
@jumde jumde force-pushed the hsts_fingerprinting branch 2 times, most recently from 05d87d0 to 221a130 Compare February 21, 2019 01:16
@diracdeltas
Copy link
Member

Note to reviewers: this should match the behavior in https://github.com/brave/browser-laptop/pull/13649/files, minus the behavior where the HSTS cache is cleared on startup.

diracdeltas
diracdeltas reviewed Feb 21, 2019
View reviewed changes
browser/net/brave_network_delegate_base.cc Outdated
@@ -124,6 +151,10 @@ int BraveNetworkDelegateBase::OnHeadersReceived(
const net::HttpResponseHeaders* original_response_headers,
scoped_refptr<net::HttpResponseHeaders>* override_response_headers,
GURL* allowed_unsafe_redirect_url) {
brave::RemoveTrackableSecurityHeadersForThirdParty(request,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need to check if (!request) first?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

@jumde jumde force-pushed the hsts_fingerprinting branch 2 times, most recently from 057df34 to 824dcbe Compare February 21, 2019 06:02
iefremov
iefremov reviewed Feb 21, 2019
View reviewed changes
browser/net/brave_network_delegate_base.cc Outdated
return;
}
if (net::registry_controlled_domains::SameDomainOrHost(
request->url(), request->site_for_cookies(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use request->top_frame_origin()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to site_for_cookies, top_frame_origin might not be set every-time. (see: https://cs.chromium.org/chromium/src/net/url_request/url_request.h?type=cs&q=top_frame_origin&sq=package:chromium&g=0&l=290)

I think using GetTabURLFromRenderFrameInfo might be better. Let me know what you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

site_for_cookies is intentionally blank for nested 3rdparty frames while top_frame_origin is not. I think it may be empty for requests that are not related to any frame at all, so we can do nothing in this case.

Basically, the comment about its experimental nature is outdated, since Chromium had a successful experiment based on this value, and it will stay with us

iefremov
iefremov reviewed Feb 21, 2019
View reviewed changes
browser/net/brave_network_delegate_base.h Outdated
@@ -20,6 +27,18 @@ namespace net {
class URLRequest;
}

namespace brave {
const std::unordered_set<std::string> kTrackableSecurityHeaders = {
Copy link
Contributor

@iefremov iefremov Feb 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Objects with non-trivial destructors are generally banned by CC (as well as unordered_set), its better (but a bit wordy) to use static const base::NoDestructor<base::flat_set<base::StringPiece>>

For example https://cs.chromium.org/chromium/src/components/google/core/common/google_util.cc?q=google_util&sq=package:chromium&g=0&l=137

iefremov
iefremov reviewed Feb 21, 2019
View reviewed changes
browser/net/brave_network_delegate_base_unittest.cc Outdated
"X-XSS-Protection: 0";

scoped_refptr<HttpResponseHeaders> headers(
new HttpResponseHeaders(net::HttpUtil::AssembleRawHeaders(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong indentation, please clang-format the whole file

@iefremov
Copy link
Contributor

Had only a quick glance, will get back to the PR later today.

@jumde jumde force-pushed the hsts_fingerprinting branch 5 times, most recently from 9aba1c0 to 5ad1b0a Compare February 22, 2019 07:12
iefremov
iefremov reviewed Feb 22, 2019
View reviewed changes
browser/net/brave_network_delegate_base.h Outdated
@@ -20,6 +28,17 @@ namespace net {
class URLRequest;
}

namespace brave {
static const base::NoDestructor<base::flat_set<base::StringPiece>>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From no_destructor.h: "Furthermore, a NoDestructor should never have global scope as that may require a static initializer"

So i'd suggest to make a function that returns this set, and make NoDestructor object a static variable in this function.

browser/net/brave_network_delegate_base.h Outdated
@@ -20,6 +28,17 @@ namespace net {
class URLRequest;
}

namespace brave {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: using namespace here looks inconsistent with other code in this file. Don't really know if it is better to not use it here.

browser/net/brave_network_delegate_base_unittest.cc Outdated
request_url, net::IDLE, &test_delegate, TRAFFIC_ANNOTATION_FOR_TESTS);

request->set_top_frame_origin(url::Origin::Create(tab_url));
std::string raw_headers =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not make a constant common for all tests?

@iefremov
Copy link
Contributor

I would also suggest making a solid browser test, though I understand that this may be time consuming.

@iefremov
Copy link
Contributor

LGTM with nits.

@jumde jumde force-pushed the hsts_fingerprinting branch 2 times, most recently from ea1f6b9 to 0d6f72b Compare February 25, 2019 07:24
iefremov
iefremov reviewed Feb 25, 2019
View reviewed changes
browser/net/brave_network_delegate_base_browsertest.cc Outdated
EXPECT_TRUE(redirect_observer.has_sts_header(third_party));
}

IN_PROC_BROWSER_TEST_F(BraveNetworkDelegateBaseBrowserTest, ThirdPartySTS) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also add a test checking that nothing is blocked if tracking protection is off

iefremov
iefremov reviewed Feb 25, 2019
View reviewed changes
browser/net/brave_network_delegate_base_browsertest.cc Outdated
if (iter != sts_header_for_url.end()) {
return iter->second;
}
return false;
Copy link
Contributor

@iefremov iefremov Feb 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can even DCHECK that something is found, otherwise the test can pass if there is a typo in a subframe address, for example.

iefremov
iefremov reviewed Feb 25, 2019
View reviewed changes
browser/net/brave_network_delegate_base_browsertest.cc Outdated
}

private:
std::map<GURL, bool> sts_header_for_url;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sts_header_for_url_

iefremov
iefremov reviewed Feb 25, 2019
View reviewed changes
browser/net/brave_network_delegate_base_browsertest.cc Outdated
const net::HttpResponseHeaders* response = handle->GetResponseHeaders();
if (response) {
bool has_sts_header = response->HasHeader("Strict-Transport-Security");
sts_header_for_url.insert(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just sts_header_for_url[handle->GetURL()] = has_sts_header;
or sts_header_for_url.insert({handle->GetURL(), has_std_header});

iefremov
iefremov reviewed Feb 25, 2019
View reviewed changes
browser/net/brave_network_delegate_base_browsertest.cc Outdated
void DidFinishNavigation(NavigationHandle* handle) override {
const net::HttpResponseHeaders* response = handle->GetResponseHeaders();
if (response) {
bool has_sts_header = response->HasHeader("Strict-Transport-Security");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: const has_sts_header, const GURL third_party, etc

iefremov
iefremov reviewed Feb 25, 2019
View reviewed changes
test/BUILD.gn Outdated
@@ -283,6 +284,7 @@ test("brave_browser_tests") {
"//brave/browser/extensions/brave_extension_functional_test.h",
"//brave/browser/extensions/api/brave_shields_api_browsertest.cc",
"//brave/browser/extensions/api/brave_theme_api_browsertest.cc",
"//brave/browser/net/brave_network_delegate_base_browsertest.cc",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe merge this and the next one? Or give more descriptive names to source files: this one against hsts, etc

@iefremov iefremov self-requested a review February 26, 2019 11:32
iefremov
iefremov previously approved these changes Feb 26, 2019
View reviewed changes
Copy link
Contributor

@iefremov iefremov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@jumde jumde force-pushed the hsts_fingerprinting branch 3 times, most recently from e14dccf to 71d1158 Compare February 27, 2019 05:22
@iefremov iefremov self-requested a review February 27, 2019 20:19
iefremov
iefremov previously approved these changes Feb 27, 2019
View reviewed changes
@jumde
Issue 3419: Disallow 3rd parties from setting HSTS.
f83d939 
HSTS supercookies are a known fingerprinting vector. This change disallow's
third parties from setting security headers:

1. "Strict-Transport-Security"
2. "Expect-CT"
3. "Public-Key-Pins"
4. "Public-Key-Pins-Report-Only"

that can be used for fingerprinting.

auditors: @diracdeltas, @bbondy, @iefremov
@jumde
HSTS fingerprinting should not be tied to shield settings.
80eaac6
@iefremov iefremov self-requested a review February 28, 2019 08:03
@jumde jumde merged commit 680cb0f into master Mar 1, 2019
@mihaiplesa mihaiplesa deleted the hsts_fingerprinting branch May 13, 2019 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diracdeltas diracdeltas diracdeltas approved these changes

@iefremov iefremov iefremov approved these changes

@bbondy bbondy Awaiting requested review from bbondy

Assignees

@jumde jumde

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Mitigate HSTS fingerprinting
3 participants
@jumde @diracdeltas @iefremov