Bug 28480 – Don't let local files access web URLs

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Adam Barth]

We shouldn't let local files access web URLs.  Our current behavior makes HTML files loaded from your local file system super dangerous.

[Adam Barth]

Created attachment 35180 [details]
Patch v1

[Adam Barth]

Chrome has been running with this setting for like a year with no problems.

[Mark Rowe (bdash)]

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 @@
> [phone]  Adam Barth  <[e-mail]>
> +
> +        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  <[e-mail]>
>  
>          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.

[Adam Barth]

Created attachment 38357 [details]
set file url pref to secure


---
 11 files changed, 61 insertions(+), 3 deletions(-)

[Eric Seidel (no email)]

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.

[Adam Barth]

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.

[Jan Alonzo]

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

[Alexey Proskuryakov]

-    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?

[Adam Barth]

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

[Alexey Proskuryakov]

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.

[Adam Barth]

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

[Adam Barth]

Comment on attachment 38357 [details]
set file url pref to secure

Clearing flags on attachment: 38357

Committed r47684: <http://trac.webkit.org/changeset/47684>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

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.

[Adam Barth]

Created attachment 38528 [details]
set file url pref to secure


---
 8 files changed, 44 insertions(+), 2 deletions(-)

[Adam Barth]

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.

[Jan Alonzo]

(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

[Adam Barth]

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

[Eric Seidel (no email)]

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

[Adam Barth]

Comment on attachment 38528 [details]
set file url pref to secure

Clearing flags on attachment: 38528

Committed r47807: <http://trac.webkit.org/changeset/47807>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Sorry.  run-webkit-tests had left an httpd process running causing all http tests to fail on the commit-queue bot.