[WebAuthN] Use a real nonce for CTAPHID_INIT
authorjiewen_tan@apple.com <jiewen_tan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 15 Nov 2018 21:24:51 +0000 (21:24 +0000)
committerjiewen_tan@apple.com <jiewen_tan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 15 Nov 2018 21:24:51 +0000 (21:24 +0000)
https://bugs.webkit.org/show_bug.cgi?id=191533
<rdar://problem/46103502>

Reviewed by Brent Fulgham.

Source/WebCore:

New tests are added into existing test files.

* Modules/webauthn/fido/FidoConstants.h:

Source/WebKit:

Use a real nonce for CTAPHID_INIT request according to:
https://fidoalliance.org/specs/fido-v2.0-ps-20170927/fido-client-to-authenticator-protocol-v2.0-ps-20170927.html#ctaphid_init-0x06.
The challenge here is the new transaction needs to start in the next runloop otherwise a dead lock will form:
wrong nonce -> new transaction -> new nonce -> write init request -> read init response from last run as it
piped in the run loop -> wrong nonce of course -> ...
To break the above dead lock, we have to start the new transaction in the next run. However, that isn't
sufficient as the arrived init response will be piped in HidConnection::m_inputReports, which is designed
on purpose to store any data packets within (initialized, terminated) time interval to prevent data loss in
the case when HidConnection::registerDataReceivedCallback is called after the first data packet's arrival.
In order to break the dead lock completely, HidConnection::invalidateCache will bnnne called prior to every
send to delete any potential init response from last run. HidConnection::invalidateCache is not necessary
for other protocols though. The above scenario is more or less a design flaw in CTAP HID.

Of course, all above scenarios are covered in our mock tests.

* UIProcess/API/C/WKWebsiteDataStoreRef.cpp:
(WKWebsiteDataStoreSetWebAuthenticationMockConfiguration):
* UIProcess/WebAuthentication/Cocoa/HidConnection.h:
(WebKit::HidConnection::invalidateCache):
* UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:
(WebKit::MockHidConnection::send):
(WebKit::MockHidConnection::parseRequest):
(WebKit::MockHidConnection::feedReports):
* UIProcess/WebAuthentication/Mock/MockHidConnection.h:
* UIProcess/WebAuthentication/Mock/MockWebAuthenticationConfiguration.h:
* UIProcess/WebAuthentication/fido/CtapHidDriver.cpp:
(WebKit::CtapHidDriver::Worker::transact):
(WebKit::CtapHidDriver::CtapHidDriver):
(WebKit::CtapHidDriver::transact):
(WebKit::CtapHidDriver::continueAfterChannelAllocated):
(WebKit::CtapHidDriver::returnResponse):
* UIProcess/WebAuthentication/fido/CtapHidDriver.h:

LayoutTests:

* http/wpt/webauthn/ctap-hid-failure.https-expected.txt:
* http/wpt/webauthn/ctap-hid-failure.https.html:
* http/wpt/webauthn/ctap-hid-success.https-expected.txt:
* http/wpt/webauthn/ctap-hid-success.https.html:

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

15 files changed:
LayoutTests/ChangeLog
LayoutTests/http/wpt/webauthn/ctap-hid-failure.https-expected.txt
LayoutTests/http/wpt/webauthn/ctap-hid-failure.https.html
LayoutTests/http/wpt/webauthn/ctap-hid-success.https-expected.txt
LayoutTests/http/wpt/webauthn/ctap-hid-success.https.html
Source/WebCore/ChangeLog
Source/WebCore/Modules/webauthn/fido/FidoConstants.h
Source/WebKit/ChangeLog
Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp
Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidConnection.h
Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp
Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.h
Source/WebKit/UIProcess/WebAuthentication/Mock/MockWebAuthenticationConfiguration.h
Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidDriver.cpp
Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidDriver.h

index dd088b1..618cd8f 100644 (file)
@@ -1,3 +1,16 @@
+2018-11-15  Jiewen Tan  <jiewen_tan@apple.com>
+
+        [WebAuthN] Use a real nonce for CTAPHID_INIT
+        https://bugs.webkit.org/show_bug.cgi?id=191533
+        <rdar://problem/46103502>
+
+        Reviewed by Brent Fulgham.
+
+        * http/wpt/webauthn/ctap-hid-failure.https-expected.txt:
+        * http/wpt/webauthn/ctap-hid-failure.https.html:
+        * http/wpt/webauthn/ctap-hid-success.https-expected.txt:
+        * http/wpt/webauthn/ctap-hid-success.https.html:
+
 2018-11-15  Justin Fan  <justin_fan@apple.com>
 
         [WebGPU] WebGPUCommandBuffer prototype
