Skip to content
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.

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

Help with the new security #11

Closed
mitchjs opened this issue Jun 13, 2020 · 50 comments · Fixed by #164
Closed

Help with the new security #11

mitchjs opened this issue Jun 13, 2020 · 50 comments · Fixed by #164

Comments

@mitchjs
Copy link

mitchjs commented Jun 13, 2020

@h2zero, im converting a project from the old API
which i had security working...
so the new way to have it going

i have this

NimBLECharacteristic *MyLEDStatusCharacteristic = pService->createCharacteristic(CHARACTERISTIC_UUID_LED_STATUS, NIMBLE_PROPERTY::READ | NIMBLE_PROPERTY::NOTIFY);

which works, to add securtity i would go

NimBLECharacteristic *MyLEDStatusCharacteristic = pService->createCharacteristic(CHARACTERISTIC_UUID_LED_STATUS, NIMBLE_PROPERTY::READ_ENC | NIMBLE_PROPERTY::NOTIFY);

once i do that, read isnt even showing up as a property for this characteristic

i also had on my 0x2902
MyLEDStatusNotificationDescriptor->setAccessPermissions(ESP_GATT_PERM_READ_ENCRYPTED | ESP_GATT_PERM_WRITE_ENCRYPTED);
so that one cant subscibe with out being authenticated

and this

NimBLESecurity *pSecurity = new NimBLESecurity();
pSecurity->setStaticPIN(123456);
pSecurity->setAuthenticationMode(ESP_LE_AUTH_REQ_SC_BOND);

does this simply turn into:

NimBLEDevice::setSecurityPasskey(123456);
NimBLEDevice::setSecurityAuth(BLE_SM_PAIR_AUTHREQ_BOND);

not getting security to work at all :)

thanks

@h2zero
Copy link
Owner

h2zero commented Jun 13, 2020

You'll need to change:

NimBLECharacteristic *MyLEDStatusCharacteristic = pService->createCharacteristic(CHARACTERISTIC_UUID_LED_STATUS, 
NIMBLE_PROPERTY::READ_ENC | 
NIMBLE_PROPERTY::NOTIFY);

to:

NimBLECharacteristic *MyLEDStatusCharacteristic = pService->createCharacteristic(CHARACTERISTIC_UUID_LED_STATUS, 
NIMBLE_PROPERTY::READ | 
NIMBLE_PROPERTY::READ_ENC | 
NIMBLE_PROPERTY::NOTIFY);

For some reason this is how NimBLE set it up, I should consider making that automatic in the library.

For the 2902 you would set the same properties but with NIMBLE_PROPERTY::WRITE_ENC added.

You can use the security class as you have it there and it should work fine. The other option to do the same this would be to use:
NimBLEDevice::setSecurityIOCap(BLE_HS_IO_DISPLAY_ONLY);
And either

NimBLEDevice::setSecurityAuth(false, false, true); 

or

NimBLEDevice::setSecurityAuth(BLE_SM_PAIR_AUTHREQ_BOND | 
BLE_SM_PAIR_AUTHREQ_MITM | 
BLE_SM_PAIR_AUTHREQ_SC);

and finally
NimBLEDevice::setSecurityPasskey(123456);

@mitchjs
Copy link
Author

mitchjs commented Jun 13, 2020

ok, i got

NimBLEDevice::setSecurityIOCap(BLE_HS_IO_DISPLAY_ONLY);
NimBLEDevice::setSecurityAuth(BLE_SM_PAIR_AUTHREQ_BOND);
NimBLEDevice::setSecurityPasskey(123456);

some other wierd stuff.. im trying to sort
(mostly in nRF Connect, god i hate this program... lightblue better, what else is there?)

i didnt create the 2902, its done by creatcharacteristic (since notifiy property there)
how do add that security?

@h2zero
Copy link
Owner

h2zero commented Jun 13, 2020

(mostly in nRF Connect, god i hate this program... lightblue better, what else is there?)

