Bug 38424 - add support for text/html-sandboxed on sandboxed iframes
Summary: add support for text/html-sandboxed on sandboxed iframes
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://0x.lv/xss.php?frame_sandbox=al...
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-01 19:21 PDT by eduardo
Modified: 2010-09-05 23:51 PDT (History)
5 users (show)

See Also:


Attachments
Proposed patch (6.25 KB, patch)
2010-08-18 05:22 PDT, Patrik Persson
no flags Details | Formatted Diff | Diff
Patch including Changelog entries (8.60 KB, patch)
2010-08-18 06:44 PDT, Patrik Persson
abarth: review-
Details | Formatted Diff | Diff
Updated patch with more tests (13.34 KB, patch)
2010-08-19 05:25 PDT, Patrik Persson
abarth: review-
Details | Formatted Diff | Diff
Patch with more tests (17.31 KB, patch)
2010-08-27 06:37 PDT, Patrik Persson
no flags Details | Formatted Diff | Diff
Revised patch (18.40 KB, patch)
2010-08-31 08:00 PDT, Patrik Persson
no flags Details | Formatted Diff | Diff
Patch with style fix (18.41 KB, patch)
2010-08-31 08:26 PDT, Patrik Persson
no flags Details | Formatted Diff | Diff
Patch with build fix for Chromium (18.59 KB, patch)
2010-09-01 01:02 PDT, Patrik Persson
no flags Details | Formatted Diff | Diff
Yet another build fix (18.65 KB, patch)
2010-09-01 04:52 PDT, Patrik Persson
abarth: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description eduardo 2010-05-01 19:21:21 PDT
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
Comment 1 Adam Barth 2010-05-03 16:18:45 PDT
Yep.  We should implement this feature.
Comment 2 Patrik Persson 2010-08-18 05:22:20 PDT
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.
Comment 3 Patrik Persson 2010-08-18 06:44:28 PDT
Created attachment 64699 [details]
Patch including Changelog entries

Changelog entries accidentally left out in the previous patch. Sorry!
Comment 4 eduardo 2010-08-18 09:49:40 PDT
(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 5 Adam Barth 2010-08-18 11:06:28 PDT
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.
Comment 6 Patrik Persson 2010-08-19 05:18:36 PDT
(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.
Comment 7 Patrik Persson 2010-08-19 05:25:23 PDT
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.
Comment 8 eduardo 2010-08-19 10:35:47 PDT
(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!!
Comment 9 Adam Barth 2010-08-19 11:02:33 PDT
Yes, that's an issue with the design of the text/html-sandboxed feature.
Comment 10 eduardo 2010-08-19 11:13:18 PDT
(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?
Comment 11 Adam Barth 2010-08-19 11:57:20 PDT
It would, but it's impossible to implement.
Comment 12 eduardo 2010-08-19 12:39:34 PDT
(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.
Comment 13 Adam Barth 2010-08-19 12:46:37 PDT
> 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...
Comment 14 eduardo 2010-08-19 13:27:35 PDT
(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.
Comment 15 Patrik Persson 2010-08-26 03:52:30 PDT
(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 :)
Comment 16 Adam Barth 2010-08-26 23:20:54 PDT
Sorry, we've been off discussing the issue with some plug-in vendors.  Looking now.
Comment 17 Adam Barth 2010-08-26 23:25:29 PDT
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.
Comment 18 eduardo 2010-08-26 23:53:29 PDT
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.
Comment 19 eduardo 2010-08-26 23:53:49 PDT
when I said documentation I meant spec
Comment 20 Adam Barth 2010-08-27 00:42:47 PDT
> I would propose it in the mailing list but last time I tried to post something my mail got bounced.

Sure.
Comment 21 Patrik Persson 2010-08-27 06:37:49 PDT
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.
Comment 22 Adam Barth 2010-08-27 09:21:47 PDT
> 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 23 Darin Adler 2010-08-29 13:35:18 PDT
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?
Comment 24 Patrik Persson 2010-08-31 08:00:59 PDT
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.
Comment 25 WebKit Review Bot 2010-08-31 08:03:19 PDT
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.
Comment 26 Patrik Persson 2010-08-31 08:26:28 PDT
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!
Comment 27 WebKit Review Bot 2010-08-31 08:55:03 PDT
Attachment 66058 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3883176
Comment 28 Patrik Persson 2010-09-01 01:02:12 PDT
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.
Comment 29 WebKit Review Bot 2010-09-01 01:04:02 PDT
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.
Comment 30 Patrik Persson 2010-09-01 04:52:24 PDT
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.
Comment 31 Adam Barth 2010-09-01 11:29:36 PDT
> 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 32 Adam Barth 2010-09-05 23:51:13 PDT
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?