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.

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

Strip xorigin navigation referrers instead of spoofing #1767

Merged
merged 1 commit into from
Mar 23, 2019

Conversation

fmarier
Copy link
Member

@fmarier fmarier commented Feb 22, 2019

Fixes brave/brave-browser#3422.

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
  • Verified that all lint errors/warnings are resolved (npm run lint)
  • 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:

This can be manually tested using https://fmarier.github.io/brave-testing/referrer-spoofing.html.

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

@fmarier fmarier added this to the 0.63.x - Nightly milestone Feb 22, 2019
@fmarier fmarier self-assigned this Feb 22, 2019
@fmarier fmarier requested a review from diracdeltas February 22, 2019 22:55
@diracdeltas diracdeltas requested a review from bridiver February 23, 2019 00:58
@diracdeltas
Copy link
Member

currently building on MacOS. thanks for making a test page. adding @bridiver for review

@diracdeltas
Copy link
Member

diracdeltas commented Feb 25, 2019

on https://fmarier.github.io/brave-testing/referrer-spoofing.html:

  1. subresource referrer can't be checked using devtools since it only shows provisional headers. does anyone know of another way? (other than using a MITM proxy). EDIT: no longer needed, i made a jsfiddle for this
  2. on same origin navigation to https://fmarier.github.io/brave-testing/blank.html, referer header shows up as https://fmarier.github.io/brave-testing/referrer-spoofing.html. same with document.referrer. 👍
  3. on cross-origin navigation to https://www.whatismyreferer.com/, referer header is blank and so is document.referrer. 👍

@diracdeltas
Copy link
Member

diracdeltas commented Feb 25, 2019

i made a test page for subresource referer: https://jsfiddle.net/r4ygLsup/

expected result with spoofing:

  1. the result iframe shows https://fiddle.jshell.net/ (instead of something from jsfiddle.net)
  2. the iframe within the result iframe shows https://www.whatismyreferer.com/

I got the expected results on current stable release. with this PR:

  1. shows the full URL, https://fiddle.jshell.net/r4ygLsup/show/, instead of just the origin
  2. shows 'no referrer / hidden'

not sure if the difference is due to this PR or changed sometime before

EDIT: tried this with cf71d56 reverted and got the same result as on stable

@fmarier
Copy link
Member Author

fmarier commented Feb 26, 2019

  1. subresource referrer can't be checked using devtools since it only shows provisional headers. does anyone know of another way? (other than using a MITM proxy). EDIT: no longer needed, i made a jsfiddle for this

I was cheating for that one since I looked at the Apache access log :)

a.b.c.d - - [26/Feb/2019:01:31:27 +0000] "GET /img/francois_marier.jpg HTTP/2.0" 200 6385 "https://fmarier.org/" "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/73.0.3683.39 Safari/537.36"

but your idea of looking at document.referrer is much better.

I updated my test page to include that and also the iframe trick for sub-resources.

@fmarier
Copy link
Member Author

fmarier commented Feb 26, 2019

Using my updated test page, the three test cases are as expected in my branch:

  1. same-origin navigations are still untouched,
  2. cross-origin navigations are now truncated, and
  3. cross-origin resources are still spoofed.

and as expected on current release, beta and dev:

  1. same-origin navigations are untouched,
  2. cross-origin navigations are spoofed, and
  3. cross-origin resources are spoofed.

@fmarier
Copy link
Member Author

fmarier commented Feb 26, 2019

I added two more test cases to my test page to cover same-origin iframes and cross-origin iframes, as leaked via document.referer and the results are:

  • referrers are spoofed on release, and
  • they are truncated with my patch.

Now that I think of it, should cross-origin iframes be spoofed or truncated? They're not top-level navigations, but aren't navigations too?

@fmarier
Copy link
Member Author

fmarier commented Feb 26, 2019

In other words, do we agree that the expected values I put down on https://fmarier.github.io/brave-testing/referrer-spoofing.html are correct after this patch is applied?

The only difference with current release is that anything that's expected to be blank on the test page is currently spoofed without my patch.

@diracdeltas
Copy link
Member

@fmarier i agree https://fmarier.github.io/brave-testing/referrer-spoofing.html shows the correct values with this patch. however the first test case in https://jsfiddle.net/yg29vnLt/ is still confusing me (i updated the jsfiddle to have better labels); instead of showing no referrer, it shows https://fiddle.jshell.net/yg29vnLt/show/

@fmarier
Copy link
Member Author

fmarier commented Feb 26, 2019

however the first test case in https://jsfiddle.net/yg29vnLt/ is still confusing me (i updated the jsfiddle to have better labels); instead of showing no referrer, it shows https://fiddle.jshell.net/yg29vnLt/show/

You're right, that makes no sense. I will dig into that test case.

@fmarier
Copy link
Member Author

fmarier commented Mar 2, 2019

@diracdeltas I made a simplified version of your jsfiddle which still exhibits the same issue: https://jsfiddle.net/06oh25xy/

As far as I can tell, the problem is that the test case in jsfiddle is different when we spoof v. strip. When we strip the referrer, it's no longer not a single iframe, but rather an iframe within a same-origin iframe.

On release (spoofing), when loading https://jsfiddle.net/06oh25xy/ I see this in the devtools:

https://jsfiddle.net/06oh25xy/ -> https://fiddle.jshell.net/06oh25xy/show/

With my patch (stripping), I see the following instead:

https://jsfiddle.net/06oh25xy/ -> https://fiddle.jshell.net/06oh25xy/show/ -> https://fiddle.jshell.net/06oh25xy/show/light/

In fact, we can tell visually that we're not getting the same payload. This is the result iframe without my patch:

without-my-patch

and this is the same result iframe with my patch:

with-my-patch

(notice the extra "tab" bar)

@diracdeltas
Copy link
Member

@fmarier interesting! are you saying the site inserts an extra iframe when the referer is stripped out?

that's definitely not a bug with this patch then

@fmarier
Copy link
Member Author

fmarier commented Mar 2, 2019

are you saying the site inserts an extra iframe when the referer is stripped out?

Indeed. This is what it looks like on the jsfiddle site,

$ curl -H "Referer: https://jsfiddle.net/" https://fiddle.jshell.net/06oh25xy/show/ 
<!DOCTYPE html>
<html>
<head>
  <meta http-equiv="content-type" content="text/html; charset=UTF-8">
  <title></title>
  <meta http-equiv="content-type" content="text/html; charset=UTF-8">
  <meta name="robots" content="noindex, nofollow">
  <meta name="googlebot" content="noindex, nofollow">
  <meta name="viewport" content="width=device-width, initial-scale=1">


  <script
    type="text/javascript"
    src="/js/lib/dummy.js"
    
  ></script>

    <link rel="stylesheet" type="text/css" href="/css/result-light.css">


  <style id="compiled-css" type="text/css">
      
  </style>


  <!-- TODO: Missing CoffeeScript 2 -->

  <script type="text/javascript">


    window.onload=function(){
      
referer.innerText = document.referrer || 'None / hidden'

    }

</script>

</head>
<body>
    <div>
1. referrer of cross-origin iframe (jsfiddle.net -> fiddle.jshell.net): 
</div>
<div id='referer'>
</div>


  
  <script>
    // tell the embed parent frame the height of the content
    if (window.parent && window.parent.parent){
      window.parent.parent.postMessage(["resultsFrame", {
        height: document.body.getBoundingClientRect().height,
        slug: "06oh25xy"
      }], "*")
    }

    // always overwrite window.name, in case users try to set it manually
    window.name = "result"
  </script>
</body>
</html>

and this is what it looks like without a referrer:

$ curl https://fiddle.jshell.net/06oh25xy/show/
<!DOCTYPE HTML>
<html>
  <head>
    <meta charset="utf-8">
    <meta http-equiv="content-type" content="text/html; charset=UTF-8">
    <meta http-equiv="edit-Type" edit="text/html; charset=utf-8">
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <title>JSFiddle</title>

      <meta name="robots" content="index, follow">
      <meta name="googlebot" content="index, follow">

      <script src="/js/embed/highlight.pack.js"></script>
      <script src="/js/embed/embed.js"></script>
      <link rel="stylesheet" media="screen" href="/css/embed/embed-light.css" />

      <script type="text/javascript">
        var height;
        var force_height = null;
        var slug = "06oh25xy";
        var show_src = "//fiddle.jshell.net/06oh25xy/show/light/";
        var resize_element_counter = 0;
        var shell_edit_url = "/06oh25xy/light/";

      </script>

    <link rel="stylesheet" media="screen" href="//fonts.googleapis.com/css?family=Inconsolata" />

    <style type="text/css" media="screen">
    </style>

  </head>
  <body>
    <div id="wrapper">
        <header>
          <h1><a href="//jsfiddle.net/06oh25xy/?utm_source=website&amp;utm_medium=embed&amp;utm_campaign=06oh25xy" target="_blank">Edit in JSFiddle</a></h1>
          <div id="actions">
            <ul class="normalRes">
                <li class=&quot;active&quot;>
                  <a data-trigger-type="result" href="#Result">Result</a>
                </li>
            </ul>
            <div class="hl"></div>
          </div>
        </header>

        <div id="tabs">
              <div class="tCont result active" id="result"></div>
                <script type="text/javascript">
                  window.addEventListener('load', function(){
                    if (typeof(EmbedManager) === undefined){
                        EmbedManager.loadResult();
                    }
                  }, false);
                </script>
        </div>
    </div>
  </body>
