Closed Bug 1454027 Opened 2 years ago Closed 2 years ago

SameSite cookie bypass via link inside iframe

Categories

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

defect

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

(Blocks 1 open bug)

Details

(Keywords: sec-moderate, Whiteboard: [domsecurity-active][adv-main60-])

Attachments

(2 files, 3 obsolete files)

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 :(
See Also: → 1453818
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)
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)
Group: firefox-core-security
Component: Untriaged → DOM: Security
Keywords: sec-low
Product: Firefox → Core
Blocks: 792207
Blocks: samesite-cookies
No longer blocks: 792207
(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)
Flags: sec-bounty?
Does Firefox for Android have intent/deeplink that allows launching URL (like in bug 1447853)? If so, that would bypass SameSite cookie too.
Assignee: nobody → ckerschb
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P1
Whiteboard: [domsecurity-active]
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)
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)
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)
(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 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.
Flags: needinfo?(gijskruitbosch+bugs)
(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.
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 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+
Attachment #8968170 - Flags: review?(valentin.gosu) → review+
Flags: needinfo?(francois)
(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 :-)
(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)
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+
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
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?
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 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+
Attachment #8968434 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/mozilla-central/rev/8c866545bc7c
https://hg.mozilla.org/mozilla-central/rev/0e0dec87cddd
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Flags: sec-bounty? → sec-bounty+
Keywords: sec-lowsec-moderate
Whiteboard: [domsecurity-active] → [domsecurity-active][adv-main60+]
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.