Attachment 358844 Details for Bug 192061 – Patch

[patch] Patch

bug-192061-20190110153453.patch (text/plain), 12.32 KB, created by Jiewen Tan on 2019-01-10 15:34:54 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Jiewen Tan

Created: 2019-01-10 15:34:54 PST

Size: 12.32 KB

patch

obsolete

>Subversion Revision: 239777
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index be8bc3ccbed68ec9e1c4ef03c11df50a9a74308c..01e434a80a2a90af3ea6e1503c699b3e18b08cb3 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,38 @@
>+2019-01-09  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 NOBODY (OOPS!).
>+
>+        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-09  Antti Koivisto  <antti@apple.com>
> 
>         [PSON] Flash of blank content while transitioning from page A to page B.
>diff --git a/Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp b/Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp
>index 2f924c93174c304a8b14b95faab5f26b49be7512..84e28a8983c2c7ab8eeeca87103d6eee5545e35f 100644
>--- a/Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp
>+++ b/Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp
>@@ -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();
> }
>diff --git a/Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidService.mm b/Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidService.mm
>index 3da87060db5762904b33ba16fccc84cfa3e12e9c..ca26e51d7a4064aa28127c0f01d54a4e0ac24463 100644
>--- a/Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidService.mm
>+++ b/Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidService.mm
>@@ -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)
>diff --git a/Source/WebKit/UIProcess/WebAuthentication/Mock/MockAuthenticatorManager.cpp b/Source/WebKit/UIProcess/WebAuthentication/Mock/MockAuthenticatorManager.cpp
>index fae88614a86055f6344d4cb9866d465ea9500818..4db27609754607be9d77fe5da130792a7ba0746c 100644
>--- a/Source/WebKit/UIProcess/WebAuthentication/Mock/MockAuthenticatorManager.cpp
>+++ b/Source/WebKit/UIProcess/WebAuthentication/Mock/MockAuthenticatorManager.cpp
>@@ -47,8 +47,6 @@ void MockAuthenticatorManager::respondReceivedInternal(Respond&& respond)
> 
>     pendingCompletionHandler()(WTFMove(respond));
>     clearStateAsync();
>-    // FIXME(192061)
>-    LOG_ERROR("Stop timer.");
>     requestTimeOutTimer().stop();
> }
> 
>diff --git a/Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp b/Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp
>index 0d985e65138b0671b6aea90de4399932178bb247..d86a3a472e6bb77390e3401a6cde82c653a6846e 100644
>--- a/Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp
>+++ b/Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp
>@@ -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;
>diff --git a/Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidAuthenticator.cpp b/Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidAuthenticator.cpp
>index ab42af66f91079a197d5c3d3bc872be2a3577126..2a4bd196ebd4a37679232e09f5cec3297f0f13ce 100644
>--- a/Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidAuthenticator.cpp
>+++ b/Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidAuthenticator.cpp
>@@ -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());
>diff --git a/Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidDriver.cpp b/Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidDriver.cpp
>index 1f9ef98394fee47953f9c40b8a25b9b4b77fde9b..f8ad5eda361417f3c393710353f2f5a4e7c004be 100644
>--- a/Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidDriver.cpp
>+++ b/Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidDriver.cpp
>@@ -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)));
>+    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>());
> }
>

Flags:

cdumez: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 192061: 356665 | 357284 | 357480 | 357622 | 357969 | 358635 | 358758 | 358844 | 358846 | 358851