Bug 21288 – Implement HTML5's sandbox attribute for iframes

• Status: RESOLVED
• Resolution: FIXED
• Component: Frames

[Robbie Paplin]

We would like to see Safari implement restricted attribute on Iframes. For more information on what we mean, please check out 

http://blogs.msdn.com/ie/archive/2008/01/18/using-frames-more-securely.aspx


For detailed design on how Internet Explorer provides this, please check: 
http://msdn.microsoft.com/en-us/library/ms534622(VS.85).aspx

[Mark Rowe (bdash)]

I think we're more likely to implement HTML 5's iframe sandboxing (<http://www.whatwg.org/specs/web-apps/current-work/#attr-iframe-sandbox>) than Microsoft's proprietary approach.

[Robbie Paplin]

I was previously unaware of the HTML 5 IFRAME sandbox attribute. Thanks for bringing it to my attention. In any event, either approach works for us.

[Mark Rowe (bdash)]

<rdar://problem/6266825>

[Yuan Song]

Created attachment 42473 [details]
HTML5 IFrame sandboxing

HTML5 IFrame sandboxing patch.

Since svn-apply currently does not handle file attributes, the following command needs to be run manually  after patching:

chmod a+x LayoutTests/http/tests/xmlhttprequest/resources/access-control-sandboxed-iframe-allow.cgi \
  LayoutTests/http/tests/xmlhttprequest/resources/access-control-sandboxed-iframe-denied-without-wildcard.cgi \
  LayoutTests/http/tests/xmlhttprequest/resources/access-control-sandboxed-iframe-denied.cgi

[Eric Seidel (no email)]

svn-apply does handle svn-properties like svn:executable, or at least it did last time I checked.

[Eric Seidel (no email)]

I have no idea if this is a good thing.  Sam or abarth would know.

[Adam Barth]

Yes.  We'd definitely like an implementation of the sandbox attribute.  I'll review this patch carefully tonight.

[Adam Barth]

Comment on attachment 42473 [details]
HTML5 IFrame sandboxing

This patch is very good.  There is one design issue that I'd like you to change.  Instead of storing the sandbox information only on the ownerElement, we should copy the information to the document's securityOrigin.  (We might as well copy the whole bit array.)  This is to make sure w can keep track of the document's privileges after it get detached from the frame.

Also, we should unify SecurityOrigin::m_noAccess with the sandbox bits.  If you see how to do that, we can do that in the first patch.  Otherwise, we can do it in a second patch.

Detailed comments below:

+ String Document::cookie(ExceptionCode &ec) const

In this function, you want to get the sandbox information out of the securityOrigin instead of the Frame to handle the case when the document is detached from the frame (i.e., with m_frame is null).

+ Document::initSecurityContext

Basically, you want to copy the sandbox state into securityOrigin here.  That way you can have wit with all the Document-lifetime objects that need it.

+ HTMLFrameOwnerElement* ownerElement = document()->frame()->ownerElement();

Careful!  frame() can be null!  Once you move the sandbox state into SecurityOrigin, then you can just get it from the document() and not have to worry about frame() being null.

+  // Parse the unordered set of unique space-separated tokens.

Please move the parser code into a separate function.  Also, you should add some tests that exercise this parser, e.g., by using mixed case and crazy parser cases.

+ isIFrameSandboxStringWhitespace

Why do we need a special function for this?  Why not just use IsASCIIWhitespace or some-such?  If the spec really requires this goofy set of characters, we can change the spec to be sane.

+ bool originSandboxed() const { return m_originSandboxed; }

We should combine these concepts.

+ src="data:text/html,

Please don't use data URLs in your test unless you're specifically testing the interaction of sandbox and data URLs.  We already sandbox data URLs to a certain extent, so it's a bit unclear what you're testing.

+ navigated_frames ++;

No space before the ++ operator.

+ LayoutTests/fast/frames/resources/sandboxed-iframe-document-cookie-read-denied.html

Please test cookie operators in HTTP tests.  The rules for cookies interacting with file URLs are tricky and not what we want to test here.

+  11           text run at (92,18) width 268: "full render tree to include Iframe contents.)"

You can use layoutTestController.dumpChildFramesAsText (or something like that) to achieve the same effect while keeping a text test (which are infinitely easier to deal with).

[Alexey Proskuryakov]

         // FIXME: the DOM spec states that this attribute can 
         // raise an exception on setting.
                  attribute [ConvertNullToNullString] DOMString cookie
-                     /*setter raises (DOMException)*/;
+                     getter raises (DOMException),
+                     setter raises (/*DOMException*/);

I'm not sure why the second line has DOMException commented out. The usual style is to use per-file and per-function comments in ChangeLogs to explain any non-trivial changes (that's why the list of changed locations is generated by prepare-ChangeLog). It doesn't look like the "setter raises (/*DOMException*/)" syntax is actually making setCookie change ExceptionCode.

+    if (accessControlOriginString != "*" && securityOrigin->originSandboxed())
+        return false;

This check could also use some explanation, probably even as a comment in code.

+layer at (0,0) size 800x600
+  RenderView at (0,0) size 800x600

Pixel tests should have pixel results, as well. As Adam said, it's best to keep these tests text-only.

> + navigated_frames ++;
> No space before the ++ operator.

It is also slightly nicer to use prefix form: "++navigated_frames". This mostly matters for complex types like C++ container iterators, but is a good habit to keep.

But since this is test code, I'm fine with doing things in unconventional ways, just for the slight chance of it ever catching a completely unrelated error by surprise :)

+CONSOLE MESSAGE: line 1: Unsafe JavaScript attempt to access frame with URL http://127.0.0.1:8000/security/xss-DENIED-sandboxed-iframe.html from frame with URL http://127.0.0.1:8000/security/resources/xss-DENIED-sandboxed-iframe-attacker.html. Domains, protocols and ports must match.
+
+CONSOLE MESSAGE: line 1: Unsafe JavaScript attempt to access frame with URL http://127.0.0.1:8000/security/xss-DENIED-sandboxed-iframe.html from frame with URL http://127.0.0.1:8000/security/resources/xss-DENIED-sandboxed-iframe-attacker.html. Domains, protocols and ports must match.
+
+This test verifies that sandboxed iframe prevents cross-domain script access.

This test only dumps its results to console, and would be surprising if opened in Safari manually. If it's not easy to make it work in Safari, please add some text to the test explaining that it only works with run-webkit-tests.

+    <p>This test case verifies that a sandboxed frame cannot break out
+        of its sandbox by modifying its own sandbox attribute. A warning
+        message (about cross-site scripting) is expected.</p>
+
+    <p id="result">FAIL</p>
+
+    <iframe id="f"
+            sandbox="allow-scripts"
+            src="data:text/html,
+                <head>
+                    <script>
+                    try {
+                        self.sandbox = 'allow-scripts allow-same-origin hacked-via-window-object';
+                    } catch (e) { /* ignore exception */ }
+                    if (top.document) {
+                        top.document.getElementById('f').sandbox = 'allow-scripts allow-same-origin hacked-via-dom';
+                    }
+                    </script>

This test is somewhat confusing - it looks like it's doing almost the same thing twice, but only expects one error message. And it's not clear if the exception that's ignored is meant to be raised.

I'm confused by this text from the spec: "If the sandbox attribute is dynamically added after the iframe has loaded a page, scripts already compiled by that page (whether in script elements, or in event handlers, or elsewhere) will continue to run. Only new scripts will be prevented from executing by this flag."

Is this what is implemented here? Does removing the attribute dynamically work in the same way?

+String Document::cookie(ExceptionCode &ec) const

Should be "ExceptionCode& ec".

+    SandboxFlags m_sandboxFlagsDecl;

Please don't abbreviate "decl" (and it's not very easy to tell from the name how this member is different from m_sandboxFlags).

+                AtomicString sandboxToken = AtomicString(characters + start, end - start);
+                if (equalIgnoringCase(sandboxToken, "allow-same-origin"))

The reason for AtomicStrings' existence is that they are compared with other AtomicStrings very quickly. So, comparing with literals defeats the purpose - you should use static constants for these strings.

+    static const SandboxFlags SandboxFlagsAll = 0x1f;

Why this isn't 0xff?

Overall, the patch looks really good.

[Yuan Song]

Thank you for your comments. We will be back with a revised patch
shortly.

I'd just like to ask a few questions, to make sure we understand your
proposed changes.

* The spec associates the sandbox status with the iframe and its
  nested browsing context (we chose to use the ownerElement for this),
  independent of the current document in the frame. Is there a
  particular advantage with associating the sandbox status with the
  document (beyond as a useful shorthand when checking)?

  This would essentially be a cached copy of the ownerElement's
  sandbox status. When the document is detached from its frame, the
  cached sandbox status becomes stale. Or am I getting it wrong?

* What do you mean by unifying SecurityOrigin::m_noAccess with the
  sandbox bits? If you mean treating m_noAccess as a sixth flag along
  with the sandbox flags, then I see what you mean (assuming the
  modification discussed in the previous bullet is made). But if you
  mean unifying m_noAccess and the origin sandbox flag, then I'd
  prefer to duck the unification (for now).

  The reason is that SecurityOrigin::m_noAccess and the origin sandbox
  flag work slightly differently. Take the SecurityOrigin::equal()
  method for example. When comparing two identical "data:" URLs (i.e.,
  m_noAccess is set) it returns true, unless any of them is
  sandboxed. Mapping this flag to the origin sandbox flag would make
  equal() behave differently for "data:" URLs. If this change is
  desirable, then we propose doing it in a separate patch.

* The funny syntax for the IDL 'cookie' setter is an attempt to work
  around (what seems to me like) an IDL quirk. I'd actually like to
  omit the line about the setter entirely (the setter throws no
  exceptions), but then the exception status of the getter seemed to
  somehow "leak" to the setter too in the generated code (as if though
  the setter was declared to throw DOMException).

  Should we add a comment to Document.idl on this (possibly along with
  a separate bug report), or are we just doing it wrong?

We hope we don't come off as too defensive to your comments -- they
are very valid, we mostly agree, and we're happy to revise the patch.

[Yuan Song]

