Re: [BlueZ 6/8] shared/gatt-db: Fix possible buffer overrun

[Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>]

Hi Bastien,

On Mon, Aug 5, 2024 at 10:09 AM Bastien Nocera <hadess@xxxxxxxxxx> wrote:
>
> uuid_to_le() returns one of the possible values from bt_uuid_len().
> bt_uuid_len() returns "type / 8".
> type is a value between 0 and 128, but could be something else
> depending on the validity of the UUID that's parsed. So an invalid
> value of type between 128 and 256 would trigger an overrun.
>
> Add a check to make sure that an invalid type isn't used to calculate
> the length.
>
> Error: OVERRUN (CWE-119): [#def6] [important]
> bluez-5.77/src/shared/gatt-db.c:612:2: assignment: Assigning: "len" = "uuid_to_le(uuid, value)". The value of "len" is now between 0 and 31 (inclusive).
> bluez-5.77/src/shared/gatt-db.c:614:2: overrun-buffer-arg: Overrunning array "value" of 16 bytes by passing it to a function which accesses it at byte offset 30 using argument "len" (which evaluates to 31).
> 612|    len = uuid_to_le(uuid, value);
> 613|
> 614|->  service->attributes[0] = new_attribute(service, handle, type, value,
> 615|                                                                    len);
> 616|    if (!service->attributes[0]) {
> ---
>  src/shared/gatt-db.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> index b35763410d17..cd0eba6bf1d0 100644
> --- a/src/shared/gatt-db.c
> +++ b/src/shared/gatt-db.c
> @@ -560,9 +560,14 @@ static int uuid_to_le(const bt_uuid_t *uuid, uint8_t *dst)
>                 return bt_uuid_len(uuid);
>         }
>
> -       bt_uuid_to_uuid128(uuid, &uuid128);
> -       bswap_128(&uuid128.value.u128, dst);
> -       return bt_uuid_len(&uuid128);
> +       if (uuid->type == BT_UUID32 ||

BT_UUID32 is not really valid in LE, so Id leave this check to be:

switch (uuid->type) {
case BT_UUID16:
        put_le16(uuid->value.u16, dst);
        return bt_uuid_len(uuid);
case BT_UUID128:
         bt_uuid_to_uuid128(uuid, &uuid128);
         bswap_128(&uuid128.value.u128, dst);
         return bt_uuid_len(&uuid128);
}

return 0;

We do however have bt_uuid_to_le with the only difference being that
it is more generic and it doesn't return the actual bytes written to
the dst, anyway we could replace the code above with:

diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index b35763410d17..71976de48569 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -553,16 +553,9 @@ bool gatt_db_isempty(struct gatt_db *db)

 static int uuid_to_le(const bt_uuid_t *uuid, uint8_t *dst)
 {
-       bt_uuid_t uuid128;
+       bt_uuid_to_le(uuid, dst);

-       if (uuid->type == BT_UUID16) {
-               put_le16(uuid->value.u16, dst);
-               return bt_uuid_len(uuid);
-       }
-
-       bt_uuid_to_uuid128(uuid, &uuid128);
-       bswap_128(&uuid128.value.u128, dst);
-       return bt_uuid_len(&uuid128);
+       return bt_uuid_len(uuid) == 4 ? 16 : bt_uuid_len(uuid);
 }


> +           uuid->type == BT_UUID128) {
> +               bt_uuid_to_uuid128(uuid, &uuid128);
> +               bswap_128(&uuid128.value.u128, dst);
> +               return bt_uuid_len(&uuid128);
> +       }
> +
> +       return 0;
>  }
>
>  static bool le_to_uuid(const uint8_t *src, size_t len, bt_uuid_t *uuid)
> --
> 2.45.2
>
>


-- 
Luiz Augusto von Dentz