ble scanner is pretty good.

i didnt create the 2902, its done by creatcharacteristic (since notifiy property there)
how do add that security?

It's strange that nimble doesn't handle this in the stack. I'm not sure how best to sort out this issue, might need to modify the library for this.

@mitchjs
Copy link
Author

mitchjs commented Jun 13, 2020

i had that app long ago..didnt work well.. trying now...
as soon as it connected it asked me to pair... and it(the app) had notify enabled by default

yikes; now i cant connect anymore, just says "connecting fail from peripheral"
yep its now only allows 1 time.. i have to delete the paring in bluetooth settings to connect again
something is wrong
the device is advertisting after disconnection...
but no app will connect to it now
unless i delete the pairing in ios

so logging says when i try to reconnect
I (98189) DS-EZLOCK-ECM9(BLE): BT - connected
I (98509) DS-EZLOCK-ECM9(BLE): BT - disconnected
GAP procedure initiated: advertise; disc_mode=2 adv_channel_map=0 own_addr_type=0 adv_filter_policy=0 adv_itvl_min=0 adv_itvl_max=0
so i get into onConnect() and right after that OnDisconnect()

@mitchjs
Copy link
Author

mitchjs commented Jun 13, 2020

i switched to my "testing" code and same issue
attached is my super simple code

main.cpp.txt

@mitchjs
Copy link
Author

mitchjs commented Jun 13, 2020

i changed to this

NimBLEDevice::setSecurityIOCap(BLE_HS_IO_DISPLAY_ONLY);
NimBLEDevice::setSecurityAuth(false, false, true);
NimBLEDevice::setSecurityPasskey(123456);

and i can connect over and over, i just have to put in the passkey every time

@h2zero
Copy link
Owner

h2zero commented Jun 13, 2020

If you bond, the keys are persisted in the phone and on the esp32, if you delete the bond on one of them they both need to be deleted (library will detect this and do it for you but you need to reconnect).

NimBLEDevice::setSecurityAuth(false, false, true); will do pairing only and will not persist the keys.

If you want to use bonding with a passkey NimBLEDevice::setSecurityAuth(true, true, true); is what you need to use, but it's a pain while testing so I leave it off until I put the device "into production".

Edit: also I would recommend using erase_flash on the command line when you flash the device to clear out any stored bond data.

@mitchjs
Copy link
Author

mitchjs commented Jun 13, 2020

hmm,,, if i let it bond, i cant connect to it again...
only once

I (114939) mycallback: onConnect()

I (115159) mycallback: onDisconnect()
GAP procedure initiated: advertise; disc_mode=2 adv_channel_map=0 own_addr_type=0 adv_filter_policy=0 adv_itvl_min=0 adv_itvl_max=0

as you can see from upbove i get kicked right out

so... something is wrong for sure... as once i bond, and then disconnect, and just try to reconnect
nope..

this mean anything?

I NimBLEDevice: "BLE Host Task Started"
GAP procedure initiated: stop advertising.
GAP procedure initiated: stop advertising.
failed to configure restored IRK
GAP procedure initiated: stop advertising.
failed to configure restored IRK
GAP procedure initiated: stop advertising.
failed to configure restored IRK
I NimBLEDevice: "NimBle host synced."

(erase_flash, reloaded... no change)

do i need any of the security callbacks? to return something? you have default ones i see

@h2zero
Copy link
Owner

h2zero commented Jun 13, 2020

failed to configure restored IRK

That's the nimble stack letting us know what the issue is, I'm not sure why it would have this problem I would need to see the logs when it first connects and bonds to see where the issue is.

@h2zero
Copy link
Owner

h2zero commented Jun 13, 2020

Actually I think I know why, you are using IDF v4.0 correct? If so can you tell me the commit you're on. I made a PR on the esp-nimble repo to address this issue and it was merged a little while ago. If you don't have that then that's probably the cause.

@mitchjs
Copy link
Author

