Privacy #151

paul-szczepanek-arm · 2018-05-25T17:35:35Z

No description provided.

put local address printing in it's own func

needs adding whitelist first

…g the peer

donatieng · 2018-05-29T15:46:27Z

BLE_Privacy/mbed-os.lib

@@ -0,0 +1 @@
+https://github.com/ARMmbed/mbed-os/#f9ee4e849f8cbd64f1ec5fdd4ad256585a208360


That's mbed OS 5.8.2 (does that work?), please make it point to mbed OS 5.9 RC1 :) (6817dc43f9f5216cbe077059fc561fa89a766dc9 )

yes, thank you

donatieng · 2018-05-29T15:53:57Z

BLE_Privacy/source/main.cpp

+            printf("Whitelist failed to generate.\r\n");
+        }
+
+        /* this callback transfer memory ownership to us so we have to dispose of it */


Actually we have ownership of it during the whole process.

we hand it over with generateWhitelistFromBondTable(whitelist) after which we are not allowed to delete it before the callback happens and we do not store a pointer to it so effectively we are handing it over

I think we should look into using shared_ptr-type abstractions (which exist in mbed OS) when it makes sense - but that's a separate topic. Looking at the doc for generateWhitelistFromBondTable(), it's not very clear what the object lifetime should be.

I will now seemingly backtrack on what I said: lifetime is decided by the user, the whitelist could be statically allocated, in my example I relinquish control but the doc doesn't require that: the only requirement is that you do not access the whitelist when callback is pending.

donatieng · 2018-05-29T15:56:23Z

BLE_Privacy/source/main.cpp

+        }
+    };
+
+    /** advertise and filer based on known devices */


filer --> filter

donatieng · 2018-05-29T18:21:29Z

BLE_Privacy/source/main.cpp

+            printf("Whitelist failed to generate.\r\n");
+        }
+
+        /* this callback transfer memory ownership to us so we have to dispose of it */


I think we should look into using shared_ptr-type abstractions (which exist in mbed OS) when it makes sense - but that's a separate topic. Looking at the doc for generateWhitelistFromBondTable(), it's not very clear what the object lifetime should be.

donatieng · 2018-05-29T19:10:22Z

BLE_Privacy/source/main.cpp

+             * byte 1: The key, it is the type of the data
+             * byte [2..N] The value. N is equal to byte0 - 1 */
+
+            const uint8_t record_length = params->advertisingData[i];


We need a length check: Fail if i + 2 + record_length >= params->advertisingDataLen otherwise we're vulnerable to buffer overflow attacks.

...in which case I should also not trust that the string is well formatted.

What we really need is the on_scan callback to return a typed list of items and not having the user look at bytes.

That's different: Badly formatted buffer: we get garbage that we can ignore; String with length flag set to an invalid value (which is a specific case of the former): undefined behaviour.

Agreed that in the long run which should do that parsing ourselves.

donatieng · 2018-05-29T19:10:54Z

BLE_Privacy/source/main.cpp

+        }
+        {
+            printf("\r\n * Device is a central *\r\n\r\n");
+            PrivacyCentral peripheral(ble, queue);


Should be called central 😄

paul-szczepanek-arm · 2018-05-30T10:44:04Z

addressed your comments

pan-

Good example, it works quite well with all the fixes.
I wouldn't use the white list at all as it is a feature orthogonal to privacy.

pan- · 2018-05-31T08:39:46Z

BLE_Privacy/mbed_app.json

+            "target.features_add": ["BLE"],
+            "target.extra_labels_add": ["ST_BLUENRG"]
+        },
+        "NUCLEO_F401RE": {


The cordio driver is not public.
I would remove st support as st own implementation doesn't implement the security manager.

pan- · 2018-05-31T08:40:14Z

BLE_Privacy/module.json

@@ -0,0 +1,15 @@
+{


This can be removed as we don't care of yotta anymore.

pan- · 2018-05-31T08:41:54Z

BLE_Privacy/shields/TARGET_ST_BLUENRG.lib

@@ -0,0 +1 @@
+https://github.com/ARMmbed/cordio-ble-x-nucleo-idb0xa1


This repository is not public; This file can be removed for now; please see my comment above.

pan- · 2018-05-31T09:00:22Z

BLE_Privacy/source/main.cpp

+                _bonded = true;
+
+                /* provide memory for the generated whitelist */
+                Gap::Whitelist_t* whitelist = new Gap::Whitelist_t;


Maybe the white list can go into a member variable that would makes the whole code a bit more readable.

pan- · 2018-05-31T09:10:07Z

BLE_Privacy/source/main.cpp

+
+        /* for use by tools we print out own address and also use it
+         * to seed RNG as the address is unique */
+        print_local_address();


Printing the local address is useless as privacy is enabled few lines below.

we use it in our tools and I use it as seed and it shows the identity address which we will later see on the other side

We use it in our tools to identify the device advertising unfortunately this won't work this time as the address is private. I would suggest to add a random vendor specific field in the advertising payload and print it so we can identify the device advertising.

Note: this is not a blocker for this to get in but it will be once we add tests for this example.

It is harmless and it does show the identity address before the privacy is enabled so you can see that you're connecting to the right device - connections is based on name

pan- · 2018-05-31T09:52:26Z

BLE_Privacy/source/main.cpp

+        }
+    };
+
+    /* TODO: remove this, pairing should be done by the strategy automatically */


Please remove this as it should be done by the strategy.

pan- · 2018-05-31T10:15:33Z

BLE_Privacy/source/main.cpp

