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.
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
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
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
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
(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 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.
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
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
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 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 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.
(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.
(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).
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
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
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
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 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 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?
(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.
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
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
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
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
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 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.
The commit-queue encountered the following flaky tests while processing attachment 324952[details]:
The commit-queue is continuing to process your patch.
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
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
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
(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?
(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.
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?
(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! :)
(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 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
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
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
2017-10-19 20:50 PDT, Brent Fulgham
2017-10-19 21:56 PDT, Build Bot
2017-10-19 22:24 PDT, Build Bot
2017-10-19 22:32 PDT, Build Bot
2017-10-20 15:31 PDT, Build Bot
2017-10-20 15:33 PDT, Brent Fulgham
2017-10-20 16:12 PDT, Build Bot
2017-10-20 17:01 PDT, Build Bot
2017-10-20 17:08 PDT, Build Bot
2017-10-20 17:13 PDT, Brent Fulgham
2017-10-21 16:48 PDT, Brent Fulgham
2017-10-21 17:36 PDT, Build Bot
2017-10-21 17:40 PDT, Build Bot
2017-10-21 17:45 PDT, Build Bot
2017-10-21 17:58 PDT, Build Bot
2017-10-24 10:23 PDT, Brent Fulgham
2017-10-24 11:27 PDT, Build Bot
2017-10-24 11:53 PDT, Build Bot
2017-10-24 11:57 PDT, Build Bot
2017-10-24 13:12 PDT, Build Bot
2017-10-25 08:55 PDT, Brent Fulgham
2017-10-25 10:29 PDT, Build Bot
2017-10-25 11:22 PDT, Brent Fulgham
2017-10-25 21:17 PDT, Brent Fulgham
2017-10-25 21:21 PDT, Brent Fulgham
2017-10-26 00:24 PDT, Build Bot
2017-10-26 01:31 PDT, Build Bot
2017-10-26 02:38 PDT, Build Bot
2017-10-26 07:42 PDT, Brent Fulgham
2017-11-08 14:51 PST, Brent Fulgham
2017-11-08 14:55 PST, Brent Fulgham
2017-11-08 15:52 PST, WebKit Commit Bot
2017-11-08 15:59 PST, Build Bot
2017-11-08 16:13 PST, Brent Fulgham