ARMmbed / mbed-os

Arm Mbed OS is a platform operating system designed for the internet of things
https://mbed.com
Other
4.67k stars 2.98k forks source link

NRF52832 (UBLOX NINA-B1) using BLE with signing enabled does not work as expected #8206

Closed edablan closed 6 years ago

edablan commented 6 years ago

Description

mbed 5.9.7 (949cb49) UBLOX NINA-B1(NRF52832) IAR 7.80.4 mbed-cli 1.7.2

Enabling signing using the securityManager().init() function does not work as expected because the mbed nRF5xSecurityManager::send_pairing_response() function overrides signing as well as distribution of link keys. Not sure why this is done?

I initialize the serucityManager like so. The input parameter setting signing it set to true.

    ble.securityManager().init(true, false, SecurityManager::IO_CAPS_NONE, NULL, true, NULL);

I hooked up the debugger and noticed the the responder_distribution key at first was correctly set for signing, but the code below from the nRF5xSecurityManager::send_pairing_response() function removes the signing bit along with the link key bit. This seems to render signing useless. I further checked to see if the bit was set using a BLE v4.2 analyzer and it wasn't.

    // override signing parameter
    initiator_dist.set_signing(false);
    responder_dist.set_signing(false);

    // override link parameter
    initiator_dist.set_link(false);
    responder_dist.set_link(false);

enc_key_off

How does one go about getting the sign key to be set, because I see no way of it working as it stands? Furthermore the link, and encryption key bits to be set? The pure Nordic SDK14.2 which mbed leverages allows for setting this bit. Seems the mbed BLE API is missing the ability to set encryption, and link key distribution in the pairing response which breaks the BLE SIG compatibility.

Issue request type

[X] Question
[ ] Enhancement
[ ] Bug

pan- commented 6 years ago

@edablan From what I can see of the BT qualification report of softdevice 4.0, none of the signing features are supported whether it is on the security manager side or gatt client/gatt server side.

We tried to set the flag for CSRK if asked by the user as the API are exposed by the softdevice but it ends with a failure at runtime. .

Seems the mbed BLE API is missing the ability to set encryption, and link key distribution in the pairing response which breaks the BLE SIG compatibility.

No, signing is not mandatory, Look at BLUETOOTH SPECIFICATION Version 5.0 | Vol 3, Part C - section 10.

edablan commented 6 years ago

Do you mean softdevice 5.0? We are using Nordic softdevice 5.0 which is what is supported by the SDK14.2 that mbed uses, or is mbed using a different nordic SDK version? We do need to allow for the encryption key distribution because the windows HID over GATT driver is expecting this. I ended up adding this line in nRF5xSecurityManager::send_pairing_response() function:

responder_dist.set_encryption(true); //<-- added to get encryption to work

this works and allows the windows HID over GATT driver to load properly.

Will mbed be adding BLE API commands to set at least some of the distribution keys (like encryption)?

pan- commented 6 years ago

@edablan I think there is a lot of confusion here. During the first phase of pairing the devices exchanges their capabilities as well as the peer's keys they are interested in and the the keys they can provide:

So encryption of the link is not related to signature of data.

ended up adding this line in nRF5xSecurityManager::send_pairing_response() function:

responder_dist.set_encryption(true); //<-- added to get encryption to work

The encryption on the responder side should be equal to the value requested by the initiator. Could you trace what is the encryption value requested by the initiator ?

https://github.com/ARMmbed/mbed-os/blob/65a0a1aecd8c631d72f2ff9896c8b8b5d07f8276/features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NRF52/source/nRF5xPalSecurityManager.cpp#L807