+    virtual ~PrivacyDevice()
+    {
+        if (_ble.hasInitialized()) {
+            _ble.shutdown();


I wouldn't initialize and shutdown the ble instance as the process is still instable on Cordio as there is not enough stack support.
Initialization the stack once and leave it active would be better.

pan- · 2018-05-31T10:16:44Z

BLE_Privacy/source/main.cpp

+        ble_error_t error;
+
+        /* to show we're running we'll blink every 500ms */
+        _event_queue.call_every(500, this, &PrivacyDevice::blink);


This event must be removed at destruction.

queue now is recreated

pan- · 2018-05-31T14:18:24Z

BLE_Privacy/source/main.cpp

+        GapAdvertisingData advertising_data;
+
+        /* add advertising flags */
+        advertising_data.addFlags(GapAdvertisingData::LE_GENERAL_DISCOVERABLE


If you intend to use the whitelist then remove the discoverable flag.

pan- · 2018-05-31T14:19:52Z

BLE_Privacy/source/main.cpp

+
+            if (record_length < 2) {
+                /* malformed record */
+            } else if ((type == GapAdvertisingData::FLAGS)) {


This bit must be removed if the flag is not set in advertising data.

paul-szczepanek-arm · 2018-05-31T17:11:02Z

~~This still requires fixing gap so that restarting scanning works~~
worked around the problem by not using the set timeout call, it's good to go now

pan-

There is still an issue with the event queue and ble signaling. As it is now it may operates with dead objects as the function onEventsToProcess is not replaced in the PrivacyDevice destructor.

paul-szczepanek-arm · 2018-06-01T08:15:13Z

thanks, fixed

pan-

Thanks for the change to the signaling strategy but it is not enough as ble may continue to report events after a PrivacyDevice destruction.

paul-szczepanek-arm · 2018-06-01T08:30:23Z

thank you

pan-

LGTM; what do you think @donatieng ?

donatieng · 2018-06-03T20:13:48Z

BLE_Privacy/source/main.cpp

+             * byte 1: The key, it is the type of the data
+             * byte [2..N] The value. N is equal to byte0 - 1 */
+
+            const uint8_t record_length = params->advertisingData[i];


That's different: Badly formatted buffer: we get garbage that we can ignore; String with length flag set to an invalid value (which is a specific case of the former): undefined behaviour.

Agreed that in the long run which should do that parsing ourselves.

paul-szczepanek-arm added 6 commits May 25, 2018 14:37

privacy example

96d3479

missing file

3ba98cc

seed from address

d041f13

read device name

00289b1

print error on privacy init error

2e9ca57

correct adv type

2bdd43a

paul-szczepanek-arm requested review from pan- and donatieng May 25, 2018 17:35

paul-szczepanek-arm added 13 commits May 25, 2018 18:45

pair manually

93a2331

missing virtual

571200d

shorter advertising

8d750b3

continue demo forever with fixed roles

ab7cea5

put local address printing in it's own func

stop scan

bc23457

remove filtering

892e645

needs adding whitelist first

remove redundancy

ee3144e

generate whitelist, adapt if no whitelist

ecccca8

syntax fix

3278a82

more documentation

90f1f16

fix compiler warnings

10a013b

put seeding after init and remove delay to maximise chances of meetin…

168a64d

…g the peer

handle not implemented whitelist generation

ec866c4

donatieng reviewed May 29, 2018

View reviewed changes

paul-szczepanek-arm added 2 commits May 29, 2018 17:02

handle failed setting of whitelist

5e14402

point at 5.9

23ca047

donatieng suggested changes May 29, 2018

View reviewed changes

review comments, fix security risks

632ff38

fix not checking enable privacy error

b7e0d49

pan- requested changes May 31, 2018

View reviewed changes

remove overload for on_disconnect since strategy works

ad14457

paul-szczepanek-arm added 6 commits May 31, 2018 15:30

remove st cordio

9e566b9

remove flags

8060fdf

recreate the queue for every run

e5c9080

remove whitelist from example

d041131

don't reinit the ble and disconnect after bond

7da9b62

fix not running dispatch

7063af6

paul-szczepanek-arm added 2 commits May 31, 2018 18:23

workaround the set interval bug in gap

e29d2bf

don't print local address after privacy is turned on

b0d87ad

pan- requested changes Jun 1, 2018

View reviewed changes

reset events callback

12077e2

pan- requested changes Jun 1, 2018

View reviewed changes

reset event handling at destruction

efe7ca1

pan- approved these changes Jun 1, 2018

View reviewed changes

donatieng approved these changes Jun 3, 2018

View reviewed changes

paul-szczepanek-arm merged commit 9985858 into ARMmbed:master Jun 4, 2018

		@@ -0,0 +1 @@
		https://github.com/ARMmbed/mbed-os/#f9ee4e849f8cbd64f1ec5fdd4ad256585a208360

		@@ -0,0 +1 @@
		https://github.com/ARMmbed/cordio-ble-x-nucleo-idb0xa1

Privacy #151

Privacy #151

Uh oh!

Conversation

paul-szczepanek-arm commented May 25, 2018

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!

paul-szczepanek-arm May 29, 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!

donatieng Jun 3, 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!

paul-szczepanek-arm commented May 30, 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!

pan- Jun 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!

Choose a reason for hiding this comment

Uh oh!

paul-szczepanek-arm commented May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pan- left a comment

Choose a reason for hiding this comment

Uh oh!

paul-szczepanek-arm commented Jun 1, 2018

Uh oh!

pan- left a comment

Choose a reason for hiding this comment

Uh oh!

paul-szczepanek-arm May 29, 2018 •

edited

Loading

donatieng Jun 3, 2018 •

edited

Loading

pan- Jun 1, 2018 •

edited

Loading

paul-szczepanek-arm commented May 31, 2018 •

edited

Loading

donatieng Jun 3, 2018 •

edited

Loading