Title: [239852] trunk/Source/WebKit
Revision
239852
Author
jiewen_...@apple.com
Date
2019-01-10 17:42:34 -0800 (Thu, 10 Jan 2019)

Log Message

[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):

Modified Paths

Diff

Modified: trunk/Source/WebKit/ChangeLog (239851 => 239852)


--- trunk/Source/WebKit/ChangeLog	2019-01-11 01:11:57 UTC (rev 239851)
+++ trunk/Source/WebKit/ChangeLog	2019-01-11 01:42:34 UTC (rev 239852)
@@ -1,3 +1,38 @@
+2019-01-10  Jiewen Tan  <jiewen_...@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  <timo...@apple.com>
 
         Add WKBundlePage SPI to temporarily force light or dark appearance on a page.

Modified: trunk/Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp (239851 => 239852)


--- trunk/Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp	2019-01-11 01:11:57 UTC (rev 239851)
+++ trunk/Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp	2019-01-11 01:42:34 UTC (rev 239852)
@@ -192,8 +192,6 @@
     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 @@
     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();
 }

Modified: trunk/Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidService.mm (239851 => 239852)


--- trunk/Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidService.mm	2019-01-11 01:11:57 UTC (rev 239851)
+++ trunk/Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidService.mm	2019-01-11 01:42:34 UTC (rev 239852)
@@ -94,8 +94,6 @@
 {
     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)

Modified: trunk/Source/WebKit/UIProcess/WebAuthentication/Mock/MockAuthenticatorManager.cpp (239851 => 239852)


--- trunk/Source/WebKit/UIProcess/WebAuthentication/Mock/MockAuthenticatorManager.cpp	2019-01-11 01:11:57 UTC (rev 239851)
+++ trunk/Source/WebKit/UIProcess/WebAuthentication/Mock/MockAuthenticatorManager.cpp	2019-01-11 01:42:34 UTC (rev 239852)
@@ -47,8 +47,6 @@
 
     pendingCompletionHandler()(WTFMove(respond));
     clearStateAsync();
-    // FIXME(192061)
-    LOG_ERROR("Stop timer.");
     requestTimeOutTimer().stop();
 }
 

Modified: trunk/Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp (239851 => 239852)


--- trunk/Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp	2019-01-11 01:11:57 UTC (rev 239851)
+++ trunk/Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp	2019-01-11 01:42:34 UTC (rev 239852)
@@ -70,23 +70,17 @@
 
 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 = "" 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 = "" 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;

Modified: trunk/Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidAuthenticator.cpp (239851 => 239852)


--- trunk/Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidAuthenticator.cpp	2019-01-11 01:11:57 UTC (rev 239851)
+++ trunk/Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidAuthenticator.cpp	2019-01-11 01:42:34 UTC (rev 239852)
@@ -49,8 +49,6 @@
 
 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::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());

Modified: trunk/Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidDriver.cpp (239851 => 239852)


--- trunk/Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidDriver.cpp	2019-01-11 01:11:57 UTC (rev 239851)
+++ trunk/Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidDriver.cpp	2019-01-11 01:42:34 UTC (rev 239852)
@@ -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::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::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::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 @@
     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 @@
     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 @@
 {
     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>());
 }
 
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to