Bug 36692 – Redo the file:// origin separation

• Status: RESOLVED
• Resolution: FIXED
• Component: Frames

[Chris Evans]

The (ab?)use of the unique origin concept breaks things such as when a local file:// legitimately creates itself a same-origin iframe via <iframe></iframe>

Patch forthcoming.

[Chris Evans]

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

[Chris Evans]

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

[Adam Barth]

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?

[Chris Evans]

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.

[Adam Barth]

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?

[Chris Evans]

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.

[Chris Evans]

Created attachment 51876 [details]
Address comments.

[Chris Evans]

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

[Adam Barth]

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.

[Chris Evans]

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.

[Adam Barth]

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

[Chris Evans]

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.

[Chris Evans]

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

[Chris Evans]

Third time lucky? :)

[WebKit Review Bot]

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.

[Chris Evans]

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

[Chris Evans]

Bitten by style-bot... attempt #4

[Adam Barth]

Comment on attachment 51977 [details]
Fix style-bot error!

Looks good.  Thanks for sticking with it.  :)

[WebKit Commit Bot]

Comment on attachment 51977 [details]
Fix style-bot error!

Clearing flags on attachment: 51977

Committed r56757: <http://trac.webkit.org/changeset/56757>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.