Bug 178565

Summary: XMLHttpRequest should not treat file URLs as same origin
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebKit Misc.Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, buildbot, cdumez, commit-queue, dbates, japhet, rniwa, ryanhaddad, sam
Priority: P2 Keywords: BlinkMergeCandidate, InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 179481    
Bug Blocks: 183028    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews100 for mac-elcapitan
none
Archive of layout-test-results from ews113 for mac-elcapitan
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
Patch
none
Archive of layout-test-results from ews103 for mac-elcapitan
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Archive of layout-test-results from ews116 for mac-elcapitan
none
Patch
none
Patch
none
Archive of layout-test-results from ews102 for mac-elcapitan
none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews112 for mac-elcapitan
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Patch
none
Archive of layout-test-results from ews102 for mac-elcapitan
none
Archive of layout-test-results from ews112 for mac-elcapitan
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Patch
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Patch
none
Patch for landing
none
Patch for landing
none
Archive of layout-test-results from ews102 for mac-elcapitan
none
Archive of layout-test-results from ews116 for mac-elcapitan
none
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Patch for landing
none
Patch for landing
none
Patch for landing
none
Archive of layout-test-results from webkit-cq-03 for mac-elcapitan
none
Archive of layout-test-results from ews102 for mac-elcapitan
none
Patch for landing none

Description Brent Fulgham 2017-10-19 20:42:22 PDT
Make handling of XMLHttpRequest::open() URL argument more consistent. Currently, blank URL is treated as part of the current origin, while 'null' URL does not satisfy same-origin requirements.

Since other browsers treat both cases as not-satisfying same-origin requirements, we should do the same.
Comment 1 Brent Fulgham 2017-10-19 20:44:26 PDT
<rdar://problem/11115901>
Comment 2 Brent Fulgham 2017-10-19 20:50:11 PDT
Created attachment 324343 [details]
Patch
Comment 3 Build Bot 2017-10-19 21:56:24 PDT
Comment on attachment 324343 [details]
Patch

Attachment 324343 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4928403

New failing tests:
http/tests/xmlhttprequest/reentrant-cancel-abort.html
http/tests/xmlhttprequest/reentrant-cancel.html
fast/loader/subresource-load-failed-crash.html
Comment 4 Build Bot 2017-10-19 21:56:25 PDT
Created attachment 324349 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 5 Build Bot 2017-10-19 22:24:33 PDT
Comment on attachment 324343 [details]
Patch

Attachment 324343 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4928455

New failing tests:
http/tests/xmlhttprequest/reentrant-cancel-abort.html
http/tests/xmlhttprequest/reentrant-cancel.html
fast/loader/subresource-load-failed-crash.html
Comment 6 Build Bot 2017-10-19 22:24:34 PDT
Created attachment 324355 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 7 Build Bot 2017-10-19 22:32:12 PDT
Comment on attachment 324343 [details]
Patch

Attachment 324343 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4928451

New failing tests:
http/tests/xmlhttprequest/reentrant-cancel-abort.html
http/tests/xmlhttprequest/reentrant-cancel.html
fast/loader/subresource-load-failed-crash.html
Comment 8 Build Bot 2017-10-19 22:32:14 PDT
Created attachment 324359 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 9 Build Bot 2017-10-20 15:31:56 PDT
Comment on attachment 324343 [details]
Patch

Attachment 324343 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4937196

