Closed
Bug 1453814
Opened 6 years ago
Closed 6 years ago
SameSite cookie bypass via redirect
Categories
(Core :: DOM: Security, defect, P1)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox59 | --- | unaffected |
firefox60 | + | fixed |
firefox61 | --- | fixed |
People
(Reporter: s.h.h.n.j.k, Assigned: ckerschb)
References
Details
(Keywords: regression, sec-moderate, Whiteboard: [domsecurity-active])
Attachments
(2 files)
1.83 KB,
patch
|
valentin
:
review+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
6.62 KB,
patch
|
mgoodwin
:
review+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•6 years ago
|
Comment 1•6 years ago
|
||
Mark/Christoph, I hear you know about our samesite cookie implementation. Can you take a look?
Flags: needinfo?(mgoodwin)
Flags: needinfo?(ckerschb)
Assignee | ||
Comment 2•6 years ago
|
||
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]
Assignee | ||
Comment 3•6 years ago
|
||
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)
Assignee | ||
Comment 4•6 years ago
|
||
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 5•6 years ago
|
||
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+
Assignee | ||
Updated•6 years ago
|
Blocks: samesite-cookies
Updated•6 years ago
|
Attachment #8967719 -
Flags: review?(mgoodwin) → review+
Assignee | ||
Comment 6•6 years ago
|
||
Dan, does this need to be a security bug or can we open that one up?
Flags: needinfo?(dveditz)
Updated•6 years ago
|
status-firefox59:
--- → unaffected
status-firefox60:
--- → affected
status-firefox61:
--- → affected
status-firefox-esr52:
--- → unaffected
tracking-firefox60:
--- → +
Reporter | ||
Comment 7•6 years 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.
Comment 8•6 years 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? 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
Updated•6 years ago
|
Group: core-security
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
Comment 10•6 years ago
|
||
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
Flags: needinfo?(ckerschb)
Comment 11•6 years ago
|
||
I apologize for the backout, it was a mistake. Relanding this now.
Flags: needinfo?(ckerschb)
Comment 12•6 years 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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ae395d5568c4 https://hg.mozilla.org/mozilla-central/rev/a6f674d433fc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
Flags: sec-bounty?
Assignee | ||
Comment 14•6 years ago
|
||
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?
Assignee | ||
Comment 15•6 years ago
|
||
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 16•6 years ago
|
||
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+
Updated•6 years ago
|
Attachment #8967719 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 17•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/a0774aff327c https://hg.mozilla.org/releases/mozilla-beta/rev/8249c87c7dd7
Updated•6 years ago
|
Flags: in-testsuite+
Updated•6 years ago
|
Flags: sec-bounty? → sec-bounty+
Comment 19•4 years ago
|
||
Bugbug thinks this bug is a regression, but please revert this change in case of error.
Keywords: regression
You need to log in
before you can comment on or make changes to this bug.
Description
•