1453814 - SameSite cookie bypass via redirect

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Jun (Request needinfo for comment, I ignore other updates)]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36

Steps to reproduce:

1. Go to https://shhnjk.azurewebsites.net/SameSite.php (Sets SameSite cookie)
2. Copy https://test.shhnjk.com/location.php?url=https://shhnjk.azurewebsites.net/SameSite.php and paste it to new tab


Actual results:

SameSite Strict cookie sent


Expected results:

Redirect from attacker site to victim site shouldn't send SameSite Strict cookie. Reproed on Nightly.

Comment 1

[:Gijs (he/him)]

Mark/Christoph, I hear you know about our samesite cookie implementation. Can you take a look?

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Yep, that is a problem - thanks for filing!

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1453814_samesite_redirect.patch

With this patch applied I get: "No cookie received" when trying to reproduce the STR from comment 0.

@Valentin: We have to explicitly check the redirect chain of the loadinfo in the light of same-site cookies because cross-origin to same-origin redirect might be a CSRF attack.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1453814_samesite_redirect_tests.patch

Mark, here are some tests for same-origin to cross-origin redirect as well as cross-origin to same-origin redirect making sure we are not sending any same-site cookies.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8967718 [details] [diff] [review]
bug_1453814_samesite_redirect.patch

Review of attachment 8967718 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsNetUtil.cpp
@@ +2181,5 @@
> +      redirectPrincipal->GetURI(getter_AddRefs(redirectURI));
> +      thirdPartyUtil->IsThirdPartyChannel(aChannel, redirectURI, &isForeign);
> +      // if at any point we encounter a cross-origin redirect we can return.
> +      if (isForeign) {
> +        return isForeign;

return true;
here and also above.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Dan, does this need to be a security bug or can we open that one up?

Comment 7

[Jun (Request needinfo for comment, I ignore other updates)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> Dan, does this need to be a security bug or can we open that one up?

Is SameSite cookie not a security feature? I will wait for Dan's reply but I have some more bugs. Let me know if SameSite cookie is in-scope of bounty. Thanks.

Comment 8

[Daniel Veditz [:dveditz]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> Dan, does this need to be a security bug or can we open that one up?

It can be both. Actual risk for Firefox users _today_ is basically nil because no one relies on this feature (only Chrome supports it) so there's no need to keep the bug hidden. But in terms of the CSRF attacks samesite cookies were supposed to prevent this bug means it's effectively no protection so sec-moderate seems fair.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d37ff0c232e
Treat any cross-origin redirects as foreign for same-site cookies. r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/86a4c50c98f6
Test redirects and same-site cookies. r=mgoodwin

Comment 10

[Andreea Pavel [:apavel]]

Backed out for failing dom/base/test/chrome/test_bug884693.xul 

Push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=86a4c50c98f64dd8b86900274147e7ca2855e586

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=173553591&repo=mozilla-inbound&lineNumber=166071

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/c945e6856a0032ceaa51870da7081b95fe07a1d1

Comment 11

[Andreea Pavel [:apavel]]

I apologize for the backout, it was a mistake. Relanding this now.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae395d5568c4
Treat any cross-origin redirects as foreign for same-site cookies. r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6f674d433fc
Test redirects and same-site cookies. r=mgoodwin on a CLOSED TREE

Comment 13

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/ae395d5568c4
https://hg.mozilla.org/mozilla-central/rev/a6f674d433fc

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8967718 [details] [diff] [review]
bug_1453814_samesite_redirect.patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1453814
[User impact if declined]: no same-site cookie support on 60
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: Bug 1452496 should be applied first
[Is the change risky?]: no
[Why is the change risky/not risky?]: same-site cookie code is encapsulated
[String changes made/needed]: no

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8967719 [details] [diff] [review]
bug_1453814_samesite_redirect_tests.patch

Approval Request Comment
See comment 14

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 8967718 [details] [diff] [review]
bug_1453814_samesite_redirect.patch

same-site cookies followup, approved for 60.0b13

Comment 17

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a0774aff327c
https://hg.mozilla.org/releases/mozilla-beta/rev/8249c87c7dd7

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.