Bug 167058 – REGRESSION(r210531): Broke local resource loads from custom local protocols

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Michael Catanzaro]

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.

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 298863 [details]
Patch

[Brent Fulgham]

Comment on attachment 298863 [details]
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.

[Darin Adler]

Comment on attachment 298863 [details]
Patch

Should find a way to make a regression test so we won’t break this again.

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

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

[Michael Catanzaro]

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.

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

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

[Michael Catanzaro]

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.

[Brent Fulgham]

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

[Michael Catanzaro]

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.

[WebKit Commit Bot]

Comment on attachment 298863 [details]
Patch

Clearing flags on attachment: 298863

Committed r210888: <http://trac.webkit.org/changeset/210888>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

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

[Brent Fulgham]

<rdar://problem/30068195>