Implemented configuration options for BLE security #360

DvdGiessen · 2019-11-11T15:08:58Z

For our product we needed the ability to configure some additional options related to BLE security, which were available in the lower level Espressif stack but were not exposed though any MicroPython interface.

This pull request exposes a few new constants, parameters and methods for configuring these properties. Namely:

A new method, bluetooth.set_pin(new_pin), which can be used to configure a new pin at any time, not just during initialization of the Bluetooth object.
A few constants Bluetooth.PERM_*, which can be used with the pre-existing permissions parameter of the .characteristic() method to configure the permissions that will be applied to a newly created characteristic. A secure connection will be established as soon as an encrypted characteristic is touched, meaning that by setting permissions it is possible to specify unencrypted characteristics which may be accessed without pairing. By default the permissions are set to encrypted if security is enabled, this was not changed and thus everything will function exactly the same as before unless a developer chooses to specifically set the parameter.
Two new named parameters for the Bluetooth initialization, namely privacy=True and secure_connections=True, which allow selectively disabling certain security features which impact how phones and other devices behave when interacting with the Pycom board. Again, by default these remain enabled and unless changed explicitly everything will function the same as before.
The existing bluetooth.set_advertisement_params() method would not work when BLE security was enabled. This has been resolved so that now it is possible to configure advertisement parameters as expected in all cases.
The Python interface would not allow pin codes to start with a 0 digit, while it is supported and allowed by the Bluetooth spec. In all our BLE products the default pin code is 000000, and for uniformity we want to use that here as well. This pull request resolves this so now pin codes are allowed to start with a zero.

The changes were verified with a number of phones, and we also double-checked by inspecting the raw BLE traffic with a sniffer.

Documentation has been updated to describe the new set_pin() method and permission constants.

knagymate · 2020-01-31T23:19:42Z

