-
-
Notifications
You must be signed in to change notification settings - Fork 745
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ble: if mitm is set, reject unauthenticated security procedures #2399
Conversation
Converted to draft because builds for some boards fail. |
Looks like the nrf sdk v12 doesn't have Is there any way to check the sdk version at compile time and have the preprocessor throw out the codepath using |
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 Using |
8eb07ec
to
924b864
Compare
Thanks, Except for the micro:bit 1 build, which now reports Now there are three different possibilities:
Still, what might be tricky is that, if the peer manager is enabled it should respond to To testYou 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. unauthenticated connectionRun the following js on the device to disable security: NRF.setSecurity();
NRF.restart(); Unpair both devices. Then try to pair with insufficient io capabilities. unauthenticated connection, encrypted UARTRun 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. authenticated connectionRun 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. After that, try to pair normally, using the passkey "123456". |
924b864
to
aab7937
Compare
Just "fixed" the micro:bit 1 build by wrapping everything in |
We could also shave off a few bytes by wrapping the changes to |
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
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 |
aab7937
to
4624ec8
Compare
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 I think in rx_char_add we need to set permissions on cccd_md too? I just tried replacing:
with
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! |
Setting mitm also causes the UART to require a connection that is mitm protected.
4624ec8
to
ea5f55f
Compare
Just added your changes to SDK 12 👍 |
Thanks! Looks good - all seems to work well as far as I can tell - merging now :) |
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 variablem_sec_params
every timejsble_update_security()
is called. Reading from that variable should be safe, becausejsble_update_security()
is called before registering the event handler withpm_register(pm_evt_handler)
, som_sec_params
should be properly initialised oncePM_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 😅