Bug 28480 - Don't let local files access web URLs
Summary: Don't let local files access web URLs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-19 20:14 PDT by Adam Barth
Modified: 2009-08-26 23:28 PDT (History)
7 users (show)

See Also:


Attachments
Patch v1 (7.21 KB, patch)
2009-08-19 20:16 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
set file url pref to secure (8.42 KB, patch)
2009-08-20 22:37 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
set file url pref to secure (5.73 KB, patch)
2009-08-24 23:11 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2009-08-19 20:14:06 PDT
We shouldn't let local files access web URLs.  Our current behavior makes HTML files loaded from your local file system super dangerous.
Comment 1 Adam Barth 2009-08-19 20:16:21 PDT
Created attachment 35180 [details]
Patch v1
Comment 2 Adam Barth 2009-08-19 20:17:07 PDT
Chrome has been running with this setting for like a year with no problems.
Comment 3 Mark Rowe (bdash) 2009-08-19 20:37:39 PDT
Comment on attachment 35180 [details]
Patch v1

> diff --git a/WebKit/mac/ChangeLog b/WebKit/mac/ChangeLog
> index 5335efb..e01f625 100644
> --- a/WebKit/mac/ChangeLog
> +++ b/WebKit/mac/ChangeLog
> @@ -1,3 +1,13 @@
> +2009-08-19  Adam Barth  <abarth@webkit.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Don't let local files access web URLs
> +        https://bugs.webkit.org/show_bug.cgi?id=28480
> +
> +        * WebView/WebPreferences.mm:
> +        (+[WebPreferences initialize]):
> +
>  2009-08-18  Anders Carlsson  <andersca@apple.com>
>  
>          Reviewed by Adele Peterson.
> diff --git a/WebKit/mac/WebView/WebPreferences.mm b/WebKit/mac/WebView/WebPreferences.mm
> index 0b40b59..cfd24ef 100644
> --- a/WebKit/mac/WebView/WebPreferences.mm
> +++ b/WebKit/mac/WebView/WebPreferences.mm
> @@ -315,7 +315,11 @@ static WebCacheModel cacheModelForMainBundle(void)
>          [NSNumber numberWithBool:YES],  WebKitJavaEnabledPreferenceKey,
>          [NSNumber numberWithBool:YES],  WebKitJavaScriptEnabledPreferenceKey,
>          [NSNumber numberWithBool:YES],  WebKitWebSecurityEnabledPreferenceKey,
> +#if ENABLE(DASHBOARD_SUPPORT)
>          [NSNumber numberWithBool:YES],  WebKitAllowUniversalAccessFromFileURLsPreferenceKey,
> +#else
> +        [NSNumber numberWithBool:NO],   WebKitAllowUniversalAccessFromFileURLsPreferenceKey,
> +#endif
>          [NSNumber numberWithBool:YES],  WebKitJavaScriptCanOpenWindowsAutomaticallyPreferenceKey,
>          [NSNumber numberWithBool:YES],  WebKitPluginsEnabledPreferenceKey,
>          [NSNumber numberWithBool:YES],  WebKitDatabasesEnabledPreferenceKey,

Since dashboard support is always compiled in on Mac OS X this still results in the setting being on for Mac OS X.  A more sensible change may be to use a linked-on-or-after check to set default value of this preference to YES for existing applications while newly-written applications would get NO as the value.

You also appear to have omitted GTK from these changes.
Comment 4 Adam Barth 2009-08-20 22:37:10 PDT
Created attachment 38357 [details]
set file url pref to secure


---
 11 files changed, 61 insertions(+), 3 deletions(-)
Comment 5 Eric Seidel (no email) 2009-08-21 09:24:17 PDT
Comment on attachment 38357 [details]
set file url pref to secure

Looks fine to me.  What about Gtk?  Mark should at least see this since he reviewed the previous patch.
Comment 6 Adam Barth 2009-08-21 09:37:25 PDT
I'm going to hold off landing this for a little while to give folks a chance to comment.