mitchjs commented Jun 13, 2020

yes. idf 4.0

not sure how exactly to switch to idf4.x im using thier "ESP-IDF Visual Studio Code Extension" i might be able to switch to 4. something...

actually says IDF v4.0.1

@mitchjs
Copy link
Author

mitchjs commented Jun 13, 2020

that IRK error happens at boot, and before any connections

@h2zero
Copy link
Owner

h2zero commented Jun 13, 2020

Yeah, that’s because it has stored bonds that it’s trying to load and cannot for some reason.

If you checkout my esp-nimble-component library and put that in your projects/components folder then disable nimble in menuconfig as you’ll be using the component instead I think you’ll find everything working.

If you prefer you could also copy the files from the NimBLE folder from that repo into your idf NimBLE folder and leave menuconfig alone.

There is one last option but we don’t want to go there lol, creates even more issues down the road. Best off if you can update nimble.

@h2zero
Copy link
Owner

h2zero commented Jun 13, 2020

Just so you know why this is happening here is the PR that fixes it.
espressif/esp-nimble#7

@mitchjs
Copy link
Author

mitchjs commented Jun 13, 2020

i switched to the MASTER IDF...
is it fixed in that? i still cant reconnect after a bond
im so confused, is the IRK error releated to the reason i cant reconnect to device after a bond?

looks like whats in the master IDF repository is linked to the master of the nimble repository
(and it looks like you fixes in it)
i dont see any IRK errors anymore

@h2zero
Copy link
Owner

h2zero commented Jun 14, 2020

This is odd, I don’t see any issues here on master branch, I’ll look into this more. I’ll get back to you after I do some testing.

@h2zero
Copy link
Owner

h2zero commented Jun 14, 2020

After you switched branches did you erase_flash?

You’ll need to start out clean once you have the updated nimble, also delete esp32 bonds from your phone.

@mitchjs
Copy link
Author

mitchjs commented Jun 14, 2020

