583889 - can use scroll position of outer frame to check for existence of ids in cross-origin frames

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

You can use

<iframe src="otherdomain.com/path/page.php#foo">

together with the scrollTop/scrollLeft properties to check if an element with id "foo" exists in the targeted page. Even for cross-origin urls.

This is very bad and allows things like checking if the current user has received email from a given email address etc.

Apparently facebook has been notified about this and has deployed protection against it by completely disallowing cross-site iframes pointing to them.

This was released at blackhat and so we should assume the world knows.

Comment 1

[LegNeato]

Marking as blocking for the branches.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Can we block scrollLeft and scrollTop cross-domain?  That would fix it, but seems risky.

Does anybody have any less-risky ideas?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think that's what we should do. Yes, might break some stuff in theory, but I don't think we have a choice.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, I don't understand why we allow that at all....  We should kill it.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sid offered to take this one!

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

This should do it. I also disabled iframe.contentDocument for cross-site iframes. I'm not sure if we want to take that part on branch, but it's IMHO definitely something we should do on trunk as exposing cross-domain document nodes doesn't provide any functionality, only risk.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This patch is lacking tests. I'm currently writing those but wanted the code changes reviewed in the meantime.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I don't understand that patch.  Isn't the problem that you can get a handle to the contentWindow, then navigate it cross-site, then get scrollTop on it?  Or do scroll* on the <iframe> reflect the state of the window inside it instead of just being the size it takes up in the parent document?

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

My understanding is that iframe.scrollTop reflects the scrolling state of the contained window. It does not appear that we are exposing anything sensitive from the Window object directly.

Comment 10

[Daniel Veditz [:dveditz]]

Is this really a problem in Firefox?

The blog post below claims it's a problem in Opera.
http://soroush.secproject.com/blog/2010/06/opera-browser-scroll-information-leakage/

PoC: http://0me.me/demo/opera_scroll_leak/test_scroll.html
("Please use OPERA BROWSER only. This page does not work for you!")

This doesn't need to be hidden, it's a public issue. I think this is invalid for Firefox.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ok, dveditz figured out what's going on here. Unfortunately the fix will be much more complex so I won't be able to finish this today. Should be able to have a fix by tuesday, at which point we'll have to figure out how risky it looks.

So if needed, feel free to bump this to next dot-release :(

Comment 12

[Daniel Veditz [:dveditz]]

Attachment: scrollleak testcase (Firefox)

I misunderstood the bug, this problem definitely exists and is a problem in Firefox. A better source for the problem is Paul Stone's slides, and the issue involves nested iframes -- a tiny outer in the attackers domain and a huge inner for the victim page. The anchor causes the outer same-origin frame to scroll and that can be read.

See https://media.blackhat.com/bh-eu-10/presentations/Stone/BlackHat-EU-2010-Stone-Next-Generation-Clickjacking-slides.pdf
starting at slide 38

Comment 13

[Boris Zbarsky [:bzbarsky]]

The attached patch won't help with that...

Can we just stop going up the tree when scrolling when we cross origins?  Suboptimal UI, but I see no other way to deal with this.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Indeed, the current patch does the entirely wrong thing.

Yeah, the safest approach would be to always break on cross-origin frames when walking up the frame tree in order to scroll something visible.

A more conservative approach would be to only do this when doing the initial scroll to a selected anchor. However that would probably let through cases when sites use the new autofocus attribute or similar things.

Do we know what other browsers do?

Comment 15

[Daniel Veditz [:dveditz]]

This affects multiple browsers. The testcase as written works in FF and Safari (on mac). Chrome doesn't like accessing the data: frame (and IE doesn't support data: at all) but when re-written to document.write() the contents Chrome seems to exhibit the bug. IE doesn't seem to support the scrollX/scrollY properties. I don't know if they plan to in the future or if they currently support an equivalent.

Although a fix would be nice, I don't think it should block the current set of releases because we have to think about how incompatible we want to be or if we can come up with a standardized "HTML 5" answer to this.

Comment 16

[Daniel Veditz [:dveditz]]

Note that all browsers visually scroll the frames the same way. I wouldn't necessarily mind not obeying cross-frame scrolls because it's at the heart of a lot of clickjacking techniques, but if we do so do it knowing our browser will be different.

Is there a legitimate case where someone can be relying on this kind of nested frame scrolling behavior?

Comment 17

[Daniel Veditz [:dveditz]]

Attachment: "scroll leak" testcase (Chrome, Safari)

This version also "works" in Chrome, and at least shows the inner frames scrolled to the right position in IE8.

Comment 18

[Elie Bursztein]

The other browsers haven't fixed it yet. They are all vulnerable to this. We have a proof of concept code that I can submit if it helps.

Comment 19

[Daniel Veditz [:dveditz]]

In mail Elie says it can be made to work on IE as well. Their testcase does use scrollTop/ScrollLeft so let's just say 'scroll position' generically.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Old approach

Attaching this for posterity. I started a thorough audit of all places where we scroll something into view and made sure each specified if we should scroll parent frames or not.

This is the approach we should take, but not for FF4. It's too big of a change too late in the game.

Instead I've started working on a patch to only fix iframe scrolling via .src hashes as that is by far the most effective attack.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

First, here is a short summary of the attack:

1. Create a webpage with an iframe (A).
2. Point iframe A to page inner.html.
3. Make inner.html contain an iframe (B) in the upper left corner, as well as
   a <div> which is large enough that iframe A gets scrollbars in both
   directions.
4. Scroll iframe A to the lower right corner.
5. Point iframe B to http://bunnies.com/#anid
6. Wait for iframe B to load.
7. Check scroll position of iframe A.

If an element with id "anid" exists in http://bunnies.com/, then iframe A will have scrolled to the upper left corner. Otherwise it will still be scrolled to the lower right corner.

The attached patch fixes this attack.


However it's possible that there are other ways that http://bunnies.com/ could cause iframe A to scroll, this patch does not attempt to fix those. We need to do a thorough audit, but that'll have to happen later. That's the audit I attempted to start in the previous patch.

I believe that the attached patch fixes most websites.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Jonas, whom did you ask for review?

Comment 23

[Paul Stone]

From my limited understanding of the patch, it looks like it causes parent frames to never scroll, even if all frames belong to the same domain? If that's correct, it seems like it could cause unintended breakages on sites that use lots of nested frames (e.g. API documentation)

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Do you know of any sites which depend on the fact that navigating an inner iframe scrolls outer frames?

I don't feel super strongly either way, but dbaron asked that we don't apply different rules for cross-site and same-site, which I agree is a laudable goal.

Comment 25

[Paul Stone]

Do <div>s with overflow=scroll/auto/hidden count as frames in this case? Comments on forums and blogs are often contained in such divs in order to protect the layout of the site. Navigating to a particular comment would require scrolling the 'parent' frame if the anchor tag was within the div. 

As for real iframes, its harder to think of real cases so that may be ok - I guess testing in the nightlies is the best way to find out.

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This doesn't affect scrollable <div>s. Only <iframe>s and <frame>s and the like.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Jonas, we need to talk to Anne about the same-origin thing.

It's probably ok to treat same vs different origin the same for this use case, but for things like DOM scroll APIs we probably do want the origin distinction...

Comment 28

[Boris Zbarsky [:bzbarsky]]

And would it make any sense to make the GoToAnchor API take the meaningfully-named flag, not an opaque boolean?

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #28)
> And would it make any sense to make the GoToAnchor API take the
> meaningfully-named flag, not an opaque boolean?

You mean make it take an enum? If that's preferred I can definitely do that.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Yes, or reuse the existing presshell enum there, right?

Comment 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v1.1

This one uses a explicit enum rather than another boolean flag.

It feels sort of silly that GoToAnchor now takes a boolean flag as well as an enum with flags. But it also seemed strange to add a DONT_SCROLL enum value for enums that are passed to ScrollToContent and similar functions.

In all this definitely seems like a nicer approach though.

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 506980 [details] [diff] [review]
Patch v1.1

Is this new GoToAnchor method solely to avoid changing the semantics 
of nsIPresShell::GoToAnchor, or is there some other reason?  Why not
just change the scroll parameter passed in PresShell::GoToAnchor and 
ScrollToAnchor, and skip the new method and 
mLastAnchorScrolledToFlags?

need to rev IID NS_IPRESSHELL_MOZILLA_2_0_BRANCH_IID

There's something weird about the indentation in
layout/base/tests/Makefile.in .  You should be consistent about tabs
or not-tabs.

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Jonas and I also discussed that, in the long term:
 - some operations should scroll just one container
 - some operations should scroll all scrollable things in the document
 - some operations should scroll all ancestor documents too

Comment 34

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v2, fixed review comments

The new signature is not actually needed. Just change the behavior of the old one. All callers in the tree should behave the same as with the previous patch.

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 511572 [details] [diff] [review]
Patch v2, fixed review comments

r=dbaron if you also get rid of mLastAnchorScrolledToFlags, which looks like it's always a constant

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v2.5

Make it so

Comment 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in!

http://hg.mozilla.org/mozilla-central/rev/69e9525dff19

Comment 38

[Daniel Veditz [:dveditz]]

Jonas: how hard would this be to backport to 1.9.2?

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

DBaron: How risky would you feel that taking this patch would be on 3.6? You know this code much better than me.

Comment 40

[Emanuel Kleinebekel]

I hope someone is reading this because your "fix" causes some problems on "legit" use of anchosrs in big inner frames and small outer frames... maybe you read my bug request bug 667859 

small summery:

if klicking an anchor (in the big inner frame) will scroll the small outer frame the attacker got no information of what anchor was klicked. Ok he can now find out, that the iframe (or in this case the small outer frame) was scrolled but not more!?!?!?!?!?! Anchors are client side and no javascript code could find out what anchor was klickt in the frame especially not on other domain iframes. 

So klicking and directly inserting are 2 different things and must be treat like this!!

Comment 41

[Daniel Veditz [:dveditz]]

Not worth the web-compat change on old branches