On Tue, 2024-08-06 at 15:08 -0400, Luiz Augusto von Dentz wrote:
> 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);
Or this with error checking?
static int uuid_to_le(const bt_uuid_t *uuid, uint8_t *dst)
{
if (bt_uuid_to_le(uuid, dst) < 0)
return 0;
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
> >
> >
>
>
