Skip to content

Commit

Permalink
[WebAuthn] Add logging for authenticators
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=270463
rdar://123704181

Reviewed by Charlie Wolfe.

This patch adds detailed logging to understand the flows through the CTAP
and U2F authenticator code. Sometimes we hit flaky or hard to reproduce
error conditions with security keys. This change will aid in debugging
these cases.

Although challenges are short-lived. We don't log the responses of successful
makeCredential and getAssertion calls containing signatures, instead logging only
if there was an issue with the response.

* Source/WebCore/Modules/webauthn/WebAuthenticationUtils.cpp:
(WebCore::toString):
* Source/WebCore/Modules/webauthn/WebAuthenticationUtils.h:
* Source/WebCore/Modules/webauthn/fido/AuthenticatorGetInfoResponse.cpp:
(fido::encodeAsCBOR):
(fido::toString): Deleted.
* Source/WebKit/UIProcess/WebAuthentication/fido/CtapAuthenticator.cpp:
(WebKit::CtapAuthenticator::makeCredential):
(WebKit::CtapAuthenticator::continueMakeCredentialAfterResponseReceived):
(WebKit::CtapAuthenticator::getAssertion):
(WebKit::CtapAuthenticator::continueGetAssertionAfterResponseReceived):
(WebKit::CtapAuthenticator::continueGetNextAssertionAfterResponseReceived):
(WebKit::CtapAuthenticator::getRetries):
(WebKit::CtapAuthenticator::continueGetKeyAgreementAfterGetRetries):
(WebKit::CtapAuthenticator::continueRequestPinAfterGetKeyAgreement):
(WebKit::CtapAuthenticator::continueGetPinTokenAfterRequestPin):
(WebKit::CtapAuthenticator::continueRequestAfterGetPinToken):
(WebKit::CtapAuthenticator::tryRestartPin):
(WebKit::CtapAuthenticator::tryDowngrade):
(WebKit::CtapAuthenticator::aaguidForDebugging const):
* Source/WebKit/UIProcess/WebAuthentication/fido/CtapAuthenticator.h:
* Source/WebKit/UIProcess/WebAuthentication/fido/FidoAuthenticator.cpp:
(WebKit::FidoAuthenticator::transportForDebugging const):
* Source/WebKit/UIProcess/WebAuthentication/fido/FidoAuthenticator.h:
* Source/WebKit/UIProcess/WebAuthentication/fido/U2fAuthenticator.cpp:
(WebKit::U2fAuthenticator::makeCredential):
(WebKit::U2fAuthenticator::checkExcludeList):
(WebKit::U2fAuthenticator::issueRegisterCommand):
(WebKit::U2fAuthenticator::getAssertion):
(WebKit::U2fAuthenticator::issueSignCommand):
(WebKit::U2fAuthenticator::issueNewCommand):
(WebKit::U2fAuthenticator::issueCommand):
(WebKit::U2fAuthenticator::continueRegisterCommandAfterResponseReceived):
(WebKit::U2fAuthenticator::continueCheckOnlyCommandAfterResponseReceived):
(WebKit::U2fAuthenticator::continueBogusCommandExcludeCredentialsMatchAfterResponseReceived):
(WebKit::U2fAuthenticator::continueBogusCommandNoCredentialsAfterResponseReceived):
(WebKit::U2fAuthenticator::continueSignCommandAfterResponseReceived):

Canonical link: https://commits.webkit.org/275657@main
  • Loading branch information
Pascoe committed Mar 4, 2024
1 parent 68c3a98 commit 7db187a
Show file tree
Hide file tree
Showing 8 changed files with 114 additions and 41 deletions.
29 changes: 29 additions & 0 deletions Source/WebCore/Modules/webauthn/WebAuthenticationUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,35 @@ Vector<uint8_t> encodeRawPublicKey(const Vector<uint8_t>& x, const Vector<uint8_
return rawKey;
}