im starting to loose track of what im doing... :(
i do delete the bonds, as it fails to connect with them in place

so... i now cant build nimble when on 4.0.1
C:/Users/mitchjs/esp-idf/components/bt/host/nimble/nimble/nimble/host/src/ble_hs.c:748: undefined reference to `ble_hs_periodic_sync_init'
at linking stage

going to IDF master, trying once more... will do erase_flash
ok... on IDF master, cleared flash, loaded code...
and i think we OK
it connected and i paired, and then i disconnected, and reconnected and boom it worked

what i think i want to be is at 4.0.1 (since that is considered stable) and have the lastest nimble
which i thought i did, but didnt fully compile that linker issue
ill get some sleep and try again...

@mitchjs
Copy link
Author

mitchjs commented Jun 14, 2020

it looks like i need to have the IDF master to get it all to work right
(doesn't encourage me on stability of IDF (esp32 in general) or their port of nimBLE)
using 4.0.1(stable) and the latest nimBLE might not be easy as that link error shows up.. .so something is missing as the method ble_hs_periodic_sync_init() is in the source... need to look at the error message in detail.. i think it should just work

@h2zero
Copy link
Owner

h2zero commented Jun 14, 2020

You can use any IDF version you wish actually. Just need to update the nimble sources to the master branch. Or use this and just put it in your project/components folder then you don't have to change IDF versions at all.

Are you using git or just downloading the installer?

@mitchjs
Copy link
Author

mitchjs commented Jun 14, 2020

i use git to get the IDF...
i tried idf v4.0.1 and putting in the newest nimble in and compiling, and it failed...
i will try again...

when i look at Nimble in 4.0.1
im a little confused
it shows im at head (detached)
clearly its not nimble-1.2.0-idf
(again my skills at git suffer)
i have done checkout --branch nibmle-1.2.0-idf
and then git pull
and i beleive i got the latest

mitchjs@Homer MINGW64 /e/esp32-idf/esp-idf/components/bt/host/nimble/nimble ((591721b7...))
$ git branch --all

  • (HEAD detached at 591721b7)
    nimble-1.2.0-idf
    remotes/origin/1_0_0_dev
    remotes/origin/HEAD -> origin/nimble-1.2.0-idf
    remotes/origin/freertos-port
    remotes/origin/idf
    remotes/origin/master
    remotes/origin/new-master
    remotes/origin/nimble-1.1.0-idf
    remotes/origin/nimble-1.1.0-idf-v3.3
    remotes/origin/nimble-1.1.0-idf-v3.3-afr
    remotes/origin/nimble-1.2.0-idf

@mitchjs
Copy link
Author

mitchjs commented Jun 14, 2020

its master that i want right?
to me nimble-1.2.0-idf looks newer

@h2zero
Copy link
Owner

h2zero commented Jun 14, 2020

You want 1.2.0-idf branch, that's the main one.

@mitchjs
Copy link
Author

mitchjs commented Jun 14, 2020

when i compile idf v4.0.1 with nimble-1.2.0-idf

cmd.exe /C "cd . && C:\Users\mitchjs\.espressif\tools\xtensa-esp32-elf\esp-2019r2-8.2.0\xtensa-esp32-elf\bin\xtensa-esp32-elf-g++.exe  -mlongcalls -Wno-frame-address  -nostdlib @CMakeFiles\MyCppTest.elf.rsp  -o MyCppTest.elf  && cd ."
c:/users/mitchjs/.espressif/tools/xtensa-esp32-elf/esp-2019r2-8.2.0/xtensa-esp32-elf/bin/../lib/gcc/xtensa-esp32-elf/8.2.0/../../../../xtensa-esp32-elf/bin/ld.exe: esp-idf/bt/libbt.a(ble_hs.c.obj):(.literal.ble_hs_init+0x30): undefined reference to `ble_hs_periodic_sync_init'
c:/users/mitchjs/.espressif/tools/xtensa-esp32-elf/esp-2019r2-8.2.0/xtensa-esp32-elf/bin/../lib/gcc/xtensa-esp32-elf/8.2.0/../../../../xtensa-esp32-elf/bin/ld.exe: esp-idf/bt/libbt.a(ble_hs.c.obj): in function `ble_hs_init':
C:/Users/mitchjs/esp-idf-v4.0.1/components/bt/host/nimble/nimble/nimble/host/src/ble_hs.c:748: undefined reference to `ble_hs_periodic_sync_init'
collect2.exe: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.

@h2zero
Copy link
Owner

h2zero commented Jun 14, 2020

I just tested this configuration and I can confirm, I think something is missing from cmakefiles in v4.0.1

@mitchjs
Copy link
Author

mitchjs commented Jun 14, 2020

thank you (for just letting me know its not me)
i did do a pull of idf v4.1-beta2, and then pulled nimble-1.2.0-idf

it built! and works as it should.. i can connect-pair-disconnect-reconnect

@mitchjs
Copy link
Author

mitchjs commented Jun 14, 2020

yup
the make file is different, for one missing

"host/nimble/nimble/nimble/host/src/ble_hs_periodic_sync.c"

i updated the cmake file and now it built
just did a compare and moved over the 3 missing that where in 4.1 to 4.0.1

@h2zero
Copy link
Owner

h2zero commented Jun 14, 2020

Nice! you beat me to it. I found the missing line in the make file but it still would not compile for me, kept telling me nimble was out of date... strange. Glad you got it sorted 👍

@mitchjs
Copy link
Author

mitchjs commented Jun 14, 2020

ok. got it working..
yes i get a warning
CMake Warning at C:/Users/mitchjs/esp-idf-v4.0.1/tools/cmake/git_submodules.cmake:52 (message):
Git submodule components/bt/host/nimble/nimble is out of date. Run 'git
submodule update --init --recursive' to fix.

but it builds now updated "esp_nimble_cfg." with the sync stuff that 1.2.0-idf uses
and got it built (YEASH)

thanks for you help... i think all this i learned alot about git and even cmake :)

