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

BLE Pairing and Encryption #156

Merged
merged 23 commits into from May 13, 2022
Merged

Conversation

unknownconstant
Copy link
Contributor

This pull request is for an implementation of the BLE Security Manager which allows arduino devices to pair & encrypt connections using the HCI specification. Specifically, it implements Secure Connect / Numeric comparison.

The branch appears stable but I'm aware it may need some refactoring. An example sketch is included to show how to pair devices.

The branch supports encryption instigated by the central when the arduino is the peripheral.

@unknownconstant
Copy link
Contributor Author

Pull request in relation to: #36 and unknownconstant#11

@shemminger
Copy link

Not all devices want to support pairing. This code doesn't seem to do that.
I would like if it were possible to respond "Pairing not supported" as described in Bluetooth Core Spec C.5.1

@unknownconstant
Copy link
Contributor Author

Happy to take a look - which bit of the spec are you looking at for this? There are a number of volumes which have a C.5.1.

The code in this PR currently only supports the peripheral device, so if the device using this code does not support pairing then it shouldn't be set to require encryption. I understand for BLE that the central will instigate encryption only after the peripheral has sent an error response indicating insufficient encryption or authentication.

Volume 3, H 3.5.1 describes bonding flags in the pairing procedure. When testing I found that these flags did not affect whether an iPhone stored pairing data or not but I would welcome more info if you're able to test this or have any other insights.

@shemminger
Copy link

A little more background. I am building an Ardunio BLE device which emulates a proprietary BLE device that provides steering angle control for the bike training game Zwift. The protocol is already reverse engineered (so thats easy). And the Nano BLE has magnetic sensor, and that can be used as directional input. This is just a dumb BLE device (similar to Battery monitor etc).

The code works talking to Android, and probably MacOs. But does not work with Windows 10 the application connects, but then gets no data. I suspect the lack of pairing support. Doing full encryption is overkill for this application. Wondered if this would be enough to just respond with "not supported"

image

@shemminger
Copy link

I did some more experiments (and tried to learn about the twisted maze of BLE security).
It looks like just setting 0 in the auth request bits in the pairing response is enough to get what I need (pairing without encrypt).
BUT the value is hard coded in this version (see HCI.localAuthReq); looks like you weren't finished with that bit yet.

What would the API be for an application to override the default authentication request settings.
Something like:
BLE.setLocalAuth(AuthReq)?

@CLAassistant
Copy link

CLAassistant commented Apr 9, 2021

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ eltos
❌ unknownconstant


unknownconstant seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@shemminger
Copy link

shemminger commented Apr 9, 2021

Compile is catching this warning:

/home/shemminger/Arduino/libraries/ArduinoBLE/src/utility/L2CAPSignaling.cpp: In member function 'virtual void L2CAPSignalingClass::handleSecurityData(uint16_t, uint8_t, uint8_t*)':
/home/shemminger/Arduino/libraries/ArduinoBLE/src/utility/L2CAPSignaling.cpp:296:13: warning: 'i' may be used uninitialized in this function [-Wmaybe-uninitialized]
     for(int i; i<6; i++) peerAddress[5-i] = identityAddress->address[i];
             ^

Looks like missing i = 0

@shemminger
Copy link

shemminger commented Apr 9, 2021

Lots of other warnings now introduced.

/home/shemminger/Arduino/libraries/ArduinoBLE/src/utility/L2CAPSignaling.cpp: In member function 'virtual void L2CAPSignalingClass::handleSecurityData(uint16_t, uint8_t, uint8_t*)':
/home/shemminger/Arduino/libraries/ArduinoBLE/src/utility/L2CAPSignaling.cpp:271:8: warning: unused variable 'pairingFailed' [-Wunused-variable]
     } *pairingFailed = (PairingFailed*)data;
        ^~~~~~~~~~~~~