</html>

diracdeltas
diracdeltas previously approved these changes Mar 4, 2019
@iefremov
Copy link
Contributor

iefremov commented Mar 4, 2019

I'd suggest to (a) at least expand current scenarios with checking same-origin vs cross-origin (b) if we have enough time, add a test against not clearing referrer for resource requests and a test against proper value in the HTTP referrer header

@fmarier fmarier force-pushed the issue3422 branch 3 times, most recently from ba29220 to 0079463 Compare March 7, 2019 01:26
@fmarier fmarier force-pushed the issue3422 branch 2 times, most recently from a1874d9 to a017a26 Compare March 16, 2019 04:07
@fmarier fmarier requested a review from iefremov March 16, 2019 04:08
@fmarier
Copy link
Member Author

fmarier commented Mar 16, 2019

@iefremov I've made the changes that @bridiver requested and I've added all of the tests that you suggested.

@fmarier fmarier removed this from the 0.63.x - Dev milestone Mar 16, 2019
@fmarier fmarier added this to the 0.64.x - Nightly milestone Mar 16, 2019
}
}

std::string last_referrer(const GURL& url) const {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this getter looks like a propery, this is slightly misleading. I'd suggest to rename last_referrer_ -> last_referrers_ and last_referrer() -> GetLastReferrer()


std::string last_referrer(const GURL& url) const {
base::AutoLock auto_lock(last_referrer_lock_);
std::map<GURL, std::string>::const_iterator pos =
Copy link
Contributor

@iefremov iefremov Mar 22, 2019

Choose a reason for hiding this comment

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

const auto pos

std::map<GURL, std::string>::const_iterator pos =
last_referrer_.find(url);
if (pos == last_referrer_.end()) {
return "(missing)"; // Fail test if we haven't seen this URL before.
Copy link
Contributor

Choose a reason for hiding this comment

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

you can just use FAIL() << "Diagnostic message";

Copy link
Member Author

Choose a reason for hiding this comment

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

That's indeed much better. In this case it doesn't actually compile if I use FAIL() because it's inside a function with a non-void return value.

@@ -499,8 +563,13 @@ IN_PROC_BROWSER_TEST_F(BraveContentSettingsObserverBrowserTest, AllowReferrer) {

EXPECT_STREQ(ExecScriptGetStr(kReferrerScript,
contents()).c_str(), "");
EXPECT_EQ(last_referrer(url()), "");
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 use EXPECT_TRUE(last_referrer(url()).empty());

@@ -499,8 +563,13 @@ IN_PROC_BROWSER_TEST_F(BraveContentSettingsObserverBrowserTest, AllowReferrer) {

EXPECT_STREQ(ExecScriptGetStr(kReferrerScript,
contents()).c_str(), "");
EXPECT_EQ(last_referrer(url()), "");
EXPECT_EQ(ExecScriptGetStr(create_image_script(), contents()),
Copy link
Contributor

Choose a reason for hiding this comment

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

expected argument typically comes first, actual - second

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks that's good to know.

I think it will leave them as they are right now because the whole file seems to be doing it wrong and fixing it everywhere would bloat my PR quite a bit.

EXPECT_EQ(last_referrer(url()), "");
EXPECT_EQ(ExecScriptGetStr(create_image_script(), contents()),
image_url().spec());
EXPECT_EQ(last_referrer(image_url()), image_url().GetOrigin().spec());
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we can add some comments about what is going on here

@iefremov
Copy link
Contributor

@fmarier Good job 👍 LGTM with small nits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Referrer spoofing could disable login CSRF protections on some sites
4 participants