Closed
Bug 1454027
Opened 6 years ago
Closed 6 years ago
SameSite cookie bypass via link inside iframe
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: sec-moderate, Whiteboard: [domsecurity-active][adv-main60-])
Attachments
(2 files, 3 obsolete files)
2.61 KB,
patch
|
valentin
:
review+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
14.54 KB,
patch
|
ckerschb
:
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. Go to https://shhnjk.azurewebsites.net/iframer.php?url=//test.shhnjk.com/open.php?url=//shhnjk.azurewebsites.net/SameSite.php 3. Click normal link Actual results: SameSite Strict cookie sent. Expected results: If victim site allows frames where attacker can place arbitrary link, SamSite Stirct cookie can be bypassed. PS: Another bug I had was Data URL issue which seems to be found by Gijs :(
Comment 1•6 years ago
|
||
So I guess here the issue is that although the toplevel is same-origin with the destination of the link, the iframe itself isn't. Does this work if the toplevel isn't same-origin with the target of the link? I assume not, correct? (needinfo for this) (In reply to Jun from comment #0) > PS: > Another bug I had was Data URL issue which seems to be found by Gijs :( Can you clarify a bit more what you mean (maybe by just filing a separate bug - dupes are cheap, we can always mark it as such if a patch in one of the other bugs fixes a root issue) ? If we're talking about my comments in bug 1453818, well - I'm not a bounty program person, and I don't see a bounty request on the bug, so I can't really speak to this, but I'll just say that the only reason I was looking at it was the bug you filed there, so I'm sure the bounty program will take into consideration any wider scope that we fix in that bug.
Flags: needinfo?(s.h.h.n.j.k)
Reporter | ||
Comment 2•6 years ago
|
||
Mine was this. https://test.shhnjk.com/iframer.php?url=data:text/html,%3Ca%20href=https://shhnjk.azurewebsites.net/SameSite.php%20target=_top%3EClick%3C/a%3E Do you think it's different than your comment on bug 1453818?
Flags: needinfo?(s.h.h.n.j.k) → needinfo?(gijskruitbosch+bugs)
Updated•6 years ago
|
Group: firefox-core-security
Component: Untriaged → DOM: Security
Keywords: sec-low
Product: Firefox → Core
Updated•6 years ago
|
Reporter | ||
Comment 3•6 years ago
|
||
(In reply to :Gijs (he/him) from comment #1) > link? I assume not, correct? (needinfo for this) I forgot to answer this. Yes, it only happens when attacker site is nested inside victim site. >If we're talking about my comments in bug 1453818, well - I'm not a bounty program person, and I don't see a bounty request on >the bug, so I can't really speak to this, but I'll just say that the only reason I was looking at it was the bug you filed there, >so I'm sure the bounty program will take into consideration any wider scope that we fix in that bug. Dan already has visibility on all the bugs so I'm not worried now :) I belieave you Mozilla :D
Flags: needinfo?(gijskruitbosch+bugs)
Updated•6 years ago
|
Flags: sec-bounty?
Reporter | ||
Comment 4•6 years ago
|
||
Does Firefox for Android have intent/deeplink that allows launching URL (like in bug 1447853)? If so, that would bypass SameSite cookie too.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → ckerschb
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P1
Whiteboard: [domsecurity-active]
Assignee | ||
Comment 5•6 years ago
|
||
Dan, the tests within this patch are only have way done. Ultimately we need tests for: * same-cross-same iframe inclusion * same-cross-same iframe navigation * and everything as well for data: URIs iframes, sandboxed iframes, etc. The whole spectrum basically. Since this needs to uplifted and also need to happen fast, I am asking for feedback now. As far as I can tell, the problem here can only occur if a cross-origin iframe navigates to a same-origin iframe, right? In that case I guess we just should take the triggeringPrincipal into account when loading iframes. Does that makes sense or am I missing something? Doing that allows us to correctly withhold same-site cookies for the testcase from comment 0. Obviously the patch does *not* address the inclusion of a data-uri-iframe, etc. yet. Only the navigation part!
Attachment #8968075 -
Flags: feedback?(dveditz)
Assignee | ||
Comment 6•6 years ago
|
||
Mark, here are various tests for navigating an iframe from cross-origin to same-origin (including data:, blob:, sandboxed iframes). I do the same for nesting yet another iframe into the cross-origin iframe. The only thing not working is the sandboxed iframe case that nests a same-origin iframe. When using postMessage I get |SecurityError: The operation is insecure.|. I tried SpecialPowers.wrap(window.parent).postMessage(), but it still doesn't work. Ideally I would like to also keep that subtest, but if it doesn't work, then I guess we have to skip this one.
Attachment #8968075 -
Attachment is obsolete: true
Attachment #8968075 -
Flags: feedback?(dveditz)
Attachment #8968169 -
Flags: review?(mgoodwin)
Assignee | ||
Comment 7•6 years ago
|
||
I am ni? a bunch of folks here. I think we simply have to also account for the nsresult which we didn't do. In case we are passing a moz-nullprincipal URI to the thirdpartyUtil, it returns an error, and I am thinking we could simply treat any of those loads as foreign. Is that a fair assumption? In addition, we need to perform another security check for loads of TYPE_SUBDOCUMENT, to make sure the load is not triggered by a cross origin site (where cross origin includes various unqiue origins; basically all nullPrincipal origins). What do you think? Does that make sense, or are we missing something?
Flags: needinfo?(valentin.gosu)
Flags: needinfo?(mgoodwin)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(francois)
Flags: needinfo?(dveditz)
Assignee | ||
Comment 8•6 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6) > The only thing not working is the sandboxed iframe case that nests a > same-origin iframe. When using postMessage I get |SecurityError: The > operation is insecure.|. I tried > SpecialPowers.wrap(window.parent).postMessage(), but it still doesn't work. > Ideally I would like to also keep that subtest, but if it doesn't work, then > I guess we have to skip this one. The problem was accessing document.cookie within a nested iframe where the parent iframe is sandboxed. I added a try-catch around that to make that particular test work again.
Attachment #8968169 -
Attachment is obsolete: true
Attachment #8968169 -
Flags: review?(mgoodwin)
Attachment #8968194 -
Flags: review?(mgoodwin)
Comment 9•6 years ago
|
||
Comment on attachment 8968170 [details] [diff] [review] bug_1454027_same_site_cookie_iframe.patch Review of attachment 8968170 [details] [diff] [review]: ----------------------------------------------------------------- I think this is fine and deals with most of it, but: 1) I think ideally we should probably have a follow-up patch that treats trusted about: pages (esp. about:newtab/about:home) as non-foreign (by making the various bits that have escapes for file: also have escapes for those specific about: URIs) 2) we should probably audit for any other places where we query ThirdPartyUtil and don't use the rv.
Updated•6 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 10•6 years ago
|
||
(In reply to :Gijs (he/him) from comment #9) > I think this is fine and deals with most of it, but: > > 1) I think ideally we should probably have a follow-up patch that treats > trusted about: pages (esp. about:newtab/about:home) as non-foreign (by > making the various bits that have escapes for file: also have escapes for > those specific about: URIs) That sounds good to me. > 2) we should probably audit for any other places where we query > ThirdPartyUtil and don't use the rv. That also sounds like a very useful exercise. Again, the reason I like this approach (at least for the purpose of uplifting to 60) is, because it doesn't interfere with any other cookie related code. The changes we are making are completely encapsulated and tied to same-site cookies. But I agree, those follow ups sound good to me.
Assignee | ||
Comment 11•6 years ago
|
||
Comment on attachment 8968170 [details] [diff] [review] bug_1454027_same_site_cookie_iframe.patch I am flagging Valentin for review in this case, but also wait with landing in case other folks have any concerns.
Attachment #8968170 -
Flags: review?(valentin.gosu)
Comment 12•6 years ago
|
||
Comment on attachment 8968194 [details] [diff] [review] bug_1454027_same_site_cookie_iframe_tests.patch Review of attachment 8968194 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!
Attachment #8968194 -
Flags: review?(mgoodwin) → review+
Updated•6 years ago
|
Attachment #8968170 -
Flags: review?(valentin.gosu) → review+
Updated•6 years ago
|
Flags: needinfo?(francois)
Comment 13•6 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #7) > In case we are passing a moz-nullprincipal URI to the thirdpartyUtil, it > returns an error, and I am thinking we could simply treat any of those loads > as foreign. Is that a fair assumption? Seems fair. We know it's not http(s):// something so how could it be same-site? > What do you think? Does that make sense, or are we missing something? Makes sense-- and we're probably missing some edgier edge case. I'm sure Jun will find it :-)
Assignee | ||
Comment 14•6 years ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #13) > Seems fair. We know it's not http(s):// something so how could it be > same-site? Right, that is the principle idea behind that approach. Given that, I'll clear the ni? for the other folks on that bug.
Flags: needinfo?(valentin.gosu)
Flags: needinfo?(mgoodwin)
Flags: needinfo?(dveditz)
Assignee | ||
Comment 15•6 years ago
|
||
I refined the tests yet once more. First, I also added baseline tests to make sure we are sending the same-site cookie in case of same-site requests. Second, I modified the caching behaviour of the test and then I realized that blobs inherit the security context (see e.g. [1]). Hence I added blob tests where the blob is created in the same site context (allows accessing same site cookies) and also in the cross site context (does not allow access to same site cookies). Carrying over r+ from mgoodwin. [1] https://searchfox.org/mozilla-central/source/dom/base/nsContentUtils.cpp#6429-6447
Attachment #8968194 -
Attachment is obsolete: true
Attachment #8968434 -
Flags: review+
Assignee | ||
Comment 16•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5d4a291089e77ebae7c57b19fa85e148b8f1618
Comment 17•6 years ago
|
||
Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8c866545bc7c Update SameSite cookie handling inside iframes.r=valentin https://hg.mozilla.org/integration/mozilla-inbound/rev/0e0dec87cddd Test SameSite cookie handling inside iframes.r=mgoodwin
Assignee | ||
Comment 18•6 years ago
|
||
Comment on attachment 8968170 [details] [diff] [review] bug_1454027_same_site_cookie_iframe.patch Approval Request Comment [Feature/Bug causing the regression]: Bug 1454027 [User impact if declined]: Possible same-site cookie bypass and hence vulnerable to CSRF attacks [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]: [Is the change risky?]: no [Why is the change risky/not risky?]: only affects same-site cookies [String changes made/needed]: no
Attachment #8968170 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 19•6 years ago
|
||
Comment on attachment 8968434 [details] [diff] [review] bug_1454027_same_site_cookie_iframe_tests.patch Approval Request Comment See Comment 18
Attachment #8968434 -
Flags: approval-mozilla-beta?
Comment 20•6 years ago
|
||
Comment on attachment 8968170 [details] [diff] [review] bug_1454027_same_site_cookie_iframe.patch thanks Christoph. approved for 60.0b14.
Attachment #8968170 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•6 years ago
|
Attachment #8968434 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 21•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8c866545bc7c https://hg.mozilla.org/mozilla-central/rev/0e0dec87cddd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 22•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/61a84d60fb96 https://hg.mozilla.org/releases/mozilla-beta/rev/65664fe48a56
status-firefox60:
--- → fixed
Updated•6 years ago
|
Flags: in-testsuite+
Updated•6 years ago
|
Flags: sec-bounty? → sec-bounty+
Keywords: sec-low → sec-moderate
Updated•6 years ago
|
Whiteboard: [domsecurity-active] → [domsecurity-active][adv-main60+]
Updated•6 years ago
|
status-firefox59:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Whiteboard: [domsecurity-active][adv-main60+] → [domsecurity-active][adv-main60-]
You need to log in
before you can comment on or make changes to this bug.
Description
•