Give WebKit clients a way to replace window.screen to foil attempts to track users with it

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Robert Hogan]

From https://bugzilla.mozilla.org/show_bug.cgi?id=418986
"The window.screen object can be used to build an identifier that can be used to
track users independent of IP. Based on my rough calculations, there should be
at least 29 bits of extractable state from desktop geometry, toolbar geometry,
window size, etc.

Currently, there is no way to obscure this information without injecting
javascript into the contentWindow to hook the screen object. It would be nice
if the browser provided a pref to say "only report my content window dimensions
as inner and outer height." This pref would cause the screen object to behave
as if the contentWindow was the size of the entire desktop, with no chrome or
desktop toolbar size information."

What he says!

[Robert Hogan]

Created attachment 66397 [details]
Patch

[Adam Barth]

Comment on attachment 66397 [details]
Patch

This changes is great, but we need a test.

[Robert Hogan]

(In reply to comment #2)
> (From update of attachment 66397 [details])
> This changes is great, but we need a test.

Cool, I'll come up with one and resubmit. Thanks for the quick feedback!

[Robert Hogan]

Created attachment 66408 [details]
Patch

[Adam Barth]

Comment on attachment 66408 [details]
Patch

Can't we test this in a LayoutTest?  I thought the Replacable IDL attribue had some web-platform facing behavior change.

[Robert Hogan]

Created attachment 66582 [details]
Patch

[Robert Hogan]

Chromium-Win and Chromium-Mac have platform-specific test results for this test - not sure how to generate png/checksum for those two so have not touched either.

[Adam Barth]

Comment on attachment 66582 [details]
Patch

Thanks for the patch.  It would be polite to give the Chromium WebKit gardener a heads up about the expected test behavior change when you land this patch.  If you ask in #webkit, someone can lookup who's on duty in the schedule.

[WebKit Commit Bot]

Comment on attachment 66582 [details]
Patch

Rejecting patch 66582 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing,media', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 20932 test cases.
fast/js/var-declarations-shadowing.html -> failed

Exiting early after 1 failures. 8583 tests run.
193.02s total testing time

8582 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

Full output: http://queues.webkit.org/results/3905279

[Robert Hogan]

Created attachment 66752 [details]
Patch

[Darin Adler]

Is there a way we can give this power to WebKit clients without giving it to webpages too? This seems an unwanted behavior change for webpages, no?

[Adam Barth]

(In reply to comment #11)
> Is there a way we can give this power to WebKit clients without giving it to webpages too? This seems an unwanted behavior change for webpages, no?

Why is it unwanted for webpages?

[Darin Adler]

(In reply to comment #12)
> (In reply to comment #11)
> > Is there a way we can give this power to WebKit clients without giving it to webpages too? This seems an unwanted behavior change for webpages, no?
> 
> Why is it unwanted for webpages?

I had presumed that the previous lack of the [Replaceable] flag was based on website compatibility and browser consistency considerations, and I see no argument here in this bug saying those have changed.

[Adam Barth]

> I had presumed that the previous lack of the [Replaceable] flag was based on website compatibility and browser consistency considerations, and I see no argument here in this bug saying those have changed.

I don't know the history here.  We had a meeting some folks from the Tor project recently and they expressed interest in being able to override the screen property also, which tells me there's desire to make it Replaceable.  I'd be inclined to try making it Replaceable and see if there are compat problems.

[WebKit Commit Bot]

Comment on attachment 66752 [details]
Patch

Rejecting patch 66752 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing,media', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 20933 test cases.
fast/js/sputnik/Conformance/08_Types/8.6_The_Object_Type/8.6.2_Internal_Properties_and_Methods/S8.6.2_A5_T1.html -> failed

Exiting early after 1 failures. 9223 tests run.
216.25s total testing time

9222 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

Full output: http://queues.webkit.org/results/3946289

[Robert Hogan]

Created attachment 66932 [details]
Patch

[Robert Hogan]

(In reply to comment #16)
> Created an attachment (id=66932) [details]
> Patch

Apologies for the churn. There are definitely no other tests that can fail!

[WebKit Commit Bot]

Comment on attachment 66932 [details]
Patch

Rejecting patch 66932 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing,media', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 20944 test cases.
http/tests/security/cross-frame-access-put.html -> failed

Exiting early after 1 failures. 20358 tests run.
641.36s total testing time

20357 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
27 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/3974338

[Robert Hogan]

(In reply to comment #18)
> http/tests/security/cross-frame-access-put.html -> failed

This test is skipped in Qt but doesn't need to be - I can recreate the failure and get it to pass otherwise.

The difference in results is an extra console message:

CONSOLE MESSAGE: line 1: Unsafe JavaScript attempt to access frame with URL http://localhost:8000/security/resources/cross-frame-iframe-for-put-test.html from frame with URL http://127.0.0.1:8000/security/cross-frame-access-put.html. Domains, protocols and ports must match.

for the attempt to shadow the screen object cross-origin. The 'replaceable' attribute generates this new piece of code that the test invokes and which generates the console message:

void setJSDOMWindowScreen(ExecState* exec, JSObject* thisObject, JSValue value)
{
    if (!static_cast<JSDOMWindow*>(thisObject)->allowsAccessFrom(exec))
        return;
    // Shadowing a built-in object
    static_cast<JSDOMWindow*>(thisObject)->putDirect(Identifier(exec, "screen"), value);
}

So the test hasn't identified a regression. Updating results and resubmitting.

[Robert Hogan]

Created attachment 67019 [details]
Patch

[Adam Barth]

Comment on attachment 67019 [details]
Patch

Ok.  Please run all the tests next time before uploading patches.  It's a bit bit of a time sink to keep spinning on these patches.

[Antonio Gomes]

(In reply to comment #20)
> Created an attachment (id=67019) [details]
> Patch

tip: Robert, it has been already reviewed. In cases when you are just updating a non-harmful test expectation, you likely do not need to re-requestion review.

Pre-full the "Reviewed by" with the reviewer name, and upload it setting cq+ to the patch. It's been working fine for me.

[WebKit Commit Bot]

Comment on attachment 67019 [details]
Patch

Clearing flags on attachment: 67019

Committed r67100: <http://trac.webkit.org/changeset/67100>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.