New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 872293 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Sep 4
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

[webauthn][ble] Crash when connecting to BLE device fails

Project Member Reported by jdoerrie@chromium.org, Aug 8

Issue description

After enabling CTAP Chrome will crash when connecting to a BLE key fails.

Reproduction on Linux:

1. Start a recent build with --enable-features="WebAuthenticationBle"
2. Open https://webauthn.io and register a credential for a BLE key.
3. Click back button, and click login.

What is the expected result?
Ideally login succeeds after pressing the BLE key.

What happens instead of that?
Chrome crashes:

[247224:247224:0808/160813.378190:ERROR:fido_ble_connection.cc(317)] CreateGattConnection() failed: ERROR_FAILED
[247224:247224:0808/160813.379303:FATAL:fido_ble_frames.cc(59)] Check failed: max_fragment_size >= 3u (0 vs. 3)
#0 0x7fac52a7f1fc base::debug::StackTrace::StackTrace()
#1 0x7fac529a449b logging::LogMessage::~LogMessage()
#2 0x7fac4c73fbe6 device::FidoBleFrame::ToFragments()
#3 0x7fac4c741ed0 device::FidoBleTransaction::WriteRequestFrame()
#4 0x7fac4c73c438 device::FidoBleDevice::SendRequestFrame()
#5 0x7fac4c73c093 device::FidoBleDevice::Transition()
#6 0x7fac4c73b810 device::FidoBleDevice::AddToPendingFrames()
#7 0x7fac4c73bc99 device::FidoBleDevice::DeviceTransact()
#8 0x7fac4c75fb8f device::DeviceOperation<>::DispatchDeviceRequest()
#9 0x7fac4c760e63 device::U2fSignOperation::Start()
#10 0x7fac4c756280 device::GetAssertionTask::U2fSign()
#11 0x7fac4c755e25 device::GetAssertionTask::StartTask()
#12 0x7fac4c73e9e4 _ZN4base8internal7InvokerINS0_9BindStateIMN6device20FidoBleDiscoveryBaseEFvvEJNS_7WeakPtrINS3_16FidoBleDiscoveryEEEEEEFvvEE7RunOnceEPNS0_13BindStateBaseE
#13 0x7fac5298330d base::debug::TaskAnnotator::RunTask()

The root cause seems to be the following:
- A connection failure will set the device state to kDeviceError and all pending callbacks are run with a base::nullopt.
- This will invoke FidoDevice::OnDeviceInfoReceived, which treats the nullopt simply as an error to understand a CTAP GetInfo command and sets the device state to kReady.
- This will also invoke GetAssertionTask::U2fSign(), which attempts to send another command to the device.
- As preparation, the command will be split into frames and fragments.
- FidoBleFrame::ToFragments() will be invoked with the control point length, but given that this was never read its value is 0.
- This triggers the DCHECK, as 0 is smaller than 3.

Enabling CTAP by default seems to be the issue here, as previously FidoDevice::DiscoverSupportedProtocolAndDeviceInfo didn't attempt to communicate with the device.

As a fix we likely should distinguish between connection errors and CTAP errors. A connection error (such as failing to create a gatt connection, failing to read the control point length or failing to subscribe to notifications) should be treated as a fatal error, from which we likely can't recover. A CTAP error however is expected in some cases (e.g. as a response to a GetInfo command), and should be treated differently. Assigning to Jun, as he is likely the most knowledgeable on this part now.
 
Labels: M-70
Status: Started (was: Assigned)
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8e2860a5cf5d8fd2e194c8ca4d8035eb2abba7b6

commit 8e2860a5cf5d8fd2e194c8ca4d8035eb2abba7b6
Author: Jun Choi <hongjunchoi@chromium.org>
Date: Fri Aug 24 07:34:51 2018

Stop request when device errors out on GetInfo

Currently when FidoDevice fails on AuthenticatorGetInfo command, we
assume that the device supports U2F protocol and initiate U2F request.
Prior to initiating request, check whether AuthenticatorGetInfo command
failed due to any device communication error. If the failure was caused
by device error, drop remaining requests on the device. Also, to prevent
similar failures, always check for device state before dispatching
consecutive requests to authenticators.

