Skip to content

Commit e140358

Browse files
pascoejpascoej
committed
WebAuthn Authentication over NFC is broken in Safari Technology Preview 234 (SEED)
rdar://168456474 https://bugs.webkit.org/show_bug.cgi?id=306279 Reviewed by Brent Fulgham. This patch makes a few changes to make CCID authenticators behave better. 1. Some newer authenticators have inconsistent behavior whenever we start a session per-message. This change moves us to maintaining a session with a smart card until invalidated and queuing messages as-needed. 2. We do not handle the kCtap2ErrUserPresenceRequired, which the authenticator returns when it was left on an NFC reader for too long. We were looping on this terminal error until we encountered the next issue fixed. 3. At one point we stopped handling the kCtap2ErrPinAuthBlocked error properly. This error can be delievered before pin entry is attempted for an already locked authenticator and this error gets returned whenever the platform ignores kCtap2ErrUserPresenceRequired. This also required an internal change in rdar://168804001. * Source/WebCore/Modules/webauthn/fido/FidoConstants.cpp: (fido::isCtapDeviceResponseCode): * Source/WebCore/Modules/webauthn/fido/FidoConstants.h: * Source/WebKit/UIProcess/WebAuthentication/Cocoa/AuthenticatorPresenterCoordinator.mm: (WebKit::AuthenticatorPresenterCoordinator::updatePresenter): * Source/WebKit/UIProcess/WebAuthentication/Cocoa/CcidConnection.h: * Source/WebKit/UIProcess/WebAuthentication/Cocoa/CcidConnection.mm: (-[WKSmartCardObserver initWithCard:invalidationHandler:]): (-[WKSmartCardObserver dealloc]): (-[WKSmartCardObserver observeValueForKeyPath:ofObject:change:context:]): (WebKit::CcidConnection::create): (WebKit::CcidConnection::CcidConnection): (WebKit::CcidConnection::~CcidConnection): (WebKit::CcidConnection::detectContactless): (WebKit::CcidConnection::transact): (WebKit::CcidConnection::processPendingRequests): (WebKit::CcidConnection::stop): (WebKit::CcidConnection::startPolling): (WebKit::CcidConnection::transact const): Deleted. (WebKit::CcidConnection::stop const): Deleted. (WebKit::CcidConnection::restartPolling): Deleted. * Source/WebKit/UIProcess/WebAuthentication/Cocoa/CcidService.h: * Source/WebKit/UIProcess/WebAuthentication/Cocoa/CcidService.mm: (WebKit::CcidService::~CcidService): (WebKit::CcidService::platformStartDiscovery): (WebKit::CcidService::onValidCard): (WebKit::CcidService::onCardRemoved): (-[_WKSmartCardSlotStateObserver observeValueForKeyPath:ofObject:change:context:]): * Source/WebKit/UIProcess/WebAuthentication/Mock/MockCcidService.mm: (WebKit::MockCcidService::platformStartDiscovery): * Source/WebKit/UIProcess/WebAuthentication/fido/CtapAuthenticator.cpp: (WebKit::CtapAuthenticator::continueSilentlyCheckCredentials): (WebKit::CtapAuthenticator::continueMakeCredentialAfterResponseReceived): (WebKit::CtapAuthenticator::continueGetAssertionAfterResponseReceived): * Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm: (TestWebKitAPI::TEST(WebAuthenticationPanel, MakeCredentialPinAuthBlockedError)): (TestWebKitAPI::TEST(WebAuthenticationPanel, GetAssertionPinAuthBlockedError)): Canonical link: https://commits.webkit.org/306280@main
1 parent a9bbaa7 commit e140358

File tree

10 files changed

+239
-43
lines changed

10 files changed

+239
-43
lines changed

‎Source/WebCore/Modules/webauthn/fido/FidoConstants.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,7 @@ bool isCtapDeviceResponseCode(CtapDeviceResponseCode code)
100100
case CtapDeviceResponseCode::kCtap2ErrKeepAliveCancel:
101101
case CtapDeviceResponseCode::kCtap2ErrNoCredentials:
102102
case CtapDeviceResponseCode::kCtap2ErrUserActionTimeout:
103+
case CtapDeviceResponseCode::kCtap2ErrUserPresenceRequired:
103104
case CtapDeviceResponseCode::kCtap2ErrNotAllowed:
104105
case CtapDeviceResponseCode::kCtap2ErrPinInvalid:
105106
case CtapDeviceResponseCode::kCtap2ErrPinBlocked:

