Skip to content

Commit

Permalink
fido: make new cryptotoken requests evict old ones
Browse files Browse the repository at this point in the history
WebAuthn requests originating from cryptotoken are executed in the
RenderFrame of the cryptotoken background page. Hence, navigation events
in the U2F request's sender tab have no influence over the lifetime of
the request. This means the corresponding WebAuthn request lives on
until it eventually times out; new requests are immediately resolved
with a PENDING_REQUEST error in the meantime.

As a workaround, have AuthenticatorImpl cancel any pending request, when
a new request from the cryptotoken extension arrives. This will make the
post-reload u2f request cancel the zombie pre-reload request. However,
it will also make subsequent requests (across tabs) cancel any pending
requests, whereas before cryptotoken proxying requests would have been
queued.

Bug: 935480
Change-Id: I6c8986ab010d687efae14927328d3a3eb900ea4a
Reviewed-on: https://chromium-review.googlesource.com/c/1490892
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Commit-Queue: Adam Langley <agl@chromium.org>
Reviewed-by: Adam Langley <agl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#635872}
  • Loading branch information
kreichgauer authored and Commit Bot committed Feb 27, 2019
1 parent 09fc4f3 commit 80f73e8
Showing 1 changed file with 28 additions and 6 deletions.
34 changes: 28 additions & 6 deletions content/browser/webauth/authenticator_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -580,10 +580,21 @@ void AuthenticatorImpl::MakeCredential(
blink::mojom::PublicKeyCredentialCreationOptionsPtr options,
MakeCredentialCallback callback) {
if (request_) {
std::move(callback).Run(blink::mojom::AuthenticatorStatus::PENDING_REQUEST,
nullptr);
return;
if (OriginIsCryptoTokenExtension(
render_frame_host_->GetLastCommittedOrigin())) {
// Requests originating from cryptotoken will generally outlive any
// navigation events on the tab of the request's sender. Evict pending
// requests if cryptotoken sends a new one such that requests from before
// a navigation event do not prevent new requests. See
// https://crbug.com/935480.
Cancel();
} else {
std::move(callback).Run(
blink::mojom::AuthenticatorStatus::PENDING_REQUEST, nullptr);
return;
}
}
DCHECK(!request_);

UpdateRequestDelegate();
if (!request_delegate_) {
Expand Down Expand Up @@ -720,10 +731,21 @@ void AuthenticatorImpl::GetAssertion(
blink::mojom::PublicKeyCredentialRequestOptionsPtr options,
GetAssertionCallback callback) {
if (request_) {
std::move(callback).Run(blink::mojom::AuthenticatorStatus::PENDING_REQUEST,
nullptr);
return;
if (OriginIsCryptoTokenExtension(
render_frame_host_->GetLastCommittedOrigin())) {
// Requests originating from cryptotoken will generally outlive any
// navigation events on the tab of the request's sender. Evict pending
// requests if cryptotoken sends a new one such that requests from before
// a navigation event do not prevent new requests. See
// https://crbug.com/935480.
Cancel();
} else {
std::move(callback).Run(
blink::mojom::AuthenticatorStatus::PENDING_REQUEST, nullptr);
return;
}
}
DCHECK(!request_);

UpdateRequestDelegate();
if (!request_delegate_) {
Expand Down

0 comments on commit 80f73e8

Please sign in to comment.