Side note: The LinkKey flag is used by devices operating in LE and BR/EDR mode (that's not supported by mbed OS!) to derive the BR/EDR link key from the LE LTK.

edablan commented 6 years ago

@pan-

The initiator is requesting the following for key distribution:

enc_key

Before I added the code mentioned above the responder (NRF52) would respond only with IdKey being set (Responder shall distribute IRK followed by its address). This would cause a windows driver error. However by adding that one line mentioned above for EncKey bit set (for the responder to distribute the LTK followed by the EDIV and Rand). The windows driver error goes away, and works properly.

pan- commented 6 years ago

@edablan I tried for myself and I'm not able to reproduce your issue. The code is pretty straightforward.

First the initiator distribution flags requested by the initiator are stored:

https://github.com/ARMmbed/mbed-os/blob/a0a9b54e977d29db445cef03bcee7eb32409d90d/features/FEATURE_BLE/source/generic/GenericSecurityManager.cpp#L1155-L1156

Then when the pairing request is accepted the local distribution flags are created and sent:

https://github.com/ARMmbed/mbed-os/blob/a0a9b54e977d29db445cef03bcee7eb32409d90d/features/FEATURE_BLE/source/generic/GenericSecurityManager.cpp#L249-L260

_default_key_distribution is a constant that contains all the distribution flags and as you can see the encryption part of the responder distribution flags is not altered in the generic layer.

From non modified sources could you post the screenshot of the pairing request and pairing response screenshot ?

edablan commented 6 years ago

@pan-

Here are screen shots of pairing request and pairing response using a clean mbed-os-5.10.0 (610e35ddc)

PAIRING REQUEST: pairing_request

PAIRING RESPONSE: pairing_response

As you can see the pairing response does not have the encryption bit set.

edablan commented 6 years ago

@pan-

The best way to see what is going on with the ENC bit is to follow the stack, and the underlying issue which I think might be the soft device. I used mbed os-5.10.0 (610e35ddc).

I set a break point in the nRF5xSecurityManager::send_pairing_response() function after the security parameters have been created. I simply try to pair the device and the break point is hit. This contains the initiator/responder distribution of keys. As you can see the responder is set to 0x02 which corresponds only to the IDkey bit being set. No encryption key bit is set.

send_pair_response

Going into the call stack and following it, one can see what occurred. The GenericSecurityManager::acceptPairingRequest() function also has the responder distribution keys with the encryption key off. It has a value of 0x02

accept_pairing_request

Going further back we can see what happened to the initiator/responder keys in the nRF5xSecurityManager::sm_handler() function. Seems the responder distribution NEVER had the encryption bit on. Its value is 0x0E. The LSbit is off, which corresponds to the encryption bit being off.

sm_handler

The reason the responder bit eventually ended up with a value of 0x02 above, deals with the _default_key_distribution after the GenericSecurityManager::init() function does the following: https://github.com/ARMmbed/mbed-os/blob/6b85ec7c5779b93a159906705bb47e8649e71d4a/features/FEATURE_BLE/source/generic/GenericSecurityManager.cpp#L75-L81 _default_key_distribution has signing (by default signing is turned off in security manager) and linking turned off. So the _default_key_distribution starts off 0x0F then becomes 0x03. Then this occurs in the GenericSecurityManager::acceptPairingRequest() function: https://github.com/ARMmbed/mbed-os/blob/6b85ec7c5779b93a159906705bb47e8649e71d4a/features/FEATURE_BLE/source/generic/GenericSecurityManager.cpp#L249-L251 0x03 AND 0x0E gets you the 0x02 value for the responder distrubtion. Leaving only the IDkey bit set.

Of course this doesn't explain why does the responder key have its value initially set to 0x0E to begin with, which has the encryption bit turned off. The lines below show that the responder_dist variable is set according to the const ble_gap_sec_params_t& params = gap_evt.params.sec_params_request.peer_params;

https://github.com/ARMmbed/mbed-os/blob/6b85ec7c5779b93a159906705bb47e8649e71d4a/features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NRF52/source/nRF5xPalSecurityManager.cpp#L781-L811

Looking further back at the call stack to see where the evt struct is originated one can see the issue starts here: nrf_sdh_ble_evts_poll

The kdist_own variable has the encryption bit turned off. From the code referenced above the kdist_own bit fields are used to set the responder_dist . The evt struct as we can see is created here:

https://github.com/ARMmbed/mbed-os/blob/6b85ec7c5779b93a159906705bb47e8649e71d4a/targets/TARGET_NORDIC/TARGET_NRF5x/TARGET_SDK_14_2/TARGET_SOFTDEVICE_COMMON/softdevice/common/nrf_sdh_ble.c#L256-L267

The function that gets the evt struct is here. It reads it from the soft device. https://github.com/ARMmbed/mbed-os/blob/6b85ec7c5779b93a159906705bb47e8649e71d4a/targets/TARGET_NORDIC/TARGET_NRF5x/TARGET_SDK_14_2/TARGET_SOFTDEVICE_S132_FULL/headers/nrf_ble.h#L466

Using the pure Nordic SDK14.2 paired with the same soft device used above (5.1.0), and using the Nordic HID example does not cause the issue, and the encryption bit is on.

I also tried this with soft device 5.0.0 and mbed-os-5.10.0 (610e35ddc), and the encryption bit is still off.

pan- commented 6 years ago

@edablan Many thanks for the detailed report; that is scary.

Could you provide us:

Note: It may take us some time to conduct that investigation.

edablan commented 6 years ago

@pan-

/ Default to no softdevice / if (!isdefinedsymbol(MBED_APP_START)) { define symbol MBED_APP_START = 0x0; }

if (!isdefinedsymbol(MBED_APP_SIZE)) { define symbol MBED_APP_SIZE = 0x80000; }

/ If softdevice is present, set aside space for it / if (isdefinedsymbol(SOFTDEVICE_PRESENT)) { define symbol MBED_RAM_START = 0x200031D0; define symbol MBED_RAM_SIZE = 0xCE30; } else { define symbol MBED_RAM_START = 0x20000000; define symbol MBED_RAM_SIZE = 0x10000; }

define symbol MBED_RAM0_START = MBED_RAM_START; define symbol MBED_RAM0_SIZE = 0xDC; define symbol MBED_RAM1_START = (MBED_RAM_START + MBED_RAM0_SIZE); define symbol MBED_RAM1_SIZE = (MBED_RAM_SIZE - MBED_RAM0_SIZE);

/-Specials-/ define symbol ICFEDIT_intvec_start = MBED_APP_START;

/-Memory Regions-/ define symbol ICFEDIT_region_ROM_start = MBED_APP_START; define symbol ICFEDIT_region_ROM_end = MBED_APP_START + MBED_APP_SIZE - 1; define symbol ICFEDIT_region_RAM_NVIC_start = MBED_RAM0_START; define symbol ICFEDIT_region_RAM_NVIC_end = MBED_RAM0_START + MBED_RAM0_SIZE - 1; define symbol ICFEDIT_region_RAM_start = MBED_RAM1_START; define symbol ICFEDIT_region_RAM_end = MBED_RAM1_START + MBED_RAM1_SIZE - 1; export symbol ICFEDIT_region_RAM_start; export symbol ICFEDIT_region_RAM_end;

/-Sizes-/ /Heap 1/4 of ram and stack 1/8/ define symbol ICFEDIT_size_cstack = 0x800; define symbol ICFEDIT_size_heap = 0x5800; /*** End of ICF editor section. ###ICF###/

define symbol __code_start_soft_device__ = 0x0;

define memory mem with size = 4G; define region ROM_region = mem:[from ICFEDIT_region_ROM_start to ICFEDIT_region_ROM_end]; define region RAM_region = mem:[from ICFEDIT_region_RAM_start to ICFEDIT_region_RAM_end];

define block CSTACK with alignment = 8, size = ICFEDIT_size_cstack { }; define block HEAP with alignment = 8, size = ICFEDIT_size_heap { };

initialize by copy { readwrite }; do not initialize { section .nvictable }; place at address mem:ICFEDIT_region_RAM_NVIC_start { section .nvictable };

do not initialize { section .noinit }; place in RAM_region { section .noinit };

keep { section .intvec }; place at address mem:ICFEDIT_intvec_start { readonly section .intvec }; place in ROM_region { readonly }; place in RAM_region { readwrite, block HEAP, block CSTACK };

/This is used for mbed applications build inside the Embedded workbench Applications build with the python scritps use a hex merge so need to merge it inside the linker. The linker can only use binary files so the hex merge is not possible through the linker. That is why a binary is used instead of a hex image for the embedded project. / if(isdefinedsymbol(SOFT_DEVICE_BIN)) { place at address mem:__code_start_soft_device__ { section .noinit_softdevice }; }


- have only tried it with IAR. I will see if I can get armcc to work on my end. Though the Nordic example that works is an IAR project provided from Nordic.

- Using a HID over GATT profile with appearance set to HID Device
adbridge commented 6 years ago

Internal Jira reference: https://jira.arm.com/browse/IOTPAN-269

edablan commented 6 years ago

@pan- Any news on why the ticket was closed?

edablan commented 5 years ago

@adbridge

Any word on what was causing this issue? or was it found not to be an issue?

adbridge commented 5 years ago

@edablan It will be fixed with the 5.11 release.