Bug 34778 – Add support to isolate file:/// URIs in their own domain by default

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Chris Evans]

Chromium will use this new mode by default. Patch forthcoming...

[Chris Evans]

Created attachment 48454 [details]
Add support for putting file:/// URIs into their own unique domain

[Chris Evans]

Adam, can you help me land this? It's mostly trivial / tedious wiring. I think I got all the places.
Once landed I can get on with the subsequent changes (Chromium wiring and then add the test, which I promise I won't neglect to do :)

[Adam Barth]

Kind of ridiculous what it takes to add one bit of state, huh?

One minor issue, you need to add new functions at the bottom of the IDL file because of the way virtual functions work in C++.  Other than that, looks reasonable.

I'm leaving this r? for now in case other folks want to discuss what we're thinking about doing here.  We've had some discussion on #webkit in the past, but Chris and my thinking has evolved slightly since then.

[Chris Evans]

Assuming your IDL comment refers to IWebPreferencesPrivate.idl -- new patch forthcoming.

NOTE - this patch doesn't change the WebKit default behaviour at all. That's left for clients to do if they want additional security for file:/// URIs.

Given that this won't affect WebKit clients, but Chromium wants to use this -- I'm in favour of submitting sooner rather than later :)

[Chris Evans]

Created attachment 48459 [details]
Updated so new methods are at bottom of IDL file

[Sam Weinig]

Can you explain what issues you are trying to mitigate with this new mode? You say your thinking in this area has changed, what changed your mind?

[Adam Barth]

> Can you explain what issues you are trying to mitigate with this new mode?

The risk is that a user might download an HTML document in an email.  When they viewed that document, the document would be able to steal any file on the their hard drive.

> You say your thinking in this area has changed, what changed your mind?

A couple things:

1) Chris noticed that he was doing the above a bunch and got scared.
2) He wants to try turning this on in a Chrome Dev channel release (the equivalent of the WebKit nightly builds) to see how the community reacts.

My guess is that we'll get a bunch of complaints and will need to back off, but I'm not sure where we'll end up.  When we discussed this on #webkit, Maciej suggested some kind of "Developer mode" that would have more permissive file:// URL restrictions.

This is all very much open for discussion, but having code seemed like a good way to start the discussion.

[Chris Evans]

Regarding 1), the threat I'm worried about is HTML attachments (e.g. repro.html) on bugs in the WebKit and Chromium bug databases. I could feasibly get my files stolen. Not good. I'd rather the browser protected me by default.

Regarding 2), I really don't expect much kick back. I have added a Chromium command-line flag override for developers that need communication between local files. 99% of our users likely won't notice or care, and could use the extra protection.
Well, there's only one way to find out. If we can get this WebKit patch landed, I'll flip the setting in the next Chromium dev channel build and report back.

[Sam Weinig]

Thanks for the explanation.

[Sam Weinig]

Comment on attachment 48459 [details]
Updated so new methods are at bottom of IDL file

Please follow this up with a test case.

[Chris Evans]

Yep, test is pending. I need to get this landed, then the Chromium changes landed, before I can land the test without breaking anything. Joy of dependencies :)

[WebKit Commit Bot]

Comment on attachment 48459 [details]
Updated so new methods are at bottom of IDL file

Rejecting patch 48459 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Sam Weinig', '--force']" exit_code: 1
Last 500 characters of output:
e WebKitTools/DumpRenderTree/mac/LayoutTestControllerMac.mm
Hunk #1 succeeded at 329 (offset 5 lines).
patching file WebKitTools/DumpRenderTree/LayoutTestController.cpp
Hunk #1 succeeded at 928 (offset 19 lines).
Hunk #2 succeeded at 1380 (offset 20 lines).
patching file WebKitTools/DumpRenderTree/win/LayoutTestControllerWin.cpp
patching file WebKitTools/DumpRenderTree/win/DumpRenderTree.cpp
patching file WebKitTools/DumpRenderTree/LayoutTestController.h
Hunk #1 succeeded at 69 (offset 1 line).

Full output: http://webkit-commit-queue.appspot.com/results/256695

[Chris Evans]

Grr! Clashes with Changeset 54587 for trunk/WebCore/page/SecurityOrigin.cpp by Mr. Barth :)
Updating patch in progress....

[Chris Evans]

Created attachment 48544 [details]
Trivial merge with Changeset 54587. Ready to go to commit queue again.

[Chris Evans]

Adam, Sam or Eric: can you r+, cq? the updated patch? The trivial conflict with SecurityOrigin.cpp has been resolved.

