[WebAuthN] Change the nonce in the CTAP kInit command to weak random values
authorjiewen_tan@apple.com <jiewen_tan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 11 Jan 2019 01:42:34 +0000 (01:42 +0000)
committerjiewen_tan@apple.com <jiewen_tan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 11 Jan 2019 01:42:34 +0000 (01:42 +0000)
https://bugs.webkit.org/show_bug.cgi?id=192061
<rdar://problem/46471091>

Reviewed by Chris Dumez.

Change the nonce in the CTAP kInit command to weak random values as the nonce is mainly
for being a probabilistically unique global identifier for hand shakes, instead of
preventing replay attacks. Otherwise, it might exhaust system entropy unnecessarily.

The patch also removes all logging when debugging the test case flakiness.

* UIProcess/WebAuthentication/AuthenticatorManager.cpp:
(WebKit::AuthenticatorManager::respondReceived):
(WebKit::AuthenticatorManager::initTimeOutTimer):
(WebKit::AuthenticatorManager::timeOutTimerFired):
* UIProcess/WebAuthentication/Cocoa/HidService.mm:
(WebKit::HidService::deviceAdded):
* UIProcess/WebAuthentication/Mock/MockAuthenticatorManager.cpp:
(WebKit::MockAuthenticatorManager::respondReceivedInternal):
* UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:
(WebKit::MockHidConnection::send):
* UIProcess/WebAuthentication/fido/CtapHidAuthenticator.cpp:
(WebKit::CtapHidAuthenticator::makeCredential):
(WebKit::CtapHidAuthenticator::getAssertion):
* UIProcess/WebAuthentication/fido/CtapHidDriver.cpp:
(WebKit::CtapHidDriver::Worker::write):
(WebKit::CtapHidDriver::Worker::read):
(WebKit::CtapHidDriver::Worker::returnMessage):
(WebKit::CtapHidDriver::transact):
(WebKit::CtapHidDriver::continueAfterChannelAllocated):
(WebKit::CtapHidDriver::continueAfterResponseReceived):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@239852 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp
Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidService.mm
Source/WebKit/UIProcess/WebAuthentication/Mock/MockAuthenticatorManager.cpp
Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp
Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidAuthenticator.cpp
Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidDriver.cpp

index 6d41926255c52b6d496fc1ce636f993d67a5dcf2..c6d17bbf70a8c724db5ab05db7b200321e6f676e 100644 (file)
@@ -1,3 +1,38 @@
+2019-01-10  Jiewen Tan  <jiewen_tan@apple.com>
+
+        [WebAuthN] Change the nonce in the CTAP kInit command to weak random values
+        https://bugs.webkit.org/show_bug.cgi?id=192061
+        <rdar://problem/46471091>
+
+        Reviewed by Chris Dumez.
+
+        Change the nonce in the CTAP kInit command to weak random values as the nonce is mainly
+        for being a probabilistically unique global identifier for hand shakes, instead of
+        preventing replay attacks. Otherwise, it might exhaust system entropy unnecessarily.
+
+        The patch also removes all logging when debugging the test case flakiness.
+
+        * UIProcess/WebAuthentication/AuthenticatorManager.cpp:
+        (WebKit::AuthenticatorManager::respondReceived):
+        (WebKit::AuthenticatorManager::initTimeOutTimer):
+        (WebKit::AuthenticatorManager::timeOutTimerFired):
+        * UIProcess/WebAuthentication/Cocoa/HidService.mm:
+        (WebKit::HidService::deviceAdded):
+        * UIProcess/WebAuthentication/Mock/MockAuthenticatorManager.cpp:
+        (WebKit::MockAuthenticatorManager::respondReceivedInternal):
+        * UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:
+        (WebKit::MockHidConnection::send):
+        * UIProcess/WebAuthentication/fido/CtapHidAuthenticator.cpp:
+        (WebKit::CtapHidAuthenticator::makeCredential):
+        (WebKit::CtapHidAuthenticator::getAssertion):
+        * UIProcess/WebAuthentication/fido/CtapHidDriver.cpp:
+        (WebKit::CtapHidDriver::Worker::write):
+        (WebKit::CtapHidDriver::Worker::read):
+        (WebKit::CtapHidDriver::Worker::returnMessage):
+        (WebKit::CtapHidDriver::transact):
+        (WebKit::CtapHidDriver::continueAfterChannelAllocated):
+        (WebKit::CtapHidDriver::continueAfterResponseReceived):
+
 2019-01-10  Timothy Hatcher  <timothy@apple.com>
 
         Add WKBundlePage SPI to temporarily force light or dark appearance on a page.
index 2f924c93174c304a8b14b95faab5f26b49be7512..84e28a8983c2c7ab8eeeca87103d6eee5545e35f 100644 (file)
@@ -192,8 +192,6 @@ void AuthenticatorManager::respondReceived(Respond&& respond)
     if (WTF::holds_alternative<PublicKeyCredentialData>(respond)) {
         m_pendingCompletionHandler(WTFMove(respond));
         clearStateAsync();
-        // FIXME(192061)
-        LOG_ERROR("Stop timer.");
         m_requestTimeOutTimer.stop();
         return;
     }
@@ -226,17 +224,12 @@ void AuthenticatorManager::initTimeOutTimer(const Optional<unsigned>& timeOutInM
     using namespace AuthenticatorManagerInternal;
 
     unsigned timeOutInMsValue = std::min(maxTimeOutValue, timeOutInMs.valueOr(maxTimeOutValue));
-    // FIXME(192061)
-    LOG_ERROR("Start timer: %d. Current time: %f.", timeOutInMsValue, MonotonicTime::now().secondsSinceEpoch().value());
     m_requestTimeOutTimer.startOneShot(Seconds::fromMilliseconds(timeOutInMsValue));
-    LOG_ERROR("Seconds until fire: %f", m_requestTimeOutTimer.secondsUntilFire().value());
 }
 
 void AuthenticatorManager::timeOutTimerFired()
 {
     ASSERT(m_requestTimeOutTimer.isActive());
-    // FIXME(192061)
-    LOG_ERROR("Timer fired: %f, Current time: %f", m_requestTimeOutTimer.secondsUntilFire().value(), MonotonicTime::now().secondsSinceEpoch().value());
     m_pendingCompletionHandler((ExceptionData { NotAllowedError, "Operation timed out."_s }));
     clearStateAsync();
 }
index 3da87060db5762904b33ba16fccc84cfa3e12e9c..ca26e51d7a4064aa28127c0f01d54a4e0ac24463 100644 (file)
@@ -94,8 +94,6 @@ void HidService::deviceAdded(IOHIDDeviceRef device)
 {
     auto driver = std::make_unique<CtapHidDriver>(createHidConnection(device));
     // Get authenticator info from the device.
-    // FIXME(192061)
-    LOG_ERROR("Start asking device info.");
     driver->transact(encodeEmptyAuthenticatorRequest(CtapRequestCommand::kAuthenticatorGetInfo), [weakThis = makeWeakPtr(*this), ptr = driver.get()](Vector<uint8_t>&& response) {
         ASSERT(RunLoop::isMain());
         if (!weakThis)
index fae88614a86055f6344d4cb9866d465ea9500818..4db27609754607be9d77fe5da130792a7ba0746c 100644 (file)
@@ -47,8 +47,6 @@ void MockAuthenticatorManager::respondReceivedInternal(Respond&& respond)
 
     pendingCompletionHandler()(WTFMove(respond));
     clearStateAsync();
-    // FIXME(192061)
-    LOG_ERROR("Stop timer.");
     requestTimeOutTimer().stop();
 }
 
index 0d985e65138b0671b6aea90de4399932178bb247..d86a3a472e6bb77390e3401a6cde82c653a6846e 100644 (file)
@@ -70,23 +70,17 @@ void MockHidConnection::terminate()
 
 void MockHidConnection::send(Vector<uint8_t>&& data, DataSentCallback&& callback)
 {
-    // FIXME(192061): Remove all LOG_ERRORs.
-    LOG_ERROR("Sending data: Phase 1. Current time: %f.", MonotonicTime::now().secondsSinceEpoch().value());
     ASSERT(m_initialized);
     auto task = makeBlockPtr([weakThis = makeWeakPtr(*this), data = WTFMove(data), callback = WTFMove(callback)]() mutable {
         ASSERT(!RunLoop::isMain());
-        LOG_ERROR("Sending data: Phase 2. Current time: %f.", MonotonicTime::now().secondsSinceEpoch().value());
         RunLoop::main().dispatch([weakThis, data = WTFMove(data), callback = WTFMove(callback)]() mutable {
-            LOG_ERROR("Sending data: Phase 3. Current time: %f.", MonotonicTime::now().secondsSinceEpoch().value());
             if (!weakThis) {
                 callback(DataSent::No);
                 return;
             }
 
-            LOG_ERROR("Sending data: Phase 4. Current time: %f.", MonotonicTime::now().secondsSinceEpoch().value());
             weakThis->assembleRequest(WTFMove(data));
 
-            LOG_ERROR("Sending data: Phase 5. Current time: %f.", MonotonicTime::now().secondsSinceEpoch().value());
             auto sent = DataSent::Yes;
             if (weakThis->stagesMatch() && weakThis->m_configuration.hid->error == Mock::Error::DataNotSent)
                 sent = DataSent::No;
index ab42af66f91079a197d5c3d3bc872be2a3577126..2a4bd196ebd4a37679232e09f5cec3297f0f13ce 100644 (file)
@@ -49,8 +49,6 @@ CtapHidAuthenticator::CtapHidAuthenticator(std::unique_ptr<CtapHidDriver>&& driv
 
 void CtapHidAuthenticator::makeCredential()
 {
-    // FIXME(192061)
-    LOG_ERROR("Start making credentials.");
     auto cborCmd = encodeMakeCredenitalRequestAsCBOR(requestData().hash, requestData().creationOptions, m_info.options().userVerificationAvailability());
     m_driver->transact(WTFMove(cborCmd), [weakThis = makeWeakPtr(*this)](Vector<uint8_t>&& data) {
         ASSERT(RunLoop::isMain());
@@ -72,8 +70,6 @@ void CtapHidAuthenticator::continueMakeCredentialAfterResponseReceived(Vector<ui
 
 void CtapHidAuthenticator::getAssertion()
 {
-    // FIXME(192061)
-    LOG_ERROR("Start getting assertions.");
     auto cborCmd = encodeGetAssertionRequestAsCBOR(requestData().hash, requestData().requestOptions, m_info.options().userVerificationAvailability());
     m_driver->transact(WTFMove(cborCmd), [weakThis = makeWeakPtr(*this)](Vector<uint8_t>&& data) {
         ASSERT(RunLoop::isMain());
index 1f9ef98394fee47953f9c40b8a25b9b4b77fde9b..96e5f59b1ef9e1cc0d5ecbb6ae9564a835973180 100644 (file)
@@ -29,7 +29,7 @@
 #if ENABLE(WEB_AUTHN) && PLATFORM(MAC)
 
 #include <WebCore/FidoConstants.h>
-#include <wtf/CryptographicallyRandomNumber.h>
+#include <wtf/RandomNumber.h>
 #include <wtf/RunLoop.h>
 #include <wtf/Vector.h>
 #include <wtf/text/Base64.h>
@@ -69,8 +69,6 @@ void CtapHidDriver::Worker::transact(fido::FidoHidMessage&& requestMessage, Mess
 void CtapHidDriver::Worker::write(HidConnection::DataSent sent)
 {
     ASSERT(m_state == State::Write);
-    // FIXME(192061)
-    LOG_ERROR("Start writing data.");
     if (sent != HidConnection::DataSent::Yes) {
         returnMessage(WTF::nullopt);
         return;
@@ -98,8 +96,6 @@ void CtapHidDriver::Worker::write(HidConnection::DataSent sent)
 void CtapHidDriver::Worker::read(const Vector<uint8_t>& data)
 {
     ASSERT(m_state == State::Read);
-    // FIXME(192061)
-    LOG_ERROR("Start reading data.");
     if (!m_responseMessage) {
         m_responseMessage = FidoHidMessage::createFromSerializedData(data);
         // The first few reports could be for other applications, and therefore ignore those.
@@ -130,8 +126,6 @@ void CtapHidDriver::Worker::read(const Vector<uint8_t>& data)
 
 void CtapHidDriver::Worker::returnMessage(Optional<fido::FidoHidMessage>&& message)
 {
-    // FIXME(192061)
-    LOG_ERROR("Start returning data.");
     m_state = State::Idle;
     m_connection->unregisterDataReceivedCallback();
     m_callback(WTFMove(message));
@@ -152,10 +146,15 @@ void CtapHidDriver::transact(Vector<uint8_t>&& data, ResponseCallback&& callback
     m_responseCallback = WTFMove(callback);
 
     // Allocate a channel.
-    // FIXME(192061)
-    LOG_ERROR("Start allocating a channel.");
-    ASSERT(m_nonce.size() == kHidInitNonceLength);
-    cryptographicallyRandomValues(m_nonce.data(), m_nonce.size());
+    // Use a pseudo random nonce instead of a cryptographically strong one as the nonce
+    // is mainly for identifications.
+    size_t steps = kHidInitNonceLength / sizeof(uint32_t);
+    ASSERT(!(kHidInitNonceLength % sizeof(uint32_t)) && steps >= 1);
+    for (size_t i = 0; i < steps; ++i) {
+        uint32_t weakRandom = weakRandomUint32();
+        memcpy(m_nonce.data() + i * sizeof(uint32_t), &weakRandom, sizeof(uint32_t));
+    }
+
     auto initCommand = FidoHidMessage::create(m_channelId, FidoHidDeviceCommand::kInit, m_nonce);
     ASSERT(initCommand);
     m_worker->transact(WTFMove(*initCommand), [weakThis = makeWeakPtr(*this)](Optional<FidoHidMessage>&& response) mutable {
@@ -195,8 +194,6 @@ void CtapHidDriver::continueAfterChannelAllocated(Optional<FidoHidMessage>&& mes
     m_channelId |= static_cast<uint32_t>(payload[index++]) << 8;
     m_channelId |= static_cast<uint32_t>(payload[index]);
     // FIXME(191534): Check the reset of the payload.
-    // FIXME(192061)
-    LOG_ERROR("Start sending the request.");
     auto cmd = FidoHidMessage::create(m_channelId, m_protocol == ProtocolVersion::kCtap ? FidoHidDeviceCommand::kCbor : FidoHidDeviceCommand::kMsg, m_requestData);
     ASSERT(cmd);
     m_worker->transact(WTFMove(*cmd), [weakThis = makeWeakPtr(*this)](Optional<FidoHidMessage>&& response) mutable {
@@ -211,8 +208,6 @@ void CtapHidDriver::continueAfterResponseReceived(Optional<fido::FidoHidMessage>
 {
     ASSERT(m_state == State::Ready);
     ASSERT(!message || message->channelId() == m_channelId);
-    // FIXME(192061)
-    LOG_ERROR("Start returning the response.");
     returnResponse(message ? message->getMessagePayload() : Vector<uint8_t>());
 }