@h2zero
Copy link
Owner

h2zero commented Jun 14, 2020

Yeah sorry for all that, I wish espressif would backport the nimble updates, it’s far more stable now than it is in those releases.

@mitchjs
Copy link
Author

mitchjs commented Jun 15, 2020

not your fault, thanks for help
the other issue.. ill open new issue (security on 0x2902 notifications)

@h2zero
Copy link
Owner

h2zero commented Jun 15, 2020

Sounds good and please do so it stays fresh and others can chime in.

@dorianim
Copy link

dorianim commented May 4, 2024

Hi there,
I am still running into the issue where iOS devices can only connect once when bonding is enabled. @mitchjs how exactly did you solve this issue?

@cmorganBE
Copy link
Contributor

cmorganBE commented May 16, 2024

@dorianim @mitchjs I'm in the same boat. Enable bonding, iOS can only connect once via lightblue, second time reports failed to connect. Have to delete the bond via Settings->Bluetooth before it will connect again. esp-idf v5.2, esp32-s3 here. Enabled 'nimble NVS persistence' via the appropriate kconfig setting.

@dorianim
Copy link

Hi @cmorganBE,
I'm on an S3 as well! And after adding the onAuthenticationComplete callback, I noticed that it is never called on iOS. So to me it looks like bonding never actually completes on the ESP side. That would explain the issue.

@cmorganBE
Copy link
Contributor

@dorianim does it get called for you on other platforms? Have you reported the issue on the espressif forums? They are usually pretty responsive but I wasn't sure if I was doing it wrong.

