Bug 175281 - Sandbox flags do not support document.domain control
Summary: Sandbox flags do not support document.domain control
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks: 175300
  Show dependency treegraph
 
Reported: 2017-08-07 13:12 PDT by Brent Fulgham
Modified: 2017-08-18 09:45 PDT (History)
7 users (show)

See Also:


Attachments
Patch (5.51 KB, patch)
2017-08-08 12:06 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (5.28 KB, patch)
2017-08-08 15:19 PDT, Brent Fulgham
cdumez: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2017-08-07 13:12:09 PDT
The SandboxingFlag enumeration is missing an option for "Document.Domain" access. We should add this so that websites can block frames from manipulating the frame's domain.
Comment 1 Brent Fulgham 2017-08-07 13:35:19 PDT
It seems like this might just be a matter of merging this Blink revision:

https://crrev.com/162703
Comment 2 Brent Fulgham 2017-08-07 13:35:42 PDT
Whoops. I meant:

https://bugs.chromium.org/p/chromium/issues/detail?id=277084
Comment 3 Radar WebKit Bug Importer 2017-08-08 11:03:46 PDT
<rdar://problem/33778936>
Comment 4 Brent Fulgham 2017-08-08 12:06:26 PDT
Created attachment 317594 [details]
Patch
Comment 5 Chris Dumez 2017-08-08 12:19:17 PDT
Comment on attachment 317594 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317594&action=review

> Source/WebCore/dom/Document.cpp:4490
> +    if (isSandboxed(SandboxDocumentDomain))

Eh.. Would you mind removing the "// FIXME(175281): Check for 'document.domain' sandbox flag and return an exception if present." comment a few lines below? :)

> LayoutTests/fast/frames/sandboxed-iframe-domain-expected.txt:6
> +Denied: SecurityError: Assignment is forbidden for sandboxed iframes.

I think this is going to be flaky. Given how the test is written, there is currently no guarantee the script in one particular iframe will run before the other. Therefore, those "Denied" lines may be switched.
I think the test need to be written in a way that first inserts the first iframe, tests it and then does so for a second iframe.

> LayoutTests/fast/frames/sandboxed-iframe-domain-expected.txt:7
> +Denied: SecurityError: The document has a null effectiveDomain.

Would this actually succeed if we moved the test to http/tests? If so, I'd suggest we do this.

> LayoutTests/fast/frames/sandboxed-iframe-domain.html:6
> +    testRunner.dumpAsText();

Not needed with js-test.

> LayoutTests/fast/frames/sandboxed-iframe-domain.html:23
> +            window.testRunner.notifyDone();

finishJSTest() since you're using js-test.

> LayoutTests/fast/frames/sandboxed-iframe-domain.html:27
> +description("This test verifies that a sandboxed iframe does not have permission to modify the document.domain property.");

I think it would look better if the description was at the beginning of the script.
Comment 6 Chris Dumez 2017-08-08 12:20:43 PDT
As an FYI, there is some discussion about removing document.origin from the spec:
https://github.com/whatwg/dom/issues/410
Comment 7 Brent Fulgham 2017-08-08 15:19:53 PDT
Created attachment 317627 [details]
Patch
Comment 8 Brent Fulgham 2017-08-08 15:25:09 PDT
Comment on attachment 317594 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317594&action=review

>> Source/WebCore/dom/Document.cpp:4490
>> +    if (isSandboxed(SandboxDocumentDomain))
> 
> Eh.. Would you mind removing the "// FIXME(175281): Check for 'document.domain' sandbox flag and return an exception if present." comment a few lines below? :)

Oh gosh! Doh!

>> LayoutTests/fast/frames/sandboxed-iframe-domain-expected.txt:6
>> +Denied: SecurityError: Assignment is forbidden for sandboxed iframes.
> 
> I think this is going to be flaky. Given how the test is written, there is currently no guarantee the script in one particular iframe will run before the other. Therefore, those "Denied" lines may be switched.
> I think the test need to be written in a way that first inserts the first iframe, tests it and then does so for a second iframe.

Good point.

I had originally hoped to be able to have one "succeeded" and one "denied", but the nature of our test environment prevents us from setting domain to anything legal.

So another option would just be to test the sandbox case, but I wanted to show that with the sandbox property, you get a sandbox error, and without you get different behavior.

>> LayoutTests/fast/frames/sandboxed-iframe-domain-expected.txt:7
>> +Denied: SecurityError: The document has a null effectiveDomain.
> 
> Would this actually succeed if we moved the test to http/tests? If so, I'd suggest we do this.

I just gave up on having a pass and a fail case. I can't get our test system (which runs on 127.0.0.1:8000 to play nice with CORS and the various rules for setting a domain as a subdomain of the current domain. It's just not feasible.

>> LayoutTests/fast/frames/sandboxed-iframe-domain.html:6
>> +    testRunner.dumpAsText();
> 
> Not needed with js-test.

OK!

>> LayoutTests/fast/frames/sandboxed-iframe-domain.html:23
>> +            window.testRunner.notifyDone();
> 
> finishJSTest() since you're using js-test.

OK!

>> LayoutTests/fast/frames/sandboxed-iframe-domain.html:27
>> +description("This test verifies that a sandboxed iframe does not have permission to modify the document.domain property.");
> 
> I think it would look better if the description was at the beginning of the script.

OK!
Comment 9 Brent Fulgham 2017-08-08 16:27:11 PDT
Committed r220427: <http://trac.webkit.org/changeset/220427>