Bug 178565 – XMLHttpRequest should not treat file URLs as same origin

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brent Fulgham]

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.

[Brent Fulgham]

<rdar://problem/11115901>

[Brent Fulgham]

Created attachment 324343 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Brent Fulgham]

Created attachment 324453 [details]
Patch

[Brent Fulgham]

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

[Ryosuke Niwa]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Brent Fulgham]

Created attachment 324475 [details]
Patch

[Brent Fulgham]

Note: The patch (and rebaselined test) now matches Blink behavior.

[Ryosuke Niwa]

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.

[Daniel Bates]

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.

[Daniel Bates]

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

[Brent Fulgham]

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

[Brent Fulgham]

Created attachment 324511 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Ryosuke Niwa]

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.

[Brent Fulgham]

Created attachment 324682 [details]
Patch

[Ryosuke Niwa]

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?

[Brent Fulgham]

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

[Brent Fulgham]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Brent Fulgham]

Created attachment 324828 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Brent Fulgham]

Created attachment 324854 [details]
Patch

[Ryosuke Niwa]

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.

[Brent Fulgham]

Created attachment 324951 [details]
Patch for landing

[Brent Fulgham]

Created attachment 324952 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 324952 [details]:

The commit-queue is continuing to process your patch.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Daniel Bates]

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?

[Brent Fulgham]

Created attachment 325012 [details]
Patch for landing

[Brent Fulgham]

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

[Brent Fulgham]

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

[Brent Fulgham]

Committed r224019: <https://trac.webkit.org/changeset/224019>

[Ryan Haddad]

This change appears to have cause API test WebKit.MSEIsPlayingAudio to time out
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK1%20(Tests)/builds/812
https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK1%20(Tests)/builds/5608

[Daniel Bates]

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?

[Ryan Haddad]

Reverted r224019 for reason:

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

Committed r224025: <https://trac.webkit.org/changeset/224025>

[Brent Fulgham]

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

[Daniel Bates]

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

[Brent Fulgham]

Created attachment 326384 [details]
Patch for landing

[WebKit Commit Bot]

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

[Brent Fulgham]

Created attachment 326385 [details]
Patch for landing

[WebKit Commit Bot]

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

[WebKit Commit Bot]

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

[Build Bot]

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

[Build Bot]

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

[Brent Fulgham]

Created attachment 326400 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 326400 [details]
Patch for landing

Clearing flags on attachment: 326400

Committed r224609: <https://trac.webkit.org/changeset/224609>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 179481

[Brent Fulgham]

I'm going to correct the test case in a subsequent commit.

[Brent Fulgham]

Committed r224631: <https://trac.webkit.org/changeset/224631>