esp32/mods/modbt.c

 }


 STATIC mp_obj_t bt_set_advertisement_params (mp_uint_t n_args, const mp_obj_t *pos_args, mp_map_t *kw_args) {


The problem with original code is that if we call bt_set_advertisement_params() with keyword argument(s), than unmentioned bt_adv_params will be reinitiated, which is probably not the intended behaviour of the function.

Example with original code:

Calling bt_set_advertisement_params(adv_int_max = 128)

bt_adv_params.adv_int_min = 32

bt_adv_params.adv_int_max = 128

Calling bt_set_advertisement_params(adv_int_min = 16)

bt_adv_params.adv_int_min = 16

bt_adv_params.adv_int_max = 64 <- adv_int_max is reinitialized

Example with new code:

Calling bt_set_advertisement_params(adv_int_max = 128)

bt_adv_params.adv_int_min = 32

bt_adv_params.adv_int_max = 128

Calling bt_set_advertisement_params(adv_int_min = 16)

bt_adv_params.adv_int_min = 16

bt_adv_params.adv_int_max = 128 <- adv_int_max remains the same

The behaviour is tested on LoPy4, I suggest to accept all the modifications inside bt_set_advertisement_params() function definition.

esp32/mods/modbt.c

knagymate · 2020-02-01T18:17:05Z

esp32/mods/modbt.c

+}
+
+static void set_secure_parameters(bool secure_connections) {
+    esp_ble_auth_req_t auth_req = secure_connections ? ESP_LE_AUTH_REQ_SC_MITM_BOND : ESP_LE_AUTH_BOND;


Further review needed.

The changes to these lines are for of the new secure_connections parameter described in the pull request's description.

Yes, thank you for improvement, we will add secure_connections.

knagymate · 2020-02-01T22:07:08Z

esp32/mods/modbt.c

-    case ESP_GAP_BLE_PASSKEY_NOTIF_EVT: {
-         printf("BLE paring passkey : %d\n", param->ble_security.key_notif.passkey);
-         break;
-    }


Do we want to inform the user about the BLE paring passkey on REPL, or is it just a remaining debug signal?

Since the changes in this pull request makes the passkey configurable from MicroPython, there was no need for this debugging print statement to remain. The updated documentation explains how a Bluetooth PIN may be set.

knagymate · 2020-02-01T23:25:28Z

esp32/mods/modbt.c

-static bool is_pin_valid(uint32_t pin)
-{
-    int digits = 0;
-
-    while(pin != 0)
-    {
-        digits++;
-        pin /= 10;
-    }
-
-    if (digits != MOD_BT_PIN_LENGTH){
-       return false;
-    }
-
-    return true;
-}


It is a problem that length of pin is not inspected anymore. If pin can start with 0, and we store it in an integer, we cannot check the length. I have tested the commit and expreienced the following:

Initiated instance with bluetooth = Bluetooth(secure=True, pin=1234) on LoPy4, without getting any error (intended because of the deleted code snippet), and tried to bond from a phone, using nRF Connect. Using 1234 pin code, connection cannot be created, instead, with 001234, it was OK.

There was no other issue with any 6 digit pin codes (i.e. 050399 or 000001), and pin longer than 6 digits still gives an error (i.e. 1234567).

Do we want to:

Ignore pins starting with zero (official version)

Handle pins starting with zeros, but doesn't check pin minimal length? (this commit)

Handle both length, and enable pin starting with zero? (new implementation, I think we cannot check length, as we represent pin as int -> 001 == 000001).

This is intentional and as expected. As a direct result of the BLE implementation using an integer type to represent the passkey the leading zero's may be omitted in the calling Python code, but the resulting passkey will always be 6 digits. From the Bluetooth Core specification:

[...] the Bluetooth passkey is a 6-digit numerical value. It is represented as integer value in the range 0x00000000 – 0x000F423F (000000 to 999999).

Note that this is also documented in the additional documentation for the user-facing set_pin() method.

knagymate · 2020-02-02T00:08:38Z

esp32/mods/modbt.c

-                        if (value == 0x0001) {  // notifications enabled
-                            bt_gatts_char_obj_t *char_obj = (bt_gatts_char_obj_t *)attr_obj->parent;
-                            // the size of value[] needs to be less than MTU size
-                            esp_ble_gatts_send_indicate(gatts_if, param->write.conn_id, char_obj->attr_obj.handle, char_obj->attr_obj.value_len, char_obj->attr_obj.value, false);
-                        }


I don't know why it is removed.

These lines should be kept to send notifications in case of WRITE events when enabled.

knagymate · 2020-02-02T00:18:51Z

esp32/mods/modbt.c

-            if (bt_obj.secure){
-                esp_ble_set_encryption(p->connect.remote_bda, ESP_BLE_SEC_ENCRYPT_MITM);
-            }


It is okay anyways, esp_ble_set_encryption(p->connect.remote_bda, ESP_BLE_SEC_ENCRYPT_MITM) instruction causes that a bonding request is sent to master, when slave tries to just connect. As @DvdGiessen mentioned:

A secure connection will be established as soon as an encrypted characteristic is touched, meaning that by setting permissions it is possible to specify unencrypted characteristics which may be accessed without pairing. By default the permissions are set to encrypted if security is enabled, this was not changed and thus everything will function exactly the same.

This modification is OK.

knagymate · 2020-02-04T19:26:34Z

esp32/mods/modbt.c

    for (int i = 0; i < dev_num; i++) {
+        if (bt_obj.gatts_conn_id >= 0 && memcmp((void *) dev_list[i].bd_addr, (void *) bt_obj.client_bda, sizeof(esp_bd_addr_t)) == 0) {
+            continue;
+        }


If a see correctly, the aim of this part is to not remove bond with device which is currently connected.

I tested this part, and bond won't be removed between LoPy4 and connected peripheral, but as you disconnect, can't be reconnect and rebond to LoPy4 anymore, I could not seen its advertisement anymore.

This was tested and confirmed to work as expected on a LoPy4, FiPy and GPy, and we are seeing advertisements in the raw traffic. The sending of advertisements is not handled by this part of the code, so I'm not sure what causes the issue in your test but it seems like it wouldn't be related to the changes in these lines.

Yes, I have also tested, and I am sure that the problem is not coming from your changes, It is also an issue without it. I have tested with LoPy4 and nRF Connect on my Android, and it both cases, I experience that:

Connect to LoPY4

Modify secured characteristics

Bonding devices

Set new pin

Disconnect (with your change) or Automatically disconnected (withhout the change)

Advertisement disappears (with manual bluetooth.advertise(True) it can be seen again)

So I recommended to add esp_ble_gap_start_advertising(&bt_adv_params); at the end of remove_bonded_devices() function.

husigeza · 2020-02-14T07:35:39Z

Thanks for the contribution, we checked this PR and approved, it will be part of the next releases.
We introduced some minor changes:

added secure_connections to bt_obj_t struct
restored remove_all_bonded_devices() instad of remove_bonded_devices
secure parameters are set inside set_pin()
secure connection is decided based on PIN set
bt_init_args secure argument is removed
removed 'BLE paring passkey: ' REPL message
removed automatic secure connection in case of connection
restored characteristics notification

* Original PR: pycom#360 by DvdGiessen * Some minor modifications were applied: - added secure_connections to bt_obj_t struct - restored remove_all_bonded_devices() instad of remove_bonded_devices - secure parameters are set inside set_pin() - secure connection is decided based on PIN set - bt_init_args secure argument is removed - removed 'BLE paring passkey: ' REPL message - removed automatic secure connection in case of connection - restored characteristics notification

Implemented configuration options for BLE security

c2032a9

catalinio requested a review from husigeza November 12, 2019 07:04

knagymate reviewed Jan 31, 2020

View reviewed changes

esp32/mods/modbt.c Show resolved Hide resolved

knagymate reviewed Jan 31, 2020

View reviewed changes

esp32/mods/modbt.c Show resolved Hide resolved

knagymate reviewed Feb 1, 2020

View reviewed changes

knagymate reviewed Feb 2, 2020

View reviewed changes

knagymate reviewed Feb 4, 2020

View reviewed changes

husigeza closed this Feb 14, 2020

		}


		STATIC mp_obj_t bt_set_advertisement_params (mp_uint_t n_args, const mp_obj_t pos_args, mp_map_t kw_args) {

Implemented configuration options for BLE security #360

Implemented configuration options for BLE security #360

Uh oh!

Conversation

DvdGiessen commented Nov 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knagymate Feb 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

husigeza commented Feb 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

knagymate Feb 8, 2020 •

edited

Loading