Bug 36692 - Redo the file:// origin separation
Summary: Redo the file:// origin separation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-26 20:45 PDT by Chris Evans
Modified: 2010-03-29 19:32 PDT (History)
3 users (show)

See Also:


Attachments
Track file:// origins by path instead of abusing the unique origin concept (7.74 KB, patch)
2010-03-26 21:12 PDT, Chris Evans
abarth: review-
Details | Formatted Diff | Diff
Address comments. (8.94 KB, patch)
2010-03-28 20:02 PDT, Chris Evans
abarth: review-
Details | Formatted Diff | Diff
Use "null" domain instead of exposing path as per chat with Adam (8.93 KB, patch)
2010-03-29 15:23 PDT, Chris Evans
no flags Details | Formatted Diff | Diff
Fix style-bot error! (8.92 KB, patch)
2010-03-29 15:44 PDT, Chris Evans
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Evans 2010-03-26 20:45:05 PDT
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.
Comment 1 Chris Evans 2010-03-26 21:12:03 PDT
Created attachment 51809 [details]
Track file:// origins by path instead of abusing the unique origin concept
Comment 2 Chris Evans 2010-03-26 21:13:43 PDT
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 3 Adam Barth 2010-03-26 22:07:30 PDT
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?
Comment 4 Chris Evans 2010-03-26 23:37:06 PDT
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 5 Adam Barth 2010-03-27 00:51:14 PDT
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?
Comment 6 Chris Evans 2010-03-27 00:58:01 PDT
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.
Comment 7 Chris Evans 2010-03-28 20:02:32 PDT
Created attachment 51876 [details]
Address comments.
Comment 8 Chris Evans 2010-03-28 20:04:16 PDT
OK, attempt #2!
Throw it on the commit-queue if you like it :)
Comment 9 Adam Barth 2010-03-29 00:16:34 PDT
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.
Comment 10 Chris Evans 2010-03-29 10:34:46 PDT
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.
Comment 11 Adam Barth 2010-03-29 10:42:31 PDT
(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.
Comment 12 Chris Evans 2010-03-29 13:43:21 PDT
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.
Comment 13 Chris Evans 2010-03-29 15:23:08 PDT
Created attachment 51973 [details]
Use "null" domain instead of exposing path as per chat with Adam
Comment 14 Chris Evans 2010-03-29 15:23:18 PDT
Third time lucky? :)
Comment 15 WebKit Review Bot 2010-03-29 15:25:03 PDT
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.
Comment 16 Chris Evans 2010-03-29 15:44:49 PDT
Created attachment 51977 [details]
Fix style-bot error!
Comment 17 Chris Evans 2010-03-29 15:45:10 PDT
Bitten by style-bot... attempt #4
Comment 18 Adam Barth 2010-03-29 15:54:28 PDT
Comment on attachment 51977 [details]
Fix style-bot error!

Looks good.  Thanks for sticking with it.  :)
Comment 19 WebKit Commit Bot 2010-03-29 19:32:20 PDT
Comment on attachment 51977 [details]
Fix style-bot error!

Clearing flags on attachment: 51977

Committed r56757: <http://trac.webkit.org/changeset/56757>
Comment 20 WebKit Commit Bot 2010-03-29 19:32:26 PDT
All reviewed patches have been landed.  Closing bug.