Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: cherry-pick eac3d9283d11 from chromium #24152

Merged
merged 3 commits into from Jun 18, 2020

Conversation

ppontes
Copy link
Member

@ppontes ppontes commented Jun 16, 2020

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}

Notes: Backported the fix to a crash in FIDO support.

@ppontes ppontes requested a review from a team as a code owner June 16, 2020 15:19
@ppontes ppontes added 8-x-y backport-check-skip Skip trop's backport validity checking labels Jun 16, 2020
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 16, 2020
@ppontes ppontes force-pushed the cherry-pick/8-x-y/chromium/eac3d9283d11 branch from 492e305 to c2cd1d1 Compare June 16, 2020 18:36
@ppontes ppontes force-pushed the cherry-pick/8-x-y/chromium/eac3d9283d11 branch from f175242 to c9cc826 Compare June 17, 2020 12:12
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 17, 2020
@codebytere codebytere merged commit b2454ee into 8-x-y Jun 18, 2020
@release-clerk
Copy link

release-clerk bot commented Jun 18, 2020

Release Notes Persisted

Backported the fix to a crash in FIDO support.

@codebytere codebytere deleted the cherry-pick/8-x-y/chromium/eac3d9283d11 branch June 18, 2020 04:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8-x-y backport-check-skip Skip trop's backport validity checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants