Bluetooth: GATT: Check len of response before parsing response PDU #94922
Conversation
zephyrbot
added
area: Bluetooth
area: Bluetooth Host
zephyrbot
requested review from
alwa-nordic,
cvinayak,
HaavardRei,
hermabe,
jhedberg,
PavelVPV,
rugeGerritsen,
sjanc and
Thalley
| @@ -4761,6 +4761,15 @@ static void parse_read_by_uuid(struct bt_conn *conn, | |||
| uint16_t handle; | |||
| uint16_t len; | |||
|
|
|||
| len = MIN(rsp->len, length); | |||
There was a problem hiding this comment.
Why the MIN here?
Don't we need to modify the
for (length--, pdu = rsp->data; length;
length -= rsp->len, pdu = (const uint8_t *)pdu + rsp->len) {as well? This assumes that length -= rsp->len is always valid, but if length = 10 and rsp->len = 13 (invalid response length), then that will still be an invalid operation, right?
There was a problem hiding this comment.
Why the
MINhere?
The original equation is len = rsp->len > length ? length - 2 : rsp->len - 2;.
It is equal to len = MIN(rsp->len, length) - 2;.
This assumes that
length -= rsp->lenis always valid, but iflength = 10andrsp->len = 13(invalid response length), then that will still be an invalid operation, right?
It will be checked here,
zephyr/subsys/bluetooth/host/gatt.c
Lines 4808 to 4811 in 617b71b
lylezhu2012
force-pushed
the
Fix_gatt_coverity_issue
branch
from
fb96b71 to
b85c500
Compare
subsys/bluetooth/host/gatt.c
Outdated
| len = MIN(rsp->len, length); | ||
| if (len < sizeof(struct bt_att_data)) { | ||
| LOG_WRN("Bad peer: ATT read-by-uuid rsp: invalid ATTR data len %u", len); | ||
| params->func(conn, BT_ATT_ERR_INVALID_ATTRIBUTE_LEN, params, NULL, 0); |
There was a problem hiding this comment.
Why not BT_ATT_ERR_INVALID_PDU? Doesn't this mean that ATT PDU is malformed?
It is even a question if we are allowed to use response error codes here as it may look like server's response error. But doesn't seem that spec defines behavior for such case unless I'm missing something.
There was a problem hiding this comment.
Why not
BT_ATT_ERR_INVALID_PDU? Doesn't this mean that ATT PDU is malformed?
Is it possible that the peer device is malfunctioning, or is it a malicious device? The peer device does this intentionally.
There was a problem hiding this comment.
BT_ATT_ERR_INVALID_PDU might actually be a better error code, as it indeed an invalid PDU
It is even a question if we are allowed to use response error codes here as it may look like server's response error. But doesn't seem that spec defines behavior for such case unless I'm missing something.
I tend to agree with this, but it also seems like something we do quite a lot in Zephyr. If the err parameter of the callback was a int instead of uint8_t we could have defined it as 0 => success, > 0 => ATT error, < 0 => local error, but that'd be an API change
There was a problem hiding this comment.
Regardless of peer's intention, the ATT PDU is not correctly formed. struct bt_att_data is 2 bytes long (added) representing only 16-bit handle. If the response has 1 byte left (half of ATT opcode attribute handle), is this invalid attribute length or PDU?
There was a problem hiding this comment.
Corrected myself: ATT opcode -> attribute handle.
There was a problem hiding this comment.
Given the definitions in the Core spec
I'd argue that "Invalid PDU" matches this case the best. I think the latter works better if you are doing a write request with invalid length
There was a problem hiding this comment.
Changed BT_ATT_ERR_INVALID_ATTRIBUTE_LEN to BT_ATT_ERR_INVALID_PDU.
lylezhu2012
force-pushed
the
Fix_gatt_coverity_issue
branch
2 times, most recently
from
32ac770 to
419a543
Compare
include/zephyr/bluetooth/gatt.h
Outdated
| * operation, so this can fail to allocate resources. However, if the received | ||
| * data length is invalid, the error @ref BT_ATT_ERR_INVALID_PDU will be | ||
| * returned. For all other API errors are reported as if the server returned | ||
| * @ref BT_ATT_ERR_UNLIKELY. There is no way to distinguish between this | ||
| * condition and a @ref BT_ATT_ERR_UNLIKELY response from the server itself. |
There was a problem hiding this comment.
Putting the new documentation here in between the sentences is not right, since this change has nothing to do with the callback returning BT_GATT_ITER_CONTINUE. Let's separate it out to its own paragraph to maintain clarity.
We might also instead put the documentation on bt_gatt_read_func_t.
In function `parse_read_by_uuid()`, the response length is not checked before parsing the response PDU. There is a potential issue that the `len` will be underflowed if the `len` is less than the size of `struct bt_att_data`. Check the length before parsing the response PDU. If the length is less then the size of `struct bt_att_data`, notify the upper layer with the error `BT_ATT_ERR_INVALID_PDU` and stop the parsing. Signed-off-by: Lyle Zhu <[email protected]>
lylezhu2012
force-pushed
the
Fix_gatt_coverity_issue
branch
from
419a543 to
14c8460
Compare
|
| * If the received data length is invalid, the callback @p params->func will | ||
| * called with the error @ref BT_ATT_ERR_INVALID_PDU. |
There was a problem hiding this comment.
Ideally I'd only put this in one place. Having the text duplicated in 2 places makes it harder to maintain. I'd perhaps just keep it in bt_gatt_read_func_t
In function
parse_read_by_uuid(), the response length is not checked before parsing the response PDU. There is a potential issue that thelenwill be underflowed if thelenis less than the size ofstruct bt_att_data.Check the length before parsing the response PDU. If the length is less then the size of
struct bt_att_data, notify the upper layer with the errorBT_ATT_ERR_INVALID_PDUand stop the parsing.