Closed
Bug 593387
Opened 14 years ago
Closed 14 years ago
"Get Add-ons" pane is being restricted by X-FRAME-OPTIONS
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
VERIFIED
FIXED
mozilla2.0b7
| Tracking | Status | |
|---|---|---|
| blocking2.0 | --- | betaN+ |
People
(Reporter: KWierso, Assigned: bsterne)
References
Details
(Whiteboard: [4b5])
Attachments
(3 files, 3 obsolete files)
|
264.94 KB,
image/png
|
Details | |
|
3.25 KB,
patch
|
bsterne
:
review+
|
Details | Diff | Splinter Review |
|
6.61 KB,
patch
|
bsterne
:
review+
|
Details | Diff | Splinter Review |
screenshot
|
||
Comment 1•14 years ago
|
||
This feels like a bug in the X-FRAME-OPTIONS implementation, since we are a chrome privileged page loading content in browser element.
|
|
||
Comment 2•14 years ago
|
||
Blocks last beta I think, though I think it'd be really really good to see a fix somehow for b6, maybe involving removing the header from AMO till this is done.
blocking2.0: --- → betaN+
|
||
Updated•14 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
|
||
Updated•14 years ago
|
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk
|
|
||
Comment 5•14 years ago
|
||
Sadly this prevents us also in Beta5 and down to Beta3 to show the one and only location where the Addons Compatibility Reporter is promoted. I would expect more negative via feedback.
Assignee: bsterne → nobody
Whiteboard: [4b5]
|
|
||
Updated•14 years ago
|
Assignee: nobody → bsterne
|
Reporter | |
Comment 6•14 years ago
|
||
Oddly, if the "Get Add-ons" pane is pre-selected when the addon manager loads (previously open the AOM, click "get addons", close the tab, reopen the tab), I get this page loading.
|
|
||
Comment 7•14 years ago
|
||
(In reply to comment #6) > Created attachment 471899 [details] > screenshot > > Oddly, if the "Get Add-ons" pane is pre-selected when the addon manager loads > (previously open the AOM, click "get addons", close the tab, reopen the tab), I > get this page loading. That is the unrelated bug 584693
|
Assignee | |
Comment 8•14 years ago
|
||
Add a system principal check. Boris, perhaps you can tell me the most efficient way to get a principal from a nsIDOMWindow. I'll also add a test.
Attachment #471922 -
Flags: review?(bzbarsky)
|
|
||
Comment 9•14 years ago
|
||
Comment on attachment 471922 [details] [diff] [review] fix >+ nsCOMPtr<nsIDOMDocument> topDOMDoc; >+ topWindow->GetDocument(getter_AddRefs(topDOMDoc)); >+ nsCOMPtr<nsINode> node = do_QueryInterface(topDOMDoc); >+ nsCOMPtr<nsIPrincipal> topPrincipal = node->NodePrincipal(); nsCOMPtr<nsIScriptObjectPrincipal> objPrin = do_QueryInterface(topWindow); nsIPrincipal* topPrincipal = objPrin->GetPrincipal(); >+ if (nsContentUtils::IsSystemPrincipal(topPrincipal)) { This should be a subsumes() check. Or more interest to me.... Why is this a problem at all? Are we seriously loading content off the web in a chrome-type iframe, and if so, why?
Attachment #471922 -
Flags: review?(bzbarsky) → review-
|
|
Assignee | |
Comment 10•14 years ago
|
||
(In reply to comment #9) > Or more interest to me.... Why is this a problem at all? Are we seriously > loading content off the web in a chrome-type iframe, and if so, why? The Add-ons Manager folks will have to speak to that, but in the meantime here's the patch with the Subsumes check. I'm still having trouble with the test, but I'll see if I can get attached to the bug relatively soon.
Attachment #471922 -
Attachment is obsolete: true
Attachment #471988 -
Flags: review?(bzbarsky)
|
|
||
Comment 11•14 years ago
|
||
Oh, wait. I guess this is about the add-ons-manager-in-browser-tab case? In that situation, I can see why this is being a problem....
|
|
||
Comment 12•14 years ago
|
||
Two comments on the patch:
1) If a chrome page in a content docshell loads site A which then frames site B, the framing will be allowed with this patch. Is this desired?
2) Getting the principal of the current document can just be done with
NodePrincipal(); no need to get it off thisWindow.|
|
||
Comment 14•14 years ago
|
||
Comment on attachment 471988 [details] [diff] [review] fix v2 Pending answers to the questions.
Attachment #471988 -
Flags: review?(bzbarsky) → review-
|
|
Assignee | |
Comment 15•14 years ago
|
||
(In reply to comment #12) > Two comments on the patch: > 1) If a chrome page in a content docshell loads site A which then frames site > B, the framing will be allowed with this patch. Is this desired? Good point. I'm not sure how often we have chrome top-level page framing web content, but this would happen in those cases. Perhaps I should get DocShells and call GetSameTypeRootTreeItem()? > 2) Getting the principal of the current document can just be done with > NodePrincipal(); no need to get it off thisWindow. Yep, I'll fix this for the next version of the patch.
|
|
||
Comment 16•14 years ago
|
||
> Perhaps I should get DocShells and call GetSameTypeRootTreeItem()?
I'm not sure what you mean; what policy are you trying to enforce?|
|
Assignee | |
Comment 17•14 years ago
|
||
X-Frame-Options 1) restricts "DENY" documents from being framed in any context and 2) restricts "SAMEORIGIN" documents to be framed only if the top-level browsing context is the same origin as the document. This bug is about adding exceptions to the above policy for chrome. We want to allow chrome to frame any resource but we don't want to allow content to frame other content if it would violate any of the above policy. My thinking about GetSameTypeRootTreeItem is: 1) any inner content document with X-F-O: DENY should be blocked if there exists a content docshell anywhere in its parent chain 2) any inner content document with X-F-O: SAMEORIGIN should be blocked if the top content docshell in its parent chain is a different origin 3) we don't need to worry about the "content > chrome > content" chain because we don't allow content to frame chrome in any case How does that sound? FYI, the canonical documentation for the feature is still Eric Lawrence's blog post: http://blogs.msdn.com/b/ieinternals/archive/2010/03/30/combating-clickjacking-with-x-frame-options.aspx
|
|
||
Comment 18•14 years ago
|
||
> My thinking about GetSameTypeRootTreeItem is:
It not relevant here, since in this case the chrome is being loaded in a content-type docshell, so the top content docshell in this case is the chrome page... That old issue with chrome principal vs chrome docshell coming back to bite us. :(|
|
||
Comment 19•14 years ago
|
||
What are the chances of this making b6? I don't think it blocks it but it'd be super awesome to get it in as AMO are going to be rolling out the new UI in there next week
|
||
Comment 20•14 years ago
|
||
(In reply to comment #14) > Comment on attachment 471988 [details] [diff] [review] > fix v2 A new issue. With current trunk and pending patch for bug 584693 applied, this patch no longer causes the Get Add-ons content to display. For whatever it is worth, the original patch here at least still works.
|
|
||
Comment 21•14 years ago
|
||
The issue here seems to be that we are trying to display content in an iframe where the server specifically says: x-frame-options: DENY It seems to me this really should just not work. Perhaps if the header said x-frame-options: SAMEORIGIN some exception for this case would make more sense? Perhaps somehow treating about:addons and addons.mozilla.org as being the same origin? Still a kludge but makes more sense than permitting something that there is an explicit specific deny on the server-side.
|
|
||
Comment 22•14 years ago
|
||
Of course, if besides DENY and SAMEORIGIN, x-frame-options could also specify a whitelist of things permitted to include this in a frame, this would probably be helpful not only here, but in other cases as well. But, I guess Microsoft never thought of that when they specified this.
|
|
||
Comment 24•14 years ago
|
||
(In reply to comment #19) > What are the chances of this making b6? I don't think it blocks it but it'd be > super awesome to get it in as AMO are going to be rolling out the new UI in > there next week This simple answer to the b6 issue would be to NOT specify the x-frame-options header on the 4.0b6 page on the server side, yet continue to specify it on the 4.0b7pre page so that this issue can continue to be worked. This would result in Get Add-ons working for the beta builds only.
|
|
Assignee | |
Comment 25•14 years ago
|
||
(In reply to comment #18) > > My thinking about GetSameTypeRootTreeItem is: > > It not relevant here, since in this case the chrome is being loaded in a > content-type docshell, so the top content docshell in this case is the chrome > page... That old issue with chrome principal vs chrome docshell coming back to > bite us. :( Bummer. Well, that proposal was only to prevent the case in the first part of your comment 12: > 1) If a chrome page in a content docshell loads site A which then frames site > B, the framing will be allowed with this patch. Is this desired? I suppose we can punt on that case since we ultimately get to decide who the site A is that we'll be framing in the product, and we can avoid choosing sites that will be framing other X-F-O:DENY pages. Sound fair?
|
|
Assignee | |
Comment 26•14 years ago
|
||
And by "punt on that case" I mean "allow chrome to frame any document regardless of its X-F-O preferences".
|
|
||
Comment 27•14 years ago
|
||
It sounds like a footgun to me... I doubt people expect that loading a chrome page in a content area will suddenly give it less secure behavior than loading it as chrome to start with.
|
|
Assignee | |
Comment 28•14 years ago
|
||
I am not sure if I am parsing your comment incorrectly or if you are misunderstanding the use case. Aren't we talking about loading a _content_ page in a _chrome area_? This bug addresses the case where about:addons frames a page from services.addons.mozilla.org. If we go forward with comment 26, the only cases where we end up with less secure behavior is if chrome frames a page that in turn frames another page who doesn't want to be framed. This proposal would allow that inner page to be framed because the top-level context is chrome. For there to be any real security impact, the page we chose to frame under the top-level chrome document would have to be a bad actor and do something malicious with the page it in turn framed (e.g. clickjacking). It seems highly unlikely that we would frame any web content in the product that we don't trust or have control over. Can you explain the danger you see here a bit more? What are the cases where we load a chrome page in a content area and how is our comparison between the framed content and the top-level page not sufficient for those cases?
|
|
||
Comment 29•14 years ago
|
||
> Aren't we talking about loading a _content_ page in a _chrome area_? We're talking about loading a document that has the system principal (so is trusted) in a type="content" docshell and this document then creating a subframe into which it loads an untrusted document. If the document that has the system principal were loaded in a type="chrome" docshell itself, there would be no issue; X-Frame-Options would not treat it as the root document for the content and everything would Just Work (this is the case for the browser itself, say, where the XUL document for the UI is loaded in a chrome docshell and has type="content" subframes). But the "ui in tab" thing is implemented in a particularly silly way in that it uses type="content" docshells. As a result, anything that gets same-type docshell parents for subframes of that UI will find the docshell the UI is loaded in (e.g. if you frame AMO in the addons manager, window.top evaluated on AMO will actually return the addons manager window). So X-Frame-Options is finding the addons UI as the root docshell, and you get this bug. Are we on the same page so far? > For there to be any real security impact, the page we chose to frame under > the top-level chrome document would have to be a bad actor and do something > malicious with the page it in turn framed Yes. > It seems highly unlikely that we would frame any web content in the product > that we don't trust or have control over. Where the first "we" includes extensions, right? It doesn't seem that unlikely to me, given that... > What are the cases where we load a chrome page in a content area All the in-tab UI stuff, plus any extensions that do similar things.
|
|
Assignee | |
Comment 30•14 years ago
|
||
Just spoke to Boris and we decided on a better approach which is to mimic the GetSameTypeRootTreeItem behavior for finding the "top" docshell, but with an additional check that will make us stop when we find the first non-SystemPrinicpal-having item. Patch coming up...
|
|
Assignee | |
Comment 31•14 years ago
|
||
Find the top non-systemPrincipal docshell in our parent chain to avoid comparing principals with chrome documents (who are allowed to frame X-F-O pages).
Attachment #471988 -
Attachment is obsolete: true
Attachment #475890 -
Flags: review?(bzbarsky)
|
|
||
Comment 32•14 years ago
|
||
Comment on attachment 475890 [details] [diff] [review] fix v3 >+ nsCOMPtr<nsIPrincipal> thisPrincipal = NodePrincipal(); Just nsIPrincipal* is fine. >+ aDocShellItem = thisDocShellItem; Call that curDocShellItem or something? >+ nsCOMPtr<nsIDocument> topDoc; >+ PRBool system = PR_FALSE; You only use these inside the loop. Why not just declare them there? >+ nsIScriptSecurityManager *ssm = nsContentUtils::GetSecurityManager(); >+ if (!ssm) >+ return NS_OK; I'd think we want to fail safe here (so deny if we can't get an SSM). >+ while (parentDocShellItem) { >+ topDoc = do_GetInterface(parentDocShellItem); >+ if (topDoc) { >+ if (NS_SUCCEEDED(ssm->IsSystemPrincipal(topDoc->NodePrincipal(), >+ &system)) && system) { I'm pretty sure you should be able to assert that topDoc is non-null, but if we want ot keep the null-check we should just fail out if !topDoc. r=me woth those nits addressed.
Attachment #475890 -
Flags: review?(bzbarsky) → review+
|
||
Comment 33•14 years ago
|
||
This is marked betaN but I would really like to see this in for feature freeze. Brandon, possible to get the comments and checkin turned around today?
|
|
Assignee | |
Comment 34•14 years ago
|
||
This addresses Boris' review comments, but I'm still struggling to get a test working. If drivers want to land this in the meantime, by all means...
Attachment #475890 -
Attachment is obsolete: true
Attachment #476053 -
Flags: review+
|
|
||
Comment 35•14 years ago
|
||
I really do not understand this bug at all. THe client is trying to display from the server in a frame. THe server sepcifacally says this content can not be displayed in a frame under any circumstances. So exactly why should code be changed to actually allow the content to be displyed in the frame??
|
|
||
Comment 36•14 years ago
|
||
Because the "frame" in this case is actually the same thing as a browser tab for practical purposes. It happens to have a slightly different implementation in the layout engine, but that's just a bug that needs to be fixed. If that bug were fixed, this issue would not arise at all; this patch is just working around that bug.
|
|
Assignee | |
Comment 37•14 years ago
|
||
Now with test. Carrying forward Boris' review.
Attachment #476434 -
Flags: review+
|
|
Assignee | |
Updated•14 years ago
|
Keywords: checkin-needed
If you're going to use checkin-needed, please include a commit message in your patch. (Really, you should always include it so the reviewer can review it.) I don't understand this well enough to write a good commit message, so I can't check it in.
|
||
Comment 39•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/f5c0015afe0e I used the bug summary as the commit message, which seems suboptimal.
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b7
|
|
||
Comment 40•14 years ago
|
||
While we're at it, this bug seems to be in the wrong component as well.
|
|
||
Comment 43•14 years ago
|
||
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) Gecko/20101006 Firefox/4.0b7pre. Also tested with 4.0b8pre builds on other platforms.
Status: RESOLVED → VERIFIED
Component: Add-ons Manager → DOM
Flags: in-testsuite+
Flags: in-litmus-
Product: Toolkit → Core
QA Contact: add-ons.manager → general
|
||
Comment 45•14 years ago
|
||
(In reply to comment #43) > Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) > Gecko/20101006 Firefox/4.0b7pre. Also tested with 4.0b8pre builds on other > platforms. User-Agent: Mozilla/5.0 (Windows NT 5.1; rv:2.0b6) Gecko/20100101 Firefox/4.0b6 Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b6) Gecko/20100101 Firefox/4.0b6 I'm here as a Dupe via Comment 44 . Fixed ? : When I click [Tools][Add-ons] I get this page as the default http://www.mozilla.org/ , and if I click another Tab below "Get Add-ons" (EG: Extensions, Themes, or Plug-ins) and then "Get Add-ons" again I get a Blank rendering - there is NO frame. A right-click the SECOND time (in the frame area) says the URL is now "about:addons" (the "frame" is gone). Rob
|
|
Reporter | |
Comment 46•14 years ago
|
||
(In reply to comment #45) > (In reply to comment #43) > > Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) > > Gecko/20101006 Firefox/4.0b7pre. Also tested with 4.0b8pre builds on other > > platforms. > > User-Agent: Mozilla/5.0 (Windows NT 5.1; rv:2.0b6) Gecko/20100101 > Firefox/4.0b6 > Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b6) Gecko/20100101 > Firefox/4.0b6 > > I'm here as a Dupe via Comment 44 . > > Fixed ? : When I click [Tools][Add-ons] I get this page as the default > http://www.mozilla.org/ , and if I click another Tab below "Get Add-ons" (EG: > Extensions, Themes, or Plug-ins) and then "Get Add-ons" again I get a Blank > rendering - there is NO frame. A right-click the SECOND time (in the frame > area) says the URL is now "about:addons" (the "frame" is gone). > > > Rob It's been fixed after beta 6 was released. The (hopefully) upcoming beta 7 will include the fix.
|
||
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•