‎Source/WebCore/Modules/webauthn/fido/FidoConstants.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,7 @@ enum class CtapDeviceResponseCode : uint8_t {
110110
kCtap2ErrPinTokenExpired = 0x38,
111111
kCtap2ErrRequestTooLarge = 0x39,
112112
kCtap2ErrActionTimeout = 0x3A,
113+
kCtap2ErrUserPresenceRequired = 0x3B,
113114
kCtap2ErrOther = 0x7F,
114115
kCtap2ErrSpecLast = 0xDF,
115116
kCtap2ErrExtensionFirst = 0xE0,

‎Source/WebKit/UIProcess/WebAuthentication/Cocoa/AuthenticatorPresenterCoordinator.mm‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,12 +115,12 @@
115115
switch (status) {
116116
case WebAuthenticationStatus::PinBlocked: {
117117
auto error = adoptNS([[NSError alloc] initWithDomain:ASCAuthorizationErrorDomain code:ASCAuthorizationErrorAuthenticatorPermanentlyLocked userInfo:nil]);
118-
m_credentialRequestHandler(nil, error.get());
118+
[m_presenter updateInterfaceForUserVisibleError:error.get()];
119119
break;
120120
}
121121
case WebAuthenticationStatus::PinAuthBlocked: {
122122
auto error = adoptNS([[NSError alloc] initWithDomain:ASCAuthorizationErrorDomain code:ASCAuthorizationErrorAuthenticatorTemporarilyLocked userInfo:nil]);
123-
m_credentialRequestHandler(nil, error.get());
123+
[m_presenter updateInterfaceForUserVisibleError:error.get()];
124124
break;
125125
}
126126
case WebAuthenticationStatus::PinInvalid: {

‎Source/WebKit/UIProcess/WebAuthentication/Cocoa/CcidConnection.h‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -27,11 +27,14 @@
2727

2828
#if ENABLE(WEB_AUTHN)
2929

30+
#include <wtf/Deque.h>
3031
#include <wtf/RetainPtr.h>
31-
#include <wtf/RunLoop.h>
3232
#include <wtf/ThreadSafeWeakPtr.h>
33+
#include <wtf/WeakPtr.h>
3334

3435
OBJC_CLASS TKSmartCard;
36+
OBJC_CLASS TKSmartCardSlot;
37+
OBJC_CLASS WKSmartCardObserver;
3538

3639
namespace WebKit {
3740

@@ -40,27 +43,29 @@ using DataReceivedCallback = Function<void(Vector<uint8_t>&&)>;
4043

4144
class CcidConnection : public ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr<CcidConnection> {
4245
public:
43-
static Ref<CcidConnection> create(RetainPtr<TKSmartCard>&&, CcidService&);
46+
static Ref<CcidConnection> create(RetainPtr<TKSmartCard>&&, RetainPtr<TKSmartCardSlot>&&, CcidService&);
4447
~CcidConnection();
4548

46-
void transact(Vector<uint8_t>&& data, DataReceivedCallback&&) const;
47-
void stop() const;
49+
void transact(Vector<uint8_t>&& data, DataReceivedCallback&&);
50+
void stop();
4851
bool contactless() const { return m_contactless; };
4952

5053
private:
51-
CcidConnection(RetainPtr<TKSmartCard>&&, CcidService&);
54+
CcidConnection(RetainPtr<TKSmartCard>&&, RetainPtr<TKSmartCardSlot>&&, CcidService&);
5255

53-
void restartPolling();
5456
void startPolling();
55-
5657
void detectContactless();
57-
5858
void trySelectFidoApplet();
59+
void processPendingRequests();
5960

6061
const RetainPtr<TKSmartCard> m_smartCard;
62+
RetainPtr<TKSmartCardSlot> m_slot;
6163
WeakPtr<CcidService> m_service;
62-
RunLoop::Timer m_retryTimer;
64+
Deque<std::pair<Vector<uint8_t>, DataReceivedCallback>> m_pendingRequests;
6365
bool m_contactless { false };
66+
bool m_hasSession { false };
67+
bool m_sessionPending { false };
68+
RetainPtr<WKSmartCardObserver> m_observer;
6469
};
6570

6671
} // namespace WebKit

‎Source/WebKit/UIProcess/WebAuthentication/Cocoa/CcidConnection.mm‎

Lines changed: 141 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -28,30 +28,98 @@
2828

2929
#if ENABLE(WEB_AUTHN)
3030
#import "CcidService.h"
31+
#import "Logging.h"
3132
#import <CryptoTokenKit/TKSmartCard.h>
3233
#import <WebCore/FidoConstants.h>
3334
#import <wtf/BlockPtr.h>
35+
#import <wtf/Function.h>
3436
#import <wtf/StdLibExtras.h>
3537
#import <wtf/cocoa/VectorCocoa.h>
3638

39+
#define CCID_RELEASE_LOG(fmt, ...) RELEASE_LOG(WebAuthn, "%p - CcidConnection::" fmt, this, ##__VA_ARGS__)
40+
41+
// SPI for TKSmartCardSlot reinsertion
42+
@interface TKSmartCardSlot (SPI)
43+
- (BOOL)simulateCardReinsertionWithError:(NSError **)error;
44+
@end
45+
46+
@interface WKSmartCardObserver : NSObject
47+
- (instancetype)initWithCard:(TKSmartCard *)card invalidationHandler:(Function<void()>&&)handler;
48+
@end
49+
50+
@implementation WKSmartCardObserver {
51+
RetainPtr<TKSmartCard> _card;
52+
Function<void()> _invalidationHandler;
53+
}
54+
55+
- (instancetype)initWithCard:(TKSmartCard *)card invalidationHandler:(Function<void()>&&)handler
56+
{
57+
if (!(self = [super init]))
58+
return nil;
59+
_card = card;
60+
_invalidationHandler = WTF::move(handler);
61+
[_card addObserver:self forKeyPath:@"valid" options:NSKeyValueObservingOptionNew context:nil];
62+
return self;
63+
}
64+
65+
- (void)dealloc
66+
{
67+
[_card removeObserver:self forKeyPath:@"valid"];
68+
[super dealloc];
69+
}
70+
71+
- (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(NSDictionary *)change context:(void *)context
72+
{
73+
if ([keyPath isEqualToString:@"valid"]) {
74+
BOOL valid = [[change objectForKey:NSKeyValueChangeNewKey] boolValue];
75+
if (!valid) {
76+
@synchronized(self) {
77+
if (_invalidationHandler) {
78+
callOnMainRunLoop([handler = WTF::move(_invalidationHandler)] () mutable {
79+
handler();
80+
});
81+
}
82+
}
83+
}
84+
}
85+
}
86+
@end
87+
3788
namespace WebKit {
3889
using namespace fido;
3990

40-
Ref<CcidConnection> CcidConnection::create(RetainPtr<TKSmartCard>&& smartCard, CcidService& service)
91+
Ref<CcidConnection> CcidConnection::create(RetainPtr<TKSmartCard>&& smartCard, RetainPtr<TKSmartCardSlot>&& slot, CcidService& service)
4192
{
42-
return adoptRef(*new CcidConnection(WTF::move(smartCard), service));
93+
return adoptRef(*new CcidConnection(WTF::move(smartCard), WTF::move(slot), service));
4394
}
4495

45-
CcidConnection::CcidConnection(RetainPtr<TKSmartCard>&& smartCard, CcidService& service)
96+
CcidConnection::CcidConnection(RetainPtr<TKSmartCard>&& smartCard, RetainPtr<TKSmartCardSlot>&& slot, CcidService& service)
4697
: m_smartCard(WTF::move(smartCard))
98+
, m_slot(WTF::move(slot))
4799
, m_service(service)
48-
, m_retryTimer(RunLoop::mainSingleton(), "CcidConnection::RetryTimer"_s, this, &CcidConnection::startPolling)
49100
{
101+
CCID_RELEASE_LOG("created, smartCard=%p, slot=%p", m_smartCard.get(), m_slot.get());
102+
103+
m_observer = adoptNS([[WKSmartCardObserver alloc]
104+
initWithCard:m_smartCard.get()
105+
invalidationHandler:[weakThis = ThreadSafeWeakPtr { *this }] {
106+
if (RefPtr protectedThis = weakThis.get()) {
107+
RELEASE_LOG(WebAuthn, "%p - CcidConnection::invalidationHandler fired, m_hasSession=%d, m_sessionPending=%d", protectedThis.get(), protectedThis->m_hasSession, protectedThis->m_sessionPending);
108+
protectedThis->m_sessionPending = false;
109+
protectedThis->m_pendingRequests.clear();
110+
if (protectedThis->m_hasSession) {
111+
[protectedThis->m_smartCard endSession];
112+
protectedThis->m_hasSession = false;
113+
}
114+
}
115+
}]);
116+
50117
startPolling();
51118
}
52119

53120
CcidConnection::~CcidConnection()
54121
{
122+
CCID_RELEASE_LOG("destroyed, m_hasSession=%d", m_hasSession);
55123
stop();
56124
}
57125

@@ -69,6 +137,7 @@
69137
// Only contactless smart cards have uid, check for longer length than apdu status
70138
if (response.size() > 2)
71139
protectedThis->m_contactless = true;
140+
protectedThis->trySelectFidoApplet();
72141
});
73142
}
74143

@@ -98,36 +167,90 @@
98167
});
99168
}
100169

