167058 2017-01-14 15:05:05 -0800 REGRESSION(r210531): Broke local resource loads from custom local protocols 2017-01-24 15:31:01 -0800 1 1 1 Unclassified WebKit WebCore Misc. WebKit Nightly Build PC Linux RESOLVED FIXED https://bugzilla.gnome.org/show_bug.cgi?id=777246 https://bugs.webkit.org/show_bug.cgi?id=167392 InRadar P2 Normal --- 1 mcatanzaro mcatanzaro achristensen bfulgham bugs-noreply cgarcia commit-queue dbates mcatanzaro tpopela oldest_to_newest 1266855 0 mcatanzaro 2017-01-14 15:05:05 -0800 r210531 broke the Epiphany overview (new tab) page, about:overview aka ephy-about://overview. WebKit no longer loads: * The large Epiphany symbolic icon that's displayed on the overview page when no history is available (first run) * Any of the webpage snapshots I added a WTFLogAlways in SecurityOrigin::canDisplay and also in CachedResourceLoader::canRequest. It prints an error like this for each overview snapshot: File overview does not have same volume as /home/mcatanzaro/.cache/thumbnails/large/f19dd053c0b3baa7ad326f9440c1ac44.png, fail! canRequest: security origin ephy-about:// cannot display file:///home/mcatanzaro/.cache/thumbnails/large/f19dd053c0b3baa7ad326f9440c1ac44.png Note that it wouldn't make sense for "overview" to have an associated volume, since it's a resource served via custom protocol and not a file on disk. 1266857 1 mcatanzaro 2017-01-14 15:21:37 -0800 This is going to be a blocker for the GTK+ port. I'm not sure the right thing to do here. Is it acceptable to enforce this check only for file URIs and not all local protocols? I'll submit a patch that does that. The downside of this approach is that it's not hard to image a custom local URI scheme that allows loading untrusted data. 1266859 2 298863 mcatanzaro 2017-01-14 15:26:21 -0800 Created attachment 298863 Patch 1266880 3 298863 bfulgham 2017-01-14 18:23:26 -0800 Comment on attachment 298863 Patch Yes, I think this is fine. The intent with the original change was to limit a local HTML file from pulling in content from other volumes. 1266887 4 298863 darin 2017-01-14 19:07:47 -0800 Comment on attachment 298863 Patch Should find a way to make a regression test so we won’t break this again. 1266914 5 mcatanzaro 2017-01-14 21:26:17 -0800 We already have GTK+ API tests /webkit2/WebKitSecurityManager/file-xhr and /webkit2/WebKitWebContext/uri-scheme in TestWebKitWebContext.cpp. I should add a test that combines the two. That said, /webkit2/WebKitSecurityManager/file-xhr happens to be broken right now, due to r210531, so we did have tests to catch it... they were just platform-specific. It also broke /webkit2/WebKitConsoleMessage/network-error (in TestConsoleMessage.cpp). My patch cannot fix either of them, as those tests do not use custom protocols. So I think my patch here is insufficient. Perhaps we need to be more lenient with our same-volume check. What is the volume of the page loaded by WebPage::loadHTMLString("...", "file:///foo")? Could it possibly be expected to match the volume of a file that does not exist? (The current check fails if both files are not real files on disk.) Or perhaps the test needs to be updated to not use this construction. 1267129 6 cgarcia 2017-01-16 05:36:15 -0800 (In reply to comment #5) > We already have GTK+ API tests /webkit2/WebKitSecurityManager/file-xhr and > /webkit2/WebKitWebContext/uri-scheme in TestWebKitWebContext.cpp. I should > add a test that combines the two. > > That said, /webkit2/WebKitSecurityManager/file-xhr happens to be broken > right now, due to r210531, so we did have tests to catch it... they were > just platform-specific. It also broke > /webkit2/WebKitConsoleMessage/network-error (in TestConsoleMessage.cpp). My > patch cannot fix either of them, as those tests do not use custom protocols. > > So I think my patch here is insufficient. Perhaps we need to be more lenient > with our same-volume check. What is the volume of the page loaded by > WebPage::loadHTMLString("...", "file:///foo")? Could it possibly be expected > to match the volume of a file that does not exist? (The current check fails > if both files are not real files on disk.) Or perhaps the test needs to be > updated to not use this construction. Yes, we are now always failing when a file doesn't exist. I don't think we should fail here in this case, but a 404 should be returned instead. This is also causing the context menu api to fail, because we use a fake video source too ( file:///movie.ogg) 1267199 7 mcatanzaro 2017-01-16 12:22:45 -0800 Hey Carlos, I wrote a patch to do that, but I realized it's not the correct solution. It fixes our tests only if the files do not actually exist on disk. Yeah, that's the usual case, but I think it's not the right thing to do. For example: webkit_web_view_load_html (view, "...", "file:///file-that-does-not-exist"); Say there really is somehow a file /file-that-does-not-exist. In that case, it's not loaded from disk, so this volume check should not apply to it, because the actual HTML content is application-controlled. But the file does still exist on disk, which will screw up the permissions check. Furthermore, using the existence of a file on disk for the permissions testing is not good enough as a malicious file could delete itself from disk in order to bypass the permissions checking. 1267200 8 298863 mcatanzaro 2017-01-16 12:23:33 -0800 Comment on attachment 298863 Patch I think we definitely want to restrict this permissions check to the "file" protocol, so this patch is correct, but it is not sufficient. 1267753 9 cgarcia 2017-01-18 08:46:30 -0800 (In reply to comment #8) > Comment on attachment 298863 [details] > Patch > > I think we definitely want to restrict this permissions check to the "file" > protocol, so this patch is correct, but it is not sufficient. Yes, please, at least push this patch. 1267860 10 mcatanzaro 2017-01-18 13:39:07 -0800 I'm not sure we should; we need to fix file protocol as well, this patch does nothing for that, and we might even need to roll out the original change. This needs further discussion. 1267869 11 bfulgham 2017-01-18 13:56:28 -0800 (In reply to comment #9) > (In reply to comment #8) > > Comment on attachment 298863 [details] > > Patch > > > > I think we definitely want to restrict this permissions check to the "file" > > protocol, so this patch is correct, but it is not sufficient. > > Yes, please, at least push this patch. I'd like to land this patch, too, because I found a few places where we do something similar with local URL schemes that should not fall under this Volume check. We (Apple) should have a test case for this case, and I can do so as a separate (related) patch. 1267878 12 298863 mcatanzaro 2017-01-18 14:20:34 -0800 Comment on attachment 298863 Patch Talked with Brent on IRC. We're going to land this patch regardless of what we decide regarding how to handle nonexistent files. 1267897 13 298863 commit-queue 2017-01-18 14:45:11 -0800 Comment on attachment 298863 Patch Clearing flags on attachment: 298863 Committed r210888: <http://trac.webkit.org/changeset/210888> 1267898 14 commit-queue 2017-01-18 14:45:17 -0800 All reviewed patches have been landed. Closing bug. 1268088 15 mcatanzaro 2017-01-19 05:06:56 -0800 (In reply to comment #12) > Comment on attachment 298863 [details] > Patch > > Talked with Brent on IRC. We're going to land this patch regardless of what > we decide regarding how to handle nonexistent files. Brent, we landed https://trac.webkit.org/changeset/210922 I think that until we discover some broken application, we might not need to do anything else here. 1269271 16 bfulgham 2017-01-23 17:18:18 -0800 <rdar://problem/30068195> 298863 2017-01-14 15:26:21 -0800 2017-01-18 14:45:11 -0800 Patch bug-167058-20170114172432.patch text/plain 1560 mcatanzaro U3VidmVyc2lvbiBSZXZpc2lvbjogMjEwNzY4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggOTNkNDNkZmVjMjJlZjUz ZGY4YzZiNzEwNzk2Yzg3OGRmNjkyNmVkNi4uOWFjY2MyODU1MTE5ZTBiZTk5YTIyMTJkOGZiYTM0 NGQ2Y2NlY2JkYiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDE3LTAxLTE0ICBNaWNo YWVsIENhdGFuemFybyAgPG1jYXRhbnphcm9AaWdhbGlhLmNvbT4KKworICAgICAgICBSRUdSRVNT SU9OKHIyMTA1MzEpOiBCcm9rZSBsb2NhbCByZXNvdXJjZSBsb2FkcyBmcm9tIGN1c3RvbSBsb2Nh bCBwcm90b2NvbHMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTE2NzA1OAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIEFsbG93IGxvY2FsIHByb3RvY29scyB0byBhY2Nlc3MgcmVzb3VyY2VzIG9uIGRpZmZlcmVu dCB2b2x1bWVzIHVubGVzcyB0aGUgcHJvdG9jb2wgaXMKKyAgICAgICAgImZpbGUiLgorCisgICAg ICAgICogcGFnZS9TZWN1cml0eU9yaWdpbi5jcHA6CisgICAgICAgIChXZWJDb3JlOjpTZWN1cml0 eU9yaWdpbjo6Y2FuRGlzcGxheSk6CisKIDIwMTctMDEtMTQgIFphbGFuIEJ1anRhcyAgPHphbGFu QGFwcGxlLmNvbT4KIAogICAgICAgICBSZW5kZXJlcnMgc2hvdWxkIGhhdmUgYSBzaW1wbGUgd2F5 IHRvIGFjY2VzcyBTZXR0aW5ncy4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BhZ2UvU2Vj dXJpdHlPcmlnaW4uY3BwIGIvU291cmNlL1dlYkNvcmUvcGFnZS9TZWN1cml0eU9yaWdpbi5jcHAK aW5kZXggZjU5NjY0MGRjMWRhN2I4YTljNjhlMTQ3YWY2NTdhZTE4MjBlMGNmOS4uNzc1ZTQ5MDVk NjdhNmRkYWI4OTJjMmNhYWIzODFmOWVlMmEwYzkzOCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNv cmUvcGFnZS9TZWN1cml0eU9yaWdpbi5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGFnZS9TZWN1 cml0eU9yaWdpbi5jcHAKQEAgLTMwNCwxMCArMzA0LDggQEAgYm9vbCBTZWN1cml0eU9yaWdpbjo6 Y2FuRGlzcGxheShjb25zdCBVUkwmIHVybCkgY29uc3QKICAgICBpZiAobV91bml2ZXJzYWxBY2Nl c3MpCiAgICAgICAgIHJldHVybiB0cnVlOwogCi0gICAgaWYgKGlzTG9jYWwoKSAmJiB1cmwuaXNM b2NhbEZpbGUoKSkgewotICAgICAgICBpZiAoIWZpbGVzSGF2ZVNhbWVWb2x1bWUobV9maWxlUGF0 aCwgdXJsLnBhdGgoKSkpCi0gICAgICAgICAgICByZXR1cm4gZmFsc2U7Ci0gICAgfQorICAgIGlm IChtX3Byb3RvY29sID09ICJmaWxlIiAmJiB1cmwuaXNMb2NhbEZpbGUoKSAmJiAhZmlsZXNIYXZl U2FtZVZvbHVtZShtX2ZpbGVQYXRoLCB1cmwucGF0aCgpKSkKKyAgICAgICAgcmV0dXJuIGZhbHNl OwogCiAgICAgaWYgKGlzRmVlZFdpdGhOZXN0ZWRQcm90b2NvbEluSFRUUEZhbWlseSh1cmwpKQog ICAgICAgICByZXR1cm4gdHJ1ZTsK