Bug 36692

Created attachment 51809 [details] Track file:// origins by path instead of abusing the unique origin concept

Heya Adam -- let's get this piece landed first. It still leaves open the question of local storage / web databases on the file:// origin. These share one database. If we decide we should split this out, I'll do in another change. (Not that I can get excited about the threat to be honest).

Comment on attachment 51809 [details] Track file:// origins by path instead of abusing the unique origin concept Can we do this as a static instead of a member? It doesn't make sense on an origin-by-origin basis. You can see how we do that with the origin white list / scheme registration stuff. + m_filePath Why not just m_path and always store it?

Re: static. I'm not a fan. It'd complicate the test, which does a runtime flag switch. re: m_filePath. I'm happy to unconditionally store m_path but it seems an unnecessary waste in the common http case. I can't see that we'd ever care about the value after construction.

Comment on attachment 51809 [details] Track file:// origins by path instead of abusing the unique origin concept + return String("file://") + m_filePath; This code seems wrong. This value is exposed to web content. We want the current value, not the extended value. + if (isLocal() && (m_enforceFilePathSeparation || other->m_enforceFilePathSeparation) && m_filePath != other->m_filePath) This code is copy/pasted. Whenever you copy/paste code, you should consider how to abstract it do you don't have to repeat yourself. Other than that, this looks ok. Do you want to add a test for the about:blank that caused us to go down this path?

Thanks for the review. I'll fix the items noted and do a little re-testing. Re: the test of about:blank iframe, it's covered in this change by tweaking the original test.

Created attachment 51876 [details] Address comments.

OK, attempt #2! Throw it on the commit-queue if you like it :)

Comment on attachment 51876 [details] Address comments. + if (m_protocol == "file") { + String str("file://"); + if (m_enforceFilePathSeparation) + str += m_filePath; This code is still wrong. As I said above, this value is exposed to web content and we can't just go changing it. Making the code conditional on m_enforceFilePathSeparation doesn't solve the problem of giving providing the wrong string to JavaScript.

It's a security vulnerability to not consider the full file:// origin context in toString(). v8, at least, uses SecurityOrigin::toString() as a cache key for cached permission checks. Since it's a security contract an unknown number of consumers are depending on, I really don't want to change it. How is this value exposed to Javascript? We can think about whether it makes logical sense or not.

(In reply to comment #10) > It's a security vulnerability to not consider the full file:// origin context > in toString(). v8, at least, uses SecurityOrigin::toString() as a cache key for > cached permission checks. If so, then there's a lot more work to do to fix this bug bug completely. > Since it's a security contract an unknown number of consumers are depending on, > I really don't want to change it. We can't land the patch you've attached here because it changes something in the web platform that we don't want to change. If you enable this setting by default an run the LayoutTests, I think you'll find at least one failing test because of this issue. > How is this value exposed to Javascript? We can think about whether it makes > logical sense or not. It's exposed as the "origin" property of message events generated via postMessage. It's also exposed in the Origin HTTP header that's part of CORS. There might be other cases, but those are the ones that come to mind.

In the case of the postMessage origin whilst we are in "isolated file origin" mode, it sounds like a risk to return simply "file://". The message recipient needs to know the full granularity of the source origin in order to avoid getting security decisions wrong. I stand by the patch. If you liked, as a simple tweak to resolve this for M5, we could return "null" for all file:// origins when we are in "isolated file origin" mode. That's what currently happens without this patch (toString() on a unique origin returns "null"). v8 knows that a null origin implies it must always do access checks.

Created attachment 51973 [details] Use "null" domain instead of exposing path as per chat with Adam

Third time lucky? :)

Attachment 51973 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/page/SecurityOrigin.cpp:352: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 51977 [details] Fix style-bot error!

Bitten by style-bot... attempt #4

Comment on attachment 51977 [details] Fix style-bot error! Looks good. Thanks for sticking with it. :)

Comment on attachment 51977 [details] Fix style-bot error! Clearing flags on attachment: 51977 Committed r56757: <http://trac.webkit.org/changeset/56757>

All reviewed patches have been landed. Closing bug.