[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 6d41926..c6d17bb 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 2f924c9..84e28a8 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 3da8706..ca26e51 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 fae8861..4db2760 100644 (file)
@@ -47,8 +47,6 @@ void MockAuthenticatorManager::respondReceivedInternal(Respond&& respond)
 
     pendingCompletionHandler()(WTFMove(respond));
     clearStateAsync();
-    // FIXME(192061)
-    LOG_ERROR("Stop timer.");
     requestTimeOutTimer().stop();
 }
 
index 0d985e6..d86a3a4 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 ab42af6..2a4bd19 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 1f9ef98..96e5f59 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>());
 }