(In reply to comment #5)
> svn-apply does handle svn-properties like svn:executable, or at least it did
> last time I checked.

I could be mistaken. I failed to get svn:executable to "bite" when running svn-apply, and blamed my failure on this:
https://bugs.webkit.org/show_bug.cgi?id=27204

[Yuan Song]

(In reply to comment #9)
> ...
> +
> +This test verifies that sandboxed iframe prevents cross-domain script access.
> 
> This test only dumps its results to console, and would be surprising if opened
> in Safari manually. If it's not easy to make it work in Safari, please add some
> text to the test explaining that it only works with run-webkit-tests.

Actually, there is a tiny line of information in the expected output (which is also visible in Safari):

> PASS: cross-domain script access disallowed from sandboxed iframe

Do you want us to make this output more elaborate, or did you miss the line above?

[Alexey Proskuryakov]

Yes, I did overlook the line above. One thing you could do to make it more discoverable is to add text such as "Should say PASS below:" to bug description.

Also, '<div id="console">...</div>' could have descriptive placeholder text for users with disabled JS, or engines that cannot parse test script, something like 'FAIL: Script didn't run'.

[Adam Barth]

(In reply to comment #10)
> * The spec associates the sandbox status with the iframe and its
>   nested browsing context (we chose to use the ownerElement for this),
>   independent of the current document in the frame. Is there a
>   particular advantage with associating the sandbox status with the
>   document (beyond as a useful shorthand when checking)?

The spec doesn't quite understand detached documents in perfect detail.  We could ask Ian to spec this completely, but it might not be worth while.  In the case of this patch, we want to do this to avoid crashing on the line I indicated.

>   This would essentially be a cached copy of the ownerElement's
>   sandbox status. When the document is detached from its frame, the
>   cached sandbox status becomes stale. Or am I getting it wrong?

That's right.  It's fine for things to get stale when the document is detached.  We just want to avoid:

1) Crashing
2) Accidentally given privileges back to the scripts associated with that document.

> * What do you mean by unifying SecurityOrigin::m_noAccess with the
>   sandbox bits?

The m_noAccess flag is implementing the same concept from the spec as one fo the sandbox flags (the concept of a unique origin).  Any differences between m_noAccess and the lack of an allow-same-origin directive are bugs.  I suspect m_noAccess has a number of such bugs because it pre-dates the concept getting added to the spec.  :)

>   If you mean treating m_noAccess as a sixth flag along
>   with the sandbox flags, then I see what you mean (assuming the
>   modification discussed in the previous bullet is made). But if you
>   mean unifying m_noAccess and the origin sandbox flag, then I'd
>   prefer to duck the unification (for now).

That's fine.  I can do that in a follow up patch if you'd rather not do it yourself.

>   The reason is that SecurityOrigin::m_noAccess and the origin sandbox
>   flag work slightly differently. Take the SecurityOrigin::equal()
>   method for example. When comparing two identical "data:" URLs (i.e.,
>   m_noAccess is set) it returns true, unless any of them is
>   sandboxed. Mapping this flag to the origin sandbox flag would make
>   equal() behave differently for "data:" URLs. If this change is
>   desirable, then we propose doing it in a separate patch.

We'd have to look at where equals is used, but I'm pretty sure we want the behavior you have for the sandbox flag for noAccess too.

> * The funny syntax for the IDL 'cookie' setter is an attempt to work
>   around (what seems to me like) an IDL quirk. I'd actually like to
>   omit the line about the setter entirely (the setter throws no
>   exceptions), but then the exception status of the getter seemed to
>   somehow "leak" to the setter too in the generated code (as if though
>   the setter was declared to throw DOMException).

Can we fix the code generator to avoid this workaround?

>   Should we add a comment to Document.idl on this (possibly along with
>   a separate bug report), or are we just doing it wrong?

In general, we're not shy about fixing bugs in the code generator.  I'm fine with fixing the code generator in another patch (either before or after this patch), but I'd like to hear Alexey's view on this topic.

> We hope we don't come off as too defensive to your comments -- they
> are very valid, we mostly agree, and we're happy to revise the patch.

In general, there is a lot of back and forth in WebKit reviews.  The goal of these reviews is to keep the quality of the code as high as possible.  If this is your first patch, you've done a remarkable job.  Most folks start off with small patches to get acquainted with the process.

[Alexey Proskuryakov]

> In general, we're not shy about fixing bugs in the code generator.  I'm fine
> with fixing the code generator in another patch (either before or after this
> patch), but I'd like to hear Alexey's view on this topic.

Yes, the code generator is just part of WebCore, and can be fixed like any other code.

But since cookie can raise exceptions on both getting and setting, we want both getter and setter to take an ExceptionCode& parameter. So, we can just use "raises (DOMException)" here and move the FIXME about exceptions on setting from IDL to DOM code.

[Adam Barth]

(In reply to comment #15)
> But since cookie can raise exceptions on both getting and setting, we want both
> getter and setter to take an ExceptionCode& parameter. So, we can just use
> "raises (DOMException)" here and move the FIXME about exceptions on setting
> from IDL to DOM code.

That's a good point, but why don't we just throw the exception from the cookie setter?  That seems like an important part of the security of the sandbox attribute.

[Sam Weinig]

Comment on attachment 42473 [details]
HTML5 IFrame sandboxing

> -String Document::cookie() const
> +String Document::cookie(ExceptionCode &ec) const

Nit: the & should go next to ExceptionCode, not ec.

>                   attribute [ConvertNullToNullString] DOMString cookie
> -                     /*setter raises (DOMException)*/;
> +                     getter raises (DOMException),
> +                     setter raises (/*DOMException*/);

The "setter raises (/*DOMException*/)" is wrong here and should be removed.


> Index: WebCore/html/HTMLFrameOwnerElement.h
> ===================================================================
> --- WebCore/html/HTMLFrameOwnerElement.h	(revision 50452)
> +++ WebCore/html/HTMLFrameOwnerElement.h	(working copy)
> @@ -32,6 +32,8 @@ class Frame;
>  class SVGDocument;
>  #endif
>  
> +typedef uint8_t SandboxFlags;
> +
>  class HTMLFrameOwnerElement : public HTMLElement {
>  public:
>      virtual ~HTMLFrameOwnerElement();
> @@ -46,17 +48,36 @@ public:
>  
>      virtual ScrollbarMode scrollingMode() const { return ScrollbarAuto; }
>  
> +    static const SandboxFlags SandboxFlagsNone = 0;
> +    static const SandboxFlags SandboxFlagsAll = 0x1f;
> +    static const SandboxFlags SandboxFlagsNavigation = 0x10;
> +    static const SandboxFlags SandboxFlagsPlugins = 0x08;
> +    static const SandboxFlags SandboxFlagsOrigin = 0x04;
> +    static const SandboxFlags SandboxFlagsForms = 0x02;
> +    static const SandboxFlags SandboxFlagsScripts = 0x01;

This seems better suited for an enum.

>  
> Index: WebCore/html/HTMLIFrameElement.cpp
> ===================================================================
> --- WebCore/html/HTMLIFrameElement.cpp	(revision 50452)
> +++ WebCore/html/HTMLIFrameElement.cpp	(working copy)
> @@ -88,6 +88,42 @@ void HTMLIFrameElement::parseMappedAttri
>          if (!attr->isNull() && !attr->value().toInt())
>              // Add a rule that nulls out our border width.
>              addCSSLength(attr, CSSPropertyBorderWidth, "0");
> +    } else if (attr->name() == sandboxAttr) {
> +        SandboxFlags newSandboxFlagsDecl;
> +        // Parse the unordered set of unique space-separated tokens.
> +        if (attr->isNull())
> +            newSandboxFlagsDecl = SandboxFlagsNone;
> +        else {
> +            newSandboxFlagsDecl = SandboxFlagsAll;
> +            const UChar* characters = attr->value().characters();
> +            unsigned length = attr->value().length();
> +            unsigned start = 0;
> +            while (true) {
> +                while (start < length && isIFrameSandboxStringWhitespace(characters[start]))
> +                    ++start;
> +                if (start >= length)
> +                    break;
> +                unsigned end = start + 1;
> +                while (end < length && !isIFrameSandboxStringWhitespace(characters[end]))
> +                    ++end;
> +
> +                // Turn off the corresponding sandbox flag if it's set as "allowed".
> +                AtomicString sandboxToken = AtomicString(characters + start, end - start);
> +                if (equalIgnoringCase(sandboxToken, "allow-same-origin"))
> +                    newSandboxFlagsDecl &= ~SandboxFlagsOrigin;
> +                else if (equalIgnoringCase(sandboxToken, "allow-forms"))
> +                    newSandboxFlagsDecl &= ~SandboxFlagsForms;
> +                else if (equalIgnoringCase(sandboxToken, "allow-scripts"))
> +                    newSandboxFlagsDecl &= ~SandboxFlagsScripts;
> +
> +                start = end + 1;
> +            }

Please split this parsing out into its own function?

> Index: WebCore/html/HTMLIFrameElement.h
> ===================================================================
> --- WebCore/html/HTMLIFrameElement.h	(revision 50452)
> +++ WebCore/html/HTMLIFrameElement.h	(working copy)
> @@ -54,6 +54,11 @@ private:
>      AtomicString m_name;
>  };
>  
> +inline bool isIFrameSandboxStringWhitespace(UChar c)
> +{
> +    return c == ' ' || c == '\r' || c == '\n' || c == '\t' || c == '\f';
> +}


Does this need to be in the header?  It seems to be only used by one .cpp file.


> Index: WebCore/inspector/InspectorController.cpp
> ===================================================================
> --- WebCore/inspector/InspectorController.cpp	(revision 50452)
> +++ WebCore/inspector/InspectorController.cpp	(working copy)
> @@ -1181,10 +1181,15 @@ void InspectorController::getCookies(lon
>              Vector<Cookie> docCookiesList;
>              rawCookiesImplemented = getRawCookies(document, document->cookieURL(), docCookiesList);
>              
> -            if (!rawCookiesImplemented)
> +            if (!rawCookiesImplemented) {
>                  // FIXME: We need duplication checking for the String representation of cookies.
> -                stringCookiesList += document->cookie();
> -            else {
> +
> +                ExceptionCode ec = 0;
> +                // Ignore exception thrown by cookie() below -- the only case an exception is
> +                // thrown here is when the iframe is sandboxed, which will never happen here
> +                // because "doc" is the document of the main frame of the page.
> +                stringCookiesList += document->cookie(ec);

Please add an ASSERT that an exception was not thrown.

[Darin Adler]

Comment on attachment 42473 [details]
HTML5 IFrame sandboxing

> -                     /*setter raises (DOMException)*/;
> +                     getter raises (DOMException),
> +                     setter raises (/*DOMException*/);

I don't understand the syntax here. It's not right to have "setter raises ()" is it? I suggest entirely commenting the setter out as before. Does sandboxing not cause trying to set the cookie to raise an exception?

> +    , m_sandboxFlagsDecl(0)

Is this "decl" abbreviation needed? I think a better name would be m_sandboxFlagsFromAttribute.

> +typedef uint8_t SandboxFlags;

We would normally use an enum for this, even though the values are flags. See examples like PaintLayerFlags in RenderLayer.h for a design pattern to follow.

> +    SandboxFlags m_sandboxFlagsDecl;

We normally try to avoid having protected data members. How about exposing a protected setter function instead. You probably do not need a getter. One advantage of that is that updateSandboxFlags could be called automatically instead of relying on the derived class to do so.

> +    } else if (attr->name() == sandboxAttr) {
> +        SandboxFlags newSandboxFlagsDecl;
> +        // Parse the unordered set of unique space-separated tokens.
> +        if (attr->isNull())
> +            newSandboxFlagsDecl = SandboxFlagsNone;
> +        else {
> +            newSandboxFlagsDecl = SandboxFlagsAll;
> +            const UChar* characters = attr->value().characters();
> +            unsigned length = attr->value().length();
> +            unsigned start = 0;
> +            while (true) {
> +                while (start < length && isIFrameSandboxStringWhitespace(characters[start]))
> +                    ++start;
> +                if (start >= length)
> +                    break;
> +                unsigned end = start + 1;
> +                while (end < length && !isIFrameSandboxStringWhitespace(characters[end]))
> +                    ++end;
> +
> +                // Turn off the corresponding sandbox flag if it's set as "allowed".
> +                AtomicString sandboxToken = AtomicString(characters + start, end - start);
> +                if (equalIgnoringCase(sandboxToken, "allow-same-origin"))
> +                    newSandboxFlagsDecl &= ~SandboxFlagsOrigin;
> +                else if (equalIgnoringCase(sandboxToken, "allow-forms"))
> +                    newSandboxFlagsDecl &= ~SandboxFlagsForms;
> +                else if (equalIgnoringCase(sandboxToken, "allow-scripts"))
> +                    newSandboxFlagsDecl &= ~SandboxFlagsScripts;
> +
> +                start = end + 1;
> +            }
> +        }
> +
> +        if (newSandboxFlagsDecl != m_sandboxFlagsDecl) {
> +            m_sandboxFlagsDecl = newSandboxFlagsDecl;
> +            updateSandboxFlags();
> +        }

This is too much code to have all inline here. You should factor all or some of this out into a separate parsing function.

It doesn't make sense to construct an AtomicString just so you can call equalIgnoringCase on it. You can just create a String, and if you do it by calling substring you might even enefit from a substring sharing optimization and avoid making a copy of the characters. It would be slightly better to write a charactersEqualIgnoringCase function for PlatformString.h -- long run we don't want to require creating a String object just to compare something with it.

> +inline bool isIFrameSandboxStringWhitespace(UChar c)
> +{
> +    return c == ' ' || c == '\r' || c == '\n' || c == '\t' || c == '\f';
> +}

Do we really need a unique function for this? I guess this handles 0x000B differently from isASCIISpace, but it's really frustrating to have yet *another* whitespace function. Does sandboxing really need its own? Since this is the same as isClassWhitespace, maybe there is a pattern here. Maybe we can put this in ASCIICType.h if we can figure out a name to give it to distinguish it from isASCIISpace. Maybe if we audit them we will find that the other clients of isASCIISpace really don't want true for 0x000B. Wouldn't that be nice if we could just have the one function! I'm surprised that web technology isn't consistent on this.

Looks like pretty good test coverage.

Since the sandboxing string has a custom parser, you need a test that tests all the edge cases; we don't want to learn about something like a buffer. All the various whitespace characters and other characters. Empty string. Leading whitespace, trailing, more than one space in the middle, etc. I didn't see that in any of the tests.

It's great to have lots of tests. But can we do more than one test in a single test file? It seems it would be better to have a smaller number of test files that tested a larger combination of things.

Normally we ask people to follow WebKit coding style for the test code too and here you have not done that.

[Yuan Song]

FYI: the revised patch will be submitted by my colleague, Patrik Persson. We're working together on this patch.

[Sam Weinig]

(In reply to comment #19)
> FYI: the revised patch will be submitted by my colleague, Patrik Persson. We're
> working together on this patch.

Thanks so much for taking this on.

[Patrik Persson]

Created attachment 42859 [details]
Revised patch for HTML5 sandboxing.

Updated in response to your feedback on the previous version. A few general comments:

* A copy of the sandbox status is now kept in the document's
  SecurityOrigin, as discussed in posts 8, 10, and 14.

* Both setter and getter for Document::cookie now raise exceptions,
  like they should). The FIXME note that used to be in Document.idl has
  been moved to Document.cpp, and made more specific.

* A new test has been added for the attribute parser, to ensure that
  it behaves nicely when given unconventional input.

* We have combined some test cases into larger ones: plugins
  (applets+embeds+objects), allowed navigation (child navigation, self
  navigation, sandbox propagation).

* Finally, we have set 'svn:executable' on our CGI scripts, but we
  just can't get that carry over from svn-create-patch to
  svn-apply. You may need to do

    cd LayoutTests/http/tests/xmlhttprequest/resources
    chmod a+x access-control-sandboxed-iframe-*.cgi

  for things to work properly after patching.

[Adam Barth]

Comment on attachment 42859 [details]
Revised patch for HTML5 sandboxing.

This is getting really close!  Thanks for working on this feature.

 106     if (accessControlOriginString != "*" && securityOrigin->isSandboxed(SandboxOrigin))

I don't quite understand why we need to look at accessControlOriginString in this check.  It seems like passesAccessControlCheck should just return true unconditionally when accessControlOriginString == "*" before we get here.  Also,

 110     if (!accessControlOrigin->isSameSchemeHostPort(securityOrigin))

Should return false when securityOrigin is sandboxed.

 90     HTMLFrameOwnerElement::inheritSandboxFlags

Shouldn't this push the new sandbox flags into the document's security origin to keep it up-to-date?

[Adam Barth]

To be slightly more precise, it seems like passesAccessControlCheck shouldn't know anything about sandboxing.  All that knowledge should be pushed into SecurityOrigin.

[Patrik Persson]

Created attachment 42959 [details]
Revised patch.

Changes from previous patch:

* Moved up to r50793.

* SecurityOrigin::isSameSchemeHostPort now considers sandbox
  status. CrossOriginAccessControl no longer knows anything about
  sandboxing.

* HTMLFrameOwnerElement::inheritSandboxFlags now copies sandbox flags
  to the document's SecurityOrigin.

  Also excluded some logic in the same method to terminate flag
  propagation early if flags have not changed: if the 'inDocument()'
  status has changed, we still need to copy flags to the descendants'
  SecurityOrigins.

* Minor changes to meet coding style guidelines.

[Adam Barth]

Comment on attachment 42959 [details]
Revised patch.

This looks great.  We can't land this automatically because of the svn:executable bits, but I'll be happy to land it manually.

Let's give other reviewers until Monday to comment before landing the patch.  Thanks again!

[Eric Seidel (no email)]

svn-apply not understanding the executable bit is tracked by bug 27204.

[Darin Adler]

Comment on attachment 42959 [details]
Revised patch.

> +    // Sandboxed iframes cannot open new auxiliary browsing contexts
> +    if (openerFrame && openerFrame->ownerElement()
> +        && openerFrame->ownerElement()->isSandboxed(SandboxNavigation))
> +        return 0;

I think it would be better to have an isSandboxed function you can call on the script controller or some other object in the family of Frame objects. Having the individual callers dig down to the owner element seems a little inelegant. This would simplify some the call sites. I think that the fact that sandboxing is controlled solely by the owner element is something we might change in the future, and it would be best to have that policy localized.

Adam may have a suggestion about which object should have it. I believe in general we would like a member of the Frame family of objects to know how sandboxed the frame keeps its documents, and be consulted in cases where there is not a document involved, and use the document's security origin in all cases where there is a document involved. We want clients to not know specifically that sandboxing comes from owner elements, and also ideally we want to make it difficult to accidentally consult the wrong sandboxing flags (the current flags on the frame instead of the ones on the document which may no longer be current that frame, for example).

> +    HTMLFrameOwnerElement* ownerElement = m_frame->ownerElement();
> +    if (ownerElement && ownerElement->isSandboxed(SandboxScripts))
> +        return false;

Same comment/question as above.

> +        if (isHTMLDocument()) {
> +            ExceptionCode ec = 0;   // Exception (for sandboxed documents) ignored
> +            static_cast<HTMLDocument*>(this)->setCookie(content, ec);
> +        }

When ignoring an exception, you do not have to initialize the ExceptionCode, and it's probably best to consistently not do it. If you were going to look at the exception code, then you would be obligated to initialize it.

Normally we use a single space before comments, not three spaces. And we put periods at the ends of comments.

> +    // Set the iframe sandbox origin flag from the corresponding property 
> +    // of its frame owner element (iframe)
> +    if (HTMLFrameOwnerElement* ownerElement = m_frame->ownerElement())
> +        securityOrigin()->setSandboxFlags(ownerElement->sandboxFlags());

Again, might be good to get the sandbox flags from some Frame-family object rather than actually going to the owner element explicitly.

> -    if (settings && settings->isJavaEnabled()) {
> +    if (settings && settings->isJavaEnabled()
> +        && !document()->securityOrigin()->isSandboxed(SandboxPlugins)) {

>      if (!settings || !settings->isJavaEnabled())
>          return 0;
>  
> +    if (inDocument() && document()->securityOrigin()->isSandboxed(SandboxPlugins))
> +        return 0;

I'd like to see the above two checks factored to share code instead of repeating the logic twice.

I know I should have commented on this in the earlier round, but I think the actual sandbox flags should live in some object in the Frame family, not in the owner element itself. The owner element could map the attribute to sandbox flags, but the actual sandboxing state could be stored elsewhere, within the Frame. This change isn't mandatory but it seems like something that could make the code clearer and more elegant. And prepare us for having other ways to control sandboxing other than attributes on the owner element.

> +void HTMLFrameOwnerElement::inheritSandboxFlags(SandboxFlags flagsFromParent)
> +{
> +    m_sandboxFlags = m_sandboxFlagsFromAttribute | flagsFromParent;
> +
> +    if (inDocument())
> +        document()->securityOrigin()->setSandboxFlags(m_sandboxFlags);

This seems wrong. It is changing the sandbox flags of a document based on an element inside that document!

> +void HTMLIFrameElement::addSandboxStatus(MappedAttribute* attr)

In a new function I'd prefer you use a whole word, attribute, rather than the abbreviation. Words are usually easier to read.

It's great that you factored this out. But I think a slightly better factoring would be to factor our just the parsing. The call to setSandboxFlagsFromAttribute could be in the caller. I think that's a slightly clearer division of responsibilities.

> +                ExceptionCode ec = 0;
> +                // Ignore exception thrown by cookie() below -- the only case an exception is
> +                // thrown here is when the iframe is sandboxed, which will never happen here
> +                // because "doc" is the document of the main frame of the page.
> +                stringCookiesList += document->cookie(ec);
> +                ASSERT(ec == 0);

The variable name is "document", not "doc", right?

The two sentences should each have periods. There's no need to splice them together with a dash.

> +    HTMLFrameOwnerElement* ownerElement = m_frame->ownerElement();
> +    if (ownerElement && ownerElement->isSandboxed(SandboxForms))
> +        return;

Same comment again about explicitly getting at the owner element.

> +        HTMLFrameOwnerElement* ownerElement = m_frame->ownerElement();
> +        if (ownerElement && ownerElement->isSandboxed(SandboxPlugins))
> +            return false;

And here.

> +    // Sandboxed frames can only navigate their descendants.
> +    HTMLFrameOwnerElement* ownerElement = m_frame->ownerElement();
> +    if (ownerElement && ownerElement->isSandboxed(SandboxNavigation)
> +        && !targetFrame->tree()->isDescendantOf(m_frame))
> +        return false;

And here. I also think you should consider putting these things on one line. In this project we tend to make longer lines and not break things so early, even though I know on others they try to stick to 80 columns.

> +        // Sandboxing status, copied from the document's ownerElement

I don't think "copied from the document's ownerElement" is a good comment. I would instead say "determined by the frame" or something along those lines. Also, please include a period.

> +    // Disable the usage of the database if the sandboxed origin browsing context flags takes effect
> +    if (origin->isSandboxed(SandboxOrigin))
> +        return false;

Need a period here.

> +                    if (cookiesEnabled(document)) {
> +                        ExceptionCode ec = 0;   // Exception (for sandboxed documents) ignored
> +                        document->setCookie(m_handshake.serverSetCookie(), ec);
> +                    }

When ignoring an exception, you do not have to initialize the ExceptionCode, and it's probably best to consistently not do it. If you were going to look at the exception code, then you would be obligated to initialize it.

Normally we use a single space before comments, not three spaces. And we put periods at the ends of comments.

Changing to review- because of the apparent error in HTMLFrameOwnerElement::inheritSandboxFlags. My other comments are not important enough and I would not have changed the review state based on them, although I think they could improve this work.

This is going to be a great feature to have. Thank you very much for your work on it!

[Adam Barth]

> Adam may have a suggestion about which object should have it. I believe in
> general we would like a member of the Frame family of objects to know how
> sandboxed the frame keeps its documents, and be consulted in cases where there
> is not a document involved, and use the document's security origin in all cases
> where there is a document involved.

I'm not sure there's a great place in Frame for this currently.  ScriptController is tempting, but sandboxing affects more than scripts.  The rest of our security-related state has document lifetime.  Maybe we need to create something here?

There's also a subtle bug for the case of

<iframe src="about:blank" sandbox></iframe>

I think that will end up sandboxing the containing frame because the SecurityOrigin object is shared by the two frames.

In general, I think it's better to land the initial implementation and iterate instead of trying to get everything perfect in the first patch.  I'd recommend addressing Darin's comments in this patch (because they affect most of the call sites) and worrying about the about:blank case in a future patch.

[Patrik Persson]

Created attachment 43307 [details]
Thank you for your informative review. Here's our updated patch.

* General: Clarified comments and added missing periods.

* Document, WebSocketChannel: Removed initializations of ignored
  exceptions.

* Frame, HTMLFrameOwnerElement:

  - Moved most of the sandbox flag management to Frame. Modified call
    sites in JSDOMWindowCustom, ScriptController, and FrameLoader to
    reflect this.

    The flags from the attribute are still kept in
    HTMLFrameOwnerElement. (There is not necessarily a frame available
    to store the flags in when the attribute is parsed.)

  - Sandbox flags are now copied to the document contained by the
    Frame, rather than the one containing the Frame. This was a bug
    on my part.

* HTMLAppletElement: Introduced private helper method isJavaEnabled().

* HTMLIFrameElement: Separated parsing of sandbox attribute from
  assignment of sandbox status.

* We have _NOT_ accounted for "about:blank" frames. We agree with
  Adam's comment that this should be addressed by a separate patch.

[Darin Adler]

(In reply to comment #29)
>   - Moved most of the sandbox flag management to Frame. Modified call
>     sites in JSDOMWindowCustom, ScriptController, and FrameLoader to
>     reflect this.

It's much better to have this in the frame rather than in the owner element. But new logic in the actual class Frame is not right. Frame is supposed to simply be a "hub" class, with the substantive code in other classes. Frame itself simply holds the pointers to these other objects. The new logic could go in FrameLoader or one of the other Frame sub-objects, but it should not be added to Frame itself. If you look at Frame.h you'll see that most of the member functions and data are planned to be moved to other objects.

Adam Barth may have an opinion about which is the right object to put this in, since he is working to refactor FrameLoader to make it smaller.

[Darin Adler]

Comment on attachment 43307 [details]
Thank you for your informative review. Here's our updated patch.

Exciting work. Looking forward to getting this feature in.

Below I'll say "the frame loader" to mean whatever object in the frame collection we use for the sandboxing logic. I think probably the frame loader. The Frame object itself should not have new functions.

> +bool HTMLAppletElement::isJavaEnabled() const
> +{
> +    if (!inDocument())
> +        return false;

This is a policy change. Before there was no check of inDocument. I presume this change is done because it fixes a bug. Is there a test that shows why this improves things? Can this bug fix be done in a separate patch?

> +void HTMLFrameOwnerElement::setSandboxFlagsFromAttribute(SandboxFlags flagsFromAttribute)
> +{
> +    m_sandboxFlagsFromAttribute = flagsFromAttribute;
> +    updateSandboxFlags();
> +}

I think the argument can just be named "flags". I also think that maybe m_sandboxFlags would be fine for the data member. After all, these are just the element's flags now, not the frame's so the "from attribute" distinction may not be necessary. These are the sandbox flags provided by the element.

> +void HTMLFrameOwnerElement::updateSandboxFlags()
> +{
> +    SandboxFlags flagsFromParent = m_sandboxFlagsFromAttribute;
> +    
> +    // Inherit sandbox flags from the parent frame, if there is one.
> +    Frame* parentFrame;
> +    if (inDocument() && (parentFrame = document()->frame()))
> +        flagsFromParent |= parentFrame->sandboxFlags();
> +
> +    if (contentFrame())
> +        contentFrame()->inheritSandboxFlags(flagsFromParent);
> +}

The logic here should be moved to FrameLoader. There's no reason to take a trip through the owner element to do this operation. A frame can find its own parent and there is no need for the owner element to pass the flags that it gets from the parent document. It is appropriate for the owner element to pass in the flags it is specifying when telling the frame the have changed, although even that is not entirely necessary. The function in the frame loader could be called setOwnerElementSandboxFlags or ownerElementSandboxFlagsChanged or even updateSandboxFlags if you decide to have it get the flags directly rather than taking an argument.

The rules about inheritance of sandbox flags do *not* belong in the owner element code, but rather in the frame loader. A frame's inheritance of its parent's sandbox flags should be done without being triggered by the owner element, simply because it's installed in the frame tree with a particular parent, and should not even depend on the presence of an owner element.

> +void HTMLFrameOwnerElement::insertedIntoDocument()
> +{
> +    HTMLElement::insertedIntoDocument();
> +
> +    updateSandboxFlags();
> +}

If insertedIntoDocument is one place we need to update these flags, what about when the element is removed from a document? Is there a good reason that can't affect sandbox flags or doesn't matter? Could we include a test case to test this? Does the frame already exist when insertedIntoDocument is called? If not, then do we really need the updateSandboxFlags call? Which test fails if you remove this function override?

> +#include "Frame.h"
>  #include "HTMLElement.h"

This is something we do not want to do; including "Frame.h" in another header is something to be avoided if possible, because Frame.h in turn includes a *lot* of other files. The SandboxFlags enum should be put into a smaller header file. FrameLoaderTypes.h is probably the right choice. We don't want all files that include HTMLFrameOwnerElement.h to include Frame.h too and all the files it includes. In general we try to not "include the world"; this is the reason for files like FrameLoaderTypes.h, RenderStyleContents.h, ScrollBehavior.h, PointerEventsHitRules.h, and others.

It's especially strange that this file includes "Frame.h" when the sandbox flags enum is in SecurityOrigin.h. But even SecurityOrigin.h probably should not be included here.

> +            AtomicString sandboxToken = AtomicString(characters + start, end - start);
> +            if (equalIgnoringCase(sandboxToken, allowSameOrigin))
> +                newSandboxFlags &= ~SandboxOrigin;
> +            else if (equalIgnoringCase(sandboxToken, allowForms))
> +                newSandboxFlags &= ~SandboxForms;
> +            else if (equalIgnoringCase(sandboxToken, allowScripts))
> +                newSandboxFlags &= ~SandboxScripts;

It's a mistake to construct an AtomicString here. There is extra cost, and no benefit, to making an AtomicString instead of just a String. Similarly, the constants for the allow strings can just be const char*. There is little to no benefit to constructing an AtomicString for each of them. Comparing with a const char* is roughly the same performance as comparing with a string object if ignoring case.

Longer term, we should create a new function, charactersEqualIgnoringCase(const UChar*, size_t, const char*), and use it in this case; short term you should just make a String and use equalIgnoringCase.

I would call the SandboxFlags variable just "flags", not "newSandboxFlags", given the purpose of the function. The extra words aren't really needed inside a function with the focused job of parsing.

> +                ASSERT(ec == 0);

WebKit coding style writes this ASSERT(!ec) rather than with "== 0".

> +        void inheritSandboxFlags(SandboxFlags); // Propagate sandbox flags to descendants in the frame tree.

This seems like the wrong interface. The caller should not be passing sandbox flags in. See my other comments above.

These functions should be on FrameLoader rather than Frame. It's too bad FrameLoader is so big, but putting new members directly into Frame is even worse than adding new FrameLoader members.

> +    // Disable database access if the sandboxed origin flag is set.
> +    if (origin->isSandboxed(SandboxOrigin))
> +        return false;

Checks directly on the sandboxing setting seem a little low level to me. Is there some higher level question we can ask the security origin? Maybe canAccess or canRequest?

I think it is strange that isSameSchemeHostPort is checking the sandboxing flags. That function now doesn't make as much sense as before, because it can return false even if you ask if a security origin is the same as itself. Are you sure that's the bst design for this? Can we look at call sites and consider a different approach?

I'm not sure all the isSandboxed calls are talking to the right frame. In particular, I am surprised that createWindow is checking openerFrame rather than either lexicalFrame or dynamicFrame and that FrameLoader::submitForm is checking the frame rather than the document. Those might be right, but I'm not sure they are.

Where is the code where a new document gets its security origin flags set based on the frame it is created in?

[Alexey Proskuryakov]

Comment on attachment 43307 [details]
Thank you for your informative review. Here's our updated patch.

A couple of minor comments.

> +                     getter raises (DOMException),
> +                     setter raises (DOMException);

This can be just "raises (DOMException)", no need for separate lines for getter and setter.

> +bool HTMLAppletElement::isJavaEnabled() const
> +{
> +    if (!inDocument())
> +        return false;
> +
> +    if (document()->securityOrigin()->isSandboxed(SandboxPlugins))
> +        return false;
> +
> +    Settings* settings = document()->settings();
> +    return settings && settings->isJavaEnabled();
> +}

<...>

 bool ScriptController::isEnabled()
 {
+    if (m_frame->isSandboxed(SandboxScripts))
+        return false;

I'm not sure if it's so great to have policy checks in simple "isEnabled" functions. I don't have better names to suggest, but for functions like these, the names should probably start with "should" or "can".

[Patrik Persson]

(In reply to comment #31)
> (From update of attachment 43307 [details])

Before we revise the patch, I'd just like to make sure I understand you right.

> > +bool HTMLAppletElement::isJavaEnabled() const
> > +{
> > +    if (!inDocument())
> > +        return false;
> 
> This is a policy change. Before there was no check of inDocument. I presume
> this change is done because it fixes a bug. Is there a test that shows why this
> improves things? Can this bug fix be done in a separate patch?

My mistake. I really intended to do "if (document())", since document() is dereferenced further down in the code.

> > +void HTMLFrameOwnerElement::updateSandboxFlags()
> > +{
> > +    ...
> > +}
> 
> The logic here should be moved to FrameLoader.

I see. I'll prepare a patch with the logic currently in Frame, and as much as possible of the logic in the HTMLFrameOwnerElement, moved to the FrameLoader.

> I think it is strange that isSameSchemeHostPort is checking the sandboxing
> flags. That function now doesn't make as much sense as before, because it can
> return false even if you ask if a security origin is the same as itself. Are
> you sure that's the bst design for this? Can we look at call sites and consider
> a different approach?

I did this in response to comment #22. It should probably return true when other==this, though. It does so without the patch, so such a check shouldn't break anything.

> Where is the code where a new document gets its security origin flags set based
> on the frame it is created in?

Currently in Frame::inheritSandboxFlags() and Document::initSecurityContext().

[Darin Adler]

(In reply to comment #33)
> My mistake. I really intended to do "if (document())", since document() is
> dereferenced further down in the code.

The document() of an element can never be 0. The only node that can have a document() of 0 is a DocumentType.

> > I think it is strange that isSameSchemeHostPort is checking the sandboxing
> > flags. That function now doesn't make as much sense as before, because it can
> > return false even if you ask if a security origin is the same as itself. Are
> > you sure that's the bst design for this? Can we look at call sites and consider
> > a different approach?
> 
> I did this in response to comment #22. It should probably return true when
> other==this, though. It does so without the patch, so such a check shouldn't
> break anything.

I don't think that's right. A function shouldn’t have a name that gives only part of what it does. Special values like NaN that are never equal to anything are confusing in IEEE floating point, and I think this would be confusing too.

I imagine Adam can suggest a cleaner way to do this. Maybe we can both rename and change what the function does. Or we can change callers to call something slightly higher level.

[Adam Barth]

FrameLoader is fine for now.  We might move it later, but I'm not sure there's another object that's better at the moment.

> I'm not sure all the isSandboxed calls are talking to the right frame. In
> particular, I am surprised that createWindow is checking openerFrame rather
> than either lexicalFrame or dynamicFrame and that FrameLoader::submitForm is
> checking the frame rather than the document. Those might be right, but I'm not
> sure they are.

lexicalFrame is likely correct here.  dynamicFrame would be the "first script" in HTML5 parlance.  openerFrame is whatever the attacker wants it to be.

w.r.t. FrameLoader::submitForm, I think it's fine to use m_frame here.  That function only works for active documents anyway, so the frame and the document are in harmony.

[Patrik Persson]

(In reply to comment #32)
> (From update of attachment 43307 [details])
> 
> > +                     getter raises (DOMException),
> > +                     setter raises (DOMException);
> 
> This can be just "raises (DOMException)", no need for separate lines for getter
> and setter.

I wasn't able to get this revised exception specification to work. The
current (two-line) specification results in this Objective-C code
(DOMDocument.mm):

- (NSString *)cookie
{
    WebCore::ExceptionCode ec = 0;
    NSString *result = IMPL->cookie(ec);
    WebCore::raiseOnDOMError(ec);
    return result;
}

- (void)setCookie:(NSString *)newCookie
{
    WebCore::ExceptionCode ec = 0;
    IMPL->setCookie(newCookie, ec);
    WebCore::raiseOnDOMError(ec);
}

However, changing this to just "raises (DOMException)" results in
generated code that does not expect exceptions (giving me
compilation errors):

  attribute [ConvertNullToNullString] DOMString cookie
                     raises (DOMException);

  --

- (NSString *)cookie
{
    return IMPL->cookie();
}

- (void)setCookie:(NSString *)newCookie
{
    IMPL->setCookie(newCookie);
}

Am I doing it wrong, or should this be treated as a separate issue?

[Patrik Persson]

Created attachment 43367 [details]
New patch for HTML5 iframe sandboxing.

Changes:

* Changed openerFrame to lexicalFrame in JSDOMWindow::createWindow().

* Moved sandbox flag management to FrameLoader. Functions
  updateSandboxFlags() and inheritSandboxFlags() have been combined
  into the single function ownerElementSandboxFlagsChanged().

* Moved SandboxFlag(s) type definitions to FrameLoaderTypes.h.

* Removed HTMLFrameOwnerElement::insertedIntoDocument() override
  (unnecessary in this design).

* Removed erroneous inDocument() check in
  HTMLAppletElement::isJavaEnabled().

* Renamed HTMLFrameOwnerElement::m_sandboxFlagsFromAttribute to
  m_sandboxFlags.

* Introduced SecurityOrigin::canCreateDatabase(). I was not able to
  figure out how to use canAccess() or canRequest() for this purpose
  in a clear way.

* Changed function parseSandboxAttribute() in HTMLIFrameElement to use
  regular Strings. Also renamed 'newSandboxFlags' to 'flags' in same
  function.

* Moved sandboxing check from SecurityOrigin::isSameSchemeHostPort()
  to SecurityOrigin::equal(). (Database origin checks depend on it.)

* Added sandboxing check to passesAccessControlCheck() in
  CrossOriginAccessControl.cpp.

* I have NOT addressed the comment on ScriptController::isEnabled()
  (comment #32). There's quite a lot of calls to it. If someone
  proposes a better name for this function (or another way of doing
  it), we'll be happy to change it.

* Minor changes to match coding guidelines:
  - changed "ec == 0" to "!ec" in InspectorController.cpp

Please let us know what you think.

[Darin Adler]

Comment on attachment 43367 [details]
New patch for HTML5 iframe sandboxing.

Looks great. Very few issues remain.

> +    if (m_frame->loader()->isSandboxed(SandboxScripts))
> +        return false;
> +        
>      Settings* settings = m_frame->settings();
>      return m_frame->loader()->client()->allowJavaScript(settings && settings->isJavaScriptEnabled());

I think maybe the sandboxing should contribute to the allowJavaScript argument rather than unconditionally returning false here. Ultimate control should go to the frame loader client. Not sure if this will ever matter in practice.

> +    // FIXME: the HTML5 DOM spec states that this attribute can raise an
> +    // INVALID_STATE_ERR exception on getting if the Document has no
> +    // browsing context.

The word "the" should have a capital "T".

> +void HTMLFrameOwnerElement::setSandboxFlags(SandboxFlags flags)
> +{
> +    m_sandboxFlags = flags;
> +    
> +    if (Frame* frame = contentFrame())
> +        frame->loader()->ownerElementSandboxFlagsChanged();
> +}

Often in a function like this we would return early if m_sandboxFlags was already == flags. Maybe that's just excess not-really-helpful code?

> +void FrameLoader::ownerElementSandboxFlagsChanged()
> +{
> +    m_sandboxFlags = SandboxNone;
> +    
> +    if (Frame* parentFrame = m_frame->tree()->parent())
> +        m_sandboxFlags |= parentFrame->loader()->sandboxFlags();
> +
> +    if (HTMLFrameOwnerElement* ownerElement = m_frame->ownerElement())
> +        m_sandboxFlags |= ownerElement->sandboxFlags();
> +
> +    if (Document* doc = m_frame->document()) 
> +        doc->securityOrigin()->setSandboxFlags(m_sandboxFlags);

I'd prefer "document" over "doc" here for the local variable name.

> +    for (Frame* child = m_frame->tree()->firstChild(); child; child = child->tree()->nextSibling())
> +        child->loader()->ownerElementSandboxFlagsChanged();
> +}

I suggest renaming this function to updateSandboxFlags and calling it from both FrameLoader::ownerElementSandboxFlagsChanged and FrameLoader::init. Then you can get rid of the call to this function in Frame.cpp.

It would be slightly nicer if this used a non-recursive algorithm. You could put most of the logic into a private function named updateSandboxFlagsForSingleFrame and then walk all descendant frames without using recursion by using tree()->traverseNext(). On the other hand, I suppose this is not a real issue, since we can't have an arbitrarily deeply nested frame tree. Still, I'd like it slightly better that way.

    for (Frame* affectedFrame = m_frame; affectedFrame; affectedFrame = affectedFrame->tree()->traverseNext(m_frame))
        affectedFrame->loader()->updateSandboxFlagsForSingleFrame();

I also think we might want to compute the new sandbox flags and do an early return in the case where m_sandboxFlags hasn't changed. Again, not sure that really is an improvement.

> @@ -369,6 +371,9 @@ bool SecurityOrigin::equal(const Securit
>      if (m_domainWasSetInDOM && m_domain != other->m_domain)
>          return false;
>  
> +    if (isSandboxed(SandboxOrigin) || other->isSandboxed(SandboxOrigin))
> +        return false;
> +
>      return true;

I still don't think this is the right. The function name should not be equal if it has a special rule that says it will return false if either frame is sandboxed. The function name should change if its behavior is changing like this. How many different callers of SecurityOrigin::equal are there? Can we easily change the name of the function? What is the concept of the function? Can we change it to be a canAccessAsPartOfSameOrigin function or something like that but less wordy? It's not really an "equal" check if it's actually an check to see if some action is allowed.

Even though this is almost perfect, I think I'll say review- because of the SecurityOrigin::equal design issue.

[Darin Adler]

(In reply to comment #37)
> * Moved sandboxing check from SecurityOrigin::isSameSchemeHostPort()
>   to SecurityOrigin::equal(). (Database origin checks depend on it.)

The database origin checks should use a higher level function rather than an "equal" function. We just need a name.

> * I have NOT addressed the comment on ScriptController::isEnabled()
>   (comment #32). There's quite a lot of calls to it. If someone
>   proposes a better name for this function (or another way of doing
>   it), we'll be happy to change it.

I agree with Alexey that we need a name change since it's now checking more than just whether scripting is enabled. I'd like to look at call sites to figure out what they are checking. Maybe they are checking if they can execute scripts, in which case canExecuteScripts() would be one good name. Or maybe they are checking something else.

It's true that there are between 10 and 20 call sites, but I don't think that's too many to rename. I won't insist on doing it in this patch, but I'd like to see it done. To keep the code readable over time it is important to avoid letting design changes creep in that subtle change the meanings of functions, while retaining their old names.

[Patrik Persson]

Created attachment 43431 [details]
Revised patch in response to comment #38 and comment #39.

I understand your concerns regarding SecurityOrigin::equal(). We had a
closer look, and think that SecurityOriginHash is a better place for
this check. The check now explicitly concerns hash table equality (for
same-origin comparisons). The change also fits nicely with a FIXME in
SecurityOrigin calling for similar refactoring.

In a sense the concern regarding equal() remains, but on a smaller
scale (SecurityOriginHash rather than SecurityOrigin). I am currently
unable to see how to get around this without a re-design of the local
storage database. (As long as the database uses hash tables for
same-origin comparisons, there has to be an equals() somewhere that
compares two SecurityOrigins and accounts for sandboxing.)

Please let us know what you think of this design.

Other changes:

* Revised ScriptController check.

* Refactored frame subtree traversal in FrameLoader.

* Added early return to HTMLFrameOwnerElement::setSandboxFlags() and
  FrameLoader::updateSandboxFlagsForSingleFrame().

* Fixed capitalization in Document.cpp comments.

[Darin Adler]

(In reply to comment #40)
> I understand your concerns regarding SecurityOrigin::equal(). We had a
> closer look, and think that SecurityOriginHash is a better place for
> this check. The check now explicitly concerns hash table equality (for
> same-origin comparisons). The change also fits nicely with a FIXME in
> SecurityOrigin calling for similar refactoring.

I don't think that's right. I appreciate your efforts, but this is still wrong.

> I am currently
> unable to see how to get around this without a re-design of the local
> storage database. (As long as the database uses hash tables for
> same-origin comparisons, there has to be an equals() somewhere that
> compares two SecurityOrigins and accounts for sandboxing.)

That's incorrect.

There's no reason we can't do security checks either before or after looking in the hash table. There's no reason to put sandboxed security origins into the table in the first place. More importantly, the sandboxing state could change on a security origin once it's in the hash table.

The point of the hash is to be able to store per-security-origin data such as the quota, database origin, usage, and storage area. There's no reason the hash's equality function also has to perform the security check.

There's no need for the equality function of the hash table itself to consider the sandbox flags. There's no need to look at the hash table at all if the origin is sandboxed.

[Adam Barth]

W.r.t. the equal() issue: The way a sandboxed origin is modeled in HTML5 is as a "unique value," which you can think of as a random string for each instance that's not equal to anything except itself.  We might improve our implementation of sandboxed SecurityOrigins by representing them this way.  If we do that, it's clear that two sandboxed origins are equal if, and only if, they are the same physical object.

[Darin Adler]

(In reply to comment #42)
> W.r.t. the equal() issue: The way a sandboxed origin is modeled in HTML5 is as
> a "unique value," which you can think of as a random string for each instance
> that's not equal to anything except itself.  We might improve our
> implementation of sandboxed SecurityOrigins by representing them this way.  If
> we do that, it's clear that two sandboxed origins are equal if, and only if,
> they are the same physical object.

That sounds like a good design. When sandboxed, a SecurityOrigin would not check anything except object identity to determine equality and to do hashing.

I'm also not sure we want to allow storage and database access from these sandboxed origins even temporarily. So it is good, but may not be enough to simply make it compare unequal to all other origins. I suspect that in any case we will need to add "can" type checks to StorageNamespaceImpl::storageArea and a some of the functions in DatabaseTracker so that storage and database access are subject to sandboxing rules.

But I don't know for sure. What behavior do we want for storage and database?

[Patrik Persson]

(In reply to comment #43)
> I'm also not sure we want to allow storage and database access from these
> sandboxed origins even temporarily. So it is good, but may not be enough to
> simply make it compare unequal to all other origins. I suspect that in any case
> we will need to add "can" type checks to StorageNamespaceImpl::storageArea and
> a some of the functions in DatabaseTracker so that storage and database access
> are subject to sandboxing rules.
> 
> But I don't know for sure. What behavior do we want for storage and database?

If you don't want storage access from sandboxed frames,, a simple alternative would be to have such frames behave as if storage/database access was disabled in Settings. This would make localStorage()/sessionStorage() return Undefined in JavaScript. I haven't tested this, but I suspect it would be as simple as a sandbox check in DOMWindow::localStorage()/sessionStorage().

[Darin Adler]

(In reply to comment #44)
> I haven't tested this, but I suspect it would be as simple as a
> sandbox check in DOMWindow::localStorage()/sessionStorage().

Sure, makes sense.

So you changed the equal function for some reason relating to storage and database. What was the design you were trying to implement? What did you expect storage-wise and database-wise? Did you expect a separate store that only lived as long as the sandboxed frame? If so, then where is the code to delete the storage and database when the frame goes away? Did you expect to prohibit storage and database access in the sandboxed frame?

We need to design before we can nail down the implementation.

And it would be OK to land this with an interim design even if we change it later.

[Patrik Persson]

(In reply to comment #45)
> (In reply to comment #44)
> > I haven't tested this, but I suspect it would be as simple as a
> > sandbox check in DOMWindow::localStorage()/sessionStorage().
> 
> Sure, makes sense.
> 
> So you changed the equal function for some reason relating to storage and
> database. What was the design you were trying to implement? What did you expect
> storage-wise and database-wise? Did you expect a separate store that only lived
> as long as the sandboxed frame? If so, then where is the code to delete the
> storage and database when the frame goes away? Did you expect to prohibit
> storage and database access in the sandboxed frame?
> 
> We need to design before we can nail down the implementation.

I understand we should have been clearer in how we interpreted sandboxed storage and databases. Our intention was to implement sandboxed frames to have a unique origin, like Adam Barth's comment suggests. In particular, the same frame at a later time would have a new origin and not be able to access data from a previous session. Your points above are very valid and suggest that we may need to think more about that alternative, if you want us to pursue it.

However, if you would prefer sandboxed frames to simply receive Undefined from window.localStorage()/sessionStorage(), then I think the equal() discussion is solved. (We were able to easily move all other sandbox checks away from SecurityOrigin equality except database/storage access.) I'm sorry for not presenting this alternative earlier, and wasting your time in the meantime.

I absolutely agree that we should reason about design rather than implementation details. We're still learning about WebKit on our side, and we need to try design alternatives to understand them. I understand this makes for frequent patches to review. Any feedback on how we can streamline the review process is welcome.

Thanks for enduring our patches and cocky comments. We appreciate your work.

[Darin Adler]

(In reply to comment #46)
> Our intention was to implement sandboxed frames to have
> a unique origin, like Adam Barth's comment suggests. In particular, the same
> frame at a later time would have a new origin and not be able to access data
> from a previous session.

Seems OK. And Adam had some ideas about how to implement that correctly.

> However, if you would prefer sandboxed frames to simply receive Undefined from
> window.localStorage()/sessionStorage(), then I think the equal() discussion is
> solved. (We were able to easily move all other sandbox checks away from
> SecurityOrigin equality except database/storage access.) I'm sorry for not
> presenting this alternative earlier, and wasting your time in the meantime.

Is this really only about storage, not databases? I guess we already sandbox databases with some other change I forgot?

It's not really my preferences that are important here; as I’m sure you know, issues that matter are cross-browser compatibility, matching the standard, and making sure the features are useful and work together well. What do you think HTML5 suggests for this?

Do you think it's important to allow ephemeral storage that's limited to a single sandboxed frame tree? Will we make some helpful use cases impossible if we don't allow this?

I think an initial version that simply blocks localStorage and sessionStorage would be a fine start; there's no need to hold the entire thing up until this is resolved.

Refining further should be straightforward.

[Patrik Persson]

(In reply to comment #43)
> (In reply to comment #42)
> > W.r.t. the equal() issue: The way a sandboxed origin is modeled in HTML5 is as
> > a "unique value," which you can think of as a random string for each instance
> > that's not equal to anything except itself.  We might improve our
> > implementation of sandboxed SecurityOrigins by representing them this way.  If
> > we do that, it's clear that two sandboxed origins are equal if, and only if,
> > they are the same physical object.
> 
> That sounds like a good design. When sandboxed, a SecurityOrigin would not
> check anything except object identity to determine equality and to do hashing.

I'm a bit confused. That was what we were trying to do, an equal()
implementation that always returns false if one or both of the
SecurityOrigins is sandboxed (unless we compare an object to itself).

> I'm also not sure we want to allow storage and database access from these
> sandboxed origins even temporarily. So it is good, but may not be enough to
> simply make it compare unequal to all other origins. I suspect that in any case
> we will need to add "can" type checks to StorageNamespaceImpl::storageArea and
> a some of the functions in DatabaseTracker so that storage and database access
> are subject to sandboxing rules.
> 
> But I don't know for sure. What behavior do we want for storage and database?

I don't think HTML5 says much about how client-side storage
(localStorage/sessionStorage/openDatabase) interacts with
sandboxing. To explain our (previous) design decisions, here's my
interpretation.

I think sessionStorage would make sense, as long as sandboxes are
respected. I think localStorage would end up equivalent to
sessionStorage in a sandboxed frame, making it somewhat less useful. I
don't think a sandboxed database would be very useful: it wouldn't
survive beyond a session.

Our idea with previous patch was to sandbox localStorage and
sessionStorage with respect to origin, and to disable databases in
sandboxes entirely. I should have been clearer about this assumption
from the start.

[Patrik Persson]

Created attachment 43508 [details]
Patch with simplified storage/database sandboxing

To solve the short-term issue of client-side storage/databases in
sandboxes, this patch makes sandboxed frames work as if though
client-side storage/databases were disabled in Settings.

Changes:

* SecurityOrigin::equal() and SecurityOriginHash no longer know about
  sandboxing.

* DOMWindow attributes localStorage and sessionStorage are null in
  sandboxed frames.

* DOMWindow method openDatabase() returns null in sandboxed frames.

* Revised test case for sandboxed storage and database
  (LayoutTests/fast/frames/sandboxed-iframe-storage.html).

[Adam Barth]

I haven't reviewed the current patch, but the discussion seems to be getting to the point where it might make sense to land the current iteration and then make improvements in followup patches.

[Darin Adler]

Comment on attachment 43508 [details]
Patch with simplified storage/database sandboxing

Looking good. Nearly done. A few small mistakes, and one bigger mistake based on bad advice from me.

I miss the canAccessDatabase function from your earlier patch; didn't see why you rolled that back. Maybe because you didn't want a separate canAccessStorage?

>  bool ScriptController::isEnabled()
>  {
>      Settings* settings = m_frame->settings();
> -    return m_frame->loader()->client()->allowJavaScript(settings && settings->isJavaScriptEnabled());
> +    return m_frame->loader()->client()->allowJavaScript(settings && settings->isJavaScriptEnabled() && !m_frame->loader()->isSandboxed(SandboxScripts));
>  }

Loose end for after this lands: ScriptController::isEnabled needs to be renamed. It already checked more than just whether JavaScript was enabled. Maybe canExecuteScripts is the right name.

> +    // FIXME: The HTML5 DOM spec states that this attribute can raise an
> +    // INVALID_STATE_ERR exception on getting if the Document has no
> +    // browsing context.

Loose end for after this lands: Test case showing the problem? Bug report for the problem?

>                   attribute [ConvertNullToNullString] DOMString cookie
> -                     /*setter raises (DOMException)*/;
> +                     setter raises (DOMException),
> +                     getter raises (DOMException);

Loose end for after this lands: Bug report for the problem that just plain "raises" does not work. Should be easy to fix.

> +bool HTMLAppletElement::isJavaEnabled() const

Same problem with this name as with ScriptController::isEnabled, but this is a new name, not an existing name. Should be named something like canEmbedJava instead. Think of it as completing this sentence "the applet element <functionName>" and you can see that "the applet element is Java enabled" does not work at all.

I'd be willing to approve landing this patch if you have firm plans to rename afterwards.

> +    SandboxFlags flags;
> +    // Parse the unordered set of unique space-separated tokens.
> +    if (attribute->isNull())
> +        flags = SandboxNone;

We love early return in the WebKit coding style. It might be nice to just do this before even declaring a local variable, and avoid nesting the whole body of the function inside an else.

> +void FrameLoader::ownerElementSandboxFlagsChanged()
> +{
> +    updateSandboxFlags();
> +}

This could be inlined by defining it in the class definition in the header. Probably not important, though there seems to be no downside to doing this.

> +void FrameLoader::updateSandboxFlags()
> +{
> +    updateSandboxFlagsForSingleFrame();
> +
> +    for (Frame* affectedFrame = m_frame; affectedFrame; affectedFrame = affectedFrame->tree()->traverseNext(m_frame))
> +        affectedFrame->loader()->updateSandboxFlagsForSingleFrame();
> +}

This calls updateSandboxFlagsForSingleFrame twice on the current frame. The first call to updateSandboxFlagsForSingleFrame outside the loop can be removed.

But also, I made you break this with my suggestion! The updateSandboxFlagsForSingleFrame function only works if the parent updates its flags before the child. And this non-recursive algorithm does not guarantee that. So you probably need to go back to the recursive version. Sorry.

> +    m_frame->document()->securityOrigin()->setSandboxFlags(m_sandboxFlags);

Probably OK as is, but we might want to change this to just call a function named updateSandboxFlags on the Document rather than digging directly into the security origin. The document already has to know that it gets its sandbox flags from the frame, and it seems a little strange for half the knowledge of that to be in Document and the other half in FrameLoader.

> +    , m_sandboxFlags(SandboxNone)

I wonder if maybe the default should instead be entirely sandboxed. Is there any code path where the default matters? Same applies to FrameLoader. We always set the flags later in initialization, so maybe it's OK to fully sandbox during initialization. Not an important issue, but this does seem backwards from usual security designs to assume everything is allowed. It would be good to notice if we attempted anything so early that we did not yet have sandbox flags set, so you could even imagine a special value "not yet set" and you would assert if ever asked to make a sandboxing decision based on that value.

Is there any problem with the fact that documents that never go into a frame, such as ones made with XMLHttpRequest, have no sandbox flags set? Maybe we should set their sandbox flags to prohibit everything or set them to the "don't you dare use this" value?

I did not carefully review the tests.

You don't need to do everything I mentioned on comments -- I don't want to make this take forever. But please do fix the things that are definitely broken like the bad updateSandboxFlags algorithm I suggested.

[Darin Adler]

(In reply to comment #48)
> (In reply to comment #43)
> > (In reply to comment #42)
> > > W.r.t. the equal() issue: The way a sandboxed origin is modeled in HTML5 is as
> > > a "unique value," which you can think of as a random string for each instance
> > > that's not equal to anything except itself.  We might improve our
> > > implementation of sandboxed SecurityOrigins by representing them this way.  If
> > > we do that, it's clear that two sandboxed origins are equal if, and only if,
> > > they are the same physical object.
> > 
> > That sounds like a good design. When sandboxed, a SecurityOrigin would not
> > check anything except object identity to determine equality and to do hashing.
> 
> I'm a bit confused. That was what we were trying to do, an equal()
> implementation that always returns false if one or both of the
> SecurityOrigins is sandboxed (unless we compare an object to itself).

Yes, it was my misunderstanding from the start, not something wrong with the patch.

The code you added only kicked in after comparing all the other aspects of the security origin, so I think it obscured the fact that you were implementing this rule and in fact all other aspects of the origin were irrelevant. Similarly, you updated the equal function but not the hash function originally, which further confused me. I think a comment might have made the difference, explaining the model.

It's possible this could be the right direction in the future, but to make it more clear why things work this way I think the sandboxing check could be earlier in the function. Or there could be a comment about why not.

I worry about the security origin having other data that's misleading. Data you should never look at in a sandboxed origin. Who cares what the host name is if it really has no effect on the security policy. Perhaps it should be illegal to even get it from the security origin for a sandboxed origin. Maybe it's useful, but only for debugging?

> I think sessionStorage would make sense, as long as sandboxes are
> respected. I think localStorage would end up equivalent to
> sessionStorage in a sandboxed frame, making it somewhat less useful. I
> don't think a sandboxed database would be very useful: it wouldn't
> survive beyond a session.
> 
> Our idea with previous patch was to sandbox localStorage and
> sessionStorage with respect to origin, and to disable databases in
> sandboxes entirely. I should have been clearer about this assumption
> from the start.

We should communicate our plans to the others implementing HTML5. Maybe we *should* have non-useful sandboxed databases that are deleted when the frames are. And have local and session storage that only lives the life of the sandbox.

[Darin Adler]

(In reply to comment #50)
> I haven't reviewed the current patch, but the discussion seems to be getting to
> the point where it might make sense to land the current iteration and then make
> improvements in followup patches.

Yes, I think we have consensus on that. There are just a few small showstoppers. We're close to landing.

[Patrik Persson]

(In reply to comment #51)
> (From update of attachment 43508 [details])
> Looking good. Nearly done. A few small mistakes, and one bigger mistake based
> on bad advice from me.
> 
> I miss the canAccessDatabase function from your earlier patch; didn't see why
> you rolled that back. Maybe because you didn't want a separate
> canAccessStorage?

I moved the check to DOMWindow and used an explicit check for SandboxOrigin, for symmetry with the storage checks. Somehow I managed to drop this from the patch changelog. My mistake.

Do you want us to use named functions for these DOMWindow checks (canAccessStorage/canAccessDatabase)?

[Darin Adler]

(In reply to comment #54)
> Do you want us to use named functions for these DOMWindow checks
> (canAccessStorage/canAccessDatabase)?

I like the clarity of the named functions and centralizing the security policy in the SecurityOrigin object. But I don't feel strongly about it. This seems an unimportant issue to me.

[Patrik Persson]

(In reply to comment #51)
> (From update of attachment 43508 [details])
> >  bool ScriptController::isEnabled()
> >  {
> >      Settings* settings = m_frame->settings();
> > -    return m_frame->loader()->client()->allowJavaScript(settings && settings->isJavaScriptEnabled());
> > +    return m_frame->loader()->client()->allowJavaScript(settings && settings->isJavaScriptEnabled() && !m_frame->loader()->isSandboxed(SandboxScripts));
> >  }
> 
> Loose end for after this lands: ScriptController::isEnabled needs to be
> renamed. It already checked more than just whether JavaScript was enabled.
> Maybe canExecuteScripts is the right name.
> 
> > +    // FIXME: The HTML5 DOM spec states that this attribute can raise an
> > +    // INVALID_STATE_ERR exception on getting if the Document has no
> > +    // browsing context.
> 
> Loose end for after this lands: Test case showing the problem? Bug report for
> the problem?

I can file (at least) bug reports for these. However, if it's fine with you, I'd prefer to do it after the patch lands. Otherwise, I'd have to refer to either code we're about to change, or code in an uncommitted patch.

> >                   attribute [ConvertNullToNullString] DOMString cookie
> > -                     /*setter raises (DOMException)*/;
> > +                     setter raises (DOMException),
> > +                     getter raises (DOMException);
> 
> Loose end for after this lands: Bug report for the problem that just plain
> "raises" does not work. Should be easy to fix.

https://bugs.webkit.org/show_bug.cgi?id=31720

[Patrik Persson]

Created attachment 43585 [details]
Revised patch in response to comment #51.

* Renamed helper JavaEnabled() in HTMLAppletElement to canEmbedJava().

* Added early return in HTMLIFrameElement, for the case when there is
  no sandbox attribute.

* Inlined FrameLoader::ownerElementSandboxFlagsChanged().

* Reverted FrameLoader::updateSandboxFlags() to recursive
  implementation.

* Added method Document::updateSandboxFlags(). Although this is a
  one-liner, I didn't inline it to avoid #including SecurityOrigin.h.

* Changed Document::initSecurityContext() and
  FrameLoader::updateSandboxFlags() to invoke
  Document::updateSandboxFlags().

* Added named policy checks for storage and database access in
  SecurityOrigin, and modified DOMWindow to use these new checks.

[Patrik Persson]

(In reply to comment #51)
> I wonder if maybe the default should instead be entirely sandboxed. Is there
> any code path where the default matters? Same applies to FrameLoader. We always
> set the flags later in initialization, so maybe it's OK to fully sandbox during
> initialization. Not an important issue, but this does seem backwards from usual
> security designs to assume everything is allowed.

Defensive programming is definitely a good idea here.  We have
experimented with assigning defensive default values for sandbox flags
in this way.  There are three locations where such a defensive
initialization can be made: HTMLFrameOwnerElement, FrameLoader and
SecurityOrigin.

The FrameLoader modification is straight-forward to make (changing to
'SandboxAll' in the constructor m_sandboxFlags initializer).  The
tests run fine with this change.  We excluded it from the current
patch, but we'll be happy to include it if you want us to.

I'm not sure about the other two modifications, though.  Here's why.


Default SecurityOrigin::m_sandboxFlags = SandboxAll
---------------------------------------------------

This modification fails for documents created without a frame.  The
test case http/tests/security/cookies/create-document.html is a
typical example of something that breaks.

> Is there any problem with the fact that documents that never go into a frame,
> such as ones made with XMLHttpRequest, have no sandbox flags set?

Yes, many (quite possibly all) XMLHttpRequest-based tests seem to fail
with this modification.

My interpretation of the spec (and comment #14) is that sandbox flags
can only end up in a document (specifically, its SecurityOrigin) by
being propagated from the document's frame.  No frame, no sandbox
flags.  I can't see how to assign a default SecurityOrigin flag value
other than SandboxNone based on this.


Default HTMLFrameOwnerElement::m_sandboxFlags = SandboxAll
----------------------------------------------------------

This modification fails for iframes without a sandbox attribute, which
now become sandboxed (of course).

If there is a better place than the constructor to assign a default
value, this modification could make more sense.

[Darin Adler]

(In reply to comment #58)
> Default SecurityOrigin::m_sandboxFlags = SandboxAll
> ---------------------------------------------------
> 
> This modification fails for documents created without a frame.  The
> test case http/tests/security/cookies/create-document.html is a
> typical example of something that breaks.

So the correct sandbox flags is "all" in a case like this? Can that really be right? Or should the sandbox flags somehow be inherited from the frame that's doing the creating?

[Patrik Persson]

Created attachment 44069 [details]
Slightly revised patch (added defensive FrameLoader initialization)

(In reply to comment #59)
> So the correct sandbox flags is "all" in a case like this? Can that really be
> right? Or should the sandbox flags somehow be inherited from the frame that's
> doing the creating?

Copying the flags from the creating frame would probably be safe.
However, there are quite many locations where SecurityOrigins are
instantiated (about 20 sites, 10 classes in different parts of
WebCore).  Each of these locations would need modification to locate
the creating frame, and I don't currently know of a common way to do
that.

I'm not sure there is any added safety in this anyway.  Sandbox flags
have no meaning without a frame context (in my interpretation of HTML5
at least).

I propose we leave this out of the patch, to avoid against adding this
complexity for (limited) defensive purposes.  Defensive initial values
could help us catch bugs, but this complexity could help create them
too.

I still think your proposal for defensive initialization in the
FrameLoader makes good sense.  I'm attaching a slightly revised patch
with that included.

[Darin Adler]

(In reply to comment #60)
> Copying the flags from the creating frame would probably be safe.
> However, there are quite many locations where SecurityOrigins are
> instantiated (about 20 sites, 10 classes in different parts of
> WebCore).  Each of these locations would need modification to locate
> the creating frame, and I don't currently know of a common way to do
> that.
> 
> I'm not sure there is any added safety in this anyway.  Sandbox flags
> have no meaning without a frame context (in my interpretation of HTML5
> at least).

I do not want this issue to block the patch. But separate from this patch, I think we should not drop this.

I suggest that we set the sandbox flags on such documents to a value that means "this document has no sandboxing context" by default. And put assertions into sandbox-flag-checking accessors to make sure such flags are never checked. If they are checked, then I want to understand why it's OK to have the values initialized to the liberal "allow anything" value.

[Adam Barth]

Comment on attachment 44069 [details]
Slightly revised patch (added defensive FrameLoader initialization)

I'd like to land this so we can start iterating.  Let's file bugs for the outstanding issues.  It might also be useful to create a meta bug to track all the feature.

[WebKit Commit Bot]

Comment on attachment 44069 [details]
Slightly revised patch (added defensive FrameLoader initialization)

Rejecting patch 44069 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Darin Adler', '--force']" exit_code: 1
Last 500 characters of output:
ol-sandboxed-iframe-allow.cgi
patching file LayoutTests/http/tests/xmlhttprequest/resources/access-control-sandboxed-iframe-denied-iframe.html
patching file LayoutTests/http/tests/xmlhttprequest/resources/access-control-sandboxed-iframe-denied-without-wildcard-iframe.html
patching file LayoutTests/http/tests/xmlhttprequest/resources/access-control-sandboxed-iframe-denied-without-wildcard.cgi
patching file LayoutTests/http/tests/xmlhttprequest/resources/access-control-sandboxed-iframe-denied.cgi

[Adam Barth]

I'll check whether these conflicts are trivial.

[Adam Barth]

Created attachment 44108 [details]
Patch

[Adam Barth]

Comment on attachment 44108 [details]
Patch

The merge was in fact trivial.  Let's try commit-queue again, but I suspect we'll have trouble with the *.cgi files not being marked as executable.  If that's the case, I'll land when I get home tonight.

[WebKit Commit Bot]

Comment on attachment 44108 [details]
Patch

Rejecting patch 44108 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit']" exit_code: 1
Last 500 characters of output:
HostCustom.cpp -o /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/i386/JSInjectedScriptHostCustom.o
** BUILD FAILED **

The following build commands failed:
WebCore:
	Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/i386/InspectorController.o /Users/eseidel/Projects/CommitQueue/WebCore/inspector/InspectorController.cpp normal i386 c++ com.apple.compilers.gcc.4_2
(1 failure)

[Adam Barth]

Comment on attachment 44108 [details]
Patch

Actually, I suck at merging.  Landing by hand now.

[Adam Barth]

Mostly landed in 51577.  I still need to do some SVN propset magic to land one of the tests.

[Adam Barth]

Committed r51580: <http://trac.webkit.org/changeset/51580>

[Adam Barth]

Have all the followup bugs been filed?  I can mine the comments and file the followups if that would be helpful.

[Patrik Persson]

Created attachment 44618 [details]
Test case for "about:blank" potential issue

(In reply to comment #28)
> There's also a subtle bug for the case of
> 
> <iframe src="about:blank" sandbox></iframe>
> 
> I think that will end up sandboxing the containing frame because the
> SecurityOrigin object is shared by the two frames.

I have not been able to reproduce this bug.  I'm attaching a test case
that does what you describe, but it passes, indicating that the
containing frame is not sandboxed.

I know that your comment is based on knowledge of the code.  Perhaps
this test case works by accident, or because I misunderstood you.  Can
I ask you to file a bug report with more hints on how to reproduce it?

[Patrik Persson]

(In reply to comment #71)
> Have all the followup bugs been filed?  I can mine the comments and file the
> followups if that would be helpful.

I have filed some followups, listed below.  If you find more, your
help is much appreciated.  Notably, the potential bug for
"about:blank" has not been addressed (see previous comment 72).


Incorrect code generated from IDL for document.cookie unless
declaration syntax is carefully adjusted:
  https://bugs.webkit.org/show_bug.cgi?id=31720

ScriptController::isEnabled needs to be renamed:
  https://bugs.webkit.org/show_bug.cgi?id=32063

document.cookie accessors should raise INVALID_STATE_ERR if there is
no browsing context:
  https://bugs.webkit.org/show_bug.cgi?id=32115

Add defensive initialization of iframe sandbox flags:
  https://bugs.webkit.org/show_bug.cgi?id=32368

Support for storage and databases in sandboxed iframes:
  https://bugs.webkit.org/show_bug.cgi?id=32369

Unify origin sandbox flag with m_noAccess in SecurityOrigin:
  https://bugs.webkit.org/show_bug.cgi?id=32372

[Adam Barth]

Comment on attachment 44618 [details]
Test case for "about:blank" potential issue

Interesting.  Ok, well, we should land the test.  Thanks!

Also, thanks for filing the followups.  I can help with fixing some of them.