[WebAuthn] Reland handle security keys with a full key store

rdar://120402606 https://bugs.webkit.org/show_bug.cgi?id=267101 Reviewed by Brent Fulgham. Whenever security keys are unable to create a credential due to their internal key storage being full they return the kCtap2ErrKeyStoreFull error code. In this case we should either retry the registeration without a discoverable credential if the preference was set to preferred, otherwise surface an error to the user notifying them they need to use a different key or clear space on the current key. This patch fixes this by handling the kCtap2ErrKeyStoreFull error case. For security keys supporting the CTAP 2.1 standard, this patch also adds support for reading the "remainingDiscoverableCredentials" value from getInfo to detect if the key store is full without having to first attempt a create. Added layout tests for new functionality. * LayoutTests/http/wpt/webauthn/public-key-credential-create-failure-hid.https-expected.txt: * LayoutTests/http/wpt/webauthn/public-key-credential-create-failure-hid.https.html: * LayoutTests/http/wpt/webauthn/public-key-credential-create-success-hid.https-expected.txt: * LayoutTests/http/wpt/webauthn/public-key-credential-create-success-hid.https.html: * LayoutTests/http/wpt/webauthn/public-key-credential-create-success-nfc.https-expected.txt: * LayoutTests/http/wpt/webauthn/public-key-credential-create-success-nfc.https.html: * LayoutTests/http/wpt/webauthn/resources/util.js: * Source/WebCore/Modules/webauthn/fido/AuthenticatorGetInfoResponse.cpp: (fido::AuthenticatorGetInfoResponse::setRemainingDiscoverableCredentials): (fido::encodeAsCBOR): * Source/WebCore/Modules/webauthn/fido/AuthenticatorGetInfoResponse.h: * Source/WebCore/Modules/webauthn/fido/DeviceResponseConverter.cpp: (fido::readCTAPGetInfoResponse): * Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h: * Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm: (WebKit::wkWebAuthenticationPanelUpdate): * Source/WebKit/UIProcess/WebAuthentication/WebAuthenticationFlags.h: * Source/WebKit/UIProcess/WebAuthentication/fido/CtapAuthenticator.cpp: (WebKit::CtapAuthenticator::makeCredential): (WebKit::CtapAuthenticator::continueMakeCredentialAfterResponseReceived): * Source/WebKit/UIProcess/WebAuthentication/fido/CtapAuthenticator.h: Canonical link: https://commits.webkit.org/272790@main
WebKit · Jan 8, 2024 · 3ec7aa1 · 3ec7aa1
1 parent e54367d
commit 3ec7aa1
Show file tree

Hide file tree

Showing 15 changed files with 126 additions and 1 deletion.
diff --git a/LayoutTests/http/wpt/webauthn/public-key-credential-create-failure-hid.https-expected.txt b/LayoutTests/http/wpt/webauthn/public-key-credential-create-failure-hid.https-expected.txt
@@ -6,4 +6,5 @@ PASS PublicKeyCredential's [[create]] with unsupported options in a mock hid aut
 PASS PublicKeyCredential's [[create]] with mixed options in a mock hid authenticator.
 PASS PublicKeyCredential's [[create]] with mixed options in a mock hid authenticator. 2
 PASS PublicKeyCredential's [[create]] with InvalidStateError in a mock hid authenticator.
+PASS PublicKeyCredential's [[create]] with full key store.
 
diff --git a/LayoutTests/http/wpt/webauthn/public-key-credential-create-failure-hid.https.html b/LayoutTests/http/wpt/webauthn/public-key-credential-create-failure-hid.https.html
@@ -159,4 +159,27 @@
             internals.setMockWebAuthenticationConfiguration({ hid: { stage: "request", subStage: "msg", error: "malicious-payload", payloadBase64: [testCtapErrCredentialExcludedOnlyResponseBase64] } });
         return promiseRejects(t, "InvalidStateError", navigator.credentials.create(options), "At least one credential matches an entry of the excludeCredentials list in the authenticator.");
     }, "PublicKeyCredential's [[create]] with InvalidStateError in a mock hid authenticator.");
