HTML 5's iframe sandbox specification requires web owners to add the content-type "text/html-sandboxed" to all content that will be served in a sandboxed iframe to avoid it being loaded directly and bypass it's sandbox. webkit's current iframe@sandbox implementation doesn't recognize this content type and is marked as a file to be downloaded. eg: http://0x.lv/xss.php?frame_sandbox=allow-scripts&frame_xss=?ct=text/html-sandboxed
Yep. We should implement this feature.
Created attachment 64691 [details] Proposed patch This implementation is based on the current WhatWG specifications: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html http://www.whatwg.org/specs/web-apps/current-work/multipage/iana.html#text/html-sandboxed Content of type 'text/html-sandboxed' is rendered only if the loading frame is sandboxed wrt. origin. (Without the patch, Safari renders 'text/html-sandboxed' like 'text/plain,' while Chrome offers to save the file instead.) We've made a few assumptions in this patch, and we invite you to challenge them: * We interpret the spec to imply that the sandbox attribute must be explicitly assigned to the iframe element, rather than implicitly inferred from the Content-type response header of the document in the iframe. * We interpret the spec to imply that the loading iframe only needs to have the 'SandboxOrigin' flag set. This means that a 'text/html-sandboxed' document can still run scripts, submit forms, and so on. * We suspect that browsers may want to provide more detailed feedback to the user when 'text/html-sandboxed' content is suppressed (perhaps by adding callbacks in FrameLoaderClient). However, we have currently not anticipated the design decisions for such feedback, since we assume browser vendors know this area better.
Created attachment 64699 [details] Patch including Changelog entries Changelog entries accidentally left out in the previous patch. Sorry!
(In reply to comment #2) > Created an attachment (id=64691) [details] > Proposed patch > > This implementation is based on the current WhatWG specifications: > > http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html > http://www.whatwg.org/specs/web-apps/current-work/multipage/iana.html#text/html-sandboxed > > Content of type 'text/html-sandboxed' is rendered only if the loading > frame is sandboxed wrt. origin. (Without the patch, Safari renders > 'text/html-sandboxed' like 'text/plain,' while Chrome offers to save > the file instead.) I don't know if that is on the spec, but if it isn't it should be.. > We've made a few assumptions in this patch, and we invite you to > challenge them: > > * We interpret the spec to imply that the sandbox attribute must be > explicitly assigned to the iframe element, rather than implicitly > inferred from the Content-type response header of the document in > the iframe. As said before, if it's not in the spec I think it should be > * We interpret the spec to imply that the loading iframe only needs to > have the 'SandboxOrigin' flag set. This means that a > 'text/html-sandboxed' document can still run scripts, submit forms, > and so on. That's correct I think. > * We suspect that browsers may want to provide more detailed feedback > to the user when 'text/html-sandboxed' content is suppressed > (perhaps by adding callbacks in FrameLoaderClient). However, we > have currently not anticipated the design decisions for such > feedback, since we assume browser vendors know this area better. cool. One more thing I am not sure if it's even possible. Could it be possible that if a plugin requests a document with text/html-sandboxed to return access denied? Greetings
Comment on attachment 64699 [details] Patch including Changelog entries WebCore/dom/DOMImplementation.cpp:291 + #if ENABLE(SANDBOX) What is ENABLE(SANDBOX) ? I don't think we should have an ENABLE for this feature. It's not that complicated. Please test loading in the main frame, as well as an object tag.
(In reply to comment #4) > One more thing I am not sure if it's even possible. Could it be possible that if a plugin requests a document with text/html-sandboxed to return access denied? If I understand your question right, you're referring to HTTP requests in a plugin's native code. There is no way for WebKit to affect these. (That is because a plugin can do what it pleases -- which is either a bug or a feature, depending on your point of view.) So, no, unfortunately.
Created attachment 64832 [details] Updated patch with more tests (In reply to comment #5) > What is ENABLE(SANDBOX) ? I don't think we should have an ENABLE for this feature. It's not that complicated. This flag was introduced in bug 35147. > Please test loading in the main frame, as well as an object tag. We have now added tests for 'text/html-sandboxed' documents in popup windows and object tags. The WebCore aspects of the patch remain unchanged.
(In reply to comment #6) > (In reply to comment #4) > > One more thing I am not sure if it's even possible. Could it be possible that if a plugin requests a document with text/html-sandboxed to return access denied? > > If I understand your question right, you're referring to HTTP requests in a plugin's native code. There is no way for WebKit to affect these. (That is because a plugin can do what it pleases -- which is either a bug or a feature, depending on your point of view.) > > So, no, unfortunately. No, I dont mean that For example, let's say that: http://webkit.com/sandboxed.php?id=1111 serves a SWF file, then an attacker could do <embed src="http://webkit.com/sandboxed.php?id=1111"> And it would load the SWF file and execute on webkit.com's domain. If for applets, objects and embeds we change its behavior, so that if the request actually returns a text/html-sandboxed content-type it is not loaded/returned, then it would solve this problem: http://lists.w3.org/Archives/Public/public-web-security/2010May/0000.html So, to clear the idea: If the request is made as a result of an EMBED, OBJECT or APPLET element, then the resource will trigger a security exception. Greetings!!
Yes, that's an issue with the design of the text/html-sandboxed feature.
(In reply to comment #9) > Yes, that's an issue with the design of the text/html-sandboxed feature. Right, but wouldn't what I just proposed solve that problem?
It would, but it's impossible to implement.
(In reply to comment #11) > It would, but it's impossible to implement. alright, I guess it's in the plugins code. I'll ask Flash/Silverlight/Sunacle to do it.
> alright, I guess it's in the plugins code. I'll ask Flash/Silverlight/Sunacle to do it. Thanks. That still leaves the issue for folks with legacy plugins though...
(In reply to comment #13) > > alright, I guess it's in the plugins code. I'll ask Flash/Silverlight/Sunacle to do it. > > Thanks. That still leaves the issue for folks with legacy plugins though... And the spec does nothing to be backwards compatible with it.. If the <embed src> trick worked it would have solved everything :( Doing a HEAD preflight is also not worth it I guess, and now that I remember java does fetch the code by itself (does not ask the browser to do it). Anyway, it would be great if the browsers alerted the user when they run unupdated software.
(In reply to comment #7) > > Please test loading in the main frame, as well as an object tag. > > We have now added tests for 'text/html-sandboxed' documents in popup windows and object tags. The WebCore aspects of the patch remain unchanged. We never got a response to our updated patch (comment #7). I definitely don't mean to nag (there is no particular hurry here). We're just spoiled by Adam's quick responses, so we're starting to wonder if the patch somehow got lost in the discussion :)
Sorry, we've been off discussing the issue with some plug-in vendors. Looking now.
Comment on attachment 64832 [details] Updated patch with more tests Generally, this looks good. Can you add a test for loading a text/html-sandboxed document inside a plain iframe in a sandboxed iframe? Thanks! LayoutTests/http/tests/security/html-sandboxed-mainwindow.html:7 + layoutTestController.setCanOpenWindows(); Do we still need to call closeRemainingWindowsWhenDone to clean up properly? (I might have gotten the name wrong.) LayoutTests/http/tests/security/html-sandboxed-mainwindow.html:15 + var nbrTrustedWindows = 0; nbr => generally we don't like these sorts of abbreviations. LayoutTests/http/tests/security/html-sandboxed-mainwindow.html:31 + for (var i = 0; i < 5; i++) { Why do we open so many windows? That seems like of silly.
Adam, do you think it could be possible to make it clear in the documentation that text/html-sandboxed should only be loaded when inside an iframe@sandbox? I would propose it in the mailing list but last time I tried to post something my mail got bounced.
when I said documentation I meant spec
> I would propose it in the mailing list but last time I tried to post something my mail got bounced. Sure.
Created attachment 65708 [details] Patch with more tests (In reply to comment #17) Thanks for the fast reply. We didn't mean to intrude on your night sleep (judging by the time of your message)... > Can you add a test for loading a text/html-sandboxed document inside a plain iframe in a sandboxed iframe? Added (LayoutTests/http/tests/security/sandboxed-iframe-inherited-html-sandboxed.html) > Do we still need to call closeRemainingWindowsWhenDone to clean up properly? (I might have gotten the name wrong.) Added call to setCloseRemainingWindowsWhenComplete() in html-sandboxed-mainwindow.html. > nbr => generally we don't like these sorts of abbreviations. Changed to numberOfTrustedWindows. > LayoutTests/http/tests/security/html-sandboxed-mainwindow.html:31 > + for (var i = 0; i < 5; i++) { > Why do we open so many windows? That seems like of silly. I added a comment in the test case to explain our reasoning. Essentially, we're trying to determine whether an asynchronous, unbounded event (rendering a loaded document in a new window) was prevented. So, how long do we wait? We came up with this (admittedly silly) solution of a set of very similar, but allowed, windows. If all those allowed windows were loaded, and the disallowed one wasn't, we should be fine. Yes, silly indeed. We'd love to figure out a more direct way of testing this.
> Yes, silly indeed. We'd love to figure out a more direct way of testing this. Ah, yes. We've had the same problem in other tests. By the way, do we want to add an inspector message explaining why the load failed? Previously, we've had problems with authors getting confused when we block loads like this for security.
Comment on attachment 65708 [details] Patch with more tests Looks good. Why are some of the special cases for text/html-sandboxed inside #if but others not? > @@ -2700,6 +2700,8 @@ void FrameLoader::committedLoad(Document > { > if (ArchiveFactory::isArchiveMimeType(loader->response().mimeType())) > return; > + if (equalIgnoringCase(loader->response().mimeType(), "text/html-sandboxed") && !isSandboxed(SandboxOrigin)) > + return; > m_client->committedLoad(loader, data, length); > } Can this possibly be right? It seems we treat this as an eternally uncommitted load rather than a failure. I don't think that's what we want. Is this really the right bottleneck? I can’t say review+ because I don’t think the ifdefs are correct. But I don’t see anything I’m sure is wrong, so I’m not doing review- yet. It seems quite error prone that in all these places we have separate text/html-sandboxed strings, and any could have a typo. Do we have sufficient test coverage so we'd get a failure if any one of these strings was wrong?
Created attachment 66053 [details] Revised patch Thanks for your comments, all very valid. (In reply to comment #22) > By the way, do we want to add an inspector message explaining why the load failed? Previously, we've had problems with authors getting confused when we block loads like this for security. Added. I also added an explicit error message (see below) for the user. (In reply to comment #23) > Why are some of the special cases for text/html-sandboxed inside #if but others not? I originally attempted to use as few #if's as possible, but I realize that's probably confusing. I have now added #if's more consistenly. However, one check remains when ENABLE(SANDBOX) is disabled, to ensure that html-sandboxed content is still rejected in that case. > Can this possibly be right? It seems we treat this as an eternally uncommitted load rather than a failure. I don't think that's what we want. Is this really the right bottleneck? Good point. I moved the check to DocumentLoader::commitLoad, to make sure the decision is made as early as possible. I also added an explicit error. The cannotShowURLError is the least surprising error I could come up with without introducing a new one. The console message (above) provides developers more hints on what happened. > It seems quite error prone that in all these places we have separate text/html-sandboxed strings, and any could have a typo. Do we have sufficient test coverage so we'd get a failure if any one of these strings was wrong? I agree. I could come up with two potential errors from a typo: * text/html-sandboxed content could sneak past the check in DocumentLoader. This is checked for in the tests that verify the difference between html and html-sandboxed content. * text/html-sandboxed content would be rendered as text/plain (as it is done without this patch). I have added a section break to the untrusted parts of the test cases, which should be rendered differently if the content is (incorrectly) handled as text/plain.
Attachment 66053 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/loader/DocumentLoader.cpp:301: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] Total errors found: 1 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 66058 [details] Patch with style fix Oops. Keeping the #if'ed code in style was slightly trickier than expected. This patch should hopefully make the style-bot happy. Sorry for the patch-spamming!
Attachment 66058 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3883176
Created attachment 66176 [details] Patch with build fix for Chromium (Sigh.) I feel the bots are teaming up on me today. Posting this tiny fix to let the bots do their thing while the Californians are sleeping.
Attachment 66176 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/loader/DocumentLoader.cpp:39: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 66200 [details] Yet another build fix Oh, how embarrassing. I can hear the entire Internet laughing at me now. For what it's worth, this patch passed check-style and was successfully built for Mac and Chromium (Mac). Fingers crossed.
> Oh, how embarrassing. I can hear the entire Internet laughing at me now. No need to be embarrassed. The same thing happens to me all the time.
Comment on attachment 66200 [details] Yet another build fix View in context: https://bugs.webkit.org/attachment.cgi?id=66200&action=prettypatch We definitely need some more tests. For example, you should test the case where you load a document into a sandboxed frame, remove the sandbox attribute, and then try to load a text/html-sandboxed resource (and the reverse). Those tests will show that we're getting the bit from the frame and not from the document (which freezes its bits). Also, we should think about how to test for the absence of the gap mentioned below. > WebCore/loader/DocumentLoader.cpp:284 > + && !frameLoader->isSandboxed(SandboxOrigin) I wonder slightly if this is the right time to ask the sandbox state. We need to be sure the frame as the sandbox bit turned on at the instant the document freezes it's sandbox bits. If there's a window of opportunity for JavaScript to change the frame's attribute, we could get in trouble, especially since that scenario can be triggered by an attacker. How can we assure ourselves that there's no gap here?
It was removed from web-spec: https://www.w3.org/html/wg/wiki/ChangeProposals/text_html_sandboxed https://www.w3.org/Bugs/Public/show_bug.cgi?id=12390 I am going to mark this as "RESOLVED WONTFIX". Thanks!