pw_bluetooth_sapphire: Fix overflow on parameter_total_size AdvertisingPacketFilter::BuildSetlocalNameCommand now validates the packet size before constructing the CommandPacket to prevent an attacker from taking control over the command queue. I also added a PW_CHECK to be defensive against this type of attack, such that we get a Bluetooth crash instead of a RCE bug if a similar vulnerability manifests somewhere else. Fixed: b/512561779 Fixed: b/535173940 Change-Id: I80bcc780ca82a695c9b9987fd227c9406a6a6964 Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/482639
diff --git a/pw_bluetooth/public/pw_bluetooth/hci_android.emb b/pw_bluetooth/public/pw_bluetooth/hci_android.emb index bb4ab46..29d323d 100644 --- a/pw_bluetooth/public/pw_bluetooth/hci_android.emb +++ b/pw_bluetooth/public/pw_bluetooth/hci_android.emb
@@ -1029,6 +1029,7 @@ struct LEApcfLocalNameCommand(size: UInt:8): [requires: 0 <= size <= 29] + let max_local_name_size = 29 let vendor_size = AndroidCommandHeader.$size_in_bytes
diff --git a/pw_bluetooth_sapphire/host/hci/advertising_packet_filter.cc b/pw_bluetooth_sapphire/host/hci/advertising_packet_filter.cc index f067752..505a2bc 100644 --- a/pw_bluetooth_sapphire/host/hci/advertising_packet_filter.cc +++ b/pw_bluetooth_sapphire/host/hci/advertising_packet_filter.cc
@@ -14,6 +14,8 @@ #include "pw_bluetooth_sapphire/internal/host/hci/advertising_packet_filter.h" +#include <limits> + #include "pw_bluetooth/hci_android.emb.h" #include "pw_bluetooth_sapphire/internal/host/common/uint128.h" #include "pw_bluetooth_sapphire/internal/host/hci-spec/vendor_protocol.h" @@ -232,20 +234,19 @@ if (!filter.name_substring().empty()) { std::optional<CommandPacket> packet = BuildSetLocalNameCommand(filter_index, filter.name_substring()); - if (packet) { - hci_cmd_runner_->QueueCommand( - packet.value(), [this](const EventPacket& event) { - hci::Result<> result = event.ToResult(); - if (bt_is_error( - result, WARN, "hci-le", "failed offloading filter")) { - return; - } - auto view = - event.view<android_emb::LEApcfCommandCompleteEventView>(); - uint8_t available_spaces = view.available_spaces().Read(); - open_slots_[OffloadedFilterType::kLocalName] = available_spaces; - }); + if (!packet.has_value()) { + return false; } + hci_cmd_runner_->QueueCommand( + packet.value(), [this](const EventPacket& event) { + hci::Result<> result = event.ToResult(); + if (bt_is_error(result, WARN, "hci-le", "failed offloading filter")) { + return; + } + auto view = event.view<android_emb::LEApcfCommandCompleteEventView>(); + uint8_t available_spaces = view.available_spaces().Read(); + open_slots_[OffloadedFilterType::kLocalName] = available_spaces; + }); } if (filter.manufacturer_code().has_value()) { @@ -425,6 +426,7 @@ view.enabled().Write(hci_spec::GenericEnableParam::DISABLE); } + PW_CHECK(view.Ok()); return packet; } @@ -495,6 +497,7 @@ // when, the delivery mode is ON_FOUND. We aren't using that delivery mode // so we don't set those fields. + PW_CHECK(view.Ok()); return packet; } @@ -523,6 +526,7 @@ view.action().Write(android_emb::ApcfAction::DELETE); view.filter_index().Write(filter_index); + PW_CHECK(view.Ok()); return packet; } @@ -548,6 +552,7 @@ view.uuid_mask().BackingStorage().WriteLittleEndianUInt<16>( std::numeric_limits<uint16_t>::max()); + PW_CHECK(view.Ok()); return packet; } @@ -573,6 +578,7 @@ view.uuid_mask().BackingStorage().WriteLittleEndianUInt<32>( std::numeric_limits<uint32_t>::max()); + PW_CHECK(view.Ok()); return packet; } @@ -595,6 +601,7 @@ mask.fill(std::numeric_limits<uint8_t>::max()); std::copy(mask.begin(), mask.end(), view.uuid_mask().BackingStorage().data()); + PW_CHECK(view.Ok()); return packet; } @@ -652,6 +659,7 @@ view.uuid_mask().BackingStorage().WriteLittleEndianUInt<16>( std::numeric_limits<uint16_t>::max()); + PW_CHECK(view.Ok()); return packet; } @@ -676,6 +684,7 @@ view.uuid_mask().BackingStorage().WriteLittleEndianUInt<32>( std::numeric_limits<uint32_t>::max()); + PW_CHECK(view.Ok()); return packet; } @@ -698,6 +707,7 @@ mask.fill(std::numeric_limits<uint8_t>::max()); std::copy(mask.begin(), mask.end(), view.uuid_mask().BackingStorage().data()); + PW_CHECK(view.Ok()); return packet; } @@ -759,6 +769,7 @@ view.service_data_mask().BackingStorage().WriteLittleEndianUInt<16>( std::numeric_limits<uint16_t>::max()); + PW_CHECK(view.Ok()); return packet; } @@ -786,6 +797,7 @@ view.service_data_mask().BackingStorage().WriteLittleEndianUInt<32>( std::numeric_limits<uint32_t>::max()); + PW_CHECK(view.Ok()); return packet; } @@ -813,6 +825,7 @@ mask.end(), view.service_data_mask().BackingStorage().data()); + PW_CHECK(view.Ok()); return packet; } @@ -850,10 +863,20 @@ return packets; } -CommandPacket AdvertisingPacketFilter::BuildSetLocalNameCommand( +std::optional<CommandPacket> AdvertisingPacketFilter::BuildSetLocalNameCommand( FilterIndex filter_index, const std::string& local_name) const { size_t packet_size = android_emb::LEApcfLocalNameCommand::MinSizeInBytes() + local_name.size(); + if (local_name.size() > + android_emb::LEApcfLocalNameCommand::max_local_name_size()) { + bt_log(WARN, + "hci", + "Invalid advertising filter local name (%zuB) would overflow the " + "maximum allowed local name (%dB) ", + local_name.size(), + android_emb::LEApcfLocalNameCommand::max_local_name_size()); + return std::nullopt; + } auto packet = hci::CommandPacket::New<android_emb::LEApcfLocalNameCommandWriter>( android_hci::kLEApcf, packet_size); @@ -863,6 +886,8 @@ android_hci::kLEApcfLocalNameSubopcode); view.filter_index().Write(filter_index); + PW_CHECK(view.Ok()); + std::copy(local_name.begin(), local_name.end(), view.local_name().BackingStorage().begin()); @@ -889,6 +914,7 @@ view.manufacturer_data_mask().BackingStorage().WriteLittleEndianUInt<16>( std::numeric_limits<uint16_t>::max()); + PW_CHECK(view.Ok()); return packet; }
diff --git a/pw_bluetooth_sapphire/host/hci/advertising_packet_filter_test.cc b/pw_bluetooth_sapphire/host/hci/advertising_packet_filter_test.cc index db4f21e..2974ec4 100644 --- a/pw_bluetooth_sapphire/host/hci/advertising_packet_filter_test.cc +++ b/pw_bluetooth_sapphire/host/hci/advertising_packet_filter_test.cc
@@ -807,4 +807,22 @@ EXPECT_TRUE(second_cb_called); EXPECT_FALSE(packet_filter.IsUsingOffloadedFiltering()); } + +TEST_F(AdvertisingPacketFilterTest, NameSubstringOverflow) { + AdvertisingPacketFilter packet_filter( + {/*offloading_supported=*/true, + /*max_filters=*/1, + /*peer_delivery_mode=*/ + AdvertisingPacketFilter::Config::DeliveryMode::kImmediate}, + transport()->GetWeakPtr()); + + DiscoveryFilter filter; + std::string long_name(50, 'A'); + filter.set_name_substring(long_name); + packet_filter.SetPacketFilters(0, {filter}); + RunUntilIdle(); + + EXPECT_FALSE(packet_filter.IsUsingOffloadedFiltering()); +} + } // namespace bt::hci
diff --git a/pw_bluetooth_sapphire/host/hci/public/pw_bluetooth_sapphire/internal/host/hci/advertising_packet_filter.h b/pw_bluetooth_sapphire/host/hci/public/pw_bluetooth_sapphire/internal/host/hci/advertising_packet_filter.h index 656c2f9..024bf38 100644 --- a/pw_bluetooth_sapphire/host/hci/public/pw_bluetooth_sapphire/internal/host/hci/advertising_packet_filter.h +++ b/pw_bluetooth_sapphire/host/hci/public/pw_bluetooth_sapphire/internal/host/hci/advertising_packet_filter.h
@@ -218,8 +218,8 @@ std::vector<CommandPacket> BuildSetServiceDataUUIDCommands( FilterIndex filter_index, const std::vector<UUID>& uuids) const; - CommandPacket BuildSetLocalNameCommand(FilterIndex filter_index, - const std::string& local_name) const; + std::optional<CommandPacket> BuildSetLocalNameCommand( + FilterIndex filter_index, const std::string& local_name) const; CommandPacket BuildSetManufacturerCodeCommand( FilterIndex filter_index, uint16_t manufacturer_code) const;
diff --git a/pw_bluetooth_sapphire/host/transport/control_packets.cc b/pw_bluetooth_sapphire/host/transport/control_packets.cc index 2a74cd7..5f746cc 100644 --- a/pw_bluetooth_sapphire/host/transport/control_packets.cc +++ b/pw_bluetooth_sapphire/host/transport/control_packets.cc
@@ -17,6 +17,8 @@ #include <pw_assert/check.h> #include <pw_bluetooth/hci_android.emb.h> +#include <limits> + #include "pw_allocator/allocator.h" #include "pw_bluetooth_sapphire/internal/host/hci-spec/vendor_protocol.h" @@ -35,9 +37,11 @@ "command packet size must be at least 3 bytes to accommodate header"); auto header = view<pw::bluetooth::emboss::CommandHeaderWriter>(); header.opcode().Write(opcode); - header.parameter_total_size().Write( + size_t parameter_total_size = packet_size - - pw::bluetooth::emboss::CommandHeader::IntrinsicSizeInBytes()); + pw::bluetooth::emboss::CommandHeader::IntrinsicSizeInBytes(); + PW_CHECK(parameter_total_size <= std::numeric_limits<uint8_t>::max()); + header.parameter_total_size().Write(parameter_total_size); } CommandPacket::CommandPacket(hci_spec::OpCode opcode, @@ -49,9 +53,11 @@ "command packet size must be at least 3 bytes to accomodate header"); auto header = view<pw::bluetooth::emboss::CommandHeaderWriter>(); header.opcode_bits().BackingStorage().WriteUInt(opcode); - header.parameter_total_size().Write( + size_t parameter_total_size = packet_size - - pw::bluetooth::emboss::CommandHeader::IntrinsicSizeInBytes()); + pw::bluetooth::emboss::CommandHeader::IntrinsicSizeInBytes(); + PW_CHECK(parameter_total_size <= std::numeric_limits<uint8_t>::max()); + header.parameter_total_size().Write(parameter_total_size); } hci_spec::OpCode CommandPacket::opcode() const {