Skip to content

Commit

Permalink
Fix OOB in OnBluetoothScanningPromptEvent
Browse files Browse the repository at this point in the history
This changes fixes an OOB access that may occur in
WebBluetoothServiceImpl::OnBluetoothScanningPromptEvent(). The method
assumes that |scanning_clients_| will be populated when the method is
called, however it can be cleared if a Mojo connection error is
triggered.

The method now returns if |scanning_clients_| is empty, and it uses the
back() and pop() methods of vector to further prevent accidental OOB
access. Additionally, in BluetoothDeviceScanningPromptController, the
EventHandler binding is updated so that the lifetime of the class is
associated with the binding.

(cherry picked from commit 405c014)

Bug: 1024116
Change-Id: I2008f7bc1ce65be1d94d39370ac8593f5ff418e8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1916686
Commit-Queue: Ovidio de Jesús Ruiz-Henríquez <odejesush@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#715472}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1918661
Reviewed-by: Krishna Govind <govind@chromium.org>
Cr-Commit-Position: refs/branch-heads/3966@{#6}
Cr-Branched-From: 71c035f-refs/heads/master@{#714552}
  • Loading branch information
odejesush authored and Krishna Govind committed Nov 15, 2019
1 parent bb46f91 commit 267dcda
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ void BluetoothDeviceScanningPromptController::ShowPermissionPrompt() {
BluetoothScanningPrompt::EventHandler prompt_event_handler =
base::BindRepeating(&BluetoothDeviceScanningPromptController::
OnBluetoothScanningPromptEvent,
base::Unretained(this));
weak_ptr_factory_.GetWeakPtr());
WebContentsDelegate* delegate =
WebContents::FromRenderFrameHost(render_frame_host_)->GetDelegate();
if (delegate) {
Expand Down
21 changes: 12 additions & 9 deletions content/browser/bluetooth/web_bluetooth_service_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -295,16 +295,19 @@ bool WebBluetoothServiceImpl::IsDevicePaired(
void WebBluetoothServiceImpl::OnBluetoothScanningPromptEvent(
BluetoothScanningPrompt::Event event,
BluetoothDeviceScanningPromptController* prompt_controller) {
DCHECK(!scanning_clients_.empty());
// It is possible for |scanning_clients_| to be empty if a Mojo connection
// error has occurred before this method was called.
if (scanning_clients_.empty())
return;

auto client = scanning_clients_.end() - 1;
auto& client = scanning_clients_.back();

DCHECK((*client)->prompt_controller() == prompt_controller);
DCHECK(client->prompt_controller() == prompt_controller);

auto result = blink::mojom::WebBluetoothResult::SUCCESS;
if (event == BluetoothScanningPrompt::Event::kAllow) {
result = blink::mojom::WebBluetoothResult::SUCCESS;
StoreAllowedScanOptions((*client)->scan_options());
StoreAllowedScanOptions(client->scan_options());
} else if (event == BluetoothScanningPrompt::Event::kBlock) {
result = blink::mojom::WebBluetoothResult::SCANNING_BLOCKED;
const url::Origin requesting_origin =
Expand All @@ -320,10 +323,10 @@ void WebBluetoothServiceImpl::OnBluetoothScanningPromptEvent(
NOTREACHED();
}

(*client)->RunRequestScanningStartCallback(std::move(result));
(*client)->set_prompt_controller(nullptr);
client->RunRequestScanningStartCallback(std::move(result));
client->set_prompt_controller(nullptr);
if (event == BluetoothScanningPrompt::Event::kAllow) {
(*client)->set_allow_send_event(true);
client->set_allow_send_event(true);
} else if (event == BluetoothScanningPrompt::Event::kBlock) {
// Here because user explicitly blocks the permission to do Bluetooth
// scanning in one request, it can be interpreted as user wants the current
Expand All @@ -333,7 +336,7 @@ void WebBluetoothServiceImpl::OnBluetoothScanningPromptEvent(
allowed_scan_filters_.clear();
accept_all_advertisements_ = false;
} else if (event == BluetoothScanningPrompt::Event::kCanceled) {
scanning_clients_.erase(client);
scanning_clients_.pop_back();
} else {
NOTREACHED();
}
Expand Down Expand Up @@ -1830,7 +1833,6 @@ void WebBluetoothServiceImpl::ClearState() {
receiver_.reset();

characteristic_id_to_notify_session_.clear();
scanning_clients_.clear();
pending_primary_services_requests_.clear();
descriptor_id_to_characteristic_id_.clear();
characteristic_id_to_service_id_.clear();
Expand All @@ -1839,6 +1841,7 @@ void WebBluetoothServiceImpl::ClearState() {
new FrameConnectedBluetoothDevices(render_frame_host_));
device_chooser_controller_.reset();
device_scanning_prompt_controller_.reset();
scanning_clients_.clear();
allowed_scan_filters_.clear();
accept_all_advertisements_ = false;
BluetoothAdapterFactoryWrapper::Get().ReleaseAdapter(this);
Expand Down

0 comments on commit 267dcda

Please sign in to comment.