Hi Bastien,
On Wed, Aug 7, 2024 at 11:39 AM Bastien Nocera <hadess@xxxxxxxxxx> wrote:
>
> 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);
> }
Yep, we should check the return of bt_uuid_to_le.
> > }
> >
> >
> > > + 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