/home/shemminger/Arduino/libraries/ArduinoBLE/src/utility/L2CAPSignaling.cpp:320:7: warning: unused variable 'readPublicKeyCommand' [-Wunused-variable]
     } readPublicKeyCommand = {
       ^~~~~~~~~~~~~~~~~~~~
/home/shemminger/Arduino/libraries/ArduinoBLE/src/utility/L2CAPSignaling.cpp: In member function 'virtual void L2CAPSignalingClass::smCalculateLTKandConfirm(uint16_t, uint8_t*)':
/home/shemminger/Arduino/libraries/ArduinoBLE/src/utility/L2CAPSignaling.cpp:421:19: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
     for(int i=0; i<sizeof(Eb); i++){
                  ~^~~~~~~~

@unknownconstant
Copy link
Contributor Author

Hoping to get some time in the next couple of days to take a look at this properly

@unknownconstant
Copy link
Contributor Author

Hi @shemminger , I've not been able to reproduce these errors using the standard Arduino compiler. Is this what you have used, or are you using another toolchain?

When cloning this library on another computer, a number of errors were generated as I had mistakenly included both this version, and the official release. It may be worth checking this?

@shemminger
Copy link

shemminger commented Apr 26, 2021 via email

@AmirMahdiNassiri
Copy link

Any update on when we can have this feature released as an official release?

@unknownconstant
Copy link
Contributor Author

I don't know, there seems to be a licence issue. I've signed but it states it's still pending which may be preventing a merge. We may be waiting on the Arduino team to review the code.

@alranel
Copy link
Member

alranel commented Aug 2, 2021

I'm so sorry for jumping late into this. It looks like there's an issue with CLAbot since I do see that the user @unknownconstant signed the CLA, but we can easily force that.
On the other hand, this pull request contains commits also from @eltos who doesn't seem to have gone through CLA. @eltos, it would be very nice if you could spend a few seconds for that! :)

@eltos
Copy link
Contributor

eltos commented Aug 2, 2021

@alranel CLA signed ✅

@muadiber
Copy link

@alranel can you merge pull request?

@brontesprocessing
Copy link

@alranel Any chance to have it merged any time soon?

Copy link
Member

@alranel alranel left a comment

Choose a reason for hiding this comment

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

I tested the examples with my Android phone and a Nano 33 BLE and they worked like a charm! :)

@brontesprocessing
Copy link

@alranel It looks like the Pull Request is approved. What happens next? Is it possible that new release will be published soon? Thank you in advance!

@alranel
Copy link
Member

alranel commented Oct 13, 2021

Sorry for being so slow - just one last confirmation: did you write all this code or did you take any part of it from other sources?
Looking forward to merge this. :)

@unknownconstant
Copy link
Contributor Author

Yes, the code is either original, refactored from this repo, or in the case of a couple of the crypto functions e.g. AES_CMAC is taken from rfc4493, and Bluetooth toolbox functions are implementations of the published Bluetooth LE spec.

@mklemarczyk
Copy link
Contributor

Do we progress or other implementation is needed?

@unknownconstant
Copy link
Contributor Author

Unsure - AFAIK it's just waiting on being merged

@Uberflup
Copy link

Uberflup commented May 12, 2022

The features provided by this pull request would be great to have, I look forward to it being merged.
Is this still waiting on resolution of a license issue?
@alranel would you be so kind to advise?

@facchinm
Copy link
Contributor

Yup we were still waiting for the CLA to be signed by all the commit authors but it looks like it was and the bot is not actually behaving correctly... I'm going to merge it anyway 😉

@facchinm facchinm merged commit 8f9e785 into arduino-libraries:master May 13, 2022
@per1234
Copy link
Contributor

per1234 commented May 13, 2022

For future reference, the reason the CLA check was not passing is because the check is done against the owner of each commit in the PR. The CLA is signed by your GitHub user account, so the commit can only be verified when its email address (as set via the user.email Git configuration key) is associated with the GitHub account.

You can see that some of the commits from @unknownconstant had one email address configuration. For example:

https://api.github.com/repos/arduino-libraries/ArduinoBLE/commits/e5ccd3b2c3d21df29a1b246959e663bcbf9f153f

and other commits had another. For example:

https://api.github.com/repos/arduino-libraries/ArduinoBLE/commits/c66379882f727276b74c319c1ec9fa824fc96bf1

The bot attempts to communicate this situation in the comments:

unknownconstant seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

@per1234 per1234 added type: enhancement Proposed improvement topic: code Related to content of the project itself labels May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet