ble: allow disabling pairing with NRF.setSecurity

[ssievert42]

This adds a new pair option to NRF.setSecurity.

That option defaults to true and allows disabling pairing by setting it to false using functionality added in ea5f55fe007f1ad753985db2bd19a4df55759266.

Example usage: NRF.setSecurity({pair:0})

[gfwilliams]

Great, thanks! Is there anything I can add to http://www.espruino.com/BLE+Security, or code examples?

So for instance I guess NRF.setSecurity({pair:0,mitm:1,encryptUart:1}) should allow only the currently bonded peers to connect/access the UART?

[ssievert42]

That'd be cool, good docs and examples are always nice!

Yup, although I think that we should be able to omit the encryptUart:1, because mitm:1 should already result in the UART requiring encryption.

https://github.com/espruino/Espruino/blob/681f115447c17eca9e9926f2394c4733a67e17b1/targets/nrf5x/bluetooth.c#L2128-L2132

I can't think of anything else that could be added to the security page right now, but I've got something in general: My gut tells me that we should make a deep copy of the options that are passed to NRF.setSecurity, do you think that this is needed?
I'd also like having a way for security options to persist across reset()s, to prevent having an (arguably short) timeframe where everything is totally open, but I guess with the way it works right now it's easier to recover from messing up bootcode :shrug:

[gfwilliams]

Ok, great. Just added that to the docs. I'm also trying to add more code examples to the reference, so if you're ever poking around and have a few lines of code that use a built-in function and there wasn't anything in the reference, a PR would be really appreciated :)

My gut tells me that we should make a deep copy of the options that are passed to NRF.setSecurity

Personally I feel like that's not really needed - the chances of that info getting corrupted is pretty small.

I'd also like having a way for security options to persist

Yes. Well, it's not just security, but services and advertising too - it'd be nice not to restart the softdevice on every restart.

There are two bits to this I think:

• Persisting as much of bleStatus flags as we can - this should be reasonably straightforward, and we do now do this with BLE_IS_SLEEPING - doing that should fix most of your issues I think.
• Persisting things like passkey/oob pairing/services/advertising/etc. This is tricker as currently all JS variables are torn down when the interpreter restarts - so really we need to change the interpreter to add some area where variables are persisted

But there is another solution - right now we call jswrap_ble_reconfigure_softdevice in jswrap_ble_init, but this is called before the first JS for the app is run. Potentially if we called jswrap_ble_reconfigure_softdevice after (maybe by creating a _postinit handler) then we're never in a situation where we are resetting things to their defaults before code it run that might set them to something else.

[gfwilliams]

I just pushed something that did the postinit at https://github.com/espruino/Espruino/tree/init_ble_after_js

I'd be interested to see what you think - right now it appears to produce double BLE Connected, queueing BLE restart for later messages - I guess because something like setServices is being a bit eager and it does effectively end up getting called twice.

I guess we could have a flag for whether setServices/setAdvertising had been called already, and if not then we don't bother calling them again.

[ssievert42]

That was quick :exploding_head:
Looking good :+1:

I guess we could have a flag for whether setServices/setAdvertising had been called already, and if not then we don't bother calling them again.

Flags that we'd clear in jswrap_ble_init, set in setServices/setAdvertising and check in jswrap_ble_reconfigure_softdevice? Sounds like a nice solution :)

I just gave this a go and I'm only getting the BLE Connected, queueing BLE restart for later message (also twice) if I change something that needs a softdevice restart. If I disconnect and reconnect after getting the message, it doesn't appear again without changing settings. So I guess those methods do get called twice, but with the same options - once from the js boot code and another time when jswrap_ble_postinit runs. Can you confirm this? If so, and the performance boost of not calling those functions twice on startup is insignificant, we wouldn't need those flags, right?
(If this is exactly what you expected, just ignore the paragraph above.)

