[WebAuthN] Change the nonce in the CTAP kInit command to weak random values

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jiewen Tan]

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

[Matt Lewis]

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

[Radar WebKit Bug Importer]

<rdar://problem/46471091>

[Matt Lewis]

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.

[Matt Lewis]

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.

[Jiewen Tan]

Created attachment 356665 [details]
Part 1.

[Jiewen Tan]

Comment on attachment 356665 [details]
Part 1.

Thanks Dewei for r+ this patch.

[WebKit Commit Bot]

Comment on attachment 356665 [details]
Part 1.

Clearing flags on attachment: 356665

Committed r238919: <https://trac.webkit.org/changeset/238919>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jiewen Tan]

https://trac.webkit.org/changeset/238936/webkit

[Truitt Savell]

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

That's the intention of this change. Jiewen can continue investigation based on extra logging info.

[Jiewen Tan]

Created attachment 357284 [details]
Part 2.

[WebKit Commit Bot]

Comment on attachment 357284 [details]
Part 2.

Clearing flags on attachment: 357284

Committed r239235: <https://trac.webkit.org/changeset/239235>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

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.

[Ryan Haddad]

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

[Jiewen Tan]

Created attachment 357480 [details]
Part 3.

[Jiewen Tan]

Comment on attachment 357480 [details]
Part 3.

Thanks Dewei for r+ the patch.

[WebKit Commit Bot]

Comment on attachment 357480 [details]
Part 3.

Clearing flags on attachment: 357480

Committed r239323: <https://trac.webkit.org/changeset/239323>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jiewen Tan]

Reopened for further investigation.

[Jiewen Tan]

From the new logs, it looks like that m_requestTimeOutTimer fires disregard the time out value.

[Jiewen Tan]

Created attachment 357622 [details]
Part 4.

[WebKit Commit Bot]

Comment on attachment 357622 [details]
Part 4.

Clearing flags on attachment: 357622

Committed r239362: <https://trac.webkit.org/changeset/239362>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jiewen Tan]

Reopen for further investigation.

[Jiewen Tan]

Created attachment 357969 [details]
Part 5.

[Jiewen Tan]

Comment on attachment 357969 [details]
Part 5.

Thanks, Dewei.

[WebKit Commit Bot]

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

[Jiewen Tan]

Committed r239518: <https://trac.webkit.org/changeset/239518>

[Jiewen Tan]

Reopened for further investigation.

[Truitt Savell]

Any further movement on this?

[Jiewen Tan]

Created attachment 358635 [details]
Part 6.

[Jiewen Tan]

Comment on attachment 358635 [details]
Part 6.

Dewei, thanks again.

[WebKit Commit Bot]

Comment on attachment 358635 [details]
Part 6.

Clearing flags on attachment: 358635

Committed r239757: <https://trac.webkit.org/changeset/239757>

[Jiewen Tan]

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.

[Jiewen Tan]

Created attachment 358758 [details]
Patch

[Chris Dumez]

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.

[Jiewen Tan]

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.

[Jiewen Tan]

Created attachment 358844 [details]
Patch

[Chris Dumez]

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);

[Jiewen Tan]

Created attachment 358846 [details]
Patch for landing

[Jiewen Tan]

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.

[WebKit Commit Bot]

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

[WebKit Commit Bot]

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

[WebKit Commit Bot]

Comment on attachment 358846 [details]
Patch for landing

Clearing flags on attachment: 358846

Committed r239852: <https://trac.webkit.org/changeset/239852>

[Jiewen Tan]

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.

[Jiewen Tan]

Committed r239958: <https://trac.webkit.org/changeset/239958>

[Shawn Roberts]

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.