+
+        promise_test(function(t) {
+        const options = {
+            publicKey: {
+                rp: {
+                    name: "example.com"
+                },
+                user: {
+                    name: "John Appleseed",
+                    id: asciiToUint8Array("123456"),
+                    displayName: "John",
+                },
+                challenge: asciiToUint8Array("123456"),
+                pubKeyCredParams: [{ type: "public-key", alg: -7 }],
+                authenticatorSelection: { residentKey: "required" },
+                timeout: 10 // We wait for another authenticator upon full.
+            }
+        };
+
+        if (window.internals)
+            internals.setMockWebAuthenticationConfiguration({ hid: { stage: "request", subStage: "msg", error: "malicious-payload", payloadBase64: [testCreateMessageFullKeyStoreBase64] } });
+        return promiseRejects(t, "NotAllowedError", navigator.credentials.create(options), "Operation timed out.");
+    }, "PublicKeyCredential's [[create]] with full key store.");
 </script>
diff --git a/LayoutTests/http/wpt/webauthn/public-key-credential-create-success-hid.https-expected.txt b/LayoutTests/http/wpt/webauthn/public-key-credential-create-success-hid.https-expected.txt
@@ -17,4 +17,5 @@ PASS PublicKeyCredential's [[create]] with direct attestation in a mock hid auth
 PASS PublicKeyCredential's [[create]] with indirect attestation in a mock hid authenticator.
 PASS PublicKeyCredential's [[create]] with googleLegacyAppidSupport extension in a mock hid authenticator.
 PASS PublicKeyCredential's [[create]] with googleLegacyAppidSupport and appid extensions in a mock hid authenticator.
+PASS PublicKeyCredential's [[create]] with authenticatorSelection { 'cross-platform', 'preferred' } in a mock hid authenticator with a full key store.
 
diff --git a/LayoutTests/http/wpt/webauthn/public-key-credential-create-success-hid.https.html b/LayoutTests/http/wpt/webauthn/public-key-credential-create-success-hid.https.html
@@ -424,4 +424,29 @@
             checkCtapMakeCredentialResult(credential);
         });
     }, "PublicKeyCredential's [[create]] with googleLegacyAppidSupport and appid extensions in a mock hid authenticator.");
+
+    promise_test(t => {
+        const options = {
+            publicKey: {
+                rp: {
+                    name: "localhost",
+                },
+                user: {
+                    name: "John Appleseed",
+                    id: Base64URL.parse(testUserhandleBase64),
+                    displayName: "Appleseed",
+                },
+                challenge: Base64URL.parse("MTIzNDU2"),
+                pubKeyCredParams: [{ type: "public-key", alg: -7 }],
+                authenticatorSelection: { authenticatorAttachment: "cross-platform", residentKey: "preferred" },
+                extensions: { credProps: true }
+            }
+        };
+
+        if (window.internals)
+            internals.setMockWebAuthenticationConfiguration({ hid: { stage: "request", subStage: "msg", error: "success", payloadBase64: [testCreateMessageFullKeyStoreBase64, testCreationMessageBase64] } });
+        return navigator.credentials.create(options).then(credential => {
+            checkCtapMakeCredentialResult(credential);
+        });
+    }, "PublicKeyCredential's [[create]] with authenticatorSelection { 'cross-platform', 'preferred' } in a mock hid authenticator with a full key store.");
 </script>
diff --git a/LayoutTests/http/wpt/webauthn/public-key-credential-create-success-nfc.https-expected.txt b/LayoutTests/http/wpt/webauthn/public-key-credential-create-success-nfc.https-expected.txt
@@ -6,4 +6,5 @@ PASS PublicKeyCredential's [[create]] with U2F in a mock nfc authenticator.
 PASS PublicKeyCredential's [[create]] with multiple physical tags in a mock nfc authenticator.
 PASS PublicKeyCredential's [[create]] with service restart in a mock nfc authenticator.
 PASS PublicKeyCredential's [[create]] with legacy U2F keys in a mock nfc authenticator.
+PASS PublicKeyCredential's [[create]] with authenticatorSelection { 'cross-platform', 'preferred' } in a mock nfc authenticator with a full key store based on getInfo
 
diff --git a/LayoutTests/http/wpt/webauthn/public-key-credential-create-success-nfc.https.html b/LayoutTests/http/wpt/webauthn/public-key-credential-create-success-nfc.https.html
@@ -166,4 +166,31 @@
             checkCtapMakeCredentialResult(credential);
         });
     }, "PublicKeyCredential's [[create]] with legacy U2F keys in a mock nfc authenticator.");
