Skip to content

Fix Out-Of-Band (OOB) data generation for BLE OOB pairing #9339

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 18, 2019
Merged

Fix Out-Of-Band (OOB) data generation for BLE OOB pairing #9339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
55507ea
Fix race condition when generating OOB data
costanic Jan 8, 2019
24d793c
Fix parameters provided to oob generator function
costanic Jan 8, 2019
7795e30
Remove own_oob and peer_oob flags from Nordic PAL
costanic Jan 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions features/FEATURE_BLE/source/generic/GenericSecurityManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This modification to _oob_local_random should be reverted if generate_secure_connections_oob fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no problem, should be easy enough.


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) {
Copy link
Member

@paul-szczepanek-arm paul-szczepanek-arm Jan 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you need to now: return status; below (on line 676 using the new count)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

status is returned on line 670 for the failure case. Do you mean you want BLE_ERROR_NOT_IMPLEMENTED to be able to be propagated back to the user? This was not the case originally.

Copy link
Member

@paul-szczepanek-arm paul-szczepanek-arm Jan 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right, it needs to be hidden - bundling this together with legacy was not a good idea, we have to hide the SC failure since it's optional

_oob_local_address = orig_local_address;
_oob_local_random = orig_local_random;
return status;
}
} else {
Expand Down
43 changes: 26 additions & 17 deletions ...ts/TARGET_NORDIC/TARGET_NORDIC_SOFTDEVICE/TARGET_NRF52/source/nRF5xPalSecurityManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/

#include <stdint.h>
#include "platform/mbed_assert.h"
#include "nRF5xPalSecurityManager.h"
#include "nRF5xn.h"
#include "ble/Gap.h"
Expand Down Expand Up @@ -80,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()
Expand Down Expand Up @@ -662,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);
Expand Down Expand Up @@ -734,7 +741,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,
Expand Down