- Revision
- 240081
- Author
- alanc...@apple.com
- Date
- 2019-01-16 15:28:34 -0800 (Wed, 16 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: branches/safari-607-branch/Source/WebKit/ChangeLog (240080 => 240081)
--- branches/safari-607-branch/Source/WebKit/ChangeLog 2019-01-16 23:28:31 UTC (rev 240080)
+++ branches/safari-607-branch/Source/WebKit/ChangeLog 2019-01-16 23:28:34 UTC (rev 240081)
@@ -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-15 Alan Coon <alanc...@apple.com>
Cherry-pick r239904. rdar://problem/4726030
Modified: branches/safari-607-branch/Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp (240080 => 240081)
--- branches/safari-607-branch/Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp 2019-01-16 23:28:31 UTC (rev 240080)
+++ branches/safari-607-branch/Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp 2019-01-16 23:28:34 UTC (rev 240081)
@@ -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: branches/safari-607-branch/Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidService.mm (240080 => 240081)
--- branches/safari-607-branch/Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidService.mm 2019-01-16 23:28:31 UTC (rev 240080)
+++ branches/safari-607-branch/Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidService.mm 2019-01-16 23:28:34 UTC (rev 240081)
@@ -93,8 +93,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: branches/safari-607-branch/Source/WebKit/UIProcess/WebAuthentication/Mock/MockAuthenticatorManager.cpp (240080 => 240081)
--- branches/safari-607-branch/Source/WebKit/UIProcess/WebAuthentication/Mock/MockAuthenticatorManager.cpp 2019-01-16 23:28:31 UTC (rev 240080)
+++ branches/safari-607-branch/Source/WebKit/UIProcess/WebAuthentication/Mock/MockAuthenticatorManager.cpp 2019-01-16 23:28:34 UTC (rev 240081)
@@ -47,8 +47,6 @@
pendingCompletionHandler()(WTFMove(respond));
clearStateAsync();
- // FIXME(192061)
- LOG_ERROR("Stop timer.");
requestTimeOutTimer().stop();
}
Modified: branches/safari-607-branch/Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidAuthenticator.cpp (240080 => 240081)
--- branches/safari-607-branch/Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidAuthenticator.cpp 2019-01-16 23:28:31 UTC (rev 240080)
+++ branches/safari-607-branch/Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidAuthenticator.cpp 2019-01-16 23:28:34 UTC (rev 240081)
@@ -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: branches/safari-607-branch/Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidDriver.cpp (240080 => 240081)
--- branches/safari-607-branch/Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidDriver.cpp 2019-01-16 23:28:31 UTC (rev 240080)
+++ branches/safari-607-branch/Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidDriver.cpp 2019-01-16 23:28:34 UTC (rev 240081)
@@ -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,9 +194,7 @@
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, FidoHidDeviceCommand::kCbor, m_requestData);
+ 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 {
ASSERT(RunLoop::isMain());
@@ -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>());
}