+
+
+
+    promise_test(t => {
+        const options = {
+            publicKey: {
+                rp: {
+                    name: "localhost",
+                },
+                user: {
+                    name: "John Appleseed",
+                    id: Base64URL.parse(testUserhandleBase64),
+                    displayName: "Appleseed",
+                },
+                challenge: Base64URL.parse("MTIzNDU2"),
+                pubKeyCredParams: [{ type: "public-key", alg: -7 }],
+                authenticatorSelection: { authenticatorAttachment: "cross-platform", residentKey: "preferred" }
+            }
+        };
+
+        if (window.internals)
+        internals.setMockWebAuthenticationConfiguration({ nfc: { error: "success", payloadBase64: [testNfcCtapVersionBase64, testGetInfoResponseApduNoRemainingDiscoverableBase64, testCreationMessageApduBase64] } });
+
+        return navigator.credentials.create(options).then(credential => {
+            checkCtapMakeCredentialResult(credential);
+        });
+    }, "PublicKeyCredential's [[create]] with authenticatorSelection { 'cross-platform', 'preferred' } in a mock nfc authenticator with a full key store based on getInfo");
 </script>
diff --git a/LayoutTests/http/wpt/webauthn/resources/util.js b/LayoutTests/http/wpt/webauthn/resources/util.js
@@ -112,6 +112,8 @@ const testNfcCtapVersionBase64 = "RklET18yXzCQAA==";
 const testGetInfoResponseApduBase64 =
     "AKYBgmZVMkZfVjJoRklET18yXzACgWtobWFjLXNlY3JldANQbUS6m/bsLkm5MAyP" +
     "6SDLcwSkYnJr9WJ1cPVkcGxhdPRpY2xpZW50UGlu9AUZBLAGgQGQAA==";
+const testGetInfoResponseApduNoRemainingDiscoverableBase64 =
+    "AKcBgmZVMkZfVjJoRklET18yXzACgWtobWFjLXNlY3JldANQbUS6m/bsLkm5MAyP6SDLcwSkYnJr9WJ1cPVkcGxhdPRpY2xpZW50UGlu9AUZBLAGgQEUAJAA";
 const testCreationMessageApduBase64 =
     "AKMBZnBhY2tlZAJYxEbMf7lnnVWy25CS4cjZ5eHQK3WA8LSBLHcJYuHkj1rYQQAA" +
     "AE74oBHzjApNFYAGFxEfntx9AEAoCK3O6P5OyXN6V/f+9nAga0NA2Cgp4V3mgSJ5" +
@@ -143,6 +145,7 @@ const testAssertionMessageApduBase64 =
     "QoJ1L7Fe64G9uBeQAA==";
 const testCcidNoUidBase64 = "aIE=";
 const testCcidValidUidBase64 = "CH+d1ZAA";
+const testCreateMessageFullKeyStoreBase64 = "KA==";
 
 const RESOURCES_DIR = "/WebKit/webauthn/resources/";
 

diff --git a/Source/WebCore/Modules/webauthn/fido/AuthenticatorGetInfoResponse.cpp b/Source/WebCore/Modules/webauthn/fido/AuthenticatorGetInfoResponse.cpp
@@ -83,6 +83,12 @@ AuthenticatorGetInfoResponse& AuthenticatorGetInfoResponse::setTransports(Vector
     return *this;
 }
 
