Bug 192061 - [WebAuthN] Change the nonce in the CTAP kInit command to weak random values
Summary: [WebAuthN] Change the nonce in the CTAP kInit command to weak random values
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks: 181943
  Show dependency treegraph
 
Reported: 2018-11-27 18:04 PST by Jiewen Tan
Modified: 2019-02-05 09:35 PST (History)
10 users (show)

See Also:


Attachments
Part 1. (2.33 KB, patch)
2018-12-05 15:03 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Part 2. (6.82 KB, patch)
2018-12-13 18:59 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Part 3. (4.95 KB, patch)
2018-12-17 14:58 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Part 4. (1.83 KB, patch)
2018-12-18 15:22 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Part 5. (2.70 KB, patch)
2018-12-21 12:56 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Part 6. (2.78 KB, patch)
2019-01-08 14:08 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (12.38 KB, patch)
2019-01-09 16:04 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (12.32 KB, patch)
2019-01-10 15:34 PST, Jiewen Tan
cdumez: review+
Details | Formatted Diff | Diff
Patch for landing (12.33 KB, patch)
2019-01-10 16:09 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-cq-02 for mac-sierra (1.08 MB, application/zip)
2019-01-10 16:50 PST, WebKit Commit Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2018-11-27 18:04:12 PST
The following layout test are flaky on [Apple Mojave Release WK2]

http/wpt/webauthn/public-key-credential-create-success-hid.https.html
http/wpt/webauthn/public-key-credential-get-success-hid.https.html

Flakiness Dashboard:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-create-success-hid.https.html
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-get-success-hid.https.html
Comment 1 Matt Lewis 2018-11-28 11:20:05 PST
This is also happening on Mac OS High Sierra.

https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/r238612%20(1064)/results.html 

diff:
--- /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/http/wpt/webauthn/public-key-credential-create-success-hid.https-expected.txt
+++ /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/http/wpt/webauthn/public-key-credential-create-success-hid.https-actual.txt
@@ -2,7 +2,7 @@
 PASS PublicKeyCredential's [[create]] with minimum options in a mock local authenticator. 
 PASS PublicKeyCredential's [[create]] with authenticatorSelection { 'cross-platform' } in a mock local authenticator. 
 PASS PublicKeyCredential's [[create]] with requireResidentKey { false } in a mock local authenticator. 
-PASS PublicKeyCredential's [[create]] with userVerification { 'preferred' } in a mock local authenticator. 
+FAIL PublicKeyCredential's [[create]] with userVerification { 'preferred' } in a mock local authenticator. promise_test: Unhandled rejection with value: object "NotAllowedError: Operation timed out."
 PASS PublicKeyCredential's [[create]] with userVerification { 'discouraged' } in a mock local authenticator. 
-PASS PublicKeyCredential's [[create]] with mixed options in a mock local authenticator. 
+FAIL PublicKeyCredential's [[create]] with mixed options in a mock local authenticator. promise_test: Unhandled rejection with value: object "NotAllowedError: Operation timed out."
Comment 2 Radar WebKit Bug Importer 2018-12-04 16:49:23 PST
<rdar://problem/46471091>
Comment 3 Matt Lewis 2018-12-04 16:52:24 PST
These test have brought on a large amount of flakiness that needs to be looked into. The original bug was filed in the intention that they would be worked on shortly after. The fact that these test have been this flaky since being added should mean we need to make sure these are fixed soon.
Comment 4 Matt Lewis 2018-12-04 16:54:26 PST
It should also be noted that the commit that these test were added in was https://trac.webkit.org/changeset/238166/webkit

As well as this test is now flaky on Sierra as well.
Comment 5 Jiewen Tan 2018-12-05 15:03:08 PST
Created attachment 356665 [details]
Part 1.
Comment 6 Jiewen Tan 2018-12-05 18:03:33 PST
Comment on attachment 356665 [details]
Part 1.

Thanks Dewei for r+ this patch.
Comment 7 WebKit Commit Bot 2018-12-05 18:28:31 PST
Comment on attachment 356665 [details]
Part 1.

Clearing flags on attachment: 356665

Committed r238919: <https://trac.webkit.org/changeset/238919>
Comment 8 WebKit Commit Bot 2018-12-05 18:28:32 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Jiewen Tan 2018-12-06 11:36:13 PST
https://trac.webkit.org/changeset/238936/webkit
Comment 10 Truitt Savell 2018-12-07 09:37:46 PST
Both these tests still appear to be flakey but with new Diffs. 

--- /Volumes/Data/slave/sierra-debug-tests-wk2/build/layout-test-results/http/wpt/webauthn/public-key-credential-create-success-hid.https-expected.txt
+++ /Volumes/Data/slave/sierra-debug-tests-wk2/build/layout-test-results/http/wpt/webauthn/public-key-credential-create-success-hid.https-actual.txt
@@ -1,5 +1,5 @@
 
-PASS PublicKeyCredential's [[create]] with minimum options in a mock local authenticator. 
+FAIL PublicKeyCredential's [[create]] with minimum options in a mock local authenticator. promise_test: Unhandled rejection with value: object "NotAllowedError: Operation timed out."
 PASS PublicKeyCredential's [[create]] with authenticatorSelection { 'cross-platform' } in a mock local authenticator. 
 PASS PublicKeyCredential's [[create]] with requireResidentKey { false } in a mock local authenticator. 
 PASS PublicKeyCredential's [[create]] with userVerification { 'preferred' } in a mock local authenticator. 



--- /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/http/wpt/webauthn/public-key-credential-get-success-hid.https-expected.txt
+++ /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/http/wpt/webauthn/public-key-credential-get-success-hid.https-actual.txt
@@ -1,7 +1,7 @@
 
 PASS PublicKeyCredential's [[get]] with minimum options in a mock hid authenticator. 
 PASS PublicKeyCredential's [[get]] with matched allow credentials in a mock hid authenticator. 
-PASS PublicKeyCredential's [[get]] with userVerification { preferred } in a mock hid authenticator. 
+FAIL PublicKeyCredential's [[get]] with userVerification { preferred } in a mock hid authenticator. promise_test: Unhandled rejection with value: object "NotAllowedError: Operation timed out."
 PASS PublicKeyCredential's [[get]] with userVerification { discouraged } in a mock hid authenticator. 
 PASS PublicKeyCredential's [[get]] with mixed options in a mock hid authenticator. 
 


Combined history:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-create-success-hid.https.html%20http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-get-success-hid.https.html
Comment 11 dewei_zhu 2018-12-07 14:35:01 PST
That's the intention of this change. Jiewen can continue investigation based on extra logging info.
Comment 12 Jiewen Tan 2018-12-13 18:59:34 PST
Created attachment 357284 [details]
Part 2.
Comment 13 WebKit Commit Bot 2018-12-14 14:49:14 PST
Comment on attachment 357284 [details]
Part 2.

Clearing flags on attachment: 357284

