ble_hs_adv_find_field() may dereference invalid memory for malformed advertising data

[kscheff]

mynewt-nimble/nimble/host/src/ble_hs_adv.c

Line 755 in 3bb2671

ble_hs_adv_parse(const uint8_t *data, uint8_t length,

The function ble_hs_adv_find_field() directly casts a buffer offset to struct ble_hs_adv_field * before verifying that the pointer is within bounds. This can lead to a crash (e.g., Load access fault) if the advertising data is malformed or truncated.

[sjanc]

Hi,

There is both min length check (in while loop) so that ->length can be check, and then later per fields length is validated.
I don't see any issues with accessing past buffer length. Could you clarify on where the issue is?

[kscheff]

The function calls ble_hs_adv_parse() to extract the fields.

The ble_hs_adv_parse() function in its current form may cause undefined behavior or crash due to unsafe dereferencing and insufficient bounds checking when parsing advertising data.

while (length > 1) {
    field = (const void *) data;

    if (field->length >= length) {
        return BLE_HS_EBADDATA;
    }

    if (func(field, user_data) == 0) {
        return 0;
    }

    length -= 1 + field->length;
    data += 1 + field->length;
}

Issues Identified

1. 🚫 Unsafe access to field->length before validating buffer size
   • field = (const void *) data; is cast before ensuring that data is at least 1 byte.
   • Accessing field->length is unsafe if length < 1, potentially causing:
   • Buffer over-read
   • Unaligned memory access
   • Segmentation fault on platforms with strict alignment rules

2. ❌ Incorrect bounds check

if (field->length >= length)

This check is incorrect. It should validate:

if (1 + field->length > length)

Why? Because field->length includes only the type+value portion, but the total size of the field includes the length byte too (1 + field->length).

Without this fix, pointer arithmetic later (data += 1 + field->length) may advance beyond the input buffer → undefined behavior in the next iteration.

3. ⚠️ Assumes minimum field size implicitly
   • BLE advertising fields must be at least 2 bytes: [Length][Type]
   • While the loop guard is while (length > 1), the function doesn’t verify that there’s enough data to process a complete field before accessing type or value.

Recommended Fix

Use explicit bounds checks and avoid casting data to a struct before validating:

int ble_hs_adv_parse(const uint8_t *data, uint8_t length,
                     ble_hs_adv_parse_func_t func, void *user_data)
{
    while (length > 0) {
        uint8_t field_len;

        // Need at least 1 byte to read length
        if (length < 1) {
            return BLE_HS_EBADDATA;
        }

        field_len = data[0]; // Safe now

        // Validate full field fits in remaining buffer
        if ((uint8_t)(field_len + 1) > length) {
            return BLE_HS_EBADDATA;
        }

        const struct ble_hs_adv_field *field = (const void *) data;

        if (func(field, user_data) == 0) {
            return 0;
        }

        data += field_len + 1;
        length -= field_len + 1;
    }

    return 0;
}

[sjanc]

Hi,

ad 1) this whole block is in while (length > 1) so accessing length (and type) is always safe
ad 2) same as above, next iteration is protected by while loop condition
ad 3) pretty much the same, both length and type are verified before access, not sure where this claim come from

Only issue I see in current implementation is that it would simply ignore leftover last byte without validating if this is terminator (ie zero), but those wouldn't be valid LTV anyway. Stil, no invalid memory access here.

regarding recommended fix: this "fix" would introduce invalid memory access if field_len==255..., and checking for length < 1 inside while (length > 0) is dead code

PS
please cross-check your chatbot output before posting

[kscheff]

I am sorry to mislead you. I have just ran the scanning again and the crash occurs at a different location when calling this:

static inline int adv_find_field(uint8_t type, const uint8_t *data, uint8_t length,
                                        const struct ble_hs_adv_field **out) {
    if (!data || !out || length == 0 || length > 31) {
        return -1;
    }

    return ble_hs_adv_find_field(type, data, length, out);
}

static int
find_field_func(const struct ble_hs_adv_field *field, void *user_data)
{
    struct find_field_data *ffd = user_data;

    if (field->type != ffd->type) {  // 🧨 here we crash
        return BLE_HS_EAGAIN;
    }

    ffd->field = field;

    return 0;
}

Here is the panic:

I (12:35:14.986) ble_app_on_sync: BLE scanning started.
I (12:35:15.022) bluebattery_sht4x_init: SHT4x serial_number: 0xd50444a at Rearside
I (12:35:15.023) wifi_init_sta: wifi_init_sta: use_sta_mode: 1, enabled: 1, was_enabled_but_disconnected: 0
I (12:35:15.026) pp: pp rom version: 9387209
I (12:35:15.028) net80211: net80211 rom version: 9387209
Guru Meditation Error: Core  0 panic'ed (Load access fault). Exception was unhandled.