The only problem I picked up is, that I may have introduced a tiny chance to run into undefined behaviour on initial startup for SDK 12 devices that use the peer manager in ea5f55fe007f1ad753985db2bd19a4df55759266. In bluetooth.c in ble_evt_handler, we may read from m_sec_params, which may not have been initialized, because the event handler is registered before m_sec_params is first written to by jsble_update_security.
Is ble_evt_handler executed in an ISR?
If it is ok to read from JsVars from the context of ble_evt_handler, we could just call get_gap_sec_params and be done with it. Otherwise the codepath for devices without the peer manager, that calls get_gap_sec_params, might be in need of fixing as well.
Is there anything that runs only once after initial startup? If there isn't, maybe another flag that is used like BLE_PM_INITIALISED could work for this?
Although I think we've already used all the bits of bleStatus, at least if it is 32 bits wide.

[gfwilliams]

So I guess those methods do get called twice, but with the same options - once from the js boot code and another time when jswrap_ble_postinit runs

Yes, that's what I meant - it's not a big deal but I think the double-warning message would annoy/confuse people

we may read from m_sec_params, which may not have been initialized

You mean inside the BLE_GAP_EVT_SEC_PARAMS_REQUEST handler? But we do actually call jsble_update_security in jswrap_ble_reconfigure_softdevice which is called at boot time anyway

I feel like to get that BLE_GAP_EVT_SEC_PARAMS_REQUEST called it's going to take absolute minimum 50ms after connection, by which time we'd have called that.

Perhaps in https://github.com/espruino/Espruino/tree/init_ble_after_js it's worth calling get_gap_sec_params in jsble_init though, just in case the JS code that executes takes maybe 1 second and by the time it exits we're connected.

But yes, ble_evt_handler is called in an ISR, and ideally no, we should try and avoid accessing JsVar from an ISR. It does happen but we can't guarantee that it'll be successful.

[ssievert42]

If it takes 50ms after the first boot until BLE_GAP_EVT_SEC_PARAMS_REQUEST fires, we should be good, because peer_manager_init calls jsble_update_security as well the first time peer_manager_init is called, which should be a lot earlier than the call from jswrap_ble_reconfigure_softdevice. I guess I'm just a tad paranoid.

Well, except for this part, that reads JsVars from an ISR: https://github.com/espruino/Espruino/blob/01193e5ebfc0591ec53ec526fa0073be54ebb7d1/targets/nrf5x/bluetooth.c#L1264-L1270

But is it actually possible for a call to jswrap_ble_reconfigure_softdevice to be the cause of the BLE Connected, queueing BLE restart for later message? Looks like the only place that message is printed is in jswrap_ble_restart, but that should never be called from jswrap_ble_reconfigure_softdevice.
Maybe that warning only comes from the boot js and is printed twice because NRF.setSecurity wants to restart the softdevice, NRF.setServices notices that BLE_NEEDS_SOFTDEVICE_RESTART is set, and tries to restart the softdevice as well?

If we want to avoid ending up in a wrong state for a short amount of time, we'd probably also need to prevent js boot code from restarting the softdevice, and restart it ourselves if needed, by adding something like the snippet below to jswrap_ble_postinit, right?

if (bleStatus & BLE_NEEDS_SOFTDEVICE_RESTART && !jsble_has_connection()) {
  jsble_restart_softdevice(NULL);
}

[gfwilliams]

except for this part, that reads JsVars from an ISR:

I guess we could make that static - it looks like it's only maybe 5 bytes? It's probably not a big deal - the one we want to avoid is jsvNew* since they will return 0 if called during GC

we'd probably also need to prevent js boot code from restarting the softdevice

I don't know - would it matter if it was restarted in the JS code run at boot time? I guess it wouldn't hurt to restart it as long as it actually restarted afterwards.

Just FYI I'm off next week, so it's very unlikely I'll be responding to many issues

[ssievert42]

would it matter if it was restarted in the JS code run at boot time?

If we do something like NRF.setSecurity({encryptUart:1}); NRF.setServices(...);, the first call should trigger a restart, but we haven't picked up the service stuff yet. When restarting we'd call 'jswrap_ble_reconfigure_softdevice', with some options that are saved to execInfo.hiddenRoot by NRF.setServices still set to undefined.
Unless I missed something and this isn't actually an issue :sweat_smile:

Just FYI I'm off next week

Thanks for the heads up - have a nice holiday :)