I see the bleprph example supports encryption and bonding. Trying it here the behavior is different, the bleprph example wants to pair right off, as soon as I hit connect. For the esp-nimble-cpp example it only prompts me to pair when accessing an encrypted property (maybe because I have a few that are like NOTIFY, or mfg/model that aren't encrypted).

But I can say that the bleprph example DOES allow my iOS device to reconnect after bonding. AND after I enabled 'nimble persist to NVS', I can reconnect without the pairing prompt after the esp32s3-devkit-c1 is rebooted.

So it looks like there could be something related to esp-nimble-cpp going on here but I'd have no idea where to start looking.

@dorianim
Copy link

@cmorganBE I have not tried other platforms yet. And I didn't report it on the espressif forums because I'm pretty sure it's not an issue with the esp/espidf but rather with this lib. The fact that the bleprph example works, confirms that.

@h2zero
Copy link
Owner

h2zero commented May 16, 2024

Try fully erasing the flash. Also please check your config for max bonds.

@dorianim
Copy link

dorianim commented May 16, 2024

@h2zero I check both, but with no success.
I used this snippet to list all bonds:

    ESP_LOGI(TAG, "Bonded devices:");
    for (int i = 0; i < NimBLEDevice::getNumBonds(); i++)
    {
        ESP_LOGI(TAG, "%d.: %s", i, NimBLEDevice::getBondedAddress(i).toString().c_str());
    }
  • After erasing the flash, there are no bonds
  • When bonding an Android device, it shows up
  • When bonding an iOS device, it does not show up (even after erasing the flash again)

@dorianim
Copy link

dorianim commented May 16, 2024

@h2zero @cmorganBE I put together a simple test repo here:
https://github.com/dorianim/esp-nimble-cpp-test

I tried it with a normal esp32 module and an esp32-s3. With the normal one, it works fine, but with the s3 it does not work after the first connection on iOS (I get "Peer removed pairing information" as the error message).
However, it works completely fine on Android. I tested with nRF Connect on both devices.

So, this seems to only occur on the esp32-s3 with iOS devices. Should I open a new, more specific issue?

@h2zero
Copy link
Owner

h2zero commented May 16, 2024

Thank you for that, I'll try it when I get a chance. Please open as a separate issue as it seems to only affect the S3 so it's cause will be different most likely.

@dorianim
Copy link

Just noticed that @cmorganBE already opened a separate issue:
#159

I'll try it when I get a chance

Thank you!

@cmorganBE
Copy link
Contributor

cmorganBE commented May 16, 2024 via email

@h2zero
Copy link
Owner

h2zero commented May 17, 2024

@cmorganBE That is certainly enticing, could you show me your server initialization code and service/characteristics setup?

@cmorganBE
Copy link
Contributor

cmorganBE commented May 17, 2024

@h2zero I modified dorianim's example after forking it here, https://github.com/cmorganBE/esp-nimble-cpp-test

Testing here on esp32s3-devkit-c1, esp-idf v5.2.1.

I did find an interesting result from this testing,

setSecurityAuth with secured connections is what is causing the issue with reconnecting after bonding. I don't understand BLE particularly well yet but its unclear why this is an issue. Shouldn't we want SC enabled? Maybe this falls into 'don't enable SC in this configuration this because of X' but I'd expect the library could tell me this is a problem if its a known bad configuration.

    // Setting A - WORKS
    // Pairs and bonds and will reconnect, even after esp32 reboot
//    NimBLEDevice::setSecurityAuth(true, false, false);

    // Setting B - DOESNT WORK
    // Will pair and bond but will NOT reconnect
    NimBLEDevice::setSecurityAuth(true, false, true);

Also the example exhibits the same issue as mine does, iOS prompts to pair ONLY when accessing a characteristic in lightblue. bleprph example prompts as soon as you select the device and lightblue shows the device info. I mention it in case its a hint about where/what things are going wrong.

Changes from his example are to drop back to 'just works' and comment out some setMinPreferred() calls (are these really helpful for iphones?)

@h2zero
Copy link
Owner

h2zero commented May 19, 2024

@cmorganBE Yes, I noticed the same behavior as well, there is no issues when secure connections is disabled but fails otherwise. This is definitely not something we want, especially with "just works" pairing as you are using. I will dig into this after I get home from this long weekend.

The setMinPreferred() etc calls are not necessary at all, those comments are legacy from many years ago that do not apply anymore.

@taks
Copy link

taks commented May 20, 2024

Hi @cmorganBE, @h2zero, @dorianim
Could you please try the following code?
I have solved this problem in the past by adding the following code.

NimBLEDevice::setSecurityInitKey(BLE_SM_PAIR_KEY_DIST_ENC | BLE_SM_PAIR_KEY_DIST_ID);
NimBLEDevice::setSecurityRespKey(BLE_SM_PAIR_KEY_DIST_ENC | BLE_SM_PAIR_KEY_DIST_ID);

@dorianim
Copy link

@taks thanks! That seems to fix the Problem :D
However, does this result in any security compromises? I noticed that you omitted the BLE_SM_PAIR_KEY_DIST_SIGN and BLE_SM_PAIR_KEY_DIST_LINK flags.

@taks
Copy link

taks commented May 20, 2024

If a random address is used, the equipment can be identified by IRK (BLE_SM_PAIR_KEY_DIST_ID).
There is no security compromise in using IRK.

For BLE_SM_PAIR_KEY_DIST_SIGN (CSRK), please refer to the following link.
https://stackoverflow.com/questions/70583605/does-ble-device-generates-new-ltk-csrk-and-irk-every-time-it-bonds-with-new-de

I couldn't find any usefulness for BLE_SM_PAIR_KEY_DIST_LINK.

@h2zero
Copy link
Owner

h2zero commented May 22, 2024

Odd that this would work, the default config for the sm keys is here: https://github.com/h2zero/NimBLE-Arduino/blob/e46123a084e0aa336e4cbdd52bf001dac71e5afb/src/NimBLEDevice.cpp#L894

As you can see, the resp key is already set, however the init key does not have BLE_SM_PAIR_KEY_DIST_ID and it should not need it unless the esp32 is using an RPA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants