Attachment 46242 Details for Bug 32368 – Patch

[patch] Patch

bug-32368-20100110131218.patch (text/plain), 14.22 KB, created by Adam Barth on 2010-01-10 13:12:19 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Adam Barth

Created: 2010-01-10 13:12:19 PST

Size: 14.22 KB

patch

obsolete

>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index b68637b..80d85d9 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,23 @@
>+2010-01-10  Adam Barth  <abarth@webkit.org>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add defensive initialization of iframe sandbox flags
>+        https://bugs.webkit.org/show_bug.cgi?id=32368
>+
>+        Test the mutating the allow-same-origin directive in the sandbox
>+        attribute does not affect the current document (but does affect the
>+        next document).
>+
>+        * http/tests/security/resources/sandboxed-iframe-origin-add-step1.html: Added.
>+        * http/tests/security/resources/sandboxed-iframe-origin-add-step2.html: Added.
>+        * http/tests/security/resources/sandboxed-iframe-origin-remove-step1.html: Added.
>+        * http/tests/security/resources/sandboxed-iframe-origin-remove-step2.html: Added.
>+        * http/tests/security/sandboxed-iframe-origin-add-expected.txt: Added.
>+        * http/tests/security/sandboxed-iframe-origin-add.html: Added.
>+        * http/tests/security/sandboxed-iframe-origin-remove-expected.txt: Added.
>+        * http/tests/security/sandboxed-iframe-origin-remove.html: Added.
>+
> 2010-01-09  Daniel Bates  <dbates@webkit.org>
> 
>         No review, rolling out r53044.
>diff --git a/LayoutTests/http/tests/security/resources/sandboxed-iframe-origin-add-step1.html b/LayoutTests/http/tests/security/resources/sandboxed-iframe-origin-add-step1.html
>new file mode 100644
>index 0000000..6c79e77
>--- /dev/null
>+++ b/LayoutTests/http/tests/security/resources/sandboxed-iframe-origin-add-step1.html
>@@ -0,0 +1,11 @@
>+<script>
>+window.addEventListener("message", function (evt) {
>+    try {
>+        top.fail();
>+    } catch(e) {
>+        top.postMessage("step1-done", "*");
>+    }
>+}, false);
>+
>+top.postMessage("step1-ready", "*");
>+</script>
>diff --git a/LayoutTests/http/tests/security/resources/sandboxed-iframe-origin-add-step2.html b/LayoutTests/http/tests/security/resources/sandboxed-iframe-origin-add-step2.html
>new file mode 100644
>index 0000000..07fd776
>--- /dev/null
>+++ b/LayoutTests/http/tests/security/resources/sandboxed-iframe-origin-add-step2.html
>@@ -0,0 +1,3 @@
>+<script>
>+top.pass();
>+</script>
>diff --git a/LayoutTests/http/tests/security/resources/sandboxed-iframe-origin-remove-step1.html b/LayoutTests/http/tests/security/resources/sandboxed-iframe-origin-remove-step1.html
>new file mode 100644
>index 0000000..fcfaf08
>--- /dev/null
>+++ b/LayoutTests/http/tests/security/resources/sandboxed-iframe-origin-remove-step1.html
>@@ -0,0 +1,8 @@
>+<script>
>+window.addEventListener("message", function (evt) {
>+    top.pass();
>+    top.postMessage("step1-done", "*");
>+}, false);
>+
>+top.postMessage("step1-ready", "*");
>+</script>
>diff --git a/LayoutTests/http/tests/security/resources/sandboxed-iframe-origin-remove-step2.html b/LayoutTests/http/tests/security/resources/sandboxed-iframe-origin-remove-step2.html
>new file mode 100644
>index 0000000..2a92a60
>--- /dev/null
>+++ b/LayoutTests/http/tests/security/resources/sandboxed-iframe-origin-remove-step2.html
>@@ -0,0 +1,7 @@
>+<script>
>+try {
>+    top.fail();
>+} catch(e) {
>+    top.postMessage("step2-done", "*");
>+}
>+</script>
>diff --git a/LayoutTests/http/tests/security/sandboxed-iframe-origin-add-expected.txt b/LayoutTests/http/tests/security/sandboxed-iframe-origin-add-expected.txt
>new file mode 100644
>index 0000000..b18d6d5
>--- /dev/null
>+++ b/LayoutTests/http/tests/security/sandboxed-iframe-origin-add-expected.txt
>@@ -0,0 +1,6 @@
>+CONSOLE MESSAGE: line 1: Unsafe JavaScript attempt to access frame with URL http://127.0.0.1:8000/security/sandboxed-iframe-origin-add.html from frame with URL http://127.0.0.1:8000/security/resources/sandboxed-iframe-origin-add-step1.html. Domains, protocols and ports must match.
>+
>+Test that adding allow-origin after creating an iframe doesn't modify the origin of an existing document, but it doesn modify the origin of the next document.
>+
>+PASS
>+
>diff --git a/LayoutTests/http/tests/security/sandboxed-iframe-origin-add.html b/LayoutTests/http/tests/security/sandboxed-iframe-origin-add.html
>new file mode 100644
>index 0000000..9bf3e7c
>--- /dev/null
>+++ b/LayoutTests/http/tests/security/sandboxed-iframe-origin-add.html
>@@ -0,0 +1,50 @@
>+<html>
>+<head>
>+<script>
>+if (window.layoutTestController) {
>+    layoutTestController.dumpAsText();
>+    layoutTestController.waitUntilDone();
>+}
>+
>+function log(msg) {
>+    var div = document.createElement("div");
>+    div.innerText = msg;
>+    document.getElementById("console").appendChild(div);
>+}
>+
>+function fail() {
>+    log("FAIL: The origin of the document was mutated.");
>+    if (window.layoutTestController)
>+        layoutTestController.notifyDone();
>+}
>+
>+function pass() {
>+    log("PASS");
>+    if (window.layoutTestController)
>+        layoutTestController.notifyDone();
>+}
>+
>+window.addEventListener("message", function (evt) {
>+    var fr = document.getElementById("theFrame");
>+    if (evt.data == "step1-ready") {
>+        fr.sandbox = "allow-scripts allow-same-origin";
>+        fr.contentWindow.postMessage("go", "*");
>+    } else if (evt.data == "step1-done")
>+        fr.src = "resources/sandboxed-iframe-origin-add-step2.html";
>+}, false);
>+
>+</script>
>+</head>
>+
>+<body>
>+<p>Test that adding allow-origin after creating an iframe doesn't modify
>+the origin of an existing document, but it doesn modify the origin of the
>+next document.</p>
>+
>+<div id="console"></div>
>+ 
>+<iframe id="theFrame" sandbox="allow-scripts"
>+        src="resources/sandboxed-iframe-origin-add-step1.html">
>+</iframe>
>+</body>
>+</html>
>diff --git a/LayoutTests/http/tests/security/sandboxed-iframe-origin-remove-expected.txt b/LayoutTests/http/tests/security/sandboxed-iframe-origin-remove-expected.txt
>new file mode 100644
>index 0000000..fda220c
>--- /dev/null
>+++ b/LayoutTests/http/tests/security/sandboxed-iframe-origin-remove-expected.txt
>@@ -0,0 +1,6 @@
>+CONSOLE MESSAGE: line 1: Unsafe JavaScript attempt to access frame with URL http://127.0.0.1:8000/security/sandboxed-iframe-origin-remove.html from frame with URL http://127.0.0.1:8000/security/resources/sandboxed-iframe-origin-remove-step2.html. Domains, protocols and ports must match.
>+
>+Test that removing allow-origin after creating an iframe doesn't modify the origin of an existing document, but it doesn modify the origin of the next document.
>+
>+PASS
>+
>diff --git a/LayoutTests/http/tests/security/sandboxed-iframe-origin-remove.html b/LayoutTests/http/tests/security/sandboxed-iframe-origin-remove.html
>new file mode 100644
>index 0000000..3e533a4
>--- /dev/null
>+++ b/LayoutTests/http/tests/security/sandboxed-iframe-origin-remove.html
>@@ -0,0 +1,52 @@
>+<html>
>+<head>
>+<script>
>+if (window.layoutTestController) {
>+    layoutTestController.dumpAsText();
>+    layoutTestController.waitUntilDone();
>+}
>+
>+function log(msg) {
>+    var div = document.createElement("div");
>+    div.innerText = msg;
>+    document.getElementById("console").appendChild(div);
>+}
>+
>+function fail() {
>+    log("FAIL: The origin of the document was mutated.");
>+    if (window.layoutTestController)
>+        layoutTestController.notifyDone();
>+}
>+
>+function pass() {
>+    log("PASS");
>+}
>+
>+window.addEventListener("message", function (evt) {
>+    var fr = document.getElementById("theFrame");
>+    if (evt.data == "step1-ready") {
>+        fr.sandbox = "allow-scripts";
>+        fr.contentWindow.postMessage("go", "*");
>+    } else if (evt.data == "step1-done")
>+        fr.src = "resources/sandboxed-iframe-origin-remove-step2.html";
>+    else if (evt.data == "step2-done") {
>+        if (window.layoutTestController)
>+            layoutTestController.notifyDone();
>+    }
>+}, false);
>+
>+</script>
>+</head>
>+
>+<body>
>+<p>Test that removing allow-origin after creating an iframe doesn't modify
>+the origin of an existing document, but it doesn modify the origin of the
>+next document.</p>
>+
>+<div id="console"></div>
>+ 
>+<iframe id="theFrame" sandbox="allow-scripts allow-same-origin"
>+        src="resources/sandboxed-iframe-origin-remove-step1.html">
>+</iframe>
>+</body>
>+</html>
>diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
>index cbb67d3..1657407 100644
>--- a/WebCore/ChangeLog
>+++ b/WebCore/ChangeLog
>@@ -1,5 +1,27 @@
> 2010-01-10  Adam Barth  <abarth@webkit.org>
> 
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add defensive initialization of iframe sandbox flags
>+        https://bugs.webkit.org/show_bug.cgi?id=32368
>+
>+        We now initialize the SecurityOrigin's sandbox state directly duing
>+        construction.  This lets us properly set the isUnique bit.
>+
>+        Tests: http/tests/security/sandboxed-iframe-origin-add.html
>+               http/tests/security/sandboxed-iframe-origin-remove.html
>+
>+        * WebCore.base.exp:
>+        * dom/Document.cpp:
>+        (WebCore::Document::initSecurityContext):
>+        * page/SecurityOrigin.cpp:
>+        (WebCore::SecurityOrigin::SecurityOrigin):
>+        (WebCore::SecurityOrigin::create):
>+        (WebCore::SecurityOrigin::setSandboxFlags):
>+        * page/SecurityOrigin.h:
>+
>+2010-01-10  Adam Barth  <abarth@webkit.org>
>+
>         Unreviewed whitespace fixes.
> 
>         * html/HTMLFrameOwnerElement.cpp:
>diff --git a/WebCore/WebCore.base.exp b/WebCore/WebCore.base.exp
>index 7e854b4..3399a4e 100644
>--- a/WebCore/WebCore.base.exp
>+++ b/WebCore/WebCore.base.exp
>@@ -300,7 +300,7 @@ __ZN7WebCore14SecurityOrigin18shouldHideReferrerERKNS_4KURLERKNS_6StringE
> __ZN7WebCore14SecurityOrigin24registerURLSchemeAsLocalERKNS_6StringE
> __ZN7WebCore14SecurityOrigin25whiteListAccessFromOriginERKS0_RKNS_6StringES5_b
> __ZN7WebCore14SecurityOrigin27resetOriginAccessWhiteListsEv
>-__ZN7WebCore14SecurityOrigin6createERKNS_4KURLE
>+__ZN7WebCore14SecurityOrigin6createERKNS_4KURLEi
> __ZN7WebCore14SecurityOrigin7canLoadERKNS_4KURLERKNS_6StringEPNS_8DocumentE
> __ZN7WebCore15ArchiveResource6createEN3WTF10PassRefPtrINS_12SharedBufferEEERKNS_4KURLERKNS_6StringESA_SA_RKNS_16ResourceResponseE
> __ZN7WebCore15BackForwardList10removeItemEPNS_11HistoryItemE
>diff --git a/WebCore/dom/Document.cpp b/WebCore/dom/Document.cpp
>index fe24a57..37635f8 100644
>--- a/WebCore/dom/Document.cpp
>+++ b/WebCore/dom/Document.cpp
>@@ -4340,10 +4340,8 @@ void Document::initSecurityContext()
>     // loading URL.
>     const KURL& url = m_frame->loader()->url();
>     m_cookieURL = url;
>-    ScriptExecutionContext::setSecurityOrigin(SecurityOrigin::create(url));
>+    ScriptExecutionContext::setSecurityOrigin(SecurityOrigin::create(url, m_frame->loader()->sandboxFlags()));
> 
>-    updateSandboxFlags();
>- 
>     if (SecurityOrigin::allowSubstituteDataAccessToLocal()) {
>         // If this document was loaded with substituteData, then the document can
>         // load local resources.  See https://bugs.webkit.org/show_bug.cgi?id=16756
>diff --git a/WebCore/page/SecurityOrigin.cpp b/WebCore/page/SecurityOrigin.cpp
>index cdc80bc..79534c5 100644
>--- a/WebCore/page/SecurityOrigin.cpp
>+++ b/WebCore/page/SecurityOrigin.cpp
>@@ -77,8 +77,8 @@ static URLSchemesMap& schemesWithUniqueOrigins()
>     return schemesWithUniqueOrigins;
> }
> 
>-SecurityOrigin::SecurityOrigin(const KURL& url)
>-    : m_sandboxFlags(SandboxNone)
>+SecurityOrigin::SecurityOrigin(const KURL& url, SandboxFlags sandboxFlags)
>+    : m_sandboxFlags(sandboxFlags)
>     , m_protocol(url.protocol().isNull() ? "" : url.protocol().lower())
>     , m_host(url.host().isNull() ? "" : url.host().lower())
>     , m_port(url.port())
>@@ -94,9 +94,8 @@ SecurityOrigin::SecurityOrigin(const KURL& url)
>     if (shouldTreatURLSchemeAsNoAccess(m_protocol))
>         m_isUnique = true;
> 
>-    // If this ASSERT becomes false in the future, please consider the impact
>-    // of m_sandoxFlags on m_isUnique.
>-    ASSERT(m_sandboxFlags == SandboxNone);
>+    if (isSandboxed(SandboxOrigin))
>+        m_isUnique = true;
> 
>     // document.domain starts as m_host, but can be set by the DOM.
>     m_domain = m_host;
>@@ -131,11 +130,11 @@ bool SecurityOrigin::isEmpty() const
>     return m_protocol.isEmpty();
> }
> 
>-PassRefPtr<SecurityOrigin> SecurityOrigin::create(const KURL& url)
>+PassRefPtr<SecurityOrigin> SecurityOrigin::create(const KURL& url, SandboxFlags sandboxFlags)
> {
>     if (!url.isValid())
>-        return adoptRef(new SecurityOrigin(KURL()));
>-    return adoptRef(new SecurityOrigin(url));
>+        return adoptRef(new SecurityOrigin(KURL(), sandboxFlags));
>+    return adoptRef(new SecurityOrigin(url, sandboxFlags));
> }
> 
> PassRefPtr<SecurityOrigin> SecurityOrigin::createEmpty()
>@@ -271,18 +270,11 @@ void SecurityOrigin::grantUniversalAccess()
> 
> void SecurityOrigin::setSandboxFlags(SandboxFlags flags)
> {
>+    // Although you might think that we should set m_isUnique based on
>+    // SandboxOrigin, that's not actually the right behavior. We're supposed to
>+    // freeze the origin of a document when it is created, even if the sandbox
>+    // flags change after that point in time.
>     m_sandboxFlags = flags;
>-    if (isSandboxed(SandboxOrigin))
>-        m_isUnique = true;
>-
>-    // Although you might think that we should set m_isUnique to false when
>-    // flags doesn't contain SandboxOrigin, that's not actually the right
>-    // behavior. We're supposed to freeze the origin of a document when it
>-    // is created, even if the sandbox flags change after that point in time.
>-    //
>-    // FIXME: Our current behavior here is buggy because we need to
>-    //        distinguish between the sandbox flags at creation and the
>-    //        sandbox flags that might come about later.
> }
> 
> bool SecurityOrigin::isLocal() const
>diff --git a/WebCore/page/SecurityOrigin.h b/WebCore/page/SecurityOrigin.h
>index 4a6eb3c..a9f9857 100644
>--- a/WebCore/page/SecurityOrigin.h
>+++ b/WebCore/page/SecurityOrigin.h
>@@ -49,7 +49,7 @@ class SecurityOrigin : public ThreadSafeShared<SecurityOrigin> {
> public:
>     static PassRefPtr<SecurityOrigin> createFromDatabaseIdentifier(const String&);
>     static PassRefPtr<SecurityOrigin> createFromString(const String&);
>-    static PassRefPtr<SecurityOrigin> create(const KURL&);
>+    static PassRefPtr<SecurityOrigin> create(const KURL&, SandboxFlags sandboxFlags = SandboxNone);
>     static PassRefPtr<SecurityOrigin> createEmpty();
> 
>     // Create a deep copy of this SecurityOrigin. This method is useful
>@@ -187,7 +187,7 @@ public:
>     static void resetOriginAccessWhiteLists();
> 
> private:
>-    explicit SecurityOrigin(const KURL&);
>+    SecurityOrigin(const KURL&, SandboxFlags);
>     explicit SecurityOrigin(const SecurityOrigin*);
> 
>     SandboxFlags m_sandboxFlags;

Flags:

darin: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 32368: 46230 | 46242