[bt][sdp] Fix buffer size check, max response size
Calculate the size of the response buffers that are being parsed in
ServiceSearchResponse, ServiceAttributeResponse, and
ServiceSearchAttributeResponse correctly before parsing the results.
Fix a problem where continuing to parse continuation packets for
ServiceAttributeResponse and ServiceSearchAttributeResponse could have
an unbounded memory allocation.
Fixed: 50628
Fixed: 50627
Test: bt-host-unittests, updated SDP parsing tests.
Multiply: bt-host-unittests:30
Change-Id: Iaa36d69620f2374de3549217b43fc43a8404e0e6
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/384686
Reviewed-by: Ani Ramakrishnan <aniramakri@google.com>
Testability-Review: Ani Ramakrishnan <aniramakri@google.com>
Commit-Queue: Marie Janssen 💖 <jamuraa@google.com>
diff --git a/src/connectivity/bluetooth/core/bt-host/sdp/pdu.cc b/src/connectivity/bluetooth/core/bt-host/sdp/pdu.cc
index 4abd59d..d035c3a 100644
--- a/src/connectivity/bluetooth/core/bt-host/sdp/pdu.cc
+++ b/src/connectivity/bluetooth/core/bt-host/sdp/pdu.cc
@@ -26,6 +26,11 @@
// Spec v5.0, Vol 3, Part B, Sec 4.5.1
constexpr size_t kMaxServiceSearchSize = 12;
+// The maximum amount of Attribute list data we will store when parsing a response to
+// ServiceAttribute or ServiceSearchAttribute responses.
+// 640kb ought to be enough for anybody.
+constexpr size_t kMaxSupportedAttributeListBytes = 655360;
+
// Validates continuation state in |buf|, which should be the configuration
// state bytes of a PDU.
// Returns true if the continuation state is valid here, false otherwise.
@@ -306,20 +311,26 @@
uint16_t record_count = betoh16(buf.view(read_size).As<uint16_t>());
read_size += sizeof(uint16_t);
- if ((buf.size() - read_size - sizeof(uint8_t)) < (sizeof(ServiceHandle) * record_count)) {
- bt_log(SPEW, "sdp", "Packet too small for %d records", record_count);
+ size_t expected_record_bytes = sizeof(ServiceHandle) * record_count;
+ if (buf.size() < (read_size + expected_record_bytes)) {
+ bt_log(SPEW, "sdp", "Packet too small for %d records: %zu", record_count, buf.size());
return Status(HostError::kPacketMalformed);
}
+ BufferView cont_state_view;
+ if (!ValidContinuationState(buf.view(read_size + expected_record_bytes), &cont_state_view)) {
+ bt_log(SPEW, "sdp", "Failed to find continuation state");
+ return Status(HostError::kPacketMalformed);
+ }
+ size_t expected_size = read_size + expected_record_bytes + cont_state_view.size() + sizeof(uint8_t);
+ if (expected_size != buf.size()) {
+ bt_log(SPEW, "sdp", "Packet should be %zu not %zu", expected_size, buf.size());
+ return Status(HostError::kPacketMalformed);
+ }
+
for (uint16_t i = 0; i < record_count; i++) {
auto view = buf.view(read_size + i * sizeof(ServiceHandle));
service_record_handle_list_.emplace_back(betoh32(view.As<uint32_t>()));
}
- read_size += sizeof(ServiceHandle) * record_count;
- BufferView cont_state_view;
- if (!ValidContinuationState(buf.view(read_size), &cont_state_view)) {
- bt_log(SPEW, "sdp", "Failed to find continuation state");
- return Status(HostError::kPacketMalformed);
- }
if (cont_state_view.size() == 0) {
continuation_state_ = nullptr;
} else {
@@ -525,9 +536,9 @@
return Status(HostError::kPacketMalformed);
}
- uint16_t attribute_list_byte_count = betoh16(buf.As<uint16_t>());
+ uint32_t attribute_list_byte_count = betoh16(buf.As<uint16_t>());
size_t read_size = sizeof(uint16_t);
- if (buf.view(read_size).size() < attribute_list_byte_count + sizeof(uint8_t)) {
+ if (buf.size() < read_size + attribute_list_byte_count + sizeof(uint8_t)) {
bt_log(SPEW, "sdp", "Not enough bytes in rest of packet");
return Status(HostError::kPacketMalformed);
}
@@ -545,6 +556,12 @@
continuation_state_->Write(cont_state_view);
}
+ size_t expected_size = read_size + attribute_list_byte_count + cont_state_view.size() + sizeof(uint8_t);
+ if (buf.size() != expected_size) {
+ bt_log(SPEW, "sdp", "Packet should be %zu not %zu", expected_size, buf.size());
+ return Status(HostError::kPacketMalformed);
+ }
+
auto attribute_list_bytes = buf.view(read_size, attribute_list_byte_count);
if (partial_response_ || ContinuationState().size()) {
// Append to the incomplete buffer.
@@ -552,6 +569,13 @@
if (partial_response_) {
new_partial_size += partial_response_->size();
}
+ // We currently don't support more than approx 10 packets of the max size.
+ if (new_partial_size > kMaxSupportedAttributeListBytes) {
+ bt_log(INFO, "sdp", "ServiceAttributeResponse exceeds supported size (%zu), dropping", new_partial_size);
+ partial_response_ = nullptr;
+ return Status(HostError::kNotSupported);
+ }
+
auto new_partial = NewSlabBuffer(new_partial_size);
if (partial_response_) {
new_partial->Write(partial_response_->view());
@@ -870,6 +894,13 @@
if (partial_response_) {
new_partial_size += partial_response_->size();
}
+ // We currently don't support more than approx 10 packets of the max size.
+ if (new_partial_size > kMaxSupportedAttributeListBytes) {
+ bt_log(INFO, "sdp", "ServiceSearchAttributeResponse exceeds supported size, dropping");
+ partial_response_ = nullptr;
+ return Status(HostError::kNotSupported);
+ }
+
auto new_partial = NewSlabBuffer(new_partial_size);
if (partial_response_) {
new_partial->Write(partial_response_->view());
diff --git a/src/connectivity/bluetooth/core/bt-host/sdp/pdu_unittest.cc b/src/connectivity/bluetooth/core/bt-host/sdp/pdu_unittest.cc
index 515433a8..2ccb8e8 100644
--- a/src/connectivity/bluetooth/core/bt-host/sdp/pdu_unittest.cc
+++ b/src/connectivity/bluetooth/core/bt-host/sdp/pdu_unittest.cc
@@ -192,6 +192,40 @@
ServiceSearchResponse resp2;
status = resp2.Parse(kNotEnoughRecords);
EXPECT_FALSE(status);
+
+ // A Truncated packet doesn't parse either.
+ const auto kTruncated =
+ CreateStaticByteBuffer(0x00, 0x02, // Total service record count: 2
+ 0x00, 0x02 // Current service record count: 2
+ );
+ ServiceSearchResponse resp3;
+ status = resp3.Parse(kTruncated);
+ EXPECT_FALSE(status);
+
+ // Too many bytes for the number of records is also not allowed (with or without a continuation
+ // state)
+ const auto kTooLong =
+ CreateStaticByteBuffer(0x00, 0x01, // Total service record count: 1
+ 0x00, 0x01, // Current service record count: 1
+ 0x00, 0x00, 0x00, 0x01, // Service Handle 1
+ 0x00, 0x00, 0x00, 0x02, // Service Handle 2
+ 0x00 // No continuation state
+ );
+ ServiceSearchResponse resp4;
+ status = resp4.Parse(kTooLong);
+ EXPECT_FALSE(status);
+
+ const auto kTooLongWithContinuation =
+ CreateStaticByteBuffer(0x00, 0x04, // Total service record count: 1
+ 0x00, 0x01, // Current service record count: 1
+ 0x00, 0x00, 0x00, 0x01, // Service Handle 1
+ 0x00, 0x00, 0x00, 0x02, // Service Handle 2
+ 0x04, // Continuation state (len: 4)
+ 0xF0, 0x9F, 0x92, 0x96 // Continuation state
+ );
+ ServiceSearchResponse resp5;
+ status = resp5.Parse(kTooLongWithContinuation);
+ EXPECT_FALSE(status);
};
TEST_F(SDP_PDUTest, ServiceSearchResponsePDU) {
@@ -577,16 +611,17 @@
0x00 // No continuation state
);
- status = resp.Parse(kValidResponseItems);
+ ServiceAttributeResponse resp2;
+ status = resp2.Parse(kValidResponseItems);
EXPECT_TRUE(status);
- EXPECT_EQ(2u, resp.attributes().size());
- auto it = resp.attributes().find(0x00);
- EXPECT_NE(resp.attributes().end(), it);
+ EXPECT_EQ(2u, resp2.attributes().size());
+ auto it = resp2.attributes().find(0x00);
+ EXPECT_NE(resp2.attributes().end(), it);
EXPECT_EQ(DataElement::Type::kUnsignedInt, it->second.type());
- it = resp.attributes().find(0x01);
- EXPECT_NE(resp.attributes().end(), it);
+ it = resp2.attributes().find(0x01);
+ EXPECT_NE(resp2.attributes().end(), it);
EXPECT_EQ(DataElement::Type::kSequence, it->second.type());
const auto kInvalidItemsWrongOrder = CreateStaticByteBuffer(
@@ -600,13 +635,14 @@
0x00 // No continuation state
);
- status = resp.Parse(kInvalidItemsWrongOrder);
+ ServiceAttributeResponse resp3;
+ status = resp3.Parse(kInvalidItemsWrongOrder);
EXPECT_FALSE(status);
const auto kInvalidByteCount = CreateStaticByteBuffer(
0x00, 0x12, // AttributeListByteCount (18 bytes)
- // Attribute List
+ // Attribute List (only 17 bytes long)
0x35, 0x0F, // Sequence uint8 15 bytes
0x09, 0x00, 0x01, // Handle: uint16_t (1)
0x35, 0x03, 0x19, 0x11, 0x01, // Element: Sequence (3) { UUID(0x1101) }
@@ -615,9 +651,29 @@
0x00 // No continuation state
);
- status = resp.Parse(kInvalidByteCount);
-
+ ServiceAttributeResponse resp4;
+ status = resp4.Parse(kInvalidByteCount);
EXPECT_FALSE(status);
+ EXPECT_EQ(HostError::kPacketMalformed, status.error());
+
+ // Sending too much data eventually fails.
+ auto kContinuationPacket = DynamicByteBuffer(4096);
+ // Put the correct byte count at the front: 4096 - 2 (byte count) - 5 (continuation size)
+ const auto kAttributeListByteCount = CreateStaticByteBuffer(0x0F, 0xF9);
+ kContinuationPacket.Write(kAttributeListByteCount, 0);
+ // Put the correct continuation at the end.
+ const auto kContinuation = CreateStaticByteBuffer(0x04, 0xF0, 0x9F, 0x92, 0x96);
+ kContinuationPacket.Write(kContinuation, kContinuationPacket.size() - 5);
+
+ ServiceAttributeResponse resp5;
+ status = resp5.Parse(kContinuationPacket);
+ EXPECT_FALSE(status);
+ EXPECT_EQ(HostError::kInProgress, status.error());
+ while (!status && status.error() == HostError::kInProgress) {
+ status = resp5.Parse(kContinuationPacket);
+ }
+ EXPECT_FALSE(status);
+ EXPECT_EQ(HostError::kNotSupported, status.error());
}
TEST_F(SDP_PDUTest, ServiceAttributeResponseGetPDU_MaxSize) {
@@ -792,7 +848,7 @@
0x19, 0x30, 0x01, // 13 UUIDs in the search
0x19, 0x30, 0x02, 0x19, 0x30, 0x03, 0x19, 0x30, 0x04, 0x19, 0x30, 0x05, 0x19, 0x30, 0x06,
0x19, 0x30, 0x07, 0x19, 0x30, 0x08, 0x19, 0x30, 0x09, 0x19, 0x30, 0x10, 0x19, 0x30, 0x11,
- 0x19, 0x30, 0x12, 0x19, 0x30, 0x13, 0xF0, 0x0F, // Maximum attribute byte coutn (61455)
+ 0x19, 0x30, 0x12, 0x19, 0x30, 0x13, 0xF0, 0x0F, // Maximum attribute byte count (61455)
0x35, 0x03, // Sequence uint8 3 bytes
0x09, 0x00, 0x02, // uint16_t (2)
0x00 // No continuation state
@@ -952,6 +1008,25 @@
status = resp3.Parse(kInvalidByteCount);
EXPECT_FALSE(status);
+
+ // Sending too much data eventually fails.
+ auto kContinuationPacket = DynamicByteBuffer(4096);
+ // Put the correct byte count at the front: 4096 - 2 (byte count) - 5 (continuation size)
+ const auto kAttributeListByteCount = CreateStaticByteBuffer(0x0F, 0xF9);
+ kContinuationPacket.Write(kAttributeListByteCount, 0);
+ // Put the correct continuation at the end.
+ const auto kContinuation = CreateStaticByteBuffer(0x04, 0xF0, 0x9F, 0x92, 0x96);
+ kContinuationPacket.Write(kContinuation, kContinuationPacket.size() - 5);
+
+ ServiceSearchAttributeResponse resp4;
+ status = resp4.Parse(kContinuationPacket);
+ EXPECT_FALSE(status);
+ EXPECT_EQ(HostError::kInProgress, status.error());
+ while (!status && status.error() == HostError::kInProgress) {
+ status = resp4.Parse(kContinuationPacket);
+ }
+ EXPECT_FALSE(status);
+ EXPECT_EQ(HostError::kNotSupported, status.error());
}
TEST_F(SDP_PDUTest, ServiceSearchAttributeResponseGetPDU) {
@@ -1098,6 +1173,7 @@
EXPECT_FALSE(buf);
}
+
} // namespace
} // namespace sdp
} // namespace bt
