Bug 38424 – add support for text/html-sandboxed on sandboxed iframes

• Component: Frames
• Status: RESOLVED WONTFIX

[eduardo]

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

[Adam Barth]

Yep.  We should implement this feature.

[Patrik Persson]

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.

[Patrik Persson]

Created attachment 64699 [details]
Patch including Changelog entries

Changelog entries accidentally left out in the previous patch. Sorry!

[eduardo]

(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

[Adam Barth]

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.

[Patrik Persson]

(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.

[Patrik Persson]

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.

[eduardo]

(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!!

[Adam Barth]

Yes, that's an issue with the design of the text/html-sandboxed feature.

[eduardo]

(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?

[Adam Barth]

It would, but it's impossible to implement.

[eduardo]

(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.

[Adam Barth]

> 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...

[eduardo]

(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.

[Patrik Persson]

(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 :)

[Adam Barth]

Sorry, we've been off discussing the issue with some plug-in vendors.  Looking now.

[Adam Barth]

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.

[eduardo]

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.

[eduardo]

when I said documentation I meant spec

[Adam Barth]

> I would propose it in the mailing list but last time I tried to post something my mail got bounced.

Sure.

[Patrik Persson]

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.

[Adam Barth]

> 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.

[Darin Adler]

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?

[Patrik Persson]

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.

[WebKit Review Bot]

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.

[Patrik Persson]

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!

[WebKit Review Bot]

Attachment 66058 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3883176

[Patrik Persson]

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.

[WebKit Review Bot]

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.

[Patrik Persson]

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.

[Adam Barth]

> 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.

[Adam Barth]

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?

[Ahmad Saleem]

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!