ble: if mitm is set, reject unauthenticated security procedures

ssievert42 commented 1 year ago

If a passkey is set using NRF.setSecurity(), it was previosly possible for another device to pair without having to supply that passkey, by supplying a combination of io capabilities that result in using unauthenticated "just works" pairing (see the bluetooth spec v5.0, vol 3, part H, page 2313, table 2.8).
This change fixes that behaviour by taking a look at the supplied io capabilities during pairing, and rejecting anything that would result in an unauthenticated connection.

Setting mitm also causes the UART to require a connection that is mitm protected.

Because we need the currently configured security parameters in the PM_EVT_CONN_SEC_PARAMS_REQ event, they are now written to the static variable m_sec_params every time jsble_update_security() is called. Reading from that variable should be safe, because jsble_update_security() is called before registering the event handler with pm_register(pm_evt_handler), so m_sec_params should be properly initialised once PM_EVT_CONN_SEC_PARAMS_REQ is fired.

This additionally adds a flag BLE_IS_NOT_PAIRABLE, that allows disabling pairing altogether by rejecting all security procedures. There is currently no way to set that flag though.

I could only test this for Bangle.js 2, as that is the only Espruino device I own; hopefully CI builds still work and nothing breaks on a real device :sweat_smile:

ssievert42 commented 1 year ago

Converted to draft because builds for some boards fail.

ssievert42 commented 1 year ago

Looks like the nrf sdk v12 doesn't have PM_EVT_CONN_SEC_PARAMS_REQ. I guess there we might be able to use BLE_GAP_EVT_SEC_PARAMS_REQUEST. Not sure though if the peer manager (if it is enabled) would try to respond to BLE_GAP_EVT_SEC_PARAMS_REQUEST as well, and how much responding to that event ourselves could break something in that case.
That is something I couldn't test myself though, because I don't have the necessary hardware.

Is there any way to check the sdk version at compile time and have the preprocessor throw out the codepath using PM_EVT_CONN_SEC_PARAMS_REQ if we are on an sdk that doesn't have that event?

gfwilliams commented 1 year ago

I think likely PM_EVT_CONN_SEC_PARAMS_REQ<->BLE_GAP_EVT_SEC_PARAMS_REQUEST is the same - Nordic have a habit of randomly changing the names of stuff between SDK versions just to make everyone's life that little bit more painful.

But if you can give me a specific set of things to test (eg run NRF.setSecurity(...) and try and connect, cancel, etc) then I can give it a go on an SDK12 device

Using #if (NRF_SD_BLE_API_VERSION >= 5) to check for SDK version should be fine.

ssievert42 commented 1 year ago

Thanks, #if (NRF_SD_BLE_API_VERSION >= 5) did the trick :)

Except for the micro:bit 1 build, which now reports CODE AND STORAGE OVERLAP by 16 bytes. No idea how to fix that :sweat_smile:

Now there are three different possibilities:

sdk 15, peer manager enabled - at least the case on Bangle.js 2 and something that I can test myself
sdk 12, peer manager disabled - should be the case on micro:bit 1
sdk 12, peer manager enabled - should be the case on micro:bit 2, Puck.js, Pixl.js, MDBT42Q and Bangle.js 1

Still, what might be tricky is that, if the peer manager is enabled it should respond to BLE_GAP_EVT_SEC_PARAMS_REQUEST itself. If sd_ble_gap_sec_params_reply errors out on us, it may not be possible to reject security procedures ourselves if we use the peer manager, but the sdk doesn't have PM_EVT_CONN_SEC_PARAMS_REQ.
We could still disable pairing though by calling pm_sec_params_set() with NULL.

To test

You need a device that reports io capabilities that would lead to an unauthenticated connection. I'm not sure, but I think my pc pretends to not have a keyboard, for example. A computer running some kind of desktop linux should work; I use Ubuntu 22.04.
To try to pair with insufficient io capabilities: Make sure to close everything else that might provide a way to enter a passkey, like for example a terminal with bluetoothctl running. Open gnome-control-center, go to the Bluetooth section and click on the device.
To try to pair normally: Open a terminal and run bluetoothctl. Type scan le to scan for devices, devices to list devices that your pc found and pair {address of device} to pair with a device.

unauthenticated connection

Run the following js on the device to disable security:

NRF.setSecurity();
NRF.restart();

Unpair both devices.

Then try to pair with insufficient io capabilities.
Pairing should succeed, and you should be able to use the UART.

unauthenticated connection, encrypted UART

Run the following js on the device to enable encrypting the UART:

NRF.setSecurity({encryptUart:1});
NRF.restart();

Unpair both devices.

Then try to pair with insufficient io capabilities.
Pairing should succeed, and you should be able to use the UART.

authenticated connection

Run the following js on the device to enable security:

NRF.setSecurity({passkey:"123456", mitm:1, display:1});
NRF.restart();

Unpair both devices.

Then try to pair with insufficient io capabilities.
You should not be asked for a passkey and pairing should fail.

After that, try to pair normally, using the passkey "123456".
Pairing should succeed, and you should be able to use the UART.

ssievert42 commented 1 year ago

Just "fixed" the micro:bit 1 build by wrapping everything in #if PEER_MANAGER_ENABLED.
Now the CI is happy again :)

ssievert42 commented 1 year ago

We could also shave off a few bytes by wrapping the changes to ble_nus.c and ble_nus.h in #if PEER_MANAGER_ENABLED, since those are only used if the peer manager is enabled anyways.
Should I add this here as well?

gfwilliams commented 1 year ago

Thanks for all this work! It's looking great. Looks like I'll be a bit busy the next few days but I'll check through this properly and merge as soon as I get a chance

We could also shave off a few bytes by wrapping the changes to ble_nus

That's a great idea! As you probably noticed, the Micro:bit 1 build is super tight, so anything we can do to help with that is really helpful

gfwilliams commented 1 year ago

I just tried this on an nRF52832DK (it's easy to get a serial console when connected) and what's a bit disconcerting is that I run n=0;setInterval(() => Bluetooth.println(n++),1000) and NRF.setSecurity({encryptUart:1}); and I can still connect with NRF Connect and enable notifications on the TX characteristic and get data without even being encrypted let alone anything else. Same with pairing!

I think in rx_char_add we need to set permissions on cccd_md too? I just tried replacing:

    BLE_GAP_CONN_SEC_MODE_SET_OPEN(&cccd_md.read_perm);
    BLE_GAP_CONN_SEC_MODE_SET_OPEN(&cccd_md.write_perm);

with

    BLE_GAP_CONN_SEC_MODE_SET_OPEN(&cccd_md.read_perm);
#if PEER_MANAGER_ENABLED
    if (p_nus_init->encrypt) {
        if (p_nus_init->mitmProtect) {
            BLE_GAP_CONN_SEC_MODE_SET_ENC_WITH_MITM(&cccd_md.write_perm);
        } else {
            BLE_GAP_CONN_SEC_MODE_SET_ENC_NO_MITM(&cccd_md.write_perm);
        }
    } else {
        BLE_GAP_CONN_SEC_MODE_SET_OPEN(&cccd_md.write_perm);
    }
#else
    BLE_GAP_CONN_SEC_MODE_SET_OPEN(&cccd_md.write_perm);
#endif

and that seems to work ok (looks like you can't make cccd read require auth). As you've got this PR do you think you could add it in for SDK 12 as well as SDK15 (and just copy it in for SDK15_3 patches as well)? Thanks!

ssievert42 commented 1 year ago

Just added your changes to SDK 12 :+1:
SDK 15 should already require auth for cccd write, but this is set in tx_char_add, while they leave cccd set to NULL in rx_char_add.
I also copied the patch to SDK 15_3, but that version of the SDK does not want to be patched. Seems like they changed too much :shrug:

gfwilliams commented 1 year ago

Thanks! Looks good - all seems to work well as far as I can tell - merging now :)

espruino / Espruino