index f7adb82..38ce8c6 100644 (file)
@@ -2,6 +2,7 @@
 PASS CTAP HID with init sub stage data not sent error in a mock hid authenticator. 
 PASS CTAP HID with init sub stage empty report error in a mock hid authenticator. 
 PASS CTAP HID with init sub stage wrong channel id error in a mock hid authenticator. 
+PASS CTAP HID with init sub stage wrong nonce error in a mock hid authenticator. 
 PASS CTAP HID with msg sub stage data not sent error in a mock hid authenticator. 
 PASS CTAP HID with msg sub stage empty report error in a mock hid authenticator. 
 PASS CTAP HID with msg sub stage wrong channel id error in a mock hid authenticator. 
index e535d8b..e24405f 100644 (file)
 
     promise_test(function(t) {
         if (window.testRunner)
+            testRunner.setWebAuthenticationMockConfiguration({ hid: { stage: "info", subStage: "init", error: "wrong-nonce" } });
+        return promiseRejects(t, "NotAllowedError", navigator.credentials.create(defaultOptions), "Operation timed out.");
+    }, "CTAP HID with init sub stage wrong nonce error in a mock hid authenticator.");
+
+    promise_test(function(t) {
+        if (window.testRunner)
             testRunner.setWebAuthenticationMockConfiguration({ hid: { stage: "info", subStage: "msg", error: "data-not-sent" } });
         return promiseRejects(t, "NotAllowedError", navigator.credentials.create(defaultOptions), "Operation timed out.");
     }, "CTAP HID with msg sub stage data not sent error in a mock hid authenticator.");
index e2a4533..dadbcac 100644 (file)
@@ -3,4 +3,5 @@ PASS CTAP HID with keep alive message in a mock hid authenticator.
 PASS CTAP HID with fast data arrival in a mock hid authenticator. 
 PASS CTAP HID with continue after empty report in a mock hid authenticator. 
 PASS CTAP HID with continue after wrong channel id in a mock hid authenticator. 
+PASS CTAP HID with continue after wrong nonce error in a mock hid authenticator. 
 
index 179c069..db02d54 100644 (file)
             assert_not_equals(credential, null);
         });
     }, "CTAP HID with continue after wrong channel id in a mock hid authenticator.");
+
+    promise_test(function(t) {
+        if (window.testRunner)
+            testRunner.setWebAuthenticationMockConfiguration({ hid: { stage: "info", subStage: "init", error: "wrong-nonce", payloadBase64: testCreationMessageBase64, continueAfterErrorData: true } });
+        return navigator.credentials.create(defaultOptions).then(credential => {
+            assert_not_equals(credential, undefined);
+            assert_not_equals(credential, null);
+        });
+    }, "CTAP HID with continue after wrong nonce error in a mock hid authenticator.");
 </script>
index 79dcdc2..242beca 100644 (file)
@@ -1,3 +1,15 @@
+2018-11-15  Jiewen Tan  <jiewen_tan@apple.com>
+
+        [WebAuthN] Use a real nonce for CTAPHID_INIT
+        https://bugs.webkit.org/show_bug.cgi?id=191533
+        <rdar://problem/46103502>
+
+        Reviewed by Brent Fulgham.
+
+        New tests are added into existing test files.
+
+        * Modules/webauthn/fido/FidoConstants.h:
+
 2018-11-15  Justin Fan  <justin_fan@apple.com>
 
         [WebGPU] WebGPUCommandBuffer prototype
index f8d3ea3..69b9199 100644 (file)
@@ -160,6 +160,7 @@ const size_t kHidMaxPacketSize = 64;
 const size_t kHidInitPacketDataSize = kHidMaxPacketSize - kHidInitPacketHeaderSize;
 const size_t kHidContinuationPacketDataSize = kHidMaxPacketSize - kHidContinuationPacketHeader;
 const size_t kHidInitResponseSize = 17;
+const size_t kHidInitNonceLength = 8;
 
 const uint8_t kHidMaxLockSeconds = 10;
 
