From 55507eaf752722ff2d80e4f46fc897c70e924452 Mon Sep 17 00:00:00 2001 From: Nic Costa Date: Tue, 8 Jan 2019 10:31:59 -0600 Subject: [PATCH 1/3] Fix race condition when generating OOB data The GenericSecurityManager tracks the most recent OOB data generated by the PAL and the PAL function to generate OOB data is expected to be asynchronous such that the OOB data is returned via a callback. There was a race condition on the security manager's oob data variable because it was cleared (set to all zeros) after calling PAL generate. The expectation was that the clear operation would occur before the callback executed, but this is proving to not be the case. Instead, the callback is being executed as if it were syncronous with PAL generate, then PAL generate returns and the oob data is cleared, thereby losing the generated oob data that was set in the callback. To fix the issue, clear the oob data variables before calling into the PAL. --- .../source/generic/GenericSecurityManager.cpp | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/features/FEATURE_BLE/source/generic/GenericSecurityManager.cpp b/features/FEATURE_BLE/source/generic/GenericSecurityManager.cpp index 0c5e047dc48..621f758680d 100644 --- a/features/FEATURE_BLE/source/generic/GenericSecurityManager.cpp +++ b/features/FEATURE_BLE/source/generic/GenericSecurityManager.cpp @@ -653,14 +653,20 @@ ble_error_t GenericSecurityManager::generateOOB( /* Secure connections. Avoid generating if we're already waiting for it. * If a local random is set to 0 it means we're already calculating. */ if (!is_all_zeros(_oob_local_random)) { - status = _pal.generate_secure_connections_oob(); + /* save the current values in case the call to + * generate_secure_connections_oob fails */ + address_t orig_local_address = _oob_local_address; + oob_lesc_value_t orig_local_random = _oob_local_random; + + _oob_local_address = *address; + /* this will be updated when calculation completes, + * a value of all zeros is an invalid random value */ + set_all_zeros(_oob_local_random); - if (status == BLE_ERROR_NONE) { - _oob_local_address = *address; - /* this will be updated when calculation completes, - * a value of all zeros is an invalid random value */ - set_all_zeros(_oob_local_random); - } else if (status != BLE_ERROR_NOT_IMPLEMENTED) { + status = _pal.generate_secure_connections_oob(); + if (status != BLE_ERROR_NONE && status != BLE_ERROR_NOT_IMPLEMENTED) { + _oob_local_address = orig_local_address; + _oob_local_random = orig_local_random; return status; } } else { From 24d793ce5da03c39c8651e1acc7f4cc534a2b82d Mon Sep 17 00:00:00 2001 From: Nic Costa Date: Tue, 8 Jan 2019 12:26:30 -0600 Subject: [PATCH 2/3] Fix parameters provided to oob generator function The function in the Nordic SDK for generating OOB data, sd_ble_gap_lesc_oob_data_get, requires local LE Secure Connection P256 Public Keys in {X,Y} format, but was being supplied with the local secret key. This caused the generated OOB data to fail to correspond to the Public Keys, which caused a mismatch during the OOB pairing phase of the OOB confirmation value by a remote peer when attempting to verify the OOB data against the Public Keys, ultimately causing the OOB pairing request to fail with a Confirm Value Failed (0x04) error. --- .../TARGET_NRF52/source/nRF5xPalSecurityManager.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NORDIC_SOFTDEVICE/TARGET_NRF52/source/nRF5xPalSecurityManager.cpp b/features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NORDIC_SOFTDEVICE/TARGET_NRF52/source/nRF5xPalSecurityManager.cpp index a1d6b1e7e90..61c26d36882 100644 --- a/features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NORDIC_SOFTDEVICE/TARGET_NRF52/source/nRF5xPalSecurityManager.cpp +++ b/features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NORDIC_SOFTDEVICE/TARGET_NRF52/source/nRF5xPalSecurityManager.cpp @@ -15,6 +15,7 @@ */ #include +#include "platform/mbed_assert.h" #include "nRF5xPalSecurityManager.h" #include "nRF5xn.h" #include "ble/Gap.h" @@ -734,7 +735,9 @@ ble_error_t nRF5xSecurityManager::generate_secure_connections_oob() ble_gap_lesc_p256_pk_t own_secret; ble_gap_lesc_oob_data_t oob_data; - memcpy(own_secret.pk, secret.data(), secret.size()); + MBED_ASSERT(sizeof(own_secret.pk) >= X.size() + Y.size()); + memcpy(own_secret.pk, X.data(), X.size()); + memcpy(own_secret.pk + X.size(), Y.data(), Y.size()); uint32_t err = sd_ble_gap_lesc_oob_data_get( BLE_CONN_HANDLE_INVALID, From 7795e30bce1cfebab8a1f2623b9e099079c0eb9b Mon Sep 17 00:00:00 2001 From: Nic Costa Date: Tue, 8 Jan 2019 12:45:39 -0600 Subject: [PATCH 3/3] Remove own_oob and peer_oob flags from Nordic PAL the own_oob and peer_oob flags were not being set to 1 even though an OOB pairing request was in progress, which therefore prevented OOB data from being passed down to the softdevice during a OOB pairing operation, thus causing the OOB pairing process to fail. --- .../source/nRF5xPalSecurityManager.cpp | 38 +++++++++++-------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NORDIC_SOFTDEVICE/TARGET_NRF52/source/nRF5xPalSecurityManager.cpp b/features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NORDIC_SOFTDEVICE/TARGET_NRF52/source/nRF5xPalSecurityManager.cpp index 61c26d36882..01126720b4e 100644 --- a/features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NORDIC_SOFTDEVICE/TARGET_NRF52/source/nRF5xPalSecurityManager.cpp +++ b/features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NORDIC_SOFTDEVICE/TARGET_NRF52/source/nRF5xPalSecurityManager.cpp @@ -81,11 +81,6 @@ struct nRF5xSecurityManager::pairing_control_block_t { ble_gap_id_key_t peer_id_key; ble_gap_sign_info_t peer_sign_key; ble_gap_lesc_p256_pk_t peer_pk; - - // flag required to help DHKey computation/process; should be removed with - // later versions of the softdevice - uint8_t own_oob:1; - uint8_t peer_oob:1; }; nRF5xSecurityManager::nRF5xSecurityManager() @@ -663,26 +658,37 @@ ble_error_t nRF5xSecurityManager::secure_connections_oob_request_reply( const oob_lesc_value_t &peer_random, const oob_confirm_t &peer_confirm ) { + bool have_oob_own; + bool have_oob_peer; + const oob_lesc_value_t zerokey; + ble_gap_lesc_oob_data_t oob_own; + ble_gap_lesc_oob_data_t oob_peer; + pairing_control_block_t* pairing_cb = get_pairing_cb(connection); if (!pairing_cb) { return BLE_ERROR_INVALID_STATE; } - ble_gap_lesc_oob_data_t oob_own; - ble_gap_lesc_oob_data_t oob_peer; - - // is own address important ? - memcpy(oob_own.r, local_random.data(), local_random.size()); - // FIXME: What to do with local confirm ??? + have_oob_own = false; + if (local_random != zerokey) { + have_oob_own = true; + // is own address important ? + memcpy(oob_own.r, local_random.data(), local_random.size()); + // FIXME: What to do with local confirm ??? + } - // is peer address important ? - memcpy(oob_peer.r, peer_random.data(), peer_random.size()); - memcpy(oob_peer.c, peer_confirm.data(), peer_confirm.size()); + have_oob_peer = false; + if (peer_random != zerokey && peer_confirm != zerokey) { + have_oob_peer = true; + // is peer address important ? + memcpy(oob_peer.r, peer_random.data(), peer_random.size()); + memcpy(oob_peer.c, peer_confirm.data(), peer_confirm.size()); + } uint32_t err = sd_ble_gap_lesc_oob_data_set( connection, - pairing_cb->own_oob ? &oob_own : NULL, - pairing_cb->peer_oob ? &oob_peer : NULL + have_oob_own ? &oob_own : NULL, + have_oob_peer ? &oob_peer : NULL ); return convert_sd_error(err);