New failing tests:
fast/loader/subresource-load-failed-crash.html
http/tests/xmlhttprequest/reentrant-cancel.html
http/tests/xmlhttprequest/reentrant-cancel-abort.html
Comment 10 Build Bot 2017-10-20 15:31:57 PDT
Created attachment 324451 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 11 Brent Fulgham 2017-10-20 15:33:58 PDT
Created attachment 324453 [details]
Patch
Comment 12 Brent Fulgham 2017-10-20 15:53:48 PDT
(In reply to Brent Fulgham from comment #0)
> Make handling of XMLHttpRequest::open() URL argument more consistent.
> Currently, blank URL is treated as part of the current origin, while 'null'
> URL does not satisfy same-origin requirements.
> 
> Since other browsers treat both cases as not-satisfying same-origin
> requirements, we should do the same.

After debugging further, it's much simpler than that. We just want to avoid granting same-origin for file URLs during XHR requests.
Comment 13 Ryosuke Niwa 2017-10-20 16:04:42 PDT
Comment on attachment 324453 [details]
Patch

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

> Source/WebCore/loader/DocumentThreadableLoader.cpp:133
> +    if (request.requester() == ResourceRequest::Requester::XHR && request.url().protocolIs("file") && !securityOrigin().hasUniversalAccess())

Can we add a helper function to securityOrigin to check this condition instead?
Spreading out security checks like this everywhere could lead to different parts of WebCore having different cross origin vulnerabilities.
Comment 14 Build Bot 2017-10-20 16:12:30 PDT
Comment on attachment 324453 [details]
Patch

Attachment 324453 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4937581

New failing tests:
fast/xmlhttprequest/xmlhttprequest-nonexistent-file.html
Comment 15 Build Bot 2017-10-20 16:12:32 PDT
Created attachment 324460 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 16 Build Bot 2017-10-20 17:01:16 PDT
Comment on attachment 324453 [details]
Patch

Attachment 324453 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4937840

New failing tests:
fast/xmlhttprequest/xmlhttprequest-nonexistent-file.html
Comment 17 Build Bot 2017-10-20 17:01:17 PDT
Created attachment 324469 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 18 Build Bot 2017-10-20 17:08:45 PDT
Comment on attachment 324453 [details]
Patch

Attachment 324453 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4937778

New failing tests:
fast/xmlhttprequest/xmlhttprequest-nonexistent-file.html
Comment 19 Build Bot 2017-10-20 17:08:46 PDT
Created attachment 324474 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 20 Brent Fulgham 2017-10-20 17:13:35 PDT
Created attachment 324475 [details]
Patch
Comment 21 Brent Fulgham 2017-10-20 17:14:24 PDT
Note: The patch (and rebaselined test) now matches Blink behavior.
Comment 22 Ryosuke Niwa 2017-10-20 17:21:52 PDT
Comment on attachment 324475 [details]
Patch

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

> Source/WebCore/loader/DocumentThreadableLoader.cpp:133
> +    if (request.requester() == ResourceRequest::Requester::XHR && request.url().protocolIs("file") && !securityOrigin().hasUniversalAccess())

Can we add a helper function to securityOrigin to check this condition instead?
Spreading out security checks like this everywhere could lead to different parts of WebCore having different cross origin vulnerabilities.
Comment 23 Daniel Bates 2017-10-20 17:46:36 PDT
Comment on attachment 324475 [details]
Patch

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

>> Source/WebCore/loader/DocumentThreadableLoader.cpp:133
>> +    if (request.requester() == ResourceRequest::Requester::XHR && request.url().protocolIs("file") && !securityOrigin().hasUniversalAccess())
> 
> Can we add a helper function to securityOrigin to check this condition instead?
> Spreading out security checks like this everywhere could lead to different parts of WebCore having different cross origin vulnerabilities.

request.url().protocolIs("file") this is not sufficient. We have other protocols that are file like.
Comment 24 Daniel Bates 2017-10-20 17:47:59 PDT
(In reply to Brent Fulgham from comment #0)
> Make handling of XMLHttpRequest::open() URL argument more consistent.
> Currently, blank URL is treated as part of the current origin, while 'null'
> URL does not satisfy same-origin requirements.
> 
> Since other browsers treat both cases as not-satisfying same-origin
> requirements, we should do the same.

How are you comparing browsers? In particular, are you comparing Safari/WebKit behavior with or without local file restrictions enabled.
Comment 25 Brent Fulgham 2017-10-20 19:47:32 PDT
(In reply to Daniel Bates from comment #24)
> (In reply to Brent Fulgham from comment #0)
> > Make handling of XMLHttpRequest::open() URL argument more consistent.
> > Currently, blank URL is treated as part of the current origin, while 'null'
> > URL does not satisfy same-origin requirements.
> > 
> > Since other browsers treat both cases as not-satisfying same-origin
> > requirements, we should do the same.
> 
> How are you comparing browsers? In particular, are you comparing
> Safari/WebKit behavior with or without local file restrictions enabled.

I’m comparing shipping state (so, local file restrictions enable for Safari and Chrome).
Comment 26 Brent Fulgham 2017-10-21 16:48:40 PDT
Created attachment 324511 [details]
Patch
Comment 27 Build Bot 2017-10-21 17:36:45 PDT
Comment on attachment 324511 [details]
Patch

Attachment 324511 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4947495

Number of test failures exceeded the failure limit.
Comment 28 Build Bot 2017-10-21 17:36:47 PDT
Created attachment 324512 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 29 Build Bot 2017-10-21 17:40:08 PDT
Comment on attachment 324511 [details]
Patch

Attachment 324511 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4947498

Number of test failures exceeded the failure limit.
Comment 30 Build Bot 2017-10-21 17:40:10 PDT
Created attachment 324513 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 31 Build Bot 2017-10-21 17:45:55 PDT
Comment on attachment 324511 [details]
Patch

Attachment 324511 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4947493

Number of test failures exceeded the failure limit.
Comment 32 Build Bot 2017-10-21 17:45:56 PDT
Created attachment 324515 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 33 Build Bot 2017-10-21 17:58:07 PDT
Comment on attachment 324511 [details]
Patch

Attachment 324511 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4947514

Number of test failures exceeded the failure limit.
Comment 34 Build Bot 2017-10-21 17:58:09 PDT
Created attachment 324516 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 35 Ryosuke Niwa 2017-10-24 10:20:33 PDT
Comment on attachment 324511 [details]
Patch

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

> Source/WebCore/page/SecurityOrigin.cpp:329
> +    // Do not treat XHR to file URLs as being same-origin (unless universal access is being used)
> +    if (request.requester() == ResourceRequest::Requester::XHR && !request.url().protocolIs("file"))

How about blob of a file URL origin?

> LayoutTests/ChangeLog:9
> +        Rebaseline test now that we reject XHR to local file URLs.

You should definitely add a new test case for this specific behavior.
The test is testing reading a nonexistent file.
We need a test for when reading an existing file URL this code change prevents.
Comment 36 Brent Fulgham 2017-10-24 10:23:36 PDT
Created attachment 324682 [details]
Patch
Comment 37 Ryosuke Niwa 2017-10-24 10:33:50 PDT
Comment on attachment 324682 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Do not treat file URLs as same-origin for XHR requests.

This definitely needs a test.

> Source/WebCore/page/SecurityOrigin.cpp:328
> +    // Do not treat XHR to file URLs as being same-origin (unless universal access is being used)

How about blob URL of file-origin?
Comment 38 Brent Fulgham 2017-10-24 10:43:06 PDT
(In reply to Ryosuke Niwa from comment #37)
> Comment on attachment 324682 [details]
> Patch
> 
> > Source/WebCore/page/SecurityOrigin.cpp:328
> > +    // Do not treat XHR to file URLs as being same-origin (unless universal access is being used)
> 
> How about blob URL of file-origin?

I don't think it is an issue, because we are only concerned with cases where the "canRequest" call returns true, which means that the URL has the same scheme, host, and port as the current security origin. I don't think a Blob (file-origin) URL would match the security origin of a "file:///" URL under that check.

But, I suppose the originating document could have been from a Blob (file-origin), which might get here...

I'll add it.
Comment 39 Brent Fulgham 2017-10-24 10:43:42 PDT
Comment on attachment 324682 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        Do not treat file URLs as same-origin for XHR requests.
> 
> This definitely needs a test.

Yes -- I have one.
Comment 40 Build Bot 2017-10-24 11:27:03 PDT
Comment on attachment 324682 [details]
Patch

Attachment 324682 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4971581

New failing tests:
fast/xmlhttprequest/xmlhttprequest-nonexistent-file.html
Comment 41 Build Bot 2017-10-24 11:27:04 PDT
Created attachment 324688 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 42 Build Bot 2017-10-24 11:53:56 PDT
Comment on attachment 324682 [details]
Patch

Attachment 324682 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4971629

New failing tests:
fast/xmlhttprequest/xmlhttprequest-nonexistent-file.html
Comment 43 Build Bot 2017-10-24 11:53:57 PDT
Created attachment 324693 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 44 Build Bot 2017-10-24 11:57:50 PDT
Comment on attachment 324682 [details]
Patch

Attachment 324682 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4971663

New failing tests:
fast/xmlhttprequest/xmlhttprequest-nonexistent-file.html
Comment 45 Build Bot 2017-10-24 11:57:52 PDT
Created attachment 324695 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 46 Build Bot 2017-10-24 13:12:11 PDT
Comment on attachment 324682 [details]
Patch

Attachment 324682 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4972810

Number of test failures exceeded the failure limit.
Comment 47 Build Bot 2017-10-24 13:12:13 PDT
Created attachment 324710 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 48 Brent Fulgham 2017-10-25 08:55:17 PDT
Created attachment 324828 [details]
Patch
Comment 49 Build Bot 2017-10-25 10:29:57 PDT
Comment on attachment 324828 [details]
Patch

Attachment 324828 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4984232

New failing tests:
fast/xmlhttprequest/xmlhttprequest-nonexistent-file.html
fast/xmlhttprequest/xmlhttprequest-access-self-as-file.html
Comment 50 Build Bot 2017-10-25 10:29:59 PDT
Created attachment 324843 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 51 Brent Fulgham 2017-10-25 11:22:26 PDT
Created attachment 324854 [details]
Patch
Comment 52 Ryosuke Niwa 2017-10-25 13:16:58 PDT
Comment on attachment 324854 [details]
Patch

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

> Source/WebCore/page/SecurityOrigin.cpp:336
> +    if (request.url().protocolIsBlob()) {
> +        if (auto blobOrigin = getCachedOrigin(request.url()))

request.url().protocolIsBlob() is already checked in getCachedOrigin.
We can just call getCachedOrigin. It's such a misleading name though...

> LayoutTests/fast/xmlhttprequest/resources/xmlhttprequest-access-self-as-file-real.html:19
> +        xhr.open("GET", "", false);
> +        xhr.send("");

Please add a test case for blob URL.
Comment 53 Brent Fulgham 2017-10-25 21:17:47 PDT
Created attachment 324951 [details]
Patch for landing
Comment 54 Brent Fulgham 2017-10-25 21:21:35 PDT
Created attachment 324952 [details]
Patch for landing
Comment 55 WebKit Commit Bot 2017-10-26 00:06:01 PDT
The commit-queue encountered the following flaky tests while processing attachment 324952 [details]:

The commit-queue is continuing to process your patch.
Comment 56 Build Bot 2017-10-26 00:24:00 PDT
Comment on attachment 324952 [details]
Patch for landing

Attachment 324952 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4995017

New failing tests:
fast/xmlhttprequest/xmlhttprequest-access-self-as-file.html
fast/xmlhttprequest/xmlhttprequest-access-self-as-blob.html
Comment 57 Build Bot 2017-10-26 00:24:02 PDT
Created attachment 324970 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 58 Build Bot 2017-10-26 01:31:01 PDT
Comment on attachment 324952 [details]
Patch for landing

Attachment 324952 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4995444

New failing tests:
fast/xmlhttprequest/xmlhttprequest-access-self-as-file.html
Comment 59 Build Bot 2017-10-26 01:31:03 PDT
Created attachment 324975 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 60 Build Bot 2017-10-26 02:38:14 PDT
Comment on attachment 324952 [details]
Patch for landing

Attachment 324952 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4995846

New failing tests:
fast/xmlhttprequest/xmlhttprequest-access-self-as-file.html
Comment 61 Build Bot 2017-10-26 02:38:15 PDT
Created attachment 324977 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 62 Daniel Bates 2017-10-26 03:02:12 PDT
Comment on attachment 324952 [details]
Patch for landing

Blink fixed this bug in <https://chromium.googlesource.com/chromium/src/+/c362e001551abc2bea392773f32eaf043d8bc29f>. Can we please merge this change or make a similar change?
Comment 63 Brent Fulgham 2017-10-26 07:42:44 PDT
Created attachment 325012 [details]
Patch for landing
Comment 64 Brent Fulgham 2017-10-26 07:49:35 PDT
(In reply to Daniel Bates from comment #62)
> Comment on attachment 324952 [details]
> Patch for landing
> 
> Blink fixed this bug in
> <https://chromium.googlesource.com/chromium/src/+/
> c362e001551abc2bea392773f32eaf043d8bc29f>. Can we please merge this change
> or make a similar change?

Wow -- I wish I had seen this earlier. Does their patch address the case of file-origin Blob URLs?
Comment 65 Brent Fulgham 2017-10-26 08:10:21 PDT
(In reply to Brent Fulgham from comment #64)
> (In reply to Daniel Bates from comment #62)
> > Comment on attachment 324952 [details]
> > Patch for landing
> > 
> > Blink fixed this bug in
> > <https://chromium.googlesource.com/chromium/src/+/
> > c362e001551abc2bea392773f32eaf043d8bc29f>. Can we please merge this change
> > or make a similar change?

I don't think this change is right for us, because it would break iOS handling of local file resources.

I also don't see how the Blink change is limited to XHR specifically. It seems like this change would break a number of platform uses (such as help documentation) that rely on file-to-file loading behavior.

For that reason, I do not want to take the Blink change at present.
Comment 66 Brent Fulgham 2017-10-26 08:14:36 PDT
Committed r224019: <https://trac.webkit.org/changeset/224019>
Comment 68 Daniel Bates 2017-10-26 10:42:48 PDT
Following the landing of this patch the security checks in XMLHttpRequest::createRequest(), <https://trac.webkit.org/browser/trunk/Source/WebCore/xml/XMLHttpRequest.cpp?rev=223868#L587>, are asymmetric with the security check used in DocumentThreadableLoader. I have not reasoned about the asymmetry. From my experience asymmetric checks almost always represent security vulnerabilities. (If the asymmetry does not represent a security vulnerability then we should add a comment in the DocumentThreadableLoader code to explain why). If we choose to use SecurityOrigin::requestIsSameOrigin() in DocumentThreadableLoader then we likely should update XMLHttpRequest::createRequest() to use it.

Additionally, it is now unclear what is the appropriate SecurityOrigin function to use to check the security of XHR requests: canRequest() or requestIsSameOrigin(). Notice that the documentation of SecurityOrigin::canRequest(), <https://trac.webkit.org/browser/trunk/Source/WebCore/page/SecurityOrigin.h?rev=224019#L98>, states it is applicable to XHR.

Moreover the name requestIsSameOrigin() does not convey to a reader its purpose with respect to our XHR policy and may be inadvertently chosen by a person not familiar with the details of SecurityOrigin over a more appropriate function due to the vagueness of its name. In general we want to make it straightforward for a developer to pick the appropriate SecurityOrigin function to use when needing to perform a security check. If the wrong security function is chosen then in the best case functionality is broken and in the worst case we have a security vulnerability.

(In reply to Brent Fulgham from comment #65)
> (In reply to Brent Fulgham from comment #64)
> > (In reply to Daniel Bates from comment #62)
> > > Comment on attachment 324952 [details]
> > > Patch for landing
> > > 
> > > Blink fixed this bug in
> > > <https://chromium.googlesource.com/chromium/src/+/
> > > c362e001551abc2bea392773f32eaf043d8bc29f>. Can we please merge this change
> > > or make a similar change?
> 
> I don't think this change is right for us, because it would break iOS
> handling of local file resources.
> 

Can you elaborate on this?

> I also don't see how the Blink change is limited to XHR specifically. It
> seems like this change would break a number of platform uses (such as help
> documentation) that rely on file-to-file loading behavior.
> 

I am unclear why we want to limit the defenses added to this patch to XHR. It seems applicable any time we want to determine if it is safe to read content.

Moreover this patch only adds a defense for file URLs. As I stated in comment #23 we have other schemes that we treat as file-like (e.g. applewebdata: on Cocoa platforms). And we provide SPI/API to add schemes to the list of schemes that we consider file-like. SecurityOrigin::isLocal() knows how to check if a scheme should be treated with the same privilege as file://. Why are the defenses we are adding in this patch not applicable to other local schemes?
Comment 69 Ryan Haddad 2017-10-26 10:45:25 PDT
Reverted r224019 for reason:

This change caused API test WebKit.MSEIsPlayingAudio to time out.

Committed r224025: <https://trac.webkit.org/changeset/224025>
Comment 70 Brent Fulgham 2017-10-26 15:11:31 PDT
(In reply to Daniel Bates from comment #68)
> > > > 
> > > > Blink fixed this bug in
> > > > <https://chromium.googlesource.com/chromium/src/+/
> > > > c362e001551abc2bea392773f32eaf043d8bc29f>. Can we please merge this change
> > > > or make a similar change?
> > 
> > I don't think this change is right for us, because it would break iOS
> > handling of local file resources.
> > 
> 
> Can you elaborate on this?

I was mistaken — it is for non-iOS cases:
There is code in SecurityOrigin protected by !PLATFORM(IOS) that uses the value of   m_filePath to determine that files are being accessed on the same volume. This is needed (rather than blindly preventing any file from viewing any other file) because we have many platform use cases where local file content needs to access other files in the system as part of help documentation, applications resources, and so forth.

Blink is able to get away with blocking all cross file access because they do not need to support these use cases.

> > I also don't see how the Blink change is limited to XHR specifically. It
> > seems like this change would break a number of platform uses (such as help
> > documentation) that rely on file-to-file loading behavior.
> > 
> 
> I am unclear why we want to limit the defenses added to this patch to XHR.
> It seems applicable any time we want to determine if it is safe to read
> content.

We already handle cross-file safety by limiting it to cross-volume checks and its quarantine state,

Blink addresses this by granting universal file access (mainly for development support), which is not appropriate in our platform for the reasons cited above. Universal file access is too blunt an instrument for our needs since it grants access to many other features we do not want to allow without developer opt-in.

> Moreover this patch only adds a defense for file URLs. As I stated in
> comment #23 we have other schemes that we treat as file-like (e.g.
> applewebdata: on Cocoa platforms). And we provide SPI/API to add schemes to
> the list of schemes that we consider file-like. SecurityOrigin::isLocal()
> knows how to check if a scheme should be treated with the same privilege as
> file://. Why are the defenses we are adding in this patch not applicable to
> other local schemes?

That’s a good point, and one I wish you had addressed when I asked for clarification last week.

Since Ryan rolled the current patch out due to TestWebkitAPI problems, i’ll See if I can tighten this further. It’s very frustrating that EWS does not identify TestWebKitAPI issues! :)
Comment 71 Daniel Bates 2017-10-26 21:41:47 PDT
(In reply to Brent Fulgham from comment #70)
> (In reply to Daniel Bates from comment #68)
> > > > > 
> > > > > Blink fixed this bug in
> > > > > <https://chromium.googlesource.com/chromium/src/+/
> > > > > c362e001551abc2bea392773f32eaf043d8bc29f>. Can we please merge this change
> > > > > or make a similar change?
> > > 
> > > I don't think this change is right for us, because it would break iOS
> > > handling of local file resources.
> > > 
> > 
> > Can you elaborate on this?
> 
> I was mistaken — it is for non-iOS cases:
> There is code in SecurityOrigin protected by !PLATFORM(IOS) that uses the
> value of   m_filePath to determine that files are being accessed on the same
> volume. This is needed (rather than blindly preventing any file from viewing
> any other file) because we have many platform use cases where local file
> content needs to access other files in the system as part of help
> documentation, applications resources, and so forth.
> 
> Blink is able to get away with blocking all cross file access because they
> do not need to support these use cases.
> 

Would it be sufficient to take the code change for SecurityOrigin::passesFileCheck() and the layout tests from the Blink change to fix this bug?
Comment 72 Brent Fulgham 2017-11-08 14:51:01 PST
Created attachment 326384 [details]
Patch for landing
Comment 73 WebKit Commit Bot 2017-11-08 14:52:16 PST
Comment on attachment 326384 [details]
Patch for landing

Rejecting attachment 326384 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 326384, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Dan Bates found in /Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.webkit.org/results/5153723
Comment 74 Brent Fulgham 2017-11-08 14:55:09 PST
Created attachment 326385 [details]
Patch for landing
Comment 75 WebKit Commit Bot 2017-11-08 15:52:10 PST
Comment on attachment 326385 [details]
Patch for landing

Rejecting attachment 326385 [details] from commit-queue.

New failing tests:
security/cannot-read-self-from-file.html
Full output: http://webkit-queues.webkit.org/results/5154382
Comment 76 WebKit Commit Bot 2017-11-08 15:52:12 PST
Created attachment 326397 [details]
Archive of layout-test-results from webkit-cq-03 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-03  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 77 Build Bot 2017-11-08 15:59:31 PST
Comment on attachment 326385 [details]
Patch for landing

Attachment 326385 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5154446

New failing tests:
security/cannot-read-self-from-file.html
Comment 78 Build Bot 2017-11-08 15:59:44 PST
Created attachment 326398 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 79 Brent Fulgham 2017-11-08 16:13:47 PST
Created attachment 326400 [details]
Patch for landing
Comment 80 WebKit Commit Bot 2017-11-08 16:46:36 PST
Comment on attachment 326400 [details]
Patch for landing

Clearing flags on attachment: 326400

Committed r224609: <https://trac.webkit.org/changeset/224609>
Comment 81 WebKit Commit Bot 2017-11-08 16:46:39 PST
All reviewed patches have been landed.  Closing bug.
Comment 82 WebKit Commit Bot 2017-11-09 08:41:49 PST
Re-opened since this is blocked by bug 179481
Comment 83 Brent Fulgham 2017-11-09 09:09:52 PST
I'm going to correct the test case in a subsequent commit.
Comment 84 Brent Fulgham 2017-11-09 09:36:13 PST
Committed r224631: <https://trac.webkit.org/changeset/224631>