index 21965ab..9ed7d7c 100644 (file)
@@ -1,3 +1,44 @@
+2018-11-15  Jiewen Tan  <jiewen_tan@apple.com>
+
+        [WebAuthN] Use a real nonce for CTAPHID_INIT
+        https://bugs.webkit.org/show_bug.cgi?id=191533
+        <rdar://problem/46103502>
+
+        Reviewed by Brent Fulgham.
+
+        Use a real nonce for CTAPHID_INIT request according to:
+        https://fidoalliance.org/specs/fido-v2.0-ps-20170927/fido-client-to-authenticator-protocol-v2.0-ps-20170927.html#ctaphid_init-0x06.
+        The challenge here is the new transaction needs to start in the next runloop otherwise a dead lock will form:
+        wrong nonce -> new transaction -> new nonce -> write init request -> read init response from last run as it
+        piped in the run loop -> wrong nonce of course -> ...
+        To break the above dead lock, we have to start the new transaction in the next run. However, that isn't
+        sufficient as the arrived init response will be piped in HidConnection::m_inputReports, which is designed
+        on purpose to store any data packets within (initialized, terminated) time interval to prevent data loss in
+        the case when HidConnection::registerDataReceivedCallback is called after the first data packet's arrival.
+        In order to break the dead lock completely, HidConnection::invalidateCache will bnnne called prior to every
+        send to delete any potential init response from last run. HidConnection::invalidateCache is not necessary
+        for other protocols though. The above scenario is more or less a design flaw in CTAP HID.
+
+        Of course, all above scenarios are covered in our mock tests.
+
+        * UIProcess/API/C/WKWebsiteDataStoreRef.cpp:
+        (WKWebsiteDataStoreSetWebAuthenticationMockConfiguration):
+        * UIProcess/WebAuthentication/Cocoa/HidConnection.h:
+        (WebKit::HidConnection::invalidateCache):
+        * UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:
+        (WebKit::MockHidConnection::send):
+        (WebKit::MockHidConnection::parseRequest):
+        (WebKit::MockHidConnection::feedReports):
+        * UIProcess/WebAuthentication/Mock/MockHidConnection.h:
+        * UIProcess/WebAuthentication/Mock/MockWebAuthenticationConfiguration.h:
+        * UIProcess/WebAuthentication/fido/CtapHidDriver.cpp:
+        (WebKit::CtapHidDriver::Worker::transact):
+        (WebKit::CtapHidDriver::CtapHidDriver):
+        (WebKit::CtapHidDriver::transact):
+        (WebKit::CtapHidDriver::continueAfterChannelAllocated):
+        (WebKit::CtapHidDriver::returnResponse):
+        * UIProcess/WebAuthentication/fido/CtapHidDriver.h:
+
 2018-11-15  Oriol Brufau  <obrufau@igalia.com>
 
         [css-logical] Implement flow-relative margin, padding and border shorthands
