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

costanic · 2019-01-10T15:32:29Z

Description

OOB data is cryptographic material exchanged by two peers prior to pairing with the BLE OOB pairing method. After trading OOB data, during the BLE OOB pairing process, peers exchange their public keys, validate the previously exchanged OOB data against the peer's public key, and calculate encryption keys using a combination of the peer's public key and OOB data and the local private key. In this way, man in the middle attacks and passive eavesdropping are prevented.

There are a few bugs in the BLE GenericSecurityManager and Nordic PAL SecurityManager that prevent OOB data from being generated correctly such that a peer is unable to mathematically validate the OOB data and public key received from the Nordic device.

These issues occur on a Nordic nRF52-DK and prevent the nRF52-DK from pairing with a Linux device using the OOB pairing method. With these fixes, the Nordic nRF52-DK successfully pairs with a Linux device using the OOB pairing method.

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

ciarmcom · 2019-01-10T16:00:38Z

@costanic, thank you for your changes.
@ARMmbed/mbed-os-pan @ARMmbed/mbed-os-maintainers please review.

pan-

Thanks @costanic for this submission and the detailed commit messages. Here's my comments:

pan- · 2019-01-10T17:24:48Z

features/FEATURE_BLE/source/generic/GenericSecurityManager.cpp

+        _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);


This modification to _oob_local_random should be reverted if generate_secure_connections_oob fail.

no problem, should be easy enough.

pan- · 2019-01-10T17:56:20Z

...rgets/TARGET_NORDIC/TARGET_NORDIC_SOFTDEVICE/TARGET_NRF52/source/nRF5xPalSecurityManager.cpp

@@ -681,8 +676,8 @@ ble_error_t nRF5xSecurityManager::secure_connections_oob_request_reply(

    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
+        &oob_own,


Content in oob_own or oob_peer should be all zeros if not available (according to the BT spec); however the nordic documentation state that we should pass NULL when this is the case. I wonder if we should look at the content then pass NULL if N/A instead of unconditionally passing the value.

ok, i'll take a closer look

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.

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.

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.

costanic · 2019-01-11T14:57:16Z

Thanks for the feedback. I've updated the PR based on review comments.

paul-szczepanek-arm · 2019-01-11T16:00:54Z

features/FEATURE_BLE/source/generic/GenericSecurityManager.cpp

-            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) {


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

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.

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

costanic · 2019-01-15T15:11:34Z

Any additional feedback on this PR?

pan-

It looks all good on my end.
@paul-szczepanek-arm Anything to add ?

cmonr · 2019-01-17T05:15:37Z

CI started

mbed-ci · 2019-01-17T10:31:17Z

Test run: FAILED

Summary: 1 of 7 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-IAR

0xc0170 · 2019-01-17T10:31:23Z

One breakage on master needs to be addressed asap, the current build was aborted and will be restarted within today.

0xc0170 · 2019-01-17T16:48:32Z

CI restarted

mbed-ci · 2019-01-17T23:50:40Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts

ciarmcom requested a review from a team January 10, 2019 16:00

ciarmcom added the needs: review label Jan 10, 2019

pan- reviewed Jan 10, 2019

View reviewed changes

costanic added 3 commits January 11, 2019 08:54

costanic force-pushed the fix_oob branch from 7413433 to 7795e30 Compare January 11, 2019 14:54

paul-szczepanek-arm reviewed Jan 11, 2019

View reviewed changes

pan- approved these changes Jan 15, 2019

View reviewed changes

paul-szczepanek-arm approved these changes Jan 15, 2019

View reviewed changes

cmonr approved these changes Jan 17, 2019

View reviewed changes

cmonr added needs: CI release-version: 5.11.3 and removed needs: review labels Jan 17, 2019

cmonr added ready for merge and removed needs: CI labels Jan 18, 2019

cmonr merged commit fd2a96e into ARMmbed:master Jan 18, 2019

cmonr removed the ready for merge label Jan 18, 2019

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

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

Uh oh!

Conversation

costanic commented Jan 10, 2019

Description

Pull request type

Reviewers

Uh oh!

ciarmcom commented Jan 10, 2019

Uh oh!

pan- left a comment

Choose a reason for hiding this comment

Uh oh!

pan- Jan 10, 2019

Choose a reason for hiding this comment

Uh oh!

costanic Jan 10, 2019

Choose a reason for hiding this comment

Uh oh!

pan- Jan 10, 2019

Choose a reason for hiding this comment

Uh oh!

costanic Jan 10, 2019

Choose a reason for hiding this comment

Uh oh!

costanic commented Jan 11, 2019

Uh oh!

paul-szczepanek-arm Jan 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costanic Jan 11, 2019

Choose a reason for hiding this comment

Uh oh!

paul-szczepanek-arm Jan 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costanic commented Jan 15, 2019

Uh oh!

pan- left a comment

Choose a reason for hiding this comment

Uh oh!

cmonr commented Jan 17, 2019

Uh oh!

mbed-ci commented Jan 17, 2019

Test run: FAILED

Uh oh!

0xc0170 commented Jan 17, 2019

Uh oh!

0xc0170 commented Jan 17, 2019

Uh oh!

mbed-ci commented Jan 17, 2019

Test run: SUCCESS

Uh oh!

Uh oh!

paul-szczepanek-arm Jan 11, 2019 •

edited

Loading

paul-szczepanek-arm Jan 11, 2019 •

edited

Loading