SameSite cookie bypass via redirect

RESOLVED FIXED in Firefox 60

Status

()

defect
P1
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: s.h.h.n.j.k, Assigned: ckerschb)

Tracking

(Blocks 1 bug, {sec-moderate})

unspecified
mozilla61
Points:
---
Bug Flags:
sec-bounty +
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox59 unaffected, firefox60+ fixed, firefox61 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(2 attachments)

(Reporter)

Description

a year ago
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

a year ago
Mark/Christoph, I hear you know about our samesite cookie implementation. Can you take a look?
Flags: needinfo?(mgoodwin)
Flags: needinfo?(ckerschb)
Yep, that is a problem - thanks for filing!
Assignee: nobody → ckerschb
Group: firefox-core-security → core-security
Status: UNCONFIRMED → ASSIGNED
Component: Untriaged → DOM: Security
Ever confirmed: true
Flags: needinfo?(mgoodwin)
Flags: needinfo?(ckerschb)
Priority: -- → P1
Product: Firefox → Core
Whiteboard: [domsecurity-active]
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.
Attachment #8967718 - Flags: review?(valentin.gosu)
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.
Attachment #8967719 - Flags: review?(mgoodwin)
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.
Attachment #8967718 - Flags: review?(valentin.gosu) → review+
Attachment #8967719 - Flags: review?(mgoodwin) → review+
Dan, does this need to be a security bug or can we open that one up?
Flags: needinfo?(dveditz)
(Reporter)

Comment 7

a year ago
(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.
(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.
Flags: needinfo?(dveditz)
Keywords: sec-moderate
Group: core-security

Comment 9

a year ago
Pushed by mozilla@christophkerschbaumer.com:
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
I apologize for the backout, it was a mistake. Relanding this now.
Flags: needinfo?(ckerschb)

Comment 12

a year ago
Pushed by apavel@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ae395d5568c4
https://hg.mozilla.org/mozilla-central/rev/a6f674d433fc
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Flags: sec-bounty?
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
Attachment #8967718 - Flags: approval-mozilla-beta?
Comment on attachment 8967719 [details] [diff] [review]
bug_1453814_samesite_redirect_tests.patch

Approval Request Comment
See comment 14
Attachment #8967719 - Flags: approval-mozilla-beta?
Comment on attachment 8967718 [details] [diff] [review]
bug_1453814_samesite_redirect.patch

same-site cookies followup, approved for 60.0b13
Attachment #8967718 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8967719 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: in-testsuite+
(Assignee)

Updated

a year ago
See Also: → 1456407
Flags: sec-bounty? → sec-bounty+
(Assignee)

Updated

11 months ago
See Also: → 1465402
You need to log in before you can comment on or make changes to this bug.