[webauthn][ble] Crash when connecting to BLE device fails |
|||
Issue descriptionAfter 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.
,
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
,
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
,
Aug 28
To verify the fix, we need BLE device. As the device is not available with the team, adding the label - TE-Hardware-Dependency. Thanks!
,
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
,
Sep 4
|
|||
►
Sign in to add a comment |
|||
Comment 1 by hongjunchoi@chromium.org
, Aug 8Status: Started (was: Assigned)