BLE: Security Manager #6188

paul-szczepanek-arm · 2018-02-23T14:18:47Z

Description

The existing Security Manager is very limited. This extends the API and actually implements the functionality. So far this only works for hardware using the Cordio stack and will require porting from maintainers of other devices.

This pull request includes legacy pairing and link security. Secure Connections are included but not tested due to lack of hardware targets supporting it.

Tested on the cordio stack port for the ST nucleo and discovery using the new Security Manager example (ARMmbed/mbed-os-example-ble#136).

(Handbook: ARMmbed/mbed-os-5-docs#418)

Pull request type

Fix
Refactor
New Target
Feature

…/mbed into palsm-cordio-implementation

Palsm cordio and Nordic implementation

…d, moved passkey classes to pal

… into pr-sm

pan- · 2018-03-01T17:23:43Z

features/FEATURE_BLE/ble/SecurityManager.h

+         *
+         * @param[in] connectionHandle connection connectionHandle
+         */
+        virtual void confirmationRequest(ble::connection_handle_t connectionHandle) {


What are you supposed to confirm ? I believe this should take the value to confirm in parameter.

those comments are bad, will clarify, this is confirmation that passkeys match

How do you push the confirmation value to the user ? According to the specification:

If both devices have output capabilities, this step requires the displaying of a user confirmation value. This value should be displayed until the end of step 2. If one or both devices do not have output capabilities, the same protocol is used but the Hosts will skip the step asking for the user confirmation.

User confirmation is required in Numeric Comparison; not Passkey entry.

you use displayPasskey on both (supplying the number to display) and then request confirmation on one or both and application calls confirmationEntered

Yes you're right!

added diagram to overview doc and comments in doxygen

paul-szczepanek-arm · 2018-03-01T17:36:10Z

features/FEATURE_BLE/source/generic/GenericSecurityManager.cpp

+    }
+
+    // set the distribution flags in the db
+    _db.set_distribution_flags(cb->db_entry, *cb);


deref of cb outside the cb check

paul-szczepanek-arm · 2018-03-01T17:36:28Z

features/FEATURE_BLE/source/generic/GenericSecurityManager.cpp

+    ControlBlock_t *cb = get_control_block(connection);
+    if (cb) {
+        if (cb->encryption_requested) {
+            enable_encryption(connection);


not needed as already encrypted

…l check

cmonr · 2018-03-01T17:57:44Z

Heads up @paul-szczepanek-arm, there's now a conflicting file in master.

pan-

Excellent; I've left few comments for the last mistakes.

pan- · 2018-03-01T19:58:16Z

features/FEATURE_BLE/ble/SecurityManager.h

@@ -14,16 +14,150 @@
 * limitations under the License.
 */

-#ifndef __SECURITY_MANAGER_H__
-#define __SECURITY_MANAGER_H__
+#ifndef _SECURITY_MANAGER_H_


Identifier starting with an underscore and an uppercase letter are reserved :(.

pan- · 2018-03-01T20:01:01Z

features/FEATURE_BLE/ble/SecurityManager.h

+ *
+ *
+ * Sequence diagram "Just Works" pairing
+ *


It might be interesting to surround diagrams with verbatim and endverbatim tags: https://www.stack.nl/~dimitri/doxygen/manual/commands.html#cmdverbatim

pan- · 2018-03-01T20:01:38Z

features/FEATURE_BLE/ble/SecurityManager.h

+ * it. If either side doesn't support it Legacy Pairing will be used. This is an older standard of pairing.
+ * If higher security is required legacy pairing can be disabled by calling allowLegacyPairing(false);
+ *
+ * How to use


It maybe interresting to use the tag par: https://www.stack.nl/~dimitri/doxygen/manual/commands.html#cmdpar

added paragraphs

pan- · 2018-03-01T20:02:27Z

features/FEATURE_BLE/ble/SecurityManager.h

+         * @param[in] whitelist pointer to the whitelist filled with entries based on bonding information
+         */
+        virtual void whitelistFromBondTable(Gap::Whitelist_t* whitelist) {
+            if (whitelist) {


This must go.

pan- · 2018-03-01T20:05:21Z

features/FEATURE_BLE/ble/generic/GenericSecurityManager.h

+namespace ble {
+namespace generic {
+
+using pal::advertising_peer_address_type_t;


using directives shouldn't be used in headers.

pan- · 2018-03-01T20:06:57Z

features/FEATURE_BLE/ble/pal/ConnectionEventMonitor.h

+ * Implemented by classes that are reacting to connection changes.
+ * @see ConnectionEventMonitor
+ */
+class ConnectionEventHandler {


This should be an inner class of ConnectionEventMonitor.

pan- · 2018-03-01T20:07:25Z

features/FEATURE_BLE/ble/pal/MemorySecurityDB.h

+ * limitations under the License.
+ */
+
+#ifndef _PAL_MEMORY_SECURITY_DB_H_


The leading underscore should be removed.

pan- · 2018-03-01T20:12:34Z

features/FEATURE_BLE/tests/generic/SecurityManager/mock/MockSecurityManagerEventHandler.h

@@ -0,0 +1,72 @@
+/* mbed Microcontroller Library


No idea how it gets here. This file should be removed.

pan-

Cheer. LGTM!

cmonr · 2018-03-01T20:44:06Z

Woo! Let's kick off CI for this PR!

/morph build

mbed-ci · 2018-03-01T21:14:27Z

Build : SUCCESS

Build number : 1318
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6188/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-03-01T23:10:47Z

Exporter Build : FAILURE

Build number : 978
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6188/

cmonr · 2018-03-01T23:22:04Z

A single Windows VM had issues pulling sources to export. Restarting the build.

/morph export-build

mbed-ci · 2018-03-02T00:23:15Z

Exporter Build : SUCCESS

Build number : 979
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6188/

mbed-ci · 2018-03-02T02:05:08Z

Test : SUCCESS

Build number : 1103
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6188/1103

pan- and others added 30 commits January 17, 2018 15:50

Nordic BLE: SM pal function draft (legacy pairing)

8c1dd2f

Nordic BLE: Draft SM event handler.

074597c

CORDIO BLE: Add PAL SM event handling (draft).

ffb59ee

BLE - Nordic: Add Skeleton for the security manager

527a316

BLE: Fix signature of get_secure_connections_support in implementations.

5277f5a

Nordic BLE: SM pal function draft (legacy pairing)

7e37cc7

Nordic BLE: Draft SM event handler.

743e57f

Merge branch 'palsm-cordio-implementation' of https://github.com/pan-…

39141fa

…/mbed into palsm-cordio-implementation

shared types in common header, connected up the events in handlers

2f04aec

removed old type

b00b746

use common io caps type

8db6427

todo

25107b1

Merge pull request #7 from pan-/palsm-cordio-implementation

5a056a1

Palsm cordio and Nordic implementation

key distribution now a class

01cdceb

removed old events that are now only in legacy handling

73e5f2d

updated implementations with new classes for auth and distribution masks

dee4777

updated old key_ditribution type and fixed refs to pointers

779d6eb

pariring typo

3f26435

fixed refs to arrays

b63930f

fixed the pointers to pointers

b88a270

missed one pointer to pointer

82c0847

io caps and key size now stored by stack, secured cahgned to encrypte…

021a1fd

…d, moved passkey classes to pal

dropped redundant passkey num class

9c1444f

propagated changes from pal to api and generic

2fe405a

link security reading from db to determine stats

081b99e

set io caps, removed redundant params from pairing request

ab056aa

constness and refs fixes

b60a464

fixed pal name of oob request

c3b434b

fixed non-matching types

8b36d5e

BLE SM: Fix compilation errors

44a366d

paul-szczepanek-arm and others added 5 commits March 1, 2018 15:29

removed get encryption key size from API

392ee6e

BLE: Remove Nordic Pal SM draft.

03f79ee

Merge branch 'master' of https://github.com/paul-szczepanek-arm/mbed-os…

8e21b53

… into pr-sm

BLE: Resolve issue with address type in advertising report.

f040569

removed redundant event for SC OOB request, fixed comments

e706b41

pan- reviewed Mar 1, 2018

View reviewed changes

paul-szczepanek-arm commented Mar 1, 2018

View reviewed changes

no need to ask for encryption after pairing, fold deref under the nul…

68f2561

…l check

paul-szczepanek-arm added 4 commits March 1, 2018 18:11

Merge https://github.com/ARMmbed/mbed-os

8347e76

more detailed comment for confirmation request

1f52e42

added one more sequence diagram

371f183

security manager overview doc fixes

ded1403

pan- reviewed Mar 1, 2018

View reviewed changes

Addressed Vincent's comments

a3383c1

donatieng force-pushed the master branch from 6fd6eec to a3383c1 Compare March 1, 2018 20:33

pan- approved these changes Mar 1, 2018

View reviewed changes

cmonr added needs: CI and removed needs: review labels Mar 1, 2018

cmonr added ready for merge and removed needs: CI labels Mar 2, 2018

adbridge merged commit ed17033 into ARMmbed:master Mar 2, 2018

adbridge removed the ready for merge label Mar 2, 2018

BLE: Security Manager #6188

BLE: Security Manager #6188

Uh oh!

Conversation

paul-szczepanek-arm commented Feb 23, 2018

Description

Pull request type

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pan- Mar 1, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmonr commented Mar 1, 2018

Uh oh!

pan- left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pan- left a comment

Choose a reason for hiding this comment

Uh oh!

cmonr commented Mar 1, 2018

Uh oh!

mbed-ci commented Mar 1, 2018

Triggering tests

Uh oh!

mbed-ci commented Mar 1, 2018

Uh oh!

cmonr commented Mar 1, 2018

Uh oh!

mbed-ci commented Mar 2, 2018

Uh oh!

mbed-ci commented Mar 2, 2018

Uh oh!

Uh oh!

pan- Mar 1, 2018 •

edited

Loading