[Eric Seidel (no email)]

Comment on attachment 48544 [details]
Trivial merge with Changeset 54587. Ready to go to commit queue again.

The commit-queue spun on this one for a while, making me think there might be something wrong with the patch.

I'm going to re-mark this r? so that the EWS bots run on it.  If those pass, i'm happy to mark it cq+

[Chris Evans]

I'm no expert, but I see green on EWS? :)

[Eric Seidel (no email)]

Comment on attachment 48544 [details]
Trivial merge with Changeset 54587. Ready to go to commit queue again.

Yup.  lets do this thang.

[WebKit Commit Bot]

Comment on attachment 48544 [details]
Trivial merge with Changeset 54587. Ready to go to commit queue again.

Rejecting patch 48544 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--non-interactive', '--no-update', '--parent-command=commit-queue', '--build-style=both', '--quiet', '48544']" exit_code: 1
Last 500 characters of output:
ebkitpy/statusserver.py", line 87, in update_status
    return NetworkTransaction().run(lambda: self._post_to_server(queue_name, status, patch, results_file))
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/networktransaction.py", line 52, in run
    self._check_for_timeout()
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/networktransaction.py", line 58, in _check_for_timeout
    raise NetworkTimeout()
webkitpy.networktransaction.NetworkTimeout

[Chris Evans]

Argh! The latest reject has no "full output" link, so I can't see what might have gone wrong. Looks like a timeout waiting for the network?
Can anyone help me land this?

[Eric Seidel (no email)]

Yeah, that's what happened last time.  The commit-queue hung for hours trying to post the actual output.

I'll attempt to land this manually this afternoon.  Sorry for the runaround.

[Chris Evans]

Thanks, Eric. No need to apologize; there's probably something weird about my patch if this has happened twice?? I can't see anything obviously wrong, though.

[Eric Seidel (no email)]

The commit-queue failing to upload the results is a commit queue bug.  The actual failure is:

Ld /Users/eseidel/Projects/build/Release/WebKit.framework/Versions/A/WebKit normal i386
    cd /Users/eseidel/Projects/WebKit/WebKit
    setenv MACOSX_DEPLOYMENT_TARGET 10.5
    /Developer/usr/bin/g++-4.2 -arch i386 -dynamiclib -L/Users/eseidel/Projects/build/Release -F/Users/eseidel/Projects/build/Release -F/System/Library/Frameworks/WebKit.framework/Versions/A/Frameworks -F/System/Library/Frameworks/ApplicationServices.framework/Frameworks -F/System/Library/Frameworks/Carbon.framework/Frameworks -F/System/Library/Frameworks/Quartz.framework/Frameworks -F/System/Library/PrivateFrameworks -filelist /Users/eseidel/Projects/build/WebKit.build/Release/WebKit.build/Objects-normal/i386/WebKit.LinkFileList -Wl,--no-demangle -exported_symbols_list mac/WebKit.exp -install_name /Users/eseidel/Projects/build/Release/WebKit.framework/Versions/A/WebKit -mmacosx-version-min=10.5 -Wl,-dead_strip -sub_umbrella WebCore -lWebKitSystemInterfaceLeopard -framework Carbon -framework Cocoa -framework JavaScriptCore -licucore -framework OpenGL -framework QuartzCore -framework Security -framework WebCore -Wl,-single_module -compatibility_version 1 -current_version 533.1 -o /Users/eseidel/Projects/build/Release/WebKit.framework/Versions/A/WebKit
Undefined symbols:
  "__ZN7WebCore8Settings30setAllowFileAccessFromFileURLsEb", referenced from:
      -[WebView(WebPrivate) _preferencesChangedNotification:] in WebView.o
ld: symbol(s) not found
collect2: ld returned 1 exit status

[Chris Evans]

Thanks!
Sigh. I'll look at this after the long weekend. Maybe I have a typo. I'm on Chromium Linux so parts of this patch are blind :(

[Adam Barth]

You just have to add __ZN7WebCore8Settings30setAllowFileAccessFromFileURLsEb to WebCore.base.exp.  Developing WebKit without a Mac is a minefield.  :)

[Chris Evans]

Created attachment 48863 [details]
Export new WebCore function to fix Mac link error

[Adam Barth]

Comment on attachment 48863 [details]
Export new WebCore function to fix Mac link error

ok, let's give it a spin

[WebKit Commit Bot]

Comment on attachment 48863 [details]
Export new WebCore function to fix Mac link error

Clearing flags on attachment: 48863

Committed r54873: <http://trac.webkit.org/changeset/54873>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.