Committed r239235: <https://trac.webkit.org/changeset/239235>
Comment 14 WebKit Commit Bot 2018-12-14 14:49:17 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Matt Lewis 2018-12-17 13:03:38 PST
I think this bug was closed accidentally as the patch was for additional logging correct? Jiewen, is this correct? If so we need to reopen this bug.
Comment 16 Ryan Haddad 2018-12-17 13:07:23 PST
(In reply to Matt Lewis from comment #15)
> I think this bug was closed accidentally as the patch was for additional
> logging correct? Jiewen, is this correct? If so we need to reopen this bug.
Looking at the change, I'm going to assume the answer is yes. Reopening.
Comment 17 Jiewen Tan 2018-12-17 14:58:55 PST
Created attachment 357480 [details]
Part 3.
Comment 18 Jiewen Tan 2018-12-17 16:51:36 PST
Comment on attachment 357480 [details]
Part 3.

Thanks Dewei for r+ the patch.
Comment 19 WebKit Commit Bot 2018-12-17 22:37:35 PST
Comment on attachment 357480 [details]
Part 3.

Clearing flags on attachment: 357480

Committed r239323: <https://trac.webkit.org/changeset/239323>
Comment 20 WebKit Commit Bot 2018-12-17 22:37:37 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Jiewen Tan 2018-12-17 23:02:54 PST
Reopened for further investigation.
Comment 22 Jiewen Tan 2018-12-18 14:17:50 PST
From the new logs, it looks like that m_requestTimeOutTimer fires disregard the time out value.
Comment 23 Jiewen Tan 2018-12-18 15:22:03 PST
Created attachment 357622 [details]
Part 4.
Comment 24 WebKit Commit Bot 2018-12-18 17:25:59 PST
Comment on attachment 357622 [details]
Part 4.

Clearing flags on attachment: 357622

Committed r239362: <https://trac.webkit.org/changeset/239362>
Comment 25 WebKit Commit Bot 2018-12-18 17:26:01 PST
All reviewed patches have been landed.  Closing bug.
Comment 26 Jiewen Tan 2018-12-18 17:29:34 PST
Reopen for further investigation.
Comment 27 Jiewen Tan 2018-12-21 12:56:59 PST
Created attachment 357969 [details]
Part 5.
Comment 28 Jiewen Tan 2018-12-21 14:19:18 PST
Comment on attachment 357969 [details]
Part 5.

Thanks, Dewei.
Comment 29 WebKit Commit Bot 2018-12-21 14:20:28 PST
Comment on attachment 357969 [details]
Part 5.

Rejecting attachment 357969 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 357969, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebKit/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/10511576
Comment 30 Jiewen Tan 2018-12-21 14:39:43 PST
Committed r239518: <https://trac.webkit.org/changeset/239518>
Comment 31 Jiewen Tan 2018-12-21 14:40:26 PST
Reopened for further investigation.
Comment 32 Truitt Savell 2019-01-07 13:54:02 PST
Any further movement on this?
Comment 33 Jiewen Tan 2019-01-08 14:08:50 PST
Created attachment 358635 [details]
Part 6.
Comment 34 Jiewen Tan 2019-01-08 14:21:36 PST
Comment on attachment 358635 [details]
Part 6.

Dewei, thanks again.
Comment 35 WebKit Commit Bot 2019-01-08 17:11:17 PST
Comment on attachment 358635 [details]
Part 6.

Clearing flags on attachment: 358635

Committed r239757: <https://trac.webkit.org/changeset/239757>
Comment 36 Jiewen Tan 2019-01-09 15:19:19 PST
I think I know what's going on here.

We use cryptographically strong nonces to do hand shakes with authenticators to establish communication channels. What happens here is the entropy of system is running out because of large amount of tests, and then we have to wait until it is refilled. That's why we have this random time outs.

To fix this, I propose to change cryptographically strong nonces to pseudo random numbers. Since the nonce is more or less an identifier (that pretends to be globally unique) instead of a protection mechanism of replay attacks, it should be safe to use a pseudo random one.
Comment 37 Jiewen Tan 2019-01-09 16:04:50 PST
Created attachment 358758 [details]
Patch
Comment 38 Chris Dumez 2019-01-10 15:08:42 PST
Comment on attachment 358758 [details]
Patch

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

> Source/WebKit/ChangeLog:11
> +        preventing replay attacks. Otherwise, it might exhaust system entrophy unnecessarily.

Did you mean entropy?

> Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidDriver.cpp:156
> +    for (size_t i = 0; i < steps; i++) {

Why do we need a union?

size_t steps = kHidInitNonceLength / sizeof(uint32_t);
for (size_t i = 0; i < steps; ++i) {
    uint32_t weakRandom =  weakRandomUint32();
    memcpy(m_nonce.data() + i * sizeof(uint32_t), &weakRandom, sizeof(uint32_t));
}

// or the following:

size_t bytesWritten = 0;
do {
    uint32_t weakRandom =  weakRandomUint32();
    size_t bytesToWrite = std::min(bytesWritten + sizeof(uint32_t), kHidInitNonceLength) - bytesWritten;
    memcpy(m_nonce.data() + bytesWritten, &weakRandom, bytesToWrite);
    bytesWritten += bytesToWrite;
} while (bytesWritten < kHidInitNonceLength);

Which has the benefit of working as well if kHidInitNonceLength if less than 4 or not a multiple of 4.
Comment 39 Jiewen Tan 2019-01-10 15:32:01 PST
Comment on attachment 358758 [details]
Patch

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

Thanks Chris for reviewing the code.

>> Source/WebKit/ChangeLog:11
>> +        preventing replay attacks. Otherwise, it might exhaust system entrophy unnecessarily.
> 
> Did you mean entropy?

Yup.

>> Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidDriver.cpp:156
>> +    for (size_t i = 0; i < steps; i++) {
> 
> Why do we need a union?
> 
> size_t steps = kHidInitNonceLength / sizeof(uint32_t);
> for (size_t i = 0; i < steps; ++i) {
>     uint32_t weakRandom =  weakRandomUint32();
>     memcpy(m_nonce.data() + i * sizeof(uint32_t), &weakRandom, sizeof(uint32_t));
> }
> 
> // or the following:
> 
> size_t bytesWritten = 0;
> do {
>     uint32_t weakRandom =  weakRandomUint32();
>     size_t bytesToWrite = std::min(bytesWritten + sizeof(uint32_t), kHidInitNonceLength) - bytesWritten;
>     memcpy(m_nonce.data() + bytesWritten, &weakRandom, bytesToWrite);
>     bytesWritten += bytesToWrite;
> } while (bytesWritten < kHidInitNonceLength);
> 
> Which has the benefit of working as well if kHidInitNonceLength if less than 4 or not a multiple of 4.

You are right that we don't need the union.
Comment 40 Jiewen Tan 2019-01-10 15:34:54 PST
Created attachment 358844 [details]
Patch
Comment 41 Chris Dumez 2019-01-10 15:43:16 PST
Comment on attachment 358844 [details]
Patch

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

> Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidDriver.cpp:152
> +    ASSERT(!(kHidInitNonceLength % sizeof(uint32_t)));

To be safe, I'd do:
ASSERT(steps >= 1);
Comment 42 Jiewen Tan 2019-01-10 16:09:56 PST
Created attachment 358846 [details]
Patch for landing
Comment 43 Jiewen Tan 2019-01-10 16:10:05 PST
Comment on attachment 358844 [details]
Patch

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

Thanks Chris for r+ the patch.

>> Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidDriver.cpp:152
>> +    ASSERT(!(kHidInitNonceLength % sizeof(uint32_t)));
> 
> To be safe, I'd do:
> ASSERT(steps >= 1);

Fixed.
Comment 44 WebKit Commit Bot 2019-01-10 16:50:36 PST
Comment on attachment 358846 [details]
Patch for landing

Rejecting attachment 358846 [details] from commit-queue.

Number of test failures exceeded the failure limit.
Full output: https://webkit-queues.webkit.org/results/10703487
Comment 45 WebKit Commit Bot 2019-01-10 16:50:38 PST
Created attachment 358851 [details]
Archive of layout-test-results from webkit-cq-02 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-02  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 46 WebKit Commit Bot 2019-01-10 17:42:44 PST
Comment on attachment 358846 [details]
Patch for landing

Clearing flags on attachment: 358846

Committed r239852: <https://trac.webkit.org/changeset/239852>
Comment 47 Jiewen Tan 2019-01-14 15:21:53 PST
The patch does fix the flakiness of ctap-hid-failure.https.html and improve overall pass rate of the affected tests. The reason why tests are still failing is that the timeout values for those tests are too soon for they to complete in bots.
Comment 48 Jiewen Tan 2019-01-14 15:45:55 PST
Committed r239958: <https://trac.webkit.org/changeset/239958>
Comment 49 Shawn Roberts 2019-02-05 09:35:50 PST
Test is still a flaky timeout failure on Mac WK2.

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-get-success-hid.https.html

Diff:

--- /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/http/wpt/webauthn/public-key-credential-get-success-hid.https-expected.txt
+++ /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/http/wpt/webauthn/public-key-credential-get-success-hid.https-actual.txt
@@ -1,7 +1,7 @@
 
 PASS PublicKeyCredential's [[get]] with minimum options in a mock hid authenticator. 
 PASS PublicKeyCredential's [[get]] with matched allow credentials in a mock hid authenticator. 
-PASS PublicKeyCredential's [[get]] with userVerification { preferred } in a mock hid authenticator. 
+FAIL PublicKeyCredential's [[get]] with userVerification { preferred } in a mock hid authenticator. promise_test: Unhandled rejection with value: object "NotAllowedError: Operation timed out."
 PASS PublicKeyCredential's [[get]] with userVerification { discouraged } in a mock hid authenticator. 
 PASS PublicKeyCredential's [[get]] with mixed options in a mock hid authenticator.