Potential buffer out-of-bound read in gatt-database.c:cli_feat_read_cb

[zxtwonder]

bluez/src/gatt-database.c

Lines 1078 to 1079 in 35a2c50

	len = sizeof(state->cli_feat) - offset;
	value = len ? &state->cli_feat[offset] : NULL;

static void cli_feat_read_cb(struct gatt_db_attribute *attrib,
					unsigned int id, uint16_t offset,
					uint8_t opcode, struct bt_att *att,
					void *user_data)
{
...
	size_t len = 0;
...
	len = sizeof(state->cli_feat) - offset;
	value = len ? &state->cli_feat[offset] : NULL;

Both len and sizeof() are unsigned and offset is an external input without validation. It seems like a malicious GATT client can cause out-of-bound read on memory locations after the given device_state::cli_feat pointer.

Similar validation like below should be sufficient:

bluez/src/gatt-database.c

Line 691 in 35a2c50

if (offset > 2) {

[zxtwonder]

@Vudentz Thanks for the super quick update. One question on the change though - I only have a practical understanding on the BT spec, but should we only return BT_ATT_ERROR_INVALID_OFFSET when offset is strictly larger than sizeof(state->cli_feat) - to give the server a chance to indicate the end of the data?

	if (offset >= sizeof(state->cli_feat)) {
		ecode = BT_ATT_ERROR_INVALID_OFFSET;
		goto done;
	}

[Vudentz]

@Vudentz Thanks for the super quick update. One question on the change though - I only have a practical understanding on the BT spec, but should we only return BT_ATT_ERROR_INVALID_OFFSET when offset is strictly larger than sizeof(state->cli_feat) - to give the server a chance to indicate the end of the data?

	if (offset >= sizeof(state->cli_feat)) {
		ecode = BT_ATT_ERROR_INVALID_OFFSET;
		goto done;
	}

If the offset is == sizeof(state->cli_feat) it would be already past the end as the offset start at index 0 so valid offsets are in a range of 0 to size of attribute - 1.

[zxtwonder]

Based on the previous code, I was under the impression that the client needs a return of 0 length to be sure the read is complete.

	len = sizeof(state->cli_feat) - offset;
	value = len ? &state->cli_feat[offset] : NULL;

But after a further look, it seems that gatt-database does not handle offset consistently.

Sometimes, it allows for empty read:

static void gap_appearance_read_cb(struct gatt_db_attribute *attrib,
...
	uint8_t appearance[2];
...
	if (offset > 2) {

But other times not:

static void gatt_ccc_read_cb(struct gatt_db_attribute *attrib,
...
	const uint8_t *value = NULL;
...
	if (offset) {

I guess it is all corner case now since these are all short values. I will have to read the spec to see what the client should expect then.

[setharnold]

Hello, please use CVE-2021-3588 for this issue. Thanks.

[jefferyto]

This issue appears to have been closed by a test commit and was never actually fixed.

[Vudentz]

@jefferyto it has been pushed: https://git.kernel.org/pub/scm/bluetooth/bluez.git/commit/?id=3a40bef49305f8327635b81ac8be52a3ca063d5a

[jefferyto]

Apologies for the noise (the fix was also replaced in 6a50b6a).