nimble/host: characteristic value reading for notification/indication compromises security set-up

[vandy]

If a GATT server provides the characteristic which requires encryption (BLE_GATT_CHR_F_READ_ENC) than a client should connect and pair with the server to read the characteristic. But if notification (BLE_GATT_CHR_F_NOTIFY) or indication (BLE_GATT_CHR_F_INDICATE) is enabled for the characteristic than the client could connect and subscribe to notifications/indications getting characteristic value even if the connection is not encrypted. So the security of the characteristic is compromised.

As I see it there are 3 alternatives for the solution:

1. Created CCCD should also has READ_ENC, WRITE_ENC (same as characteristic itself), so that the client shall pair before proceeding if encryption required.
2. Check permissions during reading (which is the current implementation). Nimble stack should respect permissions during reading for notifications (now it doesn't check permissions).
3. Application developer should ensure that connection parameters are correct for each notification. He should keep track on what characteristics what permissions require before calling ble_gatts_notify. But it's kind of weird.

// service definitions
static struct ble_gatt_svc_def environmentService[] = {
    {
        .type = BLE_GATT_SVC_TYPE_PRIMARY,
        .uuid = BLE_UUID16_DECLARE(BLE_UUID_SERVICE_ENV_SENSING),
        .characteristics = (struct ble_gatt_chr_def[]) {
            {
                .uuid = BLE_UUID16_DECLARE(BLE_UUID_CHARACTERISTIC_TEMPERATURE),
                .access_cb = characteristicValue,
                .val_handle = &esTemperatureCharacteristicAttributeHandle,
                .flags = BLE_GATT_CHR_F_READ | BLE_GATT_CHR_F_READ_ENC | BLE_GATT_CHR_F_NOTIFY,
            },
            { 0 }
        },
    },
    { BLE_GATT_SVC_TYPE_END },
};

// nimble configuration
    ble_hs_cfg.sm_sc = 1;
    ble_hs_cfg.sm_bonding = 1;
    ble_hs_cfg.sm_our_key_dist |= BLE_SM_PAIR_KEY_DIST_ENC;
    ble_hs_cfg.sm_their_key_dist |= BLE_SM_PAIR_KEY_DIST_ENC;
    ble_hs_cfg.sm_our_key_dist |= BLE_SM_PAIR_KEY_DIST_ID;
    ble_hs_cfg.sm_their_key_dist |= BLE_SM_PAIR_KEY_DIST_ID;

    ble_hs_cfg.reset_cb = nimble_callback_on_reset;
    ble_hs_cfg.sync_cb = nimble_callback_on_sync;
    ble_hs_cfg.store_status_cb = ble_store_util_status_rr;

    ble_store_config_init();

    ble_svc_gap_init();
    ble_svc_gatt_init();

    error = ble_gatts_count_cfg(environmentService);
    if (error) {
        return error;
    }

    error = ble_gatts_add_svcs(environmentService);
    if (error) {
        return error;
    }

// set random temperature value and call notify at interval
ble_gatts_notify(conn_handle, esTemperatureCharacteristicAttributeHandle);

We could reproduce the behavior using nRF Connect app on Android. Start the device, open the app, connect to the advertising device.

1. If try to read temperature characteristic the Android will show pairing dialog. Value couldn't be read unless paired;
2. Subscribe to notification without reading the value. Android app subscribes and value is coming at each interval.

Review:

1. The current implementation doesn't check handle in ble_gatts_notify_multiple, so I removed it from ble_gatts_notify_multiple_custom as well. Eventually the handle is checked during reading.
2. I've made new code more consistent to other functions. E.g. ble_gatts_notify calls ble_gatts_notify_custom, therefore I moved logic from ble_gatts_notify_multiple to ble_gatts_notify_multiple_custom, so it handles both cases. (As it did before, but made it more DRY).
3. It seems there is a bug in the current implementation in ble_gatts_notify_multiple_custom when multiple notifications are not supported (iterating over passed handles/values structure taking out of bounds element on each iteration and also not returning after the for loop).

If this changes are ok, a consideration should be taken of the necessity of ble_att_svr_read_local function as it was used to bypass permission checks. And is not used in other functions.