101-
void CcidConnection::transact(Vector<uint8_t>&& data, DataReceivedCallback&& callback) const
170+
void CcidConnection::transact(Vector<uint8_t>&& data, DataReceivedCallback&& callback)
102171
{
103-
[m_smartCard beginSessionWithReply:makeBlockPtr([this, protectedThis = Ref { *this }, data = WTF::move(data), callback = WTF::move(callback)] (BOOL success, NSError *error) mutable {
104-
if (!success)
105-
return;
106-
[m_smartCard transmitRequest:toNSData(data).get() reply:makeBlockPtr([this, protectedThis = Ref { *this }, callback = WTF::move(callback)](NSData * _Nullable nsResponse, NSError * _Nullable error) mutable {
107-
[m_smartCard endSession];
108-
callOnMainRunLoop([response = makeVector(nsResponse), callback = WTF::move(callback)] () mutable {
172+
CCID_RELEASE_LOG("transact, m_hasSession=%d, m_sessionPending=%d", m_hasSession, m_sessionPending);
173+
if (m_sessionPending) {
174+
CCID_RELEASE_LOG("session pending, queuing request");
175+
m_pendingRequests.append({ WTF::move(data), WTF::move(callback) });
176+
return;
177+
}
178+
if (m_hasSession) {
179+
CCID_RELEASE_LOG("reusing session, calling transmitRequest");
180+
[m_smartCard transmitRequest:toNSData(data).get() reply:makeBlockPtr([protectedThis = Ref { *this }, callback = WTF::move(callback)](NSData * _Nullable nsResponse, NSError * _Nullable error) mutable {
181+
RELEASE_LOG(WebAuthn, "%p - CcidConnection::transmitRequest reply, error=%p", protectedThis.ptr(), error);
182+
bool hasError = !!error;
183+
callOnMainRunLoop([protectedThis = WTF::move(protectedThis), response = makeVector(nsResponse), callback = WTF::move(callback), hasError] () mutable {
184+
if (hasError) {
185+
[protectedThis->m_smartCard endSession];
186+
protectedThis->m_hasSession = false;
187+
}
109188
callback(WTF::move(response));
110189
});
111190
}).get()];
112-
}).get()];
191+
} else {
192+
CCID_RELEASE_LOG("no session, calling beginSessionWithReply");
193+
m_sessionPending = true;
194+
[m_smartCard beginSessionWithReply:makeBlockPtr([protectedThis = Ref { *this }, data = WTF::move(data), callback = WTF::move(callback)] (BOOL success, NSError *error) mutable {
195+
RELEASE_LOG(WebAuthn, "%p - CcidConnection::beginSessionWithReply reply, success=%d, error=%p", protectedThis.ptr(), success, error);
196+
if (!success) {
197+
callOnMainRunLoop([protectedThis = WTF::move(protectedThis), callback = WTF::move(callback)] () mutable {
198+
protectedThis->m_sessionPending = false;
199+
callback({ });
200+
// Drain pending requests with empty callbacks on failure
201+
while (!protectedThis->m_pendingRequests.isEmpty()) {
202+
auto pending = protectedThis->m_pendingRequests.takeFirst();
203+
pending.second({ });
204+
}
205+
});
206+
return;
207+
}
208+
callOnMainRunLoop([protectedThis = WTF::move(protectedThis), data = WTF::move(data), callback = WTF::move(callback)] () mutable {
209+
protectedThis->m_sessionPending = false;
210+
protectedThis->m_hasSession = true;
211+
RELEASE_LOG(WebAuthn, "%p - CcidConnection::session started", protectedThis.ptr());
212+
[protectedThis->m_smartCard transmitRequest:toNSData(data).get() reply:makeBlockPtr([protectedThis = WTF::move(protectedThis), callback = WTF::move(callback)](NSData * _Nullable nsResponse, NSError * _Nullable error) mutable {
213+
RELEASE_LOG(WebAuthn, "%p - CcidConnection::transmitRequest reply, error=%p", protectedThis.ptr(), error);
214+
bool hasError = !!error;
215+
callOnMainRunLoop([protectedThis = WTF::move(protectedThis), response = makeVector(nsResponse), callback = WTF::move(callback), hasError] () mutable {
216+
if (hasError) {
217+
[protectedThis->m_smartCard endSession];
218+
protectedThis->m_hasSession = false;
219+
}
220+
callback(WTF::move(response));
221+
protectedThis->processPendingRequests();
222+
});
223+
}).get()];
224+
});
225+
}).get()];
226+
}
113227
}
114228

115229

116-
void CcidConnection::stop() const
230+
void CcidConnection::processPendingRequests()
117231
{
232+
if (m_pendingRequests.isEmpty() || !m_hasSession)
233+
return;
234+
CCID_RELEASE_LOG("processPendingRequests, count=%zu", m_pendingRequests.size());
235+
auto pending = m_pendingRequests.takeFirst();
236+
transact(WTF::move(pending.first), WTF::move(pending.second));
118237
}
119238

120-
// NearField polling is a one shot polling. It halts after tags are detected.
121-
// Therefore, a restart process is needed to resume polling after error.
122-
void CcidConnection::restartPolling()
239+
void CcidConnection::stop()
123240
{
124-
m_retryTimer.startOneShot(1_s); // Magic number to give users enough time for reactions.
241+
CCID_RELEASE_LOG("stop, m_hasSession=%d, m_sessionPending=%d", m_hasSession, m_sessionPending);
242+
m_sessionPending = false;
243+
m_pendingRequests.clear();
244+
if (m_hasSession) {
245+
CCID_RELEASE_LOG("ending session");
246+
[m_smartCard endSession];
247+
m_hasSession = false;
248+
}
125249
}
126250

127251
void CcidConnection::startPolling()
128252
{
129253
detectContactless();
130-
trySelectFidoApplet();
131254
}
132255

133256
} // namespace WebKit

‎Source/WebKit/UIProcess/WebAuthentication/Cocoa/CcidService.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,8 @@ class CcidService : public FidoService {
5151
void didConnectTag();
5252

5353
void updateSlots(NSArray *slots);
54-
void onValidCard(RetainPtr<TKSmartCard>&&);
54+
void onValidCard(RetainPtr<TKSmartCard>&&, RetainPtr<TKSmartCardSlot>&&);
55+
void onCardRemoved();
5556

5657
protected:
5758
explicit CcidService(AuthenticatorTransportServiceObserver&);

0 commit comments

Comments
 (0)