-
Notifications
You must be signed in to change notification settings - Fork 15k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
chore: cherry-pick eac3d9283d11 from chromium
- Loading branch information
Showing
14 changed files
with
327 additions
and
69 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,257 @@ | ||
| From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 | ||
| From: Martin Kreichgauer <martinkr@google.com> | ||
| Date: Wed, 3 Jun 2020 21:07:08 +0000 | ||
| Subject: Revert "fido: add FidoDiscoveryFactory::ResetRequestState()" | ||
|
|
||
| This reverts commit 9f151687295d2547bc3d7c1542b80505552f0f87. | ||
|
|
||
| Reason for revert: The original change makes an invalid assumptions | ||
| about the lifetime of FidoDiscoveryFactory (crbug/1087158). Instances of | ||
| FidoDiscoveryFactory generally belong to the | ||
| AuthenticatorRequestClientDelegate and as such should outlive the | ||
| WebAuthn request. As an exception, instances obtained via | ||
| AuthenticatorEnvironmentImpl::GetDiscoveryFactoryOverride() may be | ||
| unregistered and freed before the request finishes. | ||
|
|
||
| This revert is safe because the caBLE data reset by ResetRequestState | ||
| (a) only gets set in the first place if the | ||
| WebAuthenticationPhoneSupport flag is on (which is default-off); and (b) | ||
| gets set anew for every single request, so it will never be reused | ||
| across requests. | ||
|
|
||
| Bug: 1087158 | ||
|
|
||
| Original change's description: | ||
| > fido: add FidoDiscoveryFactory::ResetRequestState() | ||
| > | ||
| > FidoDiscoveryFactory instances generally outlive a WebAuthn request, but | ||
| > some of the state is specific to a single request (caBLE pairing and QR | ||
| > code generation keys). This is currently not an issue, because | ||
| > AuthenticatorCommon explicitly resets all that state at the beginning of | ||
| > the request. But I worry that we accidentally break that and leak state | ||
| > between requests. To mitigate, introduce an explicit ResetRequestState | ||
| > function and call it in AuthenticatorCommon::Cleanup(). | ||
| > | ||
| > Change-Id: I8333a3b14d189d7977cde17cbfe44b4b8dcf6ee2 | ||
| > Reviewed-on: | ||
| > https://chromium-review.googlesource.com/c/chromium/src/+/1793792 | ||
| > Commit-Queue: Martin Kreichgauer <martinkr@chromium.org> | ||
| > Reviewed-by: Nina Satragno <nsatragno@chromium.org> | ||
| > Reviewed-by: Adam Langley <agl@chromium.org> | ||
| > Cr-Commit-Position: refs/heads/master@{#696593} | ||
|
|
||
| Change-Id: I3b1ea46b9b1d5912cbc7ab9a82851e5132335ea8 | ||
| Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2228136 | ||
| Reviewed-by: Nina Satragno <nsatragno@chromium.org> | ||
| Reviewed-by: Adam Langley <agl@chromium.org> | ||
| Commit-Queue: Martin Kreichgauer <martinkr@google.com> | ||
| Cr-Commit-Position: refs/heads/master@{#774784} | ||
|
|
||
| diff --git a/content/browser/webauth/authenticator_common.cc b/content/browser/webauth/authenticator_common.cc | ||
| index cea81abea6f8059ff09de9c9a55c305446a0b661..7721eac960bd69351bd6963871f9f1bb96756915 100644 | ||
| --- a/content/browser/webauth/authenticator_common.cc | ||
| +++ b/content/browser/webauth/authenticator_common.cc | ||
| @@ -579,13 +579,12 @@ AuthenticatorCommon::CreateRequestDelegate(std::string relying_party_id) { | ||
|
|
||
| void AuthenticatorCommon::StartMakeCredentialRequest( | ||
| bool allow_skipping_pin_touch) { | ||
| - discovery_factory_ = | ||
| + device::FidoDiscoveryFactory* discovery_factory = | ||
| AuthenticatorEnvironmentImpl::GetInstance()->GetDiscoveryFactoryOverride( | ||
| static_cast<RenderFrameHostImpl*>(render_frame_host_) | ||
| ->frame_tree_node()); | ||
| - if (!discovery_factory_) { | ||
| - discovery_factory_ = request_delegate_->GetDiscoveryFactory(); | ||
| - } | ||
| + if (!discovery_factory) | ||
| + discovery_factory = request_delegate_->GetDiscoveryFactory(); | ||
|
|
||
| if (base::FeatureList::IsEnabled(device::kWebAuthPhoneSupport)) { | ||
| std::vector<device::CableDiscoveryData> cable_pairings = | ||
| @@ -597,13 +596,13 @@ void AuthenticatorCommon::StartMakeCredentialRequest( | ||
| if (request_delegate_->SetCableTransportInfo( | ||
| /*cable_extension_provided=*/false, have_paired_phones, | ||
| qr_generator_key)) { | ||
| - discovery_factory_->set_cable_data(cable_pairings, | ||
| - std::move(qr_generator_key)); | ||
| + discovery_factory->set_cable_data(cable_pairings, | ||
| + std::move(qr_generator_key)); | ||
| } | ||
| } | ||
|
|
||
| request_ = std::make_unique<device::MakeCredentialRequestHandler>( | ||
| - connector_, discovery_factory_, | ||
| + connector_, discovery_factory, | ||
| GetTransports(caller_origin_, transports_), | ||
| *ctap_make_credential_request_, *authenticator_selection_criteria_, | ||
| allow_skipping_pin_touch, | ||
| @@ -634,13 +633,12 @@ void AuthenticatorCommon::StartMakeCredentialRequest( | ||
|
|
||
| void AuthenticatorCommon::StartGetAssertionRequest( | ||
| bool allow_skipping_pin_touch) { | ||
| - discovery_factory_ = | ||
| + device::FidoDiscoveryFactory* discovery_factory = | ||
| AuthenticatorEnvironmentImpl::GetInstance()->GetDiscoveryFactoryOverride( | ||
| static_cast<RenderFrameHostImpl*>(render_frame_host_) | ||
| ->frame_tree_node()); | ||
| - if (!discovery_factory_) { | ||
| - discovery_factory_ = request_delegate_->GetDiscoveryFactory(); | ||
| - } | ||
| + if (!discovery_factory) | ||
| + discovery_factory = request_delegate_->GetDiscoveryFactory(); | ||
|
|
||
| std::vector<device::CableDiscoveryData> cable_pairings; | ||
| bool have_cable_extension = false; | ||
| @@ -664,12 +662,12 @@ void AuthenticatorCommon::StartGetAssertionRequest( | ||
| if ((!cable_pairings.empty() || qr_generator_key.has_value()) && | ||
| request_delegate_->SetCableTransportInfo( | ||
| have_cable_extension, have_paired_phones, qr_generator_key)) { | ||
| - discovery_factory_->set_cable_data(std::move(cable_pairings), | ||
| - std::move(qr_generator_key)); | ||
| + discovery_factory->set_cable_data(std::move(cable_pairings), | ||
| + std::move(qr_generator_key)); | ||
| } | ||
|
|
||
| request_ = std::make_unique<device::GetAssertionRequestHandler>( | ||
| - connector_, discovery_factory_, | ||
| + connector_, discovery_factory, | ||
| GetTransports(caller_origin_, transports_), *ctap_get_assertion_request_, | ||
| allow_skipping_pin_touch, | ||
| base::BindOnce(&AuthenticatorCommon::OnSignResponse, | ||
| @@ -1528,15 +1526,6 @@ void AuthenticatorCommon::Cleanup() { | ||
|
|
||
| timer_->Stop(); | ||
| request_.reset(); | ||
| - if (discovery_factory_) { | ||
| - // The FidoDiscoveryFactory instance may have been obtained via | ||
| - // AuthenticatorEnvironmentImpl::GetDiscoveryFactoryOverride() (in unit | ||
| - // tests or when WebDriver injected a virtual authenticator), in which case | ||
| - // it may be long-lived and handle more than one request. Hence, we need to | ||
| - // reset all per-request state before deleting its pointer. | ||
| - discovery_factory_->ResetRequestState(); | ||
| - discovery_factory_ = nullptr; | ||
| - } | ||
| request_delegate_.reset(); | ||
| make_credential_response_callback_.Reset(); | ||
| get_assertion_response_callback_.Reset(); | ||
| diff --git a/content/browser/webauth/authenticator_common.h b/content/browser/webauth/authenticator_common.h | ||
| index a7879a77f337e9d31afb954c2fc397cedb74256f..bd241cc8fc34ce9d831ce5fcfb97fbd6997237f0 100644 | ||
| --- a/content/browser/webauth/authenticator_common.h | ||
| +++ b/content/browser/webauth/authenticator_common.h | ||
| @@ -194,7 +194,6 @@ class CONTENT_EXPORT AuthenticatorCommon { | ||
| RenderFrameHost* const render_frame_host_; | ||
| service_manager::Connector* connector_ = nullptr; | ||
| base::flat_set<device::FidoTransportProtocol> transports_; | ||
| - device::FidoDiscoveryFactory* discovery_factory_ = nullptr; | ||
| std::unique_ptr<device::FidoRequestHandlerBase> request_; | ||
| blink::mojom::Authenticator::MakeCredentialCallback | ||
| make_credential_response_callback_; | ||
| diff --git a/device/fido/fido_discovery_factory.cc b/device/fido/fido_discovery_factory.cc | ||
| index 05d5f211b17021ac3397fd79b7f210bdf6681def..dc404ba31c2158887c3f946aecba33fb672189f0 100644 | ||
| --- a/device/fido/fido_discovery_factory.cc | ||
| +++ b/device/fido/fido_discovery_factory.cc | ||
| @@ -46,10 +46,6 @@ std::unique_ptr<FidoDiscoveryBase> CreateUsbFidoDiscovery( | ||
| FidoDiscoveryFactory::FidoDiscoveryFactory() = default; | ||
| FidoDiscoveryFactory::~FidoDiscoveryFactory() = default; | ||
|
|
||
| -void FidoDiscoveryFactory::ResetRequestState() { | ||
| - request_state_ = {}; | ||
| -} | ||
| - | ||
| std::unique_ptr<FidoDiscoveryBase> FidoDiscoveryFactory::Create( | ||
| FidoTransportProtocol transport, | ||
| service_manager::Connector* connector) { | ||
| @@ -59,13 +55,10 @@ std::unique_ptr<FidoDiscoveryBase> FidoDiscoveryFactory::Create( | ||
| case FidoTransportProtocol::kBluetoothLowEnergy: | ||
| return std::make_unique<FidoBleDiscovery>(); | ||
| case FidoTransportProtocol::kCloudAssistedBluetoothLowEnergy: | ||
| - if (request_state_.cable_data_.has_value() || | ||
| - request_state_.qr_generator_key_.has_value()) { | ||
| + if (cable_data_.has_value() || qr_generator_key_.has_value()) { | ||
| return std::make_unique<FidoCableDiscovery>( | ||
| - request_state_.cable_data_.value_or( | ||
| - std::vector<CableDiscoveryData>()), | ||
| - request_state_.qr_generator_key_, | ||
| - request_state_.cable_pairing_callback_); | ||
| + cable_data_.value_or(std::vector<CableDiscoveryData>()), | ||
| + qr_generator_key_, cable_pairing_callback_); | ||
| } | ||
| return nullptr; | ||
| case FidoTransportProtocol::kNearFieldCommunication: | ||
| @@ -88,14 +81,14 @@ std::unique_ptr<FidoDiscoveryBase> FidoDiscoveryFactory::Create( | ||
| void FidoDiscoveryFactory::set_cable_data( | ||
| std::vector<CableDiscoveryData> cable_data, | ||
| base::Optional<QRGeneratorKey> qr_generator_key) { | ||
| - request_state_.cable_data_ = std::move(cable_data); | ||
| - request_state_.qr_generator_key_ = std::move(qr_generator_key); | ||
| + cable_data_ = std::move(cable_data); | ||
| + qr_generator_key_ = std::move(qr_generator_key); | ||
| } | ||
|
|
||
| void FidoDiscoveryFactory::set_cable_pairing_callback( | ||
| base::RepeatingCallback<void(std::unique_ptr<CableDiscoveryData>)> | ||
| pairing_callback) { | ||
| - request_state_.cable_pairing_callback_.emplace(std::move(pairing_callback)); | ||
| + cable_pairing_callback_.emplace(std::move(pairing_callback)); | ||
| } | ||
|
|
||
| #if defined(OS_WIN) | ||
| @@ -121,7 +114,4 @@ FidoDiscoveryFactory::MaybeCreateWinWebAuthnApiDiscovery() { | ||
| } | ||
| #endif // defined(OS_WIN) | ||
|
|
||
| -FidoDiscoveryFactory::RequestState::RequestState() = default; | ||
| -FidoDiscoveryFactory::RequestState::~RequestState() = default; | ||
| - | ||
| } // namespace device | ||
| diff --git a/device/fido/fido_discovery_factory.h b/device/fido/fido_discovery_factory.h | ||
| index 63133e7dc2bca9dc763881d74ca1e017f5799df7..3723225f316e4766d96b24f135980321bb0e5308 100644 | ||
| --- a/device/fido/fido_discovery_factory.h | ||
| +++ b/device/fido/fido_discovery_factory.h | ||
| @@ -38,18 +38,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDiscoveryFactory { | ||
| FidoDiscoveryFactory(); | ||
| virtual ~FidoDiscoveryFactory(); | ||
|
|
||
| - // Resets all fields that are only meaningful for the duration of a single | ||
| - // request to a safe default. | ||
| - // | ||
| - // The "regular" FidoDiscoveryFactory is owned by the | ||
| - // AuthenticatorClientRequestDelegate and lives only for a single request. | ||
| - // Instances returned from | ||
| - // AuthenticatorEnvironmentImpl::GetDiscoveryFactoryOverride(), which are | ||
| - // used in unit tests or by the WebDriver virtual authenticators, are | ||
| - // long-lived and may handle multiple WebAuthn requests. Hence, they will be | ||
| - // reset at the beginning of each new request. | ||
| - void ResetRequestState(); | ||
| - | ||
| // Instantiates a FidoDiscoveryBase for the given transport. | ||
| // | ||
| // FidoTransportProtocol::kUsbHumanInterfaceDevice requires specifying a | ||
| @@ -89,22 +77,14 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDiscoveryFactory { | ||
| #endif // defined(OS_WIN) | ||
|
|
||
| private: | ||
| - // RequestState holds configuration data that is only meaningful for a | ||
| - // single WebAuthn request. | ||
| - struct RequestState { | ||
| - RequestState(); | ||
| - ~RequestState(); | ||
| - base::Optional<std::vector<CableDiscoveryData>> cable_data_; | ||
| - base::Optional<QRGeneratorKey> qr_generator_key_; | ||
| - base::Optional< | ||
| - base::RepeatingCallback<void(std::unique_ptr<CableDiscoveryData>)>> | ||
| - cable_pairing_callback_; | ||
| - }; | ||
| - | ||
| - RequestState request_state_; | ||
| #if defined(OS_MACOSX) | ||
| base::Optional<fido::mac::AuthenticatorConfig> mac_touch_id_config_; | ||
| #endif // defined(OS_MACOSX) | ||
| + base::Optional<std::vector<CableDiscoveryData>> cable_data_; | ||
| + base::Optional<QRGeneratorKey> qr_generator_key_; | ||
| + base::Optional< | ||
| + base::RepeatingCallback<void(std::unique_ptr<CableDiscoveryData>)>> | ||
| + cable_pairing_callback_; | ||
| #if defined(OS_WIN) | ||
| WinWebAuthnApi* win_webauthn_api_ = nullptr; | ||
| #endif // defined(OS_WIN) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.