+AuthenticatorGetInfoResponse& AuthenticatorGetInfoResponse::setRemainingDiscoverableCredentials(uint32_t remainingDiscoverableCredentials)
+{
+    m_remainingDiscoverableCredentials = remainingDiscoverableCredentials;
+    return *this;
+}
+
 static String toString(WebCore::AuthenticatorTransport transport)
 {
     switch (transport) {
@@ -139,6 +145,9 @@ Vector<uint8_t> encodeAsCBOR(const AuthenticatorGetInfoResponse& response)
         deviceInfoMap.emplace(CBORValue(7), toArrayValue(transports.map(toString)));
     }
 
+    if (response.remainingDiscoverableCredentials())
+        deviceInfoMap.emplace(CBORValue(8), CBORValue(static_cast<int64_t>(*response.maxMsgSize())));
+
     auto encodedBytes = CBORWriter::write(CBORValue(WTFMove(deviceInfoMap)));
     ASSERT(encodedBytes);
     return *encodedBytes;

diff --git a/Source/WebCore/Modules/webauthn/fido/AuthenticatorGetInfoResponse.h b/Source/WebCore/Modules/webauthn/fido/AuthenticatorGetInfoResponse.h
@@ -54,6 +54,7 @@ class WEBCORE_EXPORT AuthenticatorGetInfoResponse {
     AuthenticatorGetInfoResponse& setExtensions(Vector<String>&&);
     AuthenticatorGetInfoResponse& setOptions(AuthenticatorSupportedOptions&&);
     AuthenticatorGetInfoResponse& setTransports(Vector<WebCore::AuthenticatorTransport>&&);
+    AuthenticatorGetInfoResponse& setRemainingDiscoverableCredentials(uint32_t);
 
     const StdSet<ProtocolVersion>& versions() const { return m_versions; }
     const Vector<uint8_t>& aaguid() const { return m_aaguid; }
@@ -62,6 +63,7 @@ class WEBCORE_EXPORT AuthenticatorGetInfoResponse {
     const std::optional<Vector<String>>& extensions() const { return m_extensions; }
     const AuthenticatorSupportedOptions& options() const { return m_options; }
     const std::optional<Vector<WebCore::AuthenticatorTransport>>& transports() const { return m_transports; }
+    const std::optional<uint32_t>& remainingDiscoverableCredentials() const { return m_remainingDiscoverableCredentials; }
 
 private:
     StdSet<ProtocolVersion> m_versions;
@@ -71,6 +73,7 @@ class WEBCORE_EXPORT AuthenticatorGetInfoResponse {
     std::optional<Vector<String>> m_extensions;
     AuthenticatorSupportedOptions m_options;
     std::optional<Vector<WebCore::AuthenticatorTransport>> m_transports;
+    std::optional<uint32_t> m_remainingDiscoverableCredentials;
 };
 
 WEBCORE_EXPORT Vector<uint8_t> encodeAsCBOR(const AuthenticatorGetInfoResponse&);

diff --git a/Source/WebCore/Modules/webauthn/fido/DeviceResponseConverter.cpp b/Source/WebCore/Modules/webauthn/fido/DeviceResponseConverter.cpp
@@ -359,6 +359,14 @@ std::optional<AuthenticatorGetInfoResponse> readCTAPGetInfoResponse(const Vector
         response.setTransports(WTFMove(transports));
     }
 
+    it = responseMap.find(CBOR(20));
+    if (it != responseMap.end()) {
+        if (!it->second.isUnsigned())
+            return std::nullopt;
+
+        response.setRemainingDiscoverableCredentials(it->second.getUnsigned());
+    }
+
     return WTFMove(response);
 }
 

diff --git a/Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h b/Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h
@@ -51,6 +51,7 @@ typedef NS_ENUM(NSInteger, _WKWebAuthenticationPanelUpdate) {
     _WKWebAuthenticationPanelUpdateLAError,
     _WKWebAuthenticationPanelUpdateLAExcludeCredentialsMatched,
     _WKWebAuthenticationPanelUpdateLANoCredential,
+    _WKWebAuthenticationPanelUpdateKeyStoreFull,
 } WK_API_AVAILABLE(macos(10.15.4), ios(13.4));
 
 typedef NS_ENUM(NSInteger, _WKWebAuthenticationResult) {

diff --git a/Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm b/Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm
@@ -76,6 +76,8 @@ static _WKWebAuthenticationPanelUpdate wkWebAuthenticationPanelUpdate(WebAuthent
         return _WKWebAuthenticationPanelUpdateLAExcludeCredentialsMatched;
     if (status == WebAuthenticationStatus::LANoCredential)
         return _WKWebAuthenticationPanelUpdateLANoCredential;
+    if (status == WebAuthenticationStatus::KeyStoreFull)
+        return _WKWebAuthenticationPanelUpdateKeyStoreFull;
     ASSERT_NOT_REACHED();
     return _WKWebAuthenticationPanelUpdateMultipleNFCTagsPresent;
 }

diff --git a/Source/WebKit/UIProcess/WebAuthentication/WebAuthenticationFlags.h b/Source/WebKit/UIProcess/WebAuthentication/WebAuthenticationFlags.h
@@ -49,6 +49,7 @@ enum class WebAuthenticationStatus : uint8_t {
     LAError,
     LAExcludeCredentialsMatched,
     LANoCredential,
+    KeyStoreFull,
 };
 
 enum class LocalAuthenticatorPolicy : bool {

diff --git a/Source/WebKit/UIProcess/WebAuthentication/fido/CtapAuthenticator.cpp b/Source/WebKit/UIProcess/WebAuthentication/fido/CtapAuthenticator.cpp
@@ -30,6 +30,7 @@
 
 #include "CtapDriver.h"
 #include "CtapHidDriver.h"
+#include "Logging.h"
 #include "U2fAuthenticator.h"
 #include <WebCore/AuthenticationExtensionsClientOutputs.h>
 #include <WebCore/AuthenticatorAttachment.h>
@@ -97,6 +98,13 @@ void CtapAuthenticator::makeCredential()
     auto internalUVAvailability = m_info.options().userVerificationAvailability();
     auto residentKeyAvailability = m_info.options().residentKeyAvailability();
     Vector<String> authenticatorSupportedExtensions;
+    if (m_isKeyStoreFull || (m_info.remainingDiscoverableCredentials() && !m_info.remainingDiscoverableCredentials())) {
+        if (options.authenticatorSelection && (options.authenticatorSelection->requireResidentKey || options.authenticatorSelection->residentKey == ResidentKeyRequirement::Required)) {
+            observer()->authenticatorStatusUpdated(WebAuthenticationStatus::KeyStoreFull);
+            return;
+        }
+        residentKeyAvailability = AuthenticatorSupportedOptions::ResidentKeyAvailability::kNotSupported;
+    }
     // If UV is required, then either built-in uv or a pin will work.
     if (internalUVAvailability == UVAvailability::kSupportedAndConfigured && (!options.authenticatorSelection || options.authenticatorSelection->userVerification != UserVerificationRequirement::Discouraged) && m_pinAuth.isEmpty())
         cborCmd = encodeMakeCredentialRequestAsCBOR(requestData().hash, options, internalUVAvailability, residentKeyAvailability, authenticatorSupportedExtensions);
@@ -117,6 +125,7 @@ void CtapAuthenticator::continueMakeCredentialAfterResponseReceived(Vector<uint8
     auto response = readCTAPMakeCredentialResponse(data, AuthenticatorAttachment::CrossPlatform, transports(), std::get<PublicKeyCredentialCreationOptions>(requestData().options).attestation);
     if (!response) {
         auto error = getResponseCode(data);
+        RELEASE_LOG_DEBUG(WebAuthn, "Got error code: %hhu from authenticator.", error);
 
         if (error == CtapDeviceResponseCode::kCtap2ErrActionTimeout) {
             makeCredential();
@@ -127,6 +136,16 @@ void CtapAuthenticator::continueMakeCredentialAfterResponseReceived(Vector<uint8
             receiveRespond(ExceptionData { ExceptionCode::InvalidStateError, "At least one credential matches an entry of the excludeCredentials list in the authenticator."_s });
             return;
         }
+        if (error == CtapDeviceResponseCode::kCtap2ErrKeyStoreFull) {
+            auto& options = std::get<PublicKeyCredentialCreationOptions>(requestData().options);
+            if (options.authenticatorSelection->requireResidentKey || options.authenticatorSelection->residentKey == ResidentKeyRequirement::Required)
+                observer()->authenticatorStatusUpdated(WebAuthenticationStatus::KeyStoreFull);
+            else if (!m_isKeyStoreFull) {
+                m_isKeyStoreFull = true;
+                makeCredential();
+            }
+            return;
+        }
 
         if (isPinError(error)) {
             if (!m_pinAuth.isEmpty() && observer()) // Skip the very first command that acts like wink.
@@ -144,7 +163,7 @@ void CtapAuthenticator::continueMakeCredentialAfterResponseReceived(Vector<uint8
 
         auto rkSupported = m_info.options().residentKeyAvailability() == AuthenticatorSupportedOptions::ResidentKeyAvailability::kSupported;
         auto rkRequested = options.authenticatorSelection && ((options.authenticatorSelection->residentKey && options.authenticatorSelection->residentKey != ResidentKeyRequirement::Discouraged) || options.authenticatorSelection->requireResidentKey);
-        extensionOutputs.credProps = AuthenticationExtensionsClientOutputs::CredentialPropertiesOutput { rkSupported && rkRequested };
+        extensionOutputs.credProps = AuthenticationExtensionsClientOutputs::CredentialPropertiesOutput { rkSupported && rkRequested && !m_isKeyStoreFull };
         response->setExtensions(WTFMove(extensionOutputs));
     }
     receiveRespond(response.releaseNonNull());

diff --git a/Source/WebKit/UIProcess/WebAuthentication/fido/CtapAuthenticator.h b/Source/WebKit/UIProcess/WebAuthentication/fido/CtapAuthenticator.h
@@ -73,6 +73,7 @@ class CtapAuthenticator final : public FidoAuthenticator {
 
     fido::AuthenticatorGetInfoResponse m_info;
     bool m_isDowngraded { false };
+    bool m_isKeyStoreFull { false };
     size_t m_remainingAssertionResponses { 0 };
     Vector<Ref<WebCore::AuthenticatorAssertionResponse>> m_assertionResponses;
     Vector<uint8_t> m_pinAuth;