Bug 146531 - Reduce resolution of performance.now
Summary: Reduce resolution of performance.now
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-07-01 16:05 PDT by Alex Christensen
Modified: 2016-05-29 17:23 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.31 KB, patch)
2015-07-01 16:07 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (3.21 KB, patch)
2015-07-01 17:26 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (3.19 KB, patch)
2015-07-01 17:28 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (2.00 KB, patch)
2015-07-01 20:53 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2015-07-01 16:05:38 PDT
performance.now has nanosecond resolution right now, which makes timing attacks too precise.  Let's reduce that resolution by flooring the value to make sure we don't get times in the future.
Comment 1 Alex Christensen 2015-07-01 16:07:49 PDT
Created attachment 255963 [details]
Patch
Comment 2 Alex Christensen 2015-07-01 17:26:32 PDT
Created attachment 255974 [details]
Patch
Comment 3 Alex Christensen 2015-07-01 17:28:14 PDT
Created attachment 255975 [details]
Patch
Comment 4 Alex Christensen 2015-07-01 17:31:07 PDT
http://trac.webkit.org/changeset/186208
Comment 5 Chris Dumez 2015-07-01 20:10:42 PDT
This broke http/tests/misc/webtiming-resolution.html:
--- /Volumes/Data/slave/syrah-debug-wk2-tests/build/OpenSource/layout-test-results/http/tests/misc/webtiming-resolution-expected.txt
+++ /Volumes/Data/slave/syrah-debug-wk2-tests/build/OpenSource/layout-test-results/http/tests/misc/webtiming-resolution-actual.txt
@@ -3,5 +3,5 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS ((t1 - t0) / 0.005) % 1 < 1e-10 is true
+FAIL ((t1 - t0) / 0.005) % 1 < 1e-10 should be true. Was false.
Comment 6 Chris Dumez 2015-07-01 20:12:22 PDT
Hmm.. I see you actually introduced the test that is failing.
Comment 7 Chris Dumez 2015-07-01 20:20:21 PDT
(In reply to comment #5)
> This broke http/tests/misc/webtiming-resolution.html:
> ---
> /Volumes/Data/slave/syrah-debug-wk2-tests/build/OpenSource/layout-test-
> results/http/tests/misc/webtiming-resolution-expected.txt
> +++
> /Volumes/Data/slave/syrah-debug-wk2-tests/build/OpenSource/layout-test-
> results/http/tests/misc/webtiming-resolution-actual.txt
> @@ -3,5 +3,5 @@
>  On success, you will see a series of "PASS" messages, followed by "TEST
> COMPLETE".
>  
>  
> -PASS ((t1 - t0) / 0.005) % 1 < 1e-10 is true
> +FAIL ((t1 - t0) / 0.005) % 1 < 1e-10 should be true. Was false.

I can reproduce locally. The left side has the following value:
0.999999999999801
Comment 8 Chris Dumez 2015-07-01 20:52:50 PDT
Reopening due to test failure.
Comment 9 Chris Dumez 2015-07-01 20:53:23 PDT
Created attachment 255989 [details]
Patch
Comment 10 Chris Dumez 2015-07-01 20:57:37 PDT
Comment on attachment 255989 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=255989&action=review

> LayoutTests/http/tests/misc/webtiming-resolution.html:-16
> -shouldBe("((t1 - t0) / 0.005) % 1 < 1e-10", "true");

The previous check did not work for example if (t1 - t0) is 0.00499999999999:
(0.00499999999999 / 0.005) % 1 is 0.999999999998 which is greater than 1e-10.
Comment 11 Chris Dumez 2015-07-01 21:01:59 PDT
Comment on attachment 255989 [details]
Patch

Clearing flags on attachment: 255989

Committed r186216: <http://trac.webkit.org/changeset/186216>
Comment 12 Chris Dumez 2015-07-01 21:02:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Csaba Osztrogonác 2015-07-01 23:41:42 PDT
(In reply to comment #11)
> Comment on attachment 255989 [details]
> Patch
> 
> Clearing flags on attachment: 255989
> 
> Committed r186216: <http://trac.webkit.org/changeset/186216>

This new test fails everywhere:

--- /Volumes/Data/slave/mavericks-release-tests-wk1/build/layout-test-results/http/tests/misc/webtiming-resolution-expected.txt
+++ /Volumes/Data/slave/mavericks-release-tests-wk1/build/layout-test-results/http/tests/misc/webtiming-resolution-actual.txt
@@ -1,7 +1,7 @@
+CONSOLE MESSAGE: line 11: ReferenceError: Can't find variable: performance
 Verifies the minimum resolution is 5 microseconds.
 
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS Math.abs(1 - (t1 - t0) / 0.005) < 1e-10 is true
Comment 14 Alex Christensen 2015-07-02 00:12:29 PDT
More test fixing in http://trac.webkit.org/changeset/186219
Comment 15 Alex Christensen 2015-07-02 00:28:53 PDT
More bot fixing in http://trac.webkit.org/changeset/186222
Comment 16 Eli Grey (:sephr) 2016-05-29 16:37:34 PDT
This change only serves to harm victims of timing attacks even more. For example, it doesn't prevent me from running my navigator.hardwareConcurrency timing attack, it just makes it take longer to complete and wastes additional CPU cycles.

This is not how software should address timing attack vulnerabilities. You harden the vulnerable APIs with NOPs and wait times for completion time normalization.
Comment 17 Eli Grey (:sephr) 2016-05-29 17:23:33 PDT
See this comment from from the relevant spec issue:

https://github.com/tc39/ecmascript_sharedmem/issues/1#issuecomment-175315327