Bug 192061

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

<rdar://problem/46471091>

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.

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.

Created attachment 356665 [details] Part 1.

Comment on attachment 356665 [details] Part 1. Thanks Dewei for r+ this patch.

Comment on attachment 356665 [details] Part 1. Clearing flags on attachment: 356665 Committed r238919: <https://trac.webkit.org/changeset/238919>

All reviewed patches have been landed. Closing bug.

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.

Created attachment 357284 [details] Part 2.

Comment on attachment 357284 [details] Part 2. Clearing flags on attachment: 357284 Committed r239235: <https://trac.webkit.org/changeset/239235>

All reviewed patches have been landed. Closing bug.

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.

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

Created attachment 357480 [details] Part 3.

Comment on attachment 357480 [details] Part 3. Thanks Dewei for r+ the patch.

Comment on attachment 357480 [details] Part 3. Clearing flags on attachment: 357480 Committed r239323: <https://trac.webkit.org/changeset/239323>

All reviewed patches have been landed. Closing bug.

Reopened for further investigation.

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

Created attachment 357622 [details] Part 4.

Comment on attachment 357622 [details] Part 4. Clearing flags on attachment: 357622 Committed r239362: <https://trac.webkit.org/changeset/239362>

All reviewed patches have been landed. Closing bug.

Reopen for further investigation.

Created attachment 357969 [details] Part 5.

Comment on attachment 357969 [details] Part 5. Thanks, Dewei.

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

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

Reopened for further investigation.

Any further movement on this?

Created attachment 358635 [details] Part 6.

Comment on attachment 358635 [details] Part 6. Dewei, thanks again.

Comment on attachment 358635 [details] Part 6. Clearing flags on attachment: 358635 Committed r239757: <https://trac.webkit.org/changeset/239757>

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.

Created attachment 358758 [details] Patch

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

Created attachment 358844 [details] Patch

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

Created attachment 358846 [details] Patch for landing

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

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 on attachment 358846 [details] Patch for landing Clearing flags on attachment: 358846 Committed r239852: <https://trac.webkit.org/changeset/239852>

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.

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

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.