Project Fortify, an experimental security project, has identified the following potential security issue. If you’re a feature owner CC-ed on this bug, please do your best to review these reports. Please see https://chromium.googlesource.com/chromium/src/+/main/docs/security/ai-generated-security-bugs-faq.md for more information.

Overview: A potential Use-After-Free vulnerability exists in the Windows Web Bluetooth implementation when handling descriptor retrieval failures. If base::win::PostAsyncResults fails during discovery, it synchronously executes a callback that destroys the discoverer object, yet the function continues to loop over freed member variables. This can lead to a browser-process crash or potential control flow hijack.

Affected files:

• device/bluetooth/bluetooth_gatt_discoverer_winrt.cc
• device/bluetooth/bluetooth_gatt_discoverer_winrt.h
• device/bluetooth/bluetooth_device_winrt.cc

Estimated timestamp from git blame: 2018-08-06

Detailed Description

A potential Use-After-Free (UAF) vulnerability has been identified in BluetoothGattDiscovererWinrt::OnGetCharacteristics in device/bluetooth/bluetooth_gatt_discoverer_winrt.cc.

In OnGetCharacteristics, the code obtains a reference to a nested vector of characteristics stored inside the member service_to_characteristics_map_:

// device/bluetooth/bluetooth_gatt_discoverer_winrt.cc
378:   auto& characteristics_list =
379:       service_to_characteristics_map_[service_attribute_handle];

It then iterates over this list using a range-based for loop to retrieve descriptors for each characteristic:

386:   for (const auto& gatt_characteristic : characteristics_list) {
...
416:     hr = base::win::PostAsyncResults(
417:         std::move(get_descriptors_op),
418:         base::BindOnce(&BluetoothGattDiscovererWinrt::OnGetDescriptors,
419:                        weak_ptr_factory_.GetWeakPtr(),
420:                        characteristic_attribute_handle));
421: 
422:     if (FAILED(hr)) {
423:       BLUETOOTH_LOG(DEBUG) << "PostAsyncResults failed: "
424:                            << logging::SystemErrorCodeToString(hr);
425:       std::move(callback_).Run(false);
426:     }
427:   }

If base::win::PostAsyncResults fails (returning a failure HRESULT due to an unexpected disconnect or internal COM state error), the code invokes std::move(callback_).Run(false). Running this callback triggers a synchronous sequence of events that resolves to BluetoothDeviceWinrt::OnGattDiscoveryComplete, which deletes the owning BluetoothGattDiscovererWinrt instance via gatt_discoverer_.reset() (at device/bluetooth/bluetooth_device_winrt.cc:800).

Because there is no return or break statement after line 425, the range-based for loop continues to the next iteration. However, the destruction of the discoverer object has already deallocated the parent map (service_to_characteristics_map_), freeing the backing memory of the characteristics_list vector. Consequently, the loop’s compiler-generated iterators are left dangling, resulting in a Use-After-Free when they are incremented/dereferenced in subsequent iterations.

Note that our analysis is based on code inspection and static tracing; our automated tools have not run code or verified a dynamic proof-of-concept.

Potential Attack Steps / Triggering Conditions

An attacker trying to trigger this issue would potentially follow these steps:

1. Persuade a user to connect to a custom, malicious Bluetooth LE peripheral via the Web Bluetooth chooser interface.
2. During GATT discovery, when the browser attempts to retrieve descriptors for the characteristics of a service, the peripheral can abort or disconnect the connection or trigger an RPC failure.
3. This connection drop causes GetDescriptorsAsync or the subsequent PostAsyncResults call to fail (FAILED(hr) resolves to true).
4. The callback runs synchronously, destroying the discoverer, while the loop continues iterating on the freed heap memory.

Suggested Remediation

To resolve this issue, add a return; statement immediately after invoking the callback inside the FAILED(hr) block to stop further execution of the loop and safely exit the method:

    if (FAILED(hr)) {
      BLUETOOTH_LOG(DEBUG) << "PostAsyncResults failed: "
                           << logging::SystemErrorCodeToString(hr);
      std::move(callback_).Run(false);
      return;
    }

Evaluated with Chrome root at commit: b1520ef4a76878853a31f0943b565e42060edec8

Results so far have been promising, but there can be wrong deductions. Feel free to adjust as follows:

• If you are familiar with the severity guidelines, you may adjust the severity.
• If this is a false positive, and there’s no work to be done, please close as WAI.
• If there is work to do here but not a vulnerability, please change the issue type to Task/Bug/FR.

Data from false positives will be used to improve accuracy over time. And please feel free to reach out to me directly if you have concerns or feedback on the project.