Bug:  872293 
Change-Id: I82d83975eec8177adb447e5e94711126884f5b5f
Reviewed-on: https://chromium-review.googlesource.com/1168505
Commit-Queue: Jun Choi <hongjunchoi@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585744}
[modify] https://crrev.com/8e2860a5cf5d8fd2e194c8ca4d8035eb2abba7b6/device/fido/device_operation.h
[modify] https://crrev.com/8e2860a5cf5d8fd2e194c8ca4d8035eb2abba7b6/device/fido/fido_device.cc
[modify] https://crrev.com/8e2860a5cf5d8fd2e194c8ca4d8035eb2abba7b6/device/fido/hid/fido_hid_device.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2c31ebf7cb8b9462e8b093dfd65b37609c0e9f0c

commit 2c31ebf7cb8b9462e8b093dfd65b37609c0e9f0c
Author: Jun Choi <hongjunchoi@chromium.org>
Date: Mon Aug 27 23:17:38 2018

Introduce kMsgError state to FidoDevice

In order to differentiate between an unrecoverable device connection
failure (in which case we drop the device) and a device error that is
caused by an unrecognized request from the client, (in which
case we continue trying to communicate with the device),
introduce FidoDevice::kMsgError.

Bug:  872293 
Change-Id: Ic0f0eded8a0b7aa25a6918343d5b8d551fe3beb4
Reviewed-on: https://chromium-review.googlesource.com/1188981
Reviewed-by: Kim Paulhamus <kpaulhamus@chromium.org>
Commit-Queue: Jun Choi <hongjunchoi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586476}
[modify] https://crrev.com/2c31ebf7cb8b9462e8b093dfd65b37609c0e9f0c/device/fido/ble/fido_ble_device.cc
[modify] https://crrev.com/2c31ebf7cb8b9462e8b093dfd65b37609c0e9f0c/device/fido/ble/fido_ble_device.h
[modify] https://crrev.com/2c31ebf7cb8b9462e8b093dfd65b37609c0e9f0c/device/fido/ble/fido_ble_transaction.cc
[modify] https://crrev.com/2c31ebf7cb8b9462e8b093dfd65b37609c0e9f0c/device/fido/ble/fido_ble_transaction.h
[modify] https://crrev.com/2c31ebf7cb8b9462e8b093dfd65b37609c0e9f0c/device/fido/fido_device.h
[modify] https://crrev.com/2c31ebf7cb8b9462e8b093dfd65b37609c0e9f0c/device/fido/hid/fido_hid_device.cc
[modify] https://crrev.com/2c31ebf7cb8b9462e8b093dfd65b37609c0e9f0c/device/fido/hid/fido_hid_device.h

Cc: ligim...@chromium.org pnangunoori@chromium.org
Labels: TE-Hardware-Dependency
To verify the fix, we need BLE device. As the device is not available with the team, adding the label - TE-Hardware-Dependency.

Thanks!
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 29

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b2e220fd3f3bb5d2e9c2f76f32fe525f9bb5b260

commit b2e220fd3f3bb5d2e9c2f76f32fe525f9bb5b260
Author: Jun Choi <hongjunchoi@chromium.org>
Date: Wed Aug 29 00:54:59 2018

Test FidoDevice handling of transport layer errors

Test that FidoDevice are in kMsgError state when transport layer error
messages are received with known error constants.

Also, test that FidoDevice::DiscoverySupportedProtocolAndDeviceInfo()
does not invoke callback when devices are in kDeviceError state.

Bug:  872293 
Change-Id: I0baa52b74635a8bb54a990f2fef91d69a031a202
Reviewed-on: https://chromium-review.googlesource.com/1194782
Commit-Queue: Jun Choi <hongjunchoi@chromium.org>
Reviewed-by: Kim Paulhamus <kpaulhamus@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586972}
[modify] https://crrev.com/b2e220fd3f3bb5d2e9c2f76f32fe525f9bb5b260/device/fido/ble/fido_ble_device_unittest.cc
[modify] https://crrev.com/b2e220fd3f3bb5d2e9c2f76f32fe525f9bb5b260/device/fido/hid/fido_hid_device_unittest.cc

Status: Fixed (was: Started)

Sign in to add a comment