Skip to content

Commit b2454ee

Browse files
ppontesElectron Botdeepak1556
authored
chore: cherry-pick eac3d9283d11 from chromium (electron#24152)
* chore: cherry-pick eac3d9283d11 from chromium * update patches Co-authored-by: Electron Bot <anonymous@electronjs.org> Co-authored-by: Robo <hop2deep@gmail.com>
1 parent 1ee98b4 commit b2454ee

File tree

2 files changed

+258
-0
lines changed

2 files changed

+258
-0
lines changed

‎patches/chromium/.patches

+1
Original file line numberDiff line numberDiff line change
@@ -109,3 +109,4 @@ a11y_allows_klistboxoption_as_an_item_to_kgroup.patch
109109
fix_handling_non_client_pointer_events_from_pen_on_windows_10.patch
110110
a11y_iterate_all_descendants_for_getselectioncount.patch
111111
allow_ime_to_insert_zero-length_composition_string.patch
112+
cherry-pick-eac3d9283d11.patch

‎patches/chromium/cherry-pick-eac3d9283d11.patch

+257
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,257 @@
1+
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
2+
From: Martin Kreichgauer <martinkr@google.com>
3+
Date: Wed, 3 Jun 2020 21:07:08 +0000
4+
Subject: Revert "fido: add FidoDiscoveryFactory::ResetRequestState()"
5+
6+
This reverts commit 9f151687295d2547bc3d7c1542b80505552f0f87.
7+
8+
Reason for revert: The original change makes an invalid assumptions
9+
about the lifetime of FidoDiscoveryFactory (crbug/1087158). Instances of
10+
FidoDiscoveryFactory generally belong to the
11+
AuthenticatorRequestClientDelegate and as such should outlive the
12+
WebAuthn request. As an exception, instances obtained via
13+
AuthenticatorEnvironmentImpl::GetDiscoveryFactoryOverride() may be
14+
unregistered and freed before the request finishes.
15+
16+
This revert is safe because the caBLE data reset by ResetRequestState
17+
(a) only gets set in the first place if the
18+
WebAuthenticationPhoneSupport flag is on (which is default-off); and (b)
19+
gets set anew for every single request, so it will never be reused
20+
across requests.
21+
22+
Bug: 1087158
23+
24+
Original change's description:
25+
> fido: add FidoDiscoveryFactory::ResetRequestState()
26+
>
27+
> FidoDiscoveryFactory instances generally outlive a WebAuthn request, but
28+
> some of the state is specific to a single request (caBLE pairing and QR
29+
> code generation keys). This is currently not an issue, because
30+
> AuthenticatorCommon explicitly resets all that state at the beginning of
31+
> the request. But I worry that we accidentally break that and leak state
32+
> between requests. To mitigate, introduce an explicit ResetRequestState
33+
> function and call it in AuthenticatorCommon::Cleanup().
34+
>
35+
> Change-Id: I8333a3b14d189d7977cde17cbfe44b4b8dcf6ee2
36+
> Reviewed-on:
37+
> https://chromium-review.googlesource.com/c/chromium/src/+/1793792
38+
> Commit-Queue: Martin Kreichgauer <martinkr@chromium.org>
39+
> Reviewed-by: Nina Satragno <nsatragno@chromium.org>
40+
> Reviewed-by: Adam Langley <agl@chromium.org>
41+
> Cr-Commit-Position: refs/heads/master@{#696593}
42+
43+
Change-Id: I3b1ea46b9b1d5912cbc7ab9a82851e5132335ea8
44+
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2228136
45+
Reviewed-by: Nina Satragno <nsatragno@chromium.org>
46+
Reviewed-by: Adam Langley <agl@chromium.org>
47+
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
48+
Cr-Commit-Position: refs/heads/master@{#774784}
49+
50+
diff --git a/content/browser/webauth/authenticator_common.cc b/content/browser/webauth/authenticator_common.cc
51+
index cea81abea6f8059ff09de9c9a55c305446a0b661..7721eac960bd69351bd6963871f9f1bb96756915 100644
52+
--- a/content/browser/webauth/authenticator_common.cc
53+
+++ b/content/browser/webauth/authenticator_common.cc
54+
@@ -579,13 +579,12 @@ AuthenticatorCommon::CreateRequestDelegate(std::string relying_party_id) {
55+
56+
void AuthenticatorCommon::StartMakeCredentialRequest(
57+
bool allow_skipping_pin_touch) {
58+
- discovery_factory_ =
59+
+ device::FidoDiscoveryFactory* discovery_factory =
60+
AuthenticatorEnvironmentImpl::GetInstance()->GetDiscoveryFactoryOverride(
61+
static_cast<RenderFrameHostImpl*>(render_frame_host_)
62+
->frame_tree_node());
63+
- if (!discovery_factory_) {
64+
- discovery_factory_ = request_delegate_->GetDiscoveryFactory();
65+
- }
66+
+ if (!discovery_factory)
67+
+ discovery_factory = request_delegate_->GetDiscoveryFactory();
68+
69+
if (base::FeatureList::IsEnabled(device::kWebAuthPhoneSupport)) {
70+
std::vector<device::CableDiscoveryData> cable_pairings =
71+
@@ -597,13 +596,13 @@ void AuthenticatorCommon::StartMakeCredentialRequest(
72+
if (request_delegate_->SetCableTransportInfo(
73+
/*cable_extension_provided=*/false, have_paired_phones,
74+
qr_generator_key)) {
75+
- discovery_factory_->set_cable_data(cable_pairings,
76+
- std::move(qr_generator_key));
77+
+ discovery_factory->set_cable_data(cable_pairings,
78+
+ std::move(qr_generator_key));
79+
}
80+
}
81+
82+
request_ = std::make_unique<device::MakeCredentialRequestHandler>(
83+
- connector_, discovery_factory_,
84+
+ connector_, discovery_factory,
85+
GetTransports(caller_origin_, transports_),
86+
*ctap_make_credential_request_, *authenticator_selection_criteria_,
87+
allow_skipping_pin_touch,
88+
@@ -634,13 +633,12 @@ void AuthenticatorCommon::StartMakeCredentialRequest(
89+
90+
void AuthenticatorCommon::StartGetAssertionRequest(
91+
bool allow_skipping_pin_touch) {
92+
- discovery_factory_ =
93+
+ device::FidoDiscoveryFactory* discovery_factory =
94+
AuthenticatorEnvironmentImpl::GetInstance()->GetDiscoveryFactoryOverride(
95+
static_cast<RenderFrameHostImpl*>(render_frame_host_)
96+
->frame_tree_node());
97+
- if (!discovery_factory_) {
98+
- discovery_factory_ = request_delegate_->GetDiscoveryFactory();
99+
- }
100+
+ if (!discovery_factory)
101+
+ discovery_factory = request_delegate_->GetDiscoveryFactory();
102+
103+
std::vector<device::CableDiscoveryData> cable_pairings;
104+
bool have_cable_extension = false;
105+
@@ -664,12 +662,12 @@ void AuthenticatorCommon::StartGetAssertionRequest(
106+
if ((!cable_pairings.empty() || qr_generator_key.has_value()) &&
107+
request_delegate_->SetCableTransportInfo(
108+
have_cable_extension, have_paired_phones, qr_generator_key)) {
109+
- discovery_factory_->set_cable_data(std::move(cable_pairings),
110+
- std::move(qr_generator_key));
111+
+ discovery_factory->set_cable_data(std::move(cable_pairings),
112+
+ std::move(qr_generator_key));
113+
}
114+
115+
request_ = std::make_unique<device::GetAssertionRequestHandler>(
116+
- connector_, discovery_factory_,
117+
+ connector_, discovery_factory,
118+
GetTransports(caller_origin_, transports_), *ctap_get_assertion_request_,
119+
allow_skipping_pin_touch,
120+
base::BindOnce(&AuthenticatorCommon::OnSignResponse,
121+
@@ -1528,15 +1526,6 @@ void AuthenticatorCommon::Cleanup() {
122+
123+
timer_->Stop();
124+
request_.reset();
125+
- if (discovery_factory_) {
126+
- // The FidoDiscoveryFactory instance may have been obtained via
127+
- // AuthenticatorEnvironmentImpl::GetDiscoveryFactoryOverride() (in unit
128+
- // tests or when WebDriver injected a virtual authenticator), in which case
129+
- // it may be long-lived and handle more than one request. Hence, we need to
130+
- // reset all per-request state before deleting its pointer.
131+
- discovery_factory_->ResetRequestState();
132+
- discovery_factory_ = nullptr;
133+
- }
134+
request_delegate_.reset();
135+
make_credential_response_callback_.Reset();
136+
get_assertion_response_callback_.Reset();
137+
diff --git a/content/browser/webauth/authenticator_common.h b/content/browser/webauth/authenticator_common.h
138+
index a7879a77f337e9d31afb954c2fc397cedb74256f..bd241cc8fc34ce9d831ce5fcfb97fbd6997237f0 100644
139+
--- a/content/browser/webauth/authenticator_common.h
140+
+++ b/content/browser/webauth/authenticator_common.h
141+
@@ -194,7 +194,6 @@ class CONTENT_EXPORT AuthenticatorCommon {
142+
RenderFrameHost* const render_frame_host_;
143+
service_manager::Connector* connector_ = nullptr;
144+
base::flat_set<device::FidoTransportProtocol> transports_;
145+
- device::FidoDiscoveryFactory* discovery_factory_ = nullptr;
146+
std::unique_ptr<device::FidoRequestHandlerBase> request_;
147+
blink::mojom::Authenticator::MakeCredentialCallback
148+
make_credential_response_callback_;
149+
diff --git a/device/fido/fido_discovery_factory.cc b/device/fido/fido_discovery_factory.cc
150+
index 05d5f211b17021ac3397fd79b7f210bdf6681def..dc404ba31c2158887c3f946aecba33fb672189f0 100644
151+
--- a/device/fido/fido_discovery_factory.cc
152+
+++ b/device/fido/fido_discovery_factory.cc
153+
@@ -46,10 +46,6 @@ std::unique_ptr<FidoDiscoveryBase> CreateUsbFidoDiscovery(
154+
FidoDiscoveryFactory::FidoDiscoveryFactory() = default;
155+
FidoDiscoveryFactory::~FidoDiscoveryFactory() = default;
156+
157+
-void FidoDiscoveryFactory::ResetRequestState() {
158+
- request_state_ = {};
159+
-}
160+
-
161+
std::unique_ptr<FidoDiscoveryBase> FidoDiscoveryFactory::Create(
162+
FidoTransportProtocol transport,
163+
service_manager::Connector* connector) {
164+
@@ -59,13 +55,10 @@ std::unique_ptr<FidoDiscoveryBase> FidoDiscoveryFactory::Create(
165+
case FidoTransportProtocol::kBluetoothLowEnergy:
166+
return std::make_unique<FidoBleDiscovery>();
167+
case FidoTransportProtocol::kCloudAssistedBluetoothLowEnergy:
168+
- if (request_state_.cable_data_.has_value() ||
169+
- request_state_.qr_generator_key_.has_value()) {
170+
+ if (cable_data_.has_value() || qr_generator_key_.has_value()) {
171+
return std::make_unique<FidoCableDiscovery>(
172+
- request_state_.cable_data_.value_or(
173+
- std::vector<CableDiscoveryData>()),
174+
- request_state_.qr_generator_key_,
175+
- request_state_.cable_pairing_callback_);
176+
+ cable_data_.value_or(std::vector<CableDiscoveryData>()),
177+
+ qr_generator_key_, cable_pairing_callback_);
178+
}
179+
return nullptr;
180+
case FidoTransportProtocol::kNearFieldCommunication:
181+
@@ -88,14 +81,14 @@ std::unique_ptr<FidoDiscoveryBase> FidoDiscoveryFactory::Create(
182+
void FidoDiscoveryFactory::set_cable_data(
183+
std::vector<CableDiscoveryData> cable_data,
184+
base::Optional<QRGeneratorKey> qr_generator_key) {
185+
- request_state_.cable_data_ = std::move(cable_data);
186+
- request_state_.qr_generator_key_ = std::move(qr_generator_key);
187+
+ cable_data_ = std::move(cable_data);
188+
+ qr_generator_key_ = std::move(qr_generator_key);
189+
}
190+
191+
void FidoDiscoveryFactory::set_cable_pairing_callback(
192+
base::RepeatingCallback<void(std::unique_ptr<CableDiscoveryData>)>
193+
pairing_callback) {
194+
- request_state_.cable_pairing_callback_.emplace(std::move(pairing_callback));
195+
+ cable_pairing_callback_.emplace(std::move(pairing_callback));
196+
}
197+
198+
#if defined(OS_WIN)
199+
@@ -121,7 +114,4 @@ FidoDiscoveryFactory::MaybeCreateWinWebAuthnApiDiscovery() {
200+
}
201+
#endif // defined(OS_WIN)
202+
203+
-FidoDiscoveryFactory::RequestState::RequestState() = default;
204+
-FidoDiscoveryFactory::RequestState::~RequestState() = default;
205+
-
206+
} // namespace device
207+
diff --git a/device/fido/fido_discovery_factory.h b/device/fido/fido_discovery_factory.h
208+
index 63133e7dc2bca9dc763881d74ca1e017f5799df7..3723225f316e4766d96b24f135980321bb0e5308 100644
209+
--- a/device/fido/fido_discovery_factory.h
210+
+++ b/device/fido/fido_discovery_factory.h
211+
@@ -38,18 +38,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDiscoveryFactory {
212+
FidoDiscoveryFactory();
213+
virtual ~FidoDiscoveryFactory();
214+
215+
- // Resets all fields that are only meaningful for the duration of a single
216+
- // request to a safe default.
217+
- //
218+
- // The "regular" FidoDiscoveryFactory is owned by the
219+
- // AuthenticatorClientRequestDelegate and lives only for a single request.
220+
- // Instances returned from
221+
- // AuthenticatorEnvironmentImpl::GetDiscoveryFactoryOverride(), which are
222+
- // used in unit tests or by the WebDriver virtual authenticators, are
223+
- // long-lived and may handle multiple WebAuthn requests. Hence, they will be
224+
- // reset at the beginning of each new request.
225+
- void ResetRequestState();
226+
-
227+
// Instantiates a FidoDiscoveryBase for the given transport.
228+
//
229+
// FidoTransportProtocol::kUsbHumanInterfaceDevice requires specifying a
230+
@@ -89,22 +77,14 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDiscoveryFactory {
231+
#endif // defined(OS_WIN)
232+
233+
private:
234+
- // RequestState holds configuration data that is only meaningful for a
235+
- // single WebAuthn request.
236+
- struct RequestState {
237+
- RequestState();
238+
- ~RequestState();
239+
- base::Optional<std::vector<CableDiscoveryData>> cable_data_;
240+
- base::Optional<QRGeneratorKey> qr_generator_key_;
241+
- base::Optional<
242+
- base::RepeatingCallback<void(std::unique_ptr<CableDiscoveryData>)>>
243+
- cable_pairing_callback_;
244+
- };
245+
-
246+
- RequestState request_state_;
247+
#if defined(OS_MACOSX)
248+
base::Optional<fido::mac::AuthenticatorConfig> mac_touch_id_config_;
249+
#endif // defined(OS_MACOSX)
250+
+ base::Optional<std::vector<CableDiscoveryData>> cable_data_;
251+
+ base::Optional<QRGeneratorKey> qr_generator_key_;
252+
+ base::Optional<
253+
+ base::RepeatingCallback<void(std::unique_ptr<CableDiscoveryData>)>>
254+
+ cable_pairing_callback_;
255+
#if defined(OS_WIN)
256+
WinWebAuthnApi* win_webauthn_api_ = nullptr;
257+
#endif // defined(OS_WIN)

0 commit comments

Comments
 (0)