index 4012808..1dd81ed 100644 (file)
@@ -625,6 +625,8 @@ void WKWebsiteDataStoreSetWebAuthenticationMockConfiguration(WKWebsiteDataStoreR
             hid.error = MockWebAuthenticationConfiguration::Hid::Error::MaliciousPayload;
         if (error == "unsupported-options")
             hid.error = MockWebAuthenticationConfiguration::Hid::Error::UnsupportedOptions;
+        if (error == "wrong-nonce")
+            hid.error = MockWebAuthenticationConfiguration::Hid::Error::WrongNonce;
 
         if (auto payloadBase64 = static_cast<WKStringRef>(WKDictionaryGetItemForKey(hidRef, adoptWK(WKStringCreateWithUTF8CString("PayloadBase64")).get())))
             hid.payloadBase64 = WebKit::toImpl(payloadBase64)->string();
index 21bec7e..7c458ea 100644 (file)
@@ -59,6 +59,7 @@ public:
     virtual void send(Vector<uint8_t>&& data, DataSentCallback&&);
     void registerDataReceivedCallback(DataReceivedCallback&&);
     void unregisterDataReceivedCallback();
+    void invalidateCache() { m_inputReports.clear(); }
 
     void receiveReport(Vector<uint8_t>&&);
 
index e648bb8..eb7f0ea 100644 (file)
@@ -70,15 +70,17 @@ void MockHidConnection::terminate()
 void MockHidConnection::send(Vector<uint8_t>&& data, DataSentCallback&& callback)
 {
     ASSERT(m_initialized);
-    assembleRequest(WTFMove(data));
+    auto task = BlockPtr<void()>::fromCallable([weakThis = makeWeakPtr(*this), data = WTFMove(data), callback = WTFMove(callback)]() mutable {
+        ASSERT(!RunLoop::isMain());
+        RunLoop::main().dispatch([weakThis, data = WTFMove(data), callback = WTFMove(callback)]() mutable {
+            if (!weakThis)
+                return;
 
-    auto sent = DataSent::Yes;
-    if (stagesMatch() && m_configuration.hid->error == Mock::Error::DataNotSent)
-        sent = DataSent::No;
+            weakThis->assembleRequest(WTFMove(data));
 
-    auto task = BlockPtr<void()>::fromCallable([callback = WTFMove(callback), sent]() mutable {
-        ASSERT(!RunLoop::isMain());
-        RunLoop::main().dispatch([callback = WTFMove(callback), sent]() mutable {
+            auto sent = DataSent::Yes;
+            if (weakThis->stagesMatch() && weakThis->m_configuration.hid->error == Mock::Error::DataNotSent)
+                sent = DataSent::No;
             callback(sent);
         });
     });
@@ -163,6 +165,12 @@ void MockHidConnection::parseRequest()
         }
     }
 
+    // Store nonce.
+    if (m_subStage == Mock::SubStage::Init) {
+        m_nonce = m_requestMessage->getMessagePayload();
+        ASSERT(m_nonce.size() == kHidInitNonceLength);
+    }
+
     m_currentChannel = m_requestMessage->channelId();
     m_requestMessage = std::nullopt;
     if (m_configuration.hid->fastDataArrival)
@@ -176,8 +184,9 @@ void MockHidConnection::feedReports()
     if (m_subStage == Mock::SubStage::Init) {
         Vector<uint8_t> payload;
         payload.reserveInitialCapacity(kHidInitResponseSize);
-        // FIXME(191533): Use a real nonce.
-        payload.appendVector(Vector<uint8_t>({ 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07 }));
+        payload.appendVector(m_nonce);
+        if (stagesMatch() && m_configuration.hid->error == Mock::Error::WrongNonce)
+            payload[0]--;
         payload.grow(kHidInitResponseSize);
         cryptographicallyRandomValues(payload.data() + payload.size(), CtapChannelIdSize);
         auto channel = kHidBroadcastChannel;
@@ -191,7 +200,7 @@ void MockHidConnection::feedReports()
 
     std::optional<FidoHidMessage> message;
     if (m_stage == Mock::Stage::Info && m_subStage == Mock::SubStage::Msg) {
-        auto infoData = encodeAsCBOR(AuthenticatorGetInfoResponse({ ProtocolVersion::kCtap }, Vector<uint8_t>(kAaguidLength)));
+        auto infoData = encodeAsCBOR(AuthenticatorGetInfoResponse({ ProtocolVersion::kCtap }, Vector<uint8_t>(kAaguidLength, 0u)));
         infoData.insert(0, static_cast<uint8_t>(CtapDeviceResponseCode::kSuccess)); // Prepend status code.
         if (stagesMatch() && m_configuration.hid->error == Mock::Error::WrongChannelId)
             message = FidoHidMessage::create(m_currentChannel - 1, FidoHidDeviceCommand::kCbor, infoData);
index 8b5969e..316477e 100644 (file)
@@ -65,6 +65,7 @@ private:
     uint32_t m_currentChannel { fido::kHidBroadcastChannel };
     bool m_requireResidentKey { false };
     bool m_requireUserVerification  { false };
+    Vector<uint8_t> m_nonce;
 };
 
 } // namespace WebKit
index c4216c6..305efd8 100644 (file)
@@ -57,7 +57,8 @@ struct MockWebAuthenticationConfiguration {
             EmptyReport,
             WrongChannelId,
             MaliciousPayload,
-            UnsupportedOptions
+            UnsupportedOptions,
+            WrongNonce
         };
 
         String payloadBase64;
index 39d7c1a..77e2271 100644 (file)
@@ -29,7 +29,9 @@
 #if ENABLE(WEB_AUTHN) && PLATFORM(MAC)
 
 #include <WebCore/FidoConstants.h>
+#include <wtf/CryptographicallyRandomNumber.h>
 #include <wtf/RunLoop.h>
+#include <wtf/Vector.h>
 #include <wtf/text/Base64.h>
 
 namespace WebKit {
@@ -54,6 +56,8 @@ void CtapHidDriver::Worker::transact(fido::FidoHidMessage&& requestMessage, Mess
     m_responseMessage.reset();
     m_callback = WTFMove(callback);
 
+    // HidConnection could hold data from other applications, and thereofore invalidate it before each transaction.
+    m_connection->invalidateCache();
     m_connection->send(m_requestMessage->popNextPacket(), [weakThis = makeWeakPtr(*this)](HidConnection::DataSent sent) mutable {
         ASSERT(RunLoop::isMain());
         if (!weakThis)
@@ -129,6 +133,7 @@ void CtapHidDriver::Worker::returnMessage(std::optional<fido::FidoHidMessage>&&
 
 CtapHidDriver::CtapHidDriver(UniqueRef<HidConnection>&& connection)
     : m_worker(makeUniqueRef<Worker>(WTFMove(connection)))
+    , m_nonce(kHidInitNonceLength)
 {
 }
 
@@ -136,12 +141,14 @@ void CtapHidDriver::transact(Vector<uint8_t>&& data, ResponseCallback&& callback
 {
     ASSERT(m_state == State::Idle);
     m_state = State::AllocateChannel;
+    m_channelId = kHidBroadcastChannel;
     m_requestData = WTFMove(data);
     m_responseCallback = WTFMove(callback);
 
     // Allocate a channel.
-    // FIXME(191533): Get a real nonce.
-    auto initCommand = FidoHidMessage::create(kHidBroadcastChannel, FidoHidDeviceCommand::kInit, { 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07 });
+    ASSERT(m_nonce.size() == kHidInitNonceLength);
+    cryptographicallyRandomValues(m_nonce.data(), m_nonce.size());
+    auto initCommand = FidoHidMessage::create(m_channelId, FidoHidDeviceCommand::kInit, m_nonce);
     ASSERT(initCommand);
     m_worker->transact(WTFMove(*initCommand), [weakThis = makeWeakPtr(*this)](std::optional<FidoHidMessage>&& response) mutable {
         ASSERT(RunLoop::isMain());
@@ -158,17 +165,28 @@ void CtapHidDriver::continueAfterChannelAllocated(std::optional<FidoHidMessage>&
         returnResponse({ });
         return;
     }
-    m_state = State::Ready;
     ASSERT(message->channelId() == m_channelId);
 
-    // FIXME(191534): Check Payload including nonce and everything
     auto payload = message->getMessagePayload();
-    size_t index = 8;
     ASSERT(payload.size() == kHidInitResponseSize);
+    // Restart the transaction in the next run loop when nonce mismatches.
+    if (memcmp(payload.data(), m_nonce.data(), m_nonce.size())) {
+        m_state = State::Idle;
+        RunLoop::main().dispatch([weakThis = makeWeakPtr(*this), data = WTFMove(m_requestData), callback = WTFMove(m_responseCallback)]() mutable {
+            if (!weakThis)
+                return;
+            weakThis->transact(WTFMove(data), WTFMove(callback));
+        });
+        return;
+    }
+
+    m_state = State::Ready;
+    auto index = kHidInitNonceLength;
     m_channelId = static_cast<uint32_t>(payload[index++]) << 24;
     m_channelId |= static_cast<uint32_t>(payload[index++]) << 16;
     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.
     auto cmd = FidoHidMessage::create(m_channelId, FidoHidDeviceCommand::kCbor, m_requestData);
     ASSERT(cmd);
     m_worker->transact(WTFMove(*cmd), [weakThis = makeWeakPtr(*this)](std::optional<FidoHidMessage>&& response) mutable {
@@ -190,8 +208,6 @@ void CtapHidDriver::returnResponse(Vector<uint8_t>&& response)
 {
     // Reset state before calling the response callback to avoid being deleted.
     m_state = State::Idle;
-    m_channelId = fido::kHidBroadcastChannel;
-    m_requestData = { };
     m_responseCallback(WTFMove(response));
 }
 
index 68b1c46..4923a3c 100644 (file)
@@ -62,7 +62,7 @@ public:
 private:
     // Worker is the helper that maintains the transaction.
     // https://fidoalliance.org/specs/fido-v2.0-ps-20170927/fido-client-to-authenticator-protocol-v2.0-ps-20170927.html#arbitration
-    // FSM: Idle => Write => Read
+    // FSM: Idle => Write => Read.
     class Worker : public CanMakeWeakPtr<Worker> {
         WTF_MAKE_FAST_ALLOCATED;
         WTF_MAKE_NONCOPYABLE(Worker);
@@ -102,6 +102,7 @@ private:
     // One request at a time.
     Vector<uint8_t> m_requestData;
     ResponseCallback m_responseCallback;
+    Vector<uint8_t> m_nonce;
 };
 
 } // namespace WebKit