|
|
Descriptionmake file: an effectively unique origin
This, if Chrome is started without flags that reduce security,
prevents local files from being same-origin with other files
at the same path. This can especially be an issue with
pseudofilesystems in which the meaning of a path often changes.
BUG=429542
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195464
Patch Set 1 #
Total comments: 2
Patch Set 2 : added test #
Total comments: 11
Patch Set 3 : BROKEN: use test harness #Patch Set 4 : fix test #
Total comments: 2
Messages
Total messages: 30 (7 generated)
jannhorn@googlemail.com changed reviewers: + brettw@chromium.org, jln@chromium.org, nasko@chromium.org
The CQ bit was checked by jannhorn@googlemail.com
The CQ bit was unchecked by jannhorn@googlemail.com
nasko@chromium.org changed reviewers: + alexmos@chromium.org, creis@chromium.org
Adding alexmos@ and creis@, who have looked into this in the past.
alexmos@chromium.org changed reviewers: + dcheng@chromium.org
The change looks ok, but let's also get a review from dcheng@, who wrote an Intent to Ship for this (https://groups.google.com/a/chromium.org/d/msg/blink-dev/--KjSROtcMQ/C7Ce13NR...). Do we need to do something about the localStorage question on that thread?
On 2015/05/15 17:45:52, alexmos (OOO until 5-26) wrote: > Do we need to do something about the localStorage question on that thread? localStorage is currently global for file:, and this patch doesn't change that, so I don't think so. However, if localStorage should later be made per-file, m_filePath should probably be kept in SecurityOrigin? Should I keep that as an unused member in SecurityOrigin for now?
jannhorn@googlemail.com changed reviewers: + mkwst@chromium.org
Oh, I didn't know about this: https://www.chromestatus.com/feature/5755326842273792 Adding mkwst since he is listed as one of the feature owners there. This patch just changes normal SOP and not the localStorage stuff or so because that looks more complicated and I just want to get the exploitable bug 429542 closed.
https://codereview.chromium.org/1140203002/diff/1/Source/platform/weborigin/S... File Source/platform/weborigin/SecurityOrigin.cpp (right): https://codereview.chromium.org/1140203002/diff/1/Source/platform/weborigin/S... Source/platform/weborigin/SecurityOrigin.cpp:277: return false; Doesn't this break layout tests, which do set allowFileAccessFromFileURLs to true?
https://codereview.chromium.org/1140203002/diff/1/Source/platform/weborigin/S... File Source/platform/weborigin/SecurityOrigin.cpp (right): https://codereview.chromium.org/1140203002/diff/1/Source/platform/weborigin/S... Source/platform/weborigin/SecurityOrigin.cpp:277: return false; On 2015/05/16 01:55:10, dcheng wrote: > Doesn't this break layout tests, which do set allowFileAccessFromFileURLs to > true? Don't see how it would. This is never reached if m_enforceFilePathSeparation is unset for both frames, and third_party/WebKit/Source/core/dom/Document.cpp only calls enforceFilePathSeparation() if allowFileAccessFromFileURLs isn't set. All this change does is block access in cases where it was previously only allowed because the paths are equal.
https://codereview.chromium.org/1140203002/diff/1/Source/platform/weborigin/S... File Source/platform/weborigin/SecurityOrigin.cpp (right): https://codereview.chromium.org/1140203002/diff/1/Source/platform/weborigin/S... Source/platform/weborigin/SecurityOrigin.cpp:277: return false; On 2015/05/16 01:55:10, dcheng wrote: > Doesn't this break layout tests, which do set allowFileAccessFromFileURLs to > true? Don't see how it would. This is never reached if m_enforceFilePathSeparation is unset for both frames, and third_party/WebKit/Source/core/dom/Document.cpp only calls enforceFilePathSeparation() if allowFileAccessFromFileURLs isn't set. All this change does is block access in cases where it was previously only allowed because the paths are equal.
This looks like something we could try to land on Monday (we're branching for M44 sometime yesterday or today). Please add tests, however, to verify the new behavior does what you expect it to do.
On 2015/05/16 05:07:32, Mike West (holiday in DE) wrote: > Please add tests, however, to verify the new > behavior does what you expect it to do. Ok, added a test to verify that it's no longer possible for a local file to access itself with XHR. (The same codepath is used for other kinds of SOP checks usable to disclose file contents as well, so this one test should be enough, right?)
A few nits on the tests. Otherwise looking good. https://codereview.chromium.org/1140203002/diff/20001/LayoutTests/security/ca... File LayoutTests/security/cannot-read-self-from-file.html (right): https://codereview.chromium.org/1140203002/diff/20001/LayoutTests/security/ca... LayoutTests/security/cannot-read-self-from-file.html:1: <html> Nit: doctype. https://codereview.chromium.org/1140203002/diff/20001/LayoutTests/security/ca... LayoutTests/security/cannot-read-self-from-file.html:4: testRunner.dumpAsText(); Rather than setting these manually, please convert this to use `testrunner.js`, which will set up the environment for you. That is, include: <script src="/resources/testharness.js"></script> <script src="/resources/testharnessreport.js"></script> at the top of the document, and use the provided assertion mechanisms to execute your test. http://darobin.github.io/test-harness-tutorial/docs/using-testharness.html has details. https://codereview.chromium.org/1140203002/diff/20001/LayoutTests/security/ca... LayoutTests/security/cannot-read-self-from-file.html:6: testRunner.setAllowFileAccessFromFileURLs(false); These default to `false`, don't they? Why do you need to set them? https://codereview.chromium.org/1140203002/diff/20001/LayoutTests/security/ca... LayoutTests/security/cannot-read-self-from-file.html:10: <iframe src="resources/cannot-read-self-from-file.html"></iframe> Why do this work in an `<iframe>`? Wouldn't the result be the same if you executed the same code in this file? https://codereview.chromium.org/1140203002/diff/20001/LayoutTests/security/re... File LayoutTests/security/resources/cannot-read-self-from-file.html (right): https://codereview.chromium.org/1140203002/diff/20001/LayoutTests/security/re... LayoutTests/security/resources/cannot-read-self-from-file.html:5: req.open('GET', location); Nit: s/location/window.location.href/ Nit: Ideally, this would be an async XHR. You could also use `fetch()`, which might be simpler, and nicely Promise-based. https://codereview.chromium.org/1140203002/diff/20001/LayoutTests/security/re... LayoutTests/security/resources/cannot-read-self-from-file.html:7: console.log(req.responseText); Please convert this into an ASSERTion. https://codereview.chromium.org/1140203002/diff/20001/Source/platform/weborig... File Source/platform/weborigin/SecurityOrigin.cpp (right): https://codereview.chromium.org/1140203002/diff/20001/Source/platform/weborig... Source/platform/weborigin/SecurityOrigin.cpp:277: return false; Nit: You can now simplify this to `return !m_enforceFilePathSeparation && !other->m_enforceFilePathSeparation;`.
Also: what is Firefox's behavior for your test? Does it also block the XHR? If not, we should chat with them to see if we can change this behavior at the same time (that shouldn't block landing this, of course).
On 2015/05/17 08:41:40, Mike West (holiday in DE) wrote: > Also: what is Firefox's behavior for your test? Does it also block the XHR? No. Firefox even allows an HTML file to perform XHR to any file in the same directory or below it, as far as I understand it. So file:///etc/hosts can dump file:///etc/passwd and file:///etc/ssh/sshd_config, but can't access file:///tmp/test. (That's not symmetric; file:///etc/ssh/sshd_config can't access file:///etc/hosts.) The issue seems quite a bit harder to exploit on Firefox (and I can't say for sure whether it is exploitable at all): They lstat() the target file about 150 microseconds before opening it, and if the file is a symlink, that's treated as a redirect. > If > not, we should chat with them to see if we can change this behavior at the same > time (that shouldn't block landing this, of course). It would probably be good if they could also add a reliable defense against this, but giving documents from file: URIs unique origins would be a much bigger change for Firefox, so I don't think they'll want to adopt this approach.
https://codereview.chromium.org/1140203002/diff/20001/LayoutTests/security/ca... File LayoutTests/security/cannot-read-self-from-file.html (right): https://codereview.chromium.org/1140203002/diff/20001/LayoutTests/security/ca... LayoutTests/security/cannot-read-self-from-file.html:1: <html> On 2015/05/17 08:40:48, Mike West (holiday in DE) wrote: > Nit: doctype. Hm, ok, added one to both HTML documents. (Not one the documents really conform to, as far as I understand, since there's no <title>.) https://codereview.chromium.org/1140203002/diff/20001/LayoutTests/security/ca... LayoutTests/security/cannot-read-self-from-file.html:4: testRunner.dumpAsText(); On 2015/05/17 08:40:48, Mike West (holiday in DE) wrote: > Rather than setting these manually, please convert this to use `testrunner.js`, > which will set up the environment for you. That is, include: > > <script src="/resources/testharness.js"></script> > <script src="/resources/testharnessreport.js"></script> Note that I need to use relative URLs because I'm running from file:. > at the top of the document, and use the provided assertion mechanisms to execute > your test. I can't get it to work. That is, the tested code clearly does run, and the tested code would print different console output if the test failed, so you'd notice a fail, but that's just the "CONSOLE ERROR: XMLHttpRequest cannot load cannot-read-self-from-file.html. Cross origin requests are only supported for protocol schemes: http, data, https." message. Whether or not code in assert_throws() throws seems to have no impact on the outcome of the test. To see it for yourself: 1. Run the test - it passes. 2. Apply this patch: diff --git a/LayoutTests/security/resources/cannot-read-self-from-file.html b/LayoutTests/security/resources/cannot-read-self-from-file.html index ce246fa..12d8969 100644 --- a/LayoutTests/security/resources/cannot-read-self-from-file.html +++ b/LayoutTests/security/resources/cannot-read-self-from-file.html @@ -7,7 +7,8 @@ test(function() { var req = new XMLHttpRequest(); req.open('GET', location); - assert_throws("NetworkError", function(){req.send()}, "request should fail"); + try { req.send() } catch(e) {} + assert_throws("NetworkError", function(){}, "request should fail"); }, "file: URI documents can't request themselves"); </script> </head> 3. Run the test again. Still passes. Can someone who is a bit more familiar with this stuff tell me what I'm doing wrong? https://codereview.chromium.org/1140203002/diff/20001/LayoutTests/security/ca... LayoutTests/security/cannot-read-self-from-file.html:6: testRunner.setAllowFileAccessFromFileURLs(false); On 2015/05/17 08:40:48, Mike West (holiday in DE) wrote: > These default to `false`, don't they? Why do you need to set them? See ./content/shell/common/test_runner/test_preferences.cc: void TestPreferences::Reset() { [...] allow_file_access_from_file_urls = true; [...] // Allow those layout tests running as local files, i.e. under // LayoutTests/http/tests/local, to access http server. allow_universal_access_from_file_urls = true; [...] } https://codereview.chromium.org/1140203002/diff/20001/LayoutTests/security/ca... LayoutTests/security/cannot-read-self-from-file.html:10: <iframe src="resources/cannot-read-self-from-file.html"></iframe> On 2015/05/17 08:40:48, Mike West (holiday in DE) wrote: > Why do this work in an `<iframe>`? Wouldn't the result be the same if you > executed the same code in this file? As far as I understand, I need to create a new origin to let testRunner.setAllowUniversalAccessFromFileURLs(false); and testRunner.setAllowFileAccessFromFileURLs(false); take effect, which can be achieved by loading a new document in an iframe. There's a bunch of other tests that follow the same pattern. I actually tried it without the iframe first, which didn't work.
On 2015/05/17 14:35:19, TheJH wrote: > Can someone who is a bit more familiar with this stuff tell me what I'm doing > wrong? Ah, could it be that the test harness stuff doesn't work properly inside iframes? Because the test result isn't written into the top-level document? If so, how should this be handled? Go back to testing without the harness? Mess around with the test harness code to log results to the console or forward them to the upper frame with postMessage or so?
On 2015/05/17 at 14:41:43, jannhorn wrote: > On 2015/05/17 14:35:19, TheJH wrote: > > Can someone who is a bit more familiar with this stuff tell me what I'm doing > > wrong? > > Ah, could it be that the test harness stuff doesn't work properly inside iframes? Because the test result isn't written into the top-level document? It's not that testharness doesn't work inside iframes, but that the data ends up being in the wrong place. Here, you need to be doing something like `testRunner.dumpChildFramesAsText();` to see the output inside the frame, but that won't make the expectations parser happy, because the result will be surrounded with a bunch of other junk that it doesn't understand. :) If you need the frame (which I guess you do, though that's unfortunate), then you can `postMessage()` up a result that can be asserted against in the top-level frame. That is: ``` var x = xhr(); x.open(...); try { x.send(); window.top.postMessage("FAIL", "*"); } catch (e) { window.top.postMessage(e.name, "*"); } ``` ``` window.addEventListener("message", function (e) { assert_eq(e.data, "NetworkError"); }); ``` or something similar.
On 2015/05/18 09:04:02, Mike West (holiday in DE) wrote: > If you need the frame (which I guess you do, though that's unfortunate), then > you can `postMessage()` up a result that can be asserted against in the > top-level frame. OK, did that.
On 2015/05/18 at 10:54:47, jannhorn wrote: > On 2015/05/18 09:04:02, Mike West (holiday in DE) wrote: > > If you need the frame (which I guess you do, though that's unfortunate), then > > you can `postMessage()` up a result that can be asserted against in the > > top-level frame. > > OK, did that. LGTM. You can just delete the `-expected.txt` file; as long as it's in the testharness format, it just looks for `PASS` at the beginning of lines.
On 2015/05/18 10:56:23, Mike West (holiday in DE) wrote: > On 2015/05/18 at 10:54:47, jannhorn wrote: > > On 2015/05/18 09:04:02, Mike West (holiday in DE) wrote: > > > If you need the frame (which I guess you do, though that's unfortunate), > then > > > you can `postMessage()` up a result that can be asserted against in the > > > top-level frame. > > > > OK, did that. > > LGTM. You can just delete the `-expected.txt` file; as long as it's in the > testharness format, it just looks for `PASS` at the beginning of lines. Are you sure? I tried that, but it didn't seem to work - apparently because of the first line with the console error, it treated that as a Failure.
The CQ bit was checked by jannhorn@googlemail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1140203002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=195464
Message was sent while issue was closed.
https://codereview.chromium.org/1140203002/diff/60001/Source/platform/weborig... File Source/platform/weborigin/SecurityOrigin.cpp (right): https://codereview.chromium.org/1140203002/diff/60001/Source/platform/weborig... Source/platform/weborigin/SecurityOrigin.cpp:274: return !m_enforceFilePathSeparation && !other->m_enforceFilePathSeparation; I think we should rename m_enforceFilePathSeparation to make it clearer what the real purpose of this bool is. There's nothing about "filepath separation" anymore after this patch: it's become strictly a "give file URLs access to other file URLs" check. jannhorn@, do you mind cleaning this up in a followup patch?
Message was sent while issue was closed.
https://codereview.chromium.org/1140203002/diff/60001/Source/platform/weborig... File Source/platform/weborigin/SecurityOrigin.cpp (right): https://codereview.chromium.org/1140203002/diff/60001/Source/platform/weborig... Source/platform/weborigin/SecurityOrigin.cpp:274: return !m_enforceFilePathSeparation && !other->m_enforceFilePathSeparation; On 2015/06/01 21:08:53, dcheng wrote: > I think we should rename m_enforceFilePathSeparation to make it clearer what the > real purpose of this bool is. There's nothing about "filepath separation" > anymore after this patch: it's become strictly a "give file URLs access to other > file URLs" check. jannhorn@, do you mind cleaning this up in a followup patch? What would be a good name? m_treatFileAsUniqueOrigin? m_forbidFileFileAccess? |