Core  0 register dump:
MEPC    : 0x42005e12  RA      : 0x42005e0a  SP      : 0x3fcae0a0  GP      : 0x3fc9be00  
  #0  0x42005e12 in check_for_target_services at src/ble_scan.c:269 (discriminator 2)

TP      : 0x3fcae180  T0      : 0x00815448  T1      : 0x00000000  T2      : 0xe6883d70  
S0/FP   : 0x3c156978  S1      : 0x00000000  A0      : 0xffffffff  A1      : 0x3fcae078  
  #0  0x3c156978 in ?? at src/blue_level.c:23

A2      : 0x42024876  A3      : 0x3fcae078  A4      : 0x000000ff  A5      : 0x00000000  
  #0  0x42024876 in find_field_func at /somewhere/.platformio/packages/framework-espidf/components/bt/host/nimble/nimble/nimble/host/src/ble_hs_adv.c:965

Then I enriched the debug log to show the data which lead to the crash:

I (00:02:56.264) BLE SCAN: adv_find_field type 21, length 27
I (00:02:56.265) adv_find_field: 02 01 06 11 07 0e 61 6a 44 46 6b 20 af 9f 43 0d
I (00:02:56.266) adv_find_field: 78 74 14 02 aa 05 12 06 00 10 00
I (00:02:56.275) BLE SCAN: adv_find_field type 60, length 27
I (00:02:56.276) adv_find_field: 02 01 06 11 07 0e 61 6a 44 46 6b 20 af 9f 43 0d
I (00:02:56.276) adv_find_field: 78 74 14 02 aa 05 12 06 00 10 00
I (00:02:56.277) BLE SCAN: adv_find_field type 1, length 27
I (00:02:56.277) adv_find_field: 02 01 06 11 07 0e 61 6a 44 46 6b 20 af 9f 43 0d
I (00:02:56.278) adv_find_field: 78 74 14 02 aa 05 12 06 00 10 00
I (00:02:56.279) BLE SCAN: adv_find_field type 255, length 27
I (00:02:56.280) adv_find_field: 02 01 06 11 07 0e 61 6a 44 46 6b 20 af 9f 43 0d
I (00:02:56.280) adv_find_field: 78 74 14 02 aa 05 12 06 00 10 00
I (00:02:56.281) BLE SCAN: adv_find_field type 7, length 18
I (00:02:56.282) adv_find_field: 11 ff 33 5b 00 00 01 00 00 00 00 00 00 00 ff fe
I (00:02:56.282) adv_find_field: 64 00
Guru Meditation Error: Core  0 panic'ed (Load access fault). Exception was unhandled.

I then replace the function with this version to fix the issue:

// replacement of ble_hs_adv_find_field() with a more robust version 
/// @brief Find a field in advertising data by type
/// @param type The type of the field to find.
/// @param data Pointer to the advertising data.
/// @param length Length of the advertising data.
/// @param out Pointer to store the found field.
/// @return The offset of the found field, or a negative error code.
///         -1 if data is NULL or out is NULL or length is 0 or length > 31.
///         -2 if the length field is invalid.
///         -3 if there is an overflow in the data.
///         -4 if the field is not found.
///         The field is stored in the out pointer if found.
static inline int adv_find_field(uint8_t type, const uint8_t *data, uint8_t length,
                                        const struct ble_hs_adv_field **out) {
    if (!data || !out || length == 0 || length > 31) {
        return -1;
    }

//    return ble_hs_adv_find_field(type, data, length, out);

    uint8_t offset = 0;
    while (offset < length) {
        if ((offset + 1) > length) {
            return -2; // invalid length field
        }

        uint8_t field_len = data[offset];
        if (field_len == 0) {
            break; // end of fields
        }

        if ((offset + field_len) >= length) {
            return -3; // overflow
        }

        const struct ble_hs_adv_field *field = (const struct ble_hs_adv_field *)(data + offset);
        if (field->type == type) {
            *out = field;
            return offset;
        }

        offset += field_len + 1;
    }

    return -4; // not found
}

[sjanc]

Hi,

I really fail to see on how 11 ff 33 5b 00 00 01 00 00 00 00 00 00 00 ff fe 64 00 test data would result in invalid memory access here. It will skip parsing after first iteration in ble_hs_adv_parse() while loop since remaining length will be zero and thus loop stops, ffd.field remains NULL and ble_hs_adv_find_field() returns BLE_HS_ENOENT.

However, looking at adv_find_field() wrapper you provided around ble_hs_adv_find_field(), please note that ble_hs_adv_find_field() return 0 on success and BLE_HS_ENOENT (value 5) on error.

This is not on how you seem to use that in your wrapper where you assume error is negative value,

[kscheff]

Point is that it results in a panic - there is no discussion.

The field is a 2 byte value. It seems to crash when the length is an odd number. So when accessing the last byte it crashes due to the field access.

Thanks for pointing out the difference in my implementation (however that return code it's not used in my app).

[sjanc]

Could you show how you call this API in your application?