In the blackout, I commented that I omitted GTK on purpose because I didn't see a public API for manipulating this setting.  I'll file a followup bug about that after we resolve this one.
Comment 7 Jan Alonzo 2009-08-21 14:57:22 PDT
(In reply to comment #6)
> I'm going to hold off landing this for a little while to give folks a chance to
> comment.
> 
> In the blackout, I commented that I omitted GTK on purpose because I didn't see
> a public API for manipulating this setting.  I'll file a followup bug about
> that after we resolve this one.

FWIW, for GTK, we have WebKit/gtk/webkit/webkitwebsettings for the API and update the values of WebCore::Settings in WebKit/gtk/webkit/webkitwebview (webkit_web_view_update_settings function).
Comment 8 Alexey Proskuryakov 2009-08-21 15:00:35 PDT
-    CFDictionaryAddValue(defaults, CFSTR(WebKitAllowUniversalAccessFromFileURLsPreferenceKey), kCFBooleanTrue);
+    CFDictionaryAddValue(defaults, CFSTR(WebKitAllowUniversalAccessFromFileURLsPreferenceKey), kCFBooleanFalse);

These are default preferences - what about users who already had Safari installed on their machines before upgrading to a version that has this change?
Comment 9 Adam Barth 2009-08-21 23:06:46 PDT
(In reply to comment #8)
> These are default preferences - what about users who already had Safari
> installed on their machines before upgrading to a version that has this change?

Oh, I have no idea how preferences work on the Mac.  If they haven't overridden the default, will this not affect them?

I guess one option is to rename the pref key.
Comment 10 Alexey Proskuryakov 2009-08-22 08:46:49 PDT
Hmm, you are probably right - I don't see the preference written in com.apple.Safari defaults, so the default doesn't seem to be permanent.
Comment 11 Adam Barth 2009-08-22 21:21:08 PDT
Comment on attachment 38357 [details]
set file url pref to secure

Rejecting patch 38357 from commit-queue.  This patch will require manual commit.

Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
http://webkit.org/coding/contributing.html
Comment 12 Adam Barth 2009-08-22 21:24:20 PDT
Comment on attachment 38357 [details]
set file url pref to secure

Clearing flags on attachment: 38357

Committed r47684: <http://trac.webkit.org/changeset/47684>
Comment 13 Adam Barth 2009-08-22 21:24:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Adam Barth 2009-08-22 21:45:04 PDT
After a discussion on IRC, I've reverted this change.  The consensus was to make this change later after various clients have had a chance to opt into the insecure setting if they need it for some reason.
Comment 15 Adam Barth 2009-08-24 23:11:42 PDT
Created attachment 38528 [details]
set file url pref to secure


---
 8 files changed, 44 insertions(+), 2 deletions(-)
Comment 16 Adam Barth 2009-08-24 23:13:10 PDT
As discussed on webkit-dev, I've updated this patch to affect win and qt, but not mac.  I'd still like to do GTK in another patch because I don't quite understand its preference system.
Comment 17 Jan Alonzo 2009-08-25 05:57:40 PDT
(In reply to comment #16)
> not mac.  I'd still like to do GTK in another patch because I don't quite
> understand its preference system.

Hi Adam. 

This is already implemented for GTK[1] following your email to webkit-dev.

[1] http://trac.webkit.org/changeset/47690
Comment 18 Adam Barth 2009-08-25 09:13:44 PDT
(In reply to comment #17)
> This is already implemented for GTK[1] following your email to webkit-dev.

Awesome!  One detail: you might want to set the setting to true in DumpRenderTree because some of the LayoutTests assume file URLs can access data URLs.
Comment 19 Eric Seidel (no email) 2009-08-26 22:16:04 PDT
Comment on attachment 38528 [details]
set file url pref to secure

Rejecting patch 38528 from commit-queue.  This patch will require manual commit.

['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1
Comment 20 Adam Barth 2009-08-26 22:21:43 PDT
Comment on attachment 38528 [details]
set file url pref to secure

Clearing flags on attachment: 38528

Committed r47807: <http://trac.webkit.org/changeset/47807>
Comment 21 Adam Barth 2009-08-26 22:21:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Eric Seidel (no email) 2009-08-26 23:28:01 PDT
Sorry.  run-webkit-tests had left an httpd process running causing all http tests to fail on the commit-queue bot.