String toString(AuthenticatorTransport transport)
{
switch (transport) {
case AuthenticatorTransport::Usb:
return authenticatorTransportUsb;
break;
case AuthenticatorTransport::Nfc:
return authenticatorTransportNfc;
break;
case AuthenticatorTransport::Ble:
return authenticatorTransportBle;
break;
case AuthenticatorTransport::Internal:
return authenticatorTransportInternal;
break;
case AuthenticatorTransport::Cable:
return authenticatorTransportCable;
case AuthenticatorTransport::Hybrid:
return authenticatorTransportHybrid;
case AuthenticatorTransport::SmartCard:
return authenticatorTransportSmartCard;
default:
break;
}
ASSERT_NOT_REACHED();
return nullString();
}


} // namespace WebCore

#endif // ENABLE(WEB_AUTHN)
4 changes: 4 additions & 0 deletions Source/WebCore/Modules/webauthn/WebAuthenticationUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#if ENABLE(WEB_AUTHN)

#include "AttestationConveyancePreference.h"
#include "AuthenticatorTransport.h"
#include "BufferSource.h"
#include "CBORValue.h"
#include "SecurityOrigin.h"
Expand Down Expand Up @@ -64,6 +65,9 @@ WEBCORE_EXPORT cbor::CBORValue::MapValue buildUserEntityMap(const Vector<uint8_t

// encodeRawPublicKey takes X & Y and returns them as a 0x04 || X || Y byte array.
WEBCORE_EXPORT Vector<uint8_t> encodeRawPublicKey(const Vector<uint8_t>& X, const Vector<uint8_t>& Y);

WEBCORE_EXPORT String toString(AuthenticatorTransport);

} // namespace WebCore

#endif // ENABLE(WEB_AUTHN)
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
#include "CBORValue.h"
#include "CBORWriter.h"
#include "WebAuthenticationConstants.h"
#include "WebAuthenticationUtils.h"

namespace fido {

Expand Down Expand Up @@ -89,34 +90,6 @@ AuthenticatorGetInfoResponse& AuthenticatorGetInfoResponse::setRemainingDiscover
return *this;
}

static String toString(WebCore::AuthenticatorTransport transport)
{
switch (transport) {
case WebCore::AuthenticatorTransport::Usb:
return WebCore::authenticatorTransportUsb;
break;
case WebCore::AuthenticatorTransport::Nfc:
return WebCore::authenticatorTransportNfc;
break;
case WebCore::AuthenticatorTransport::Ble:
return WebCore::authenticatorTransportBle;
break;
case WebCore::AuthenticatorTransport::Internal:
return WebCore::authenticatorTransportInternal;
break;
case WebCore::AuthenticatorTransport::Cable:
return WebCore::authenticatorTransportCable;
case WebCore::AuthenticatorTransport::Hybrid:
return WebCore::authenticatorTransportHybrid;
case WebCore::AuthenticatorTransport::SmartCard:
return WebCore::authenticatorTransportSmartCard;
default:
break;
}
ASSERT_NOT_REACHED();
return nullString();
}

Vector<uint8_t> encodeAsCBOR(const AuthenticatorGetInfoResponse& response)
{
using namespace cbor;
Expand All @@ -142,7 +115,7 @@ Vector<uint8_t> encodeAsCBOR(const AuthenticatorGetInfoResponse& response)

if (response.transports()) {
auto transports = *response.transports();
deviceInfoMap.emplace(CBORValue(7), toArrayValue(transports.map(toString)));
deviceInfoMap.emplace(CBORValue(7), toArrayValue(transports.map(WebCore::toString)));
}

if (response.remainingDiscoverableCredentials())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,14 @@
#include <WebCore/ExceptionData.h>
#include <WebCore/Pin.h>
#include <WebCore/U2fCommandConstructor.h>
#include <WebCore/WebAuthenticationUtils.h>
#include <wtf/EnumTraits.h>
#include <wtf/RunLoop.h>
#include <wtf/text/Base64.h>
#include <wtf/text/StringConcatenateNumbers.h>

#define CTAP_RELEASE_LOG(fmt, ...) RELEASE_LOG(WebAuthn, "%p [aaguid=%s, transport=%s] - CtapAuthenticator::" fmt, this, aaguidForDebugging().utf8().data(), transportForDebugging().utf8().data(), ##__VA_ARGS__)

namespace WebKit {
using namespace WebCore;
using namespace fido;
Expand Down Expand Up @@ -93,6 +97,7 @@ CtapAuthenticator::CtapAuthenticator(std::unique_ptr<CtapDriver>&& driver, Authe

void CtapAuthenticator::makeCredential()
{
CTAP_RELEASE_LOG("makeCredential");
ASSERT(!m_isDowngraded);
Vector<uint8_t> cborCmd;
auto& options = std::get<PublicKeyCredentialCreationOptions>(requestData().options);
Expand All @@ -113,6 +118,7 @@ void CtapAuthenticator::makeCredential()
cborCmd = encodeMakeCredentialRequestAsCBOR(requestData().hash, options, internalUVAvailability, residentKeyAvailability, authenticatorSupportedExtensions, PinParameters { pin::kProtocolVersion, m_pinAuth });
else
cborCmd = encodeMakeCredentialRequestAsCBOR(requestData().hash, options, internalUVAvailability, residentKeyAvailability, authenticatorSupportedExtensions);
CTAP_RELEASE_LOG("makeCredential: Sending %s", base64EncodeToString(cborCmd).utf8().data());
driver().transact(WTFMove(cborCmd), [weakThis = WeakPtr { *this }](Vector<uint8_t>&& data) {
ASSERT(RunLoop::isMain());
if (!weakThis)
Expand All @@ -123,10 +129,11 @@ void CtapAuthenticator::makeCredential()

void CtapAuthenticator::continueMakeCredentialAfterResponseReceived(Vector<uint8_t>&& data)
{
auto error = getResponseCode(data);
CTAP_RELEASE_LOG("continueMakeCredentialAfterResponseReceived: Got error code: %hhu from authenticator.", error);
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.", enumToUnderlyingType(error));
CTAP_RELEASE_LOG("makeCredential: Failed to parse response %s", base64EncodeToString(data).utf8().data());

if (error == CtapDeviceResponseCode::kCtap2ErrActionTimeout) {
makeCredential();
Expand Down Expand Up @@ -184,6 +191,7 @@ void CtapAuthenticator::getAssertion()
cborCmd = encodeGetAssertionRequestAsCBOR(requestData().hash, options, internalUVAvailability, authenticatorSupportedExtensions, PinParameters { pin::kProtocolVersion, m_pinAuth });
else
cborCmd = encodeGetAssertionRequestAsCBOR(requestData().hash, options, internalUVAvailability, authenticatorSupportedExtensions);
CTAP_RELEASE_LOG("getAssertion: Sending %s", base64EncodeToString(cborCmd).utf8().data());
driver().transact(WTFMove(cborCmd), [weakThis = WeakPtr { *this }](Vector<uint8_t>&& data) {
ASSERT(RunLoop::isMain());
if (!weakThis)
Expand All @@ -195,9 +203,10 @@ void CtapAuthenticator::getAssertion()
void CtapAuthenticator::continueGetAssertionAfterResponseReceived(Vector<uint8_t>&& data)
{
auto response = readCTAPGetAssertionResponse(data, AuthenticatorAttachment::CrossPlatform);
auto error = getResponseCode(data);
CTAP_RELEASE_LOG("continueGetAssertionAfterResponseReceived: errorcode: %hhu", error);
if (!response) {
auto error = getResponseCode(data);

CTAP_RELEASE_LOG("continueGetAssertionAfterResponseReceived: Failed to parse response %s", base64EncodeToString(data).utf8().data());
if (error == CtapDeviceResponseCode::kCtap2ErrActionTimeout) {
getAssertion();
return;
Expand All @@ -216,9 +225,11 @@ void CtapAuthenticator::continueGetAssertionAfterResponseReceived(Vector<uint8_t
if (error == CtapDeviceResponseCode::kCtap2ErrNoCredentials && observer())
observer()->authenticatorStatusUpdated(WebAuthenticationStatus::NoCredentialsFound);

CTAP_RELEASE_LOG("continueGetAssertionAfterResponseReceived: No credentials found.");
receiveRespond(ExceptionData { ExceptionCode::UnknownError, makeString("Unknown internal error. Error code: ", static_cast<uint8_t>(error)) });
return;
}
CTAP_RELEASE_LOG("continueGetAssertionAfterResponseReceived: Get %lu credentials back.", response->numberOfCredentials());

if (response->numberOfCredentials() <= 1) {
receiveRespond(response.releaseNonNull());
Expand All @@ -228,7 +239,9 @@ void CtapAuthenticator::continueGetAssertionAfterResponseReceived(Vector<uint8_t
m_remainingAssertionResponses = response->numberOfCredentials() - 1;
m_assertionResponses.reserveInitialCapacity(response->numberOfCredentials());
m_assertionResponses.append(response.releaseNonNull());
driver().transact(encodeEmptyAuthenticatorRequest(CtapRequestCommand::kAuthenticatorGetNextAssertion), [weakThis = WeakPtr { *this }](Vector<uint8_t>&& data) {
auto cborCmd = encodeEmptyAuthenticatorRequest(CtapRequestCommand::kAuthenticatorGetNextAssertion);
CTAP_RELEASE_LOG("continueGetAssertionAfterResponseReceived: Sending %s", base64EncodeToString(cborCmd).utf8().data());
driver().transact(WTFMove(cborCmd), [weakThis = WeakPtr { *this }](Vector<uint8_t>&& data) {
ASSERT(RunLoop::isMain());
if (!weakThis)
return;
Expand All @@ -238,14 +251,17 @@ void CtapAuthenticator::continueGetAssertionAfterResponseReceived(Vector<uint8_t

void CtapAuthenticator::continueGetNextAssertionAfterResponseReceived(Vector<uint8_t>&& data)
{
auto error = getResponseCode(data);
CTAP_RELEASE_LOG("continueGetNextAssertionAfterResponseReceived: errorcode: %hhu", error);
auto response = readCTAPGetAssertionResponse(data, AuthenticatorAttachment::CrossPlatform);
if (!response) {
auto error = getResponseCode(data);
CTAP_RELEASE_LOG("continueGetNextAssertionAfterResponseReceived: Unable to parse response: %s", base64EncodeToString(data).utf8().data());
receiveRespond(ExceptionData { ExceptionCode::UnknownError, makeString("Unknown internal error. Error code: ", static_cast<uint8_t>(error)) });
return;
}
m_remainingAssertionResponses--;
m_assertionResponses.append(response.releaseNonNull());
CTAP_RELEASE_LOG("continueGetNextAssertionAfterResponseReceived: Remaining responses: %lu", m_remainingAssertionResponses);

if (!m_remainingAssertionResponses) {
if (auto* observer = this->observer()) {
Expand All @@ -264,7 +280,9 @@ void CtapAuthenticator::continueGetNextAssertionAfterResponseReceived(Vector<uin
return;
}

driver().transact(encodeEmptyAuthenticatorRequest(CtapRequestCommand::kAuthenticatorGetNextAssertion), [weakThis = WeakPtr { *this }](Vector<uint8_t>&& data) {
auto cborCmd = encodeEmptyAuthenticatorRequest(CtapRequestCommand::kAuthenticatorGetNextAssertion);
CTAP_RELEASE_LOG("continueGetNextAssertionAfterResponseReceived: Sending %s", base64EncodeToString(cborCmd).utf8().data());
driver().transact(WTFMove(cborCmd), [weakThis = WeakPtr { *this }](Vector<uint8_t>&& data) {
ASSERT(RunLoop::isMain());
if (!weakThis)
return;
Expand All @@ -275,16 +293,19 @@ void CtapAuthenticator::continueGetNextAssertionAfterResponseReceived(Vector<uin
void CtapAuthenticator::getRetries()
{
auto cborCmd = encodeAsCBOR(pin::RetriesRequest { });
driver().transact(WTFMove(cborCmd), [weakThis = WeakPtr { *this }](Vector<uint8_t>&& data) {
CTAP_RELEASE_LOG("getRetries: Sending %s", base64EncodeToString(cborCmd).utf8().data());
driver().transact(WTFMove(cborCmd), [weakThis = WeakPtr { *this }, this](Vector<uint8_t>&& data) {
ASSERT(RunLoop::isMain());
if (!weakThis)
return;
CTAP_RELEASE_LOG("getRetries: Response %s", base64EncodeToString(data).utf8().data());
weakThis->continueGetKeyAgreementAfterGetRetries(WTFMove(data));
});
}

void CtapAuthenticator::continueGetKeyAgreementAfterGetRetries(Vector<uint8_t>&& data)
{
CTAP_RELEASE_LOG("continueGetKeyAgreementAfterGetRetries");
auto retries = pin::RetriesResponse::parse(data);
if (!retries) {
auto error = getResponseCode(data);
Expand All @@ -293,16 +314,19 @@ void CtapAuthenticator::continueGetKeyAgreementAfterGetRetries(Vector<uint8_t>&&
}

auto cborCmd = encodeAsCBOR(pin::KeyAgreementRequest { });
driver().transact(WTFMove(cborCmd), [weakThis = WeakPtr { *this }, retries = retries->retries] (Vector<uint8_t>&& data) {
CTAP_RELEASE_LOG("continueGetKeyAgreementAfterGetRetries: Sending %s", base64EncodeToString(cborCmd).utf8().data());
driver().transact(WTFMove(cborCmd), [weakThis = WeakPtr { *this }, this, retries = retries->retries] (Vector<uint8_t>&& data) {
ASSERT(RunLoop::isMain());
if (!weakThis)
return;
CTAP_RELEASE_LOG("continueGetKeyAgreementAfterGetRetries: Response %s", base64EncodeToString(data).utf8().data());
weakThis->continueRequestPinAfterGetKeyAgreement(WTFMove(data), retries);
});
}

void CtapAuthenticator::continueRequestPinAfterGetKeyAgreement(Vector<uint8_t>&& data, uint64_t retries)
{
CTAP_RELEASE_LOG("continueRequestPinAfterGetKeyAgreement");
auto keyAgreement = pin::KeyAgreementResponse::parse(data);
if (!keyAgreement) {
auto error = getResponseCode(data);
Expand All @@ -311,17 +335,20 @@ void CtapAuthenticator::continueRequestPinAfterGetKeyAgreement(Vector<uint8_t>&&
}

if (auto* observer = this->observer()) {
observer->requestPin(retries, [weakThis = WeakPtr { *this }, keyAgreement = WTFMove(*keyAgreement)] (const String& pin) {
CTAP_RELEASE_LOG("continueRequestPinAfterGetKeyAgreement: Requesting pin from observer.");
observer->requestPin(retries, [weakThis = WeakPtr { *this }, this, keyAgreement = WTFMove(*keyAgreement)] (const String& pin) {
RELEASE_ASSERT(RunLoop::isMain());
if (!weakThis)
return;
CTAP_RELEASE_LOG("continueRequestPinAfterGetKeyAgreement: Got pin from observer.");
weakThis->continueGetPinTokenAfterRequestPin(pin, keyAgreement.peerKey);
});
}
}

void CtapAuthenticator::continueGetPinTokenAfterRequestPin(const String& pin, const CryptoKeyEC& peerKey)
{
CTAP_RELEASE_LOG("continueGetNextAssertionAfterResponseReceived");
if (pin.isNull()) {
receiveRespond(ExceptionData { ExceptionCode::UnknownError, "Pin is null."_s });
return;
Expand All @@ -342,16 +369,19 @@ void CtapAuthenticator::continueGetPinTokenAfterRequestPin(const String& pin, co
}

auto cborCmd = encodeAsCBOR(*tokenRequest);
driver().transact(WTFMove(cborCmd), [weakThis = WeakPtr { *this }, tokenRequest = WTFMove(*tokenRequest)] (Vector<uint8_t>&& data) {
CTAP_RELEASE_LOG("continueGetPinTokenAfterRequestPin: Sending %s", base64EncodeToString(cborCmd).utf8().data());
driver().transact(WTFMove(cborCmd), [weakThis = WeakPtr { *this }, this, tokenRequest = WTFMove(*tokenRequest)] (Vector<uint8_t>&& data) {
ASSERT(RunLoop::isMain());
if (!weakThis)
return;
CTAP_RELEASE_LOG("continueGetPinTokenAfterRequestPin: Response %s", base64EncodeToString(data).utf8().data());
weakThis->continueRequestAfterGetPinToken(WTFMove(data), tokenRequest);
});
}

void CtapAuthenticator::continueRequestAfterGetPinToken(Vector<uint8_t>&& data, const fido::pin::TokenRequest& tokenRequest)
{
CTAP_RELEASE_LOG("continueGetNextAssertionAfterResponseReceived");
auto token = pin::TokenResponse::parse(tokenRequest.sharedKey(), data);
if (!token) {
auto error = getResponseCode(data);
Expand All @@ -377,6 +407,7 @@ void CtapAuthenticator::continueRequestAfterGetPinToken(Vector<uint8_t>&& data,

bool CtapAuthenticator::tryRestartPin(const CtapDeviceResponseCode& error)
{
CTAP_RELEASE_LOG("tryRestartPin: Error code: %hhu", error);
switch (error) {
case CtapDeviceResponseCode::kCtap2ErrPinAuthInvalid:
case CtapDeviceResponseCode::kCtap2ErrPinInvalid:
Expand All @@ -390,6 +421,7 @@ bool CtapAuthenticator::tryRestartPin(const CtapDeviceResponseCode& error)

bool CtapAuthenticator::tryDowngrade()
{
CTAP_RELEASE_LOG("tryDowngrade");
if (m_info.versions().find(ProtocolVersion::kU2f) == m_info.versions().end())
return false;
if (!observer())
Expand All @@ -404,6 +436,7 @@ bool CtapAuthenticator::tryDowngrade()
if (!isConvertible)
return false;

CTAP_RELEASE_LOG("tryDowngrade: Downgrading to U2F.");
m_isDowngraded = true;
driver().setProtocol(ProtocolVersion::kU2f);
observer()->downgrade(this, U2fAuthenticator::create(releaseDriver()));
Expand All @@ -418,6 +451,11 @@ Vector<AuthenticatorTransport> CtapAuthenticator::transports() const
return Vector { driver().transport() };
}

String CtapAuthenticator::aaguidForDebugging() const
{
return WTF::UUID { std::span<const uint8_t, 16> { m_info.aaguid() } }.toString();
}

} // namespace WebKit

#endif // ENABLE(WEB_AUTHN)
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ class CtapAuthenticator final : public FidoAuthenticator {

Vector<WebCore::AuthenticatorTransport> transports() const;

String aaguidForDebugging() const;

fido::AuthenticatorGetInfoResponse m_info;
bool m_isDowngraded { false };
bool m_isKeyStoreFull { false };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

#include "config.h"
#include "FidoAuthenticator.h"
#include <WebCore/WebAuthenticationUtils.h>

#if ENABLE(WEB_AUTHN)

Expand Down Expand Up @@ -56,6 +57,11 @@ std::unique_ptr<CtapDriver> FidoAuthenticator::releaseDriver()
return WTFMove(m_driver);
}

String FidoAuthenticator::transportForDebugging() const
{
return WebCore::toString(driver().transport());
}

} // namespace WebKit

#endif // ENABLE(WEB_AUTHN)
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ class FidoAuthenticator : public Authenticator {
CtapDriver& driver() const;
std::unique_ptr<CtapDriver> releaseDriver();

String transportForDebugging() const;

private:
std::unique_ptr<CtapDriver> m_driver;
};
Expand Down
Loading

0 comments on commit 7db187a

Please sign in to comment.