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.
Marking as blocking for the branches.
Can we block scrollLeft and scrollTop cross-domain? That would fix it, but seems risky. Does anybody have any less-risky ideas?
I think that's what we should do. Yes, might break some stuff in theory, but I don't think we have a choice.
Yeah, I don't understand why we allow that at all.... We should kill it.
Sid offered to take this one!
Created attachment 465440 [details] [diff] [review] 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.
This patch is lacking tests. I'm currently writing those but wanted the code changes reviewed in the meantime.
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?
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.
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.
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 :(
Created attachment 465535 [details] 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
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.
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?
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.
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?
Created attachment 465540 [details] "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.
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.
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.
Created attachment 505521 [details] [diff] [review] 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.
Created attachment 506596 [details] [diff] [review] 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.
Jonas, whom did you ask for review?
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)
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.
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.
This doesn't affect scrollable <div>s. Only <iframe>s and <frame>s and the like.
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...
And would it make any sense to make the GoToAnchor API take the meaningfully-named flag, not an opaque boolean?
(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.
Yes, or reuse the existing presshell enum there, right?
Created attachment 506980 [details] [diff] [review] 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 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.
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
Created attachment 511572 [details] [diff] [review] 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 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
Created attachment 511577 [details] [diff] [review] Patch v2.5 Make it so
Jonas: how hard would this be to backport to 1.9.2?
DBaron: How risky would you feel that taking this patch would be on 3.6? You know this code much better than me.
Not worth the web-compat change on old branches