Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

BLE Pairing and Encryption #156

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

Merged
facchinm merged 23 commits into arduino-libraries:master from unknownconstant:master
May 13, 2022
Merged

Conversation

Copy link
Contributor

@unknownconstant unknownconstant commented Jan 24, 2021

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.

AmirMahdiNassiri, theholypumpkin, muadiber, LRagji, Uberflup, and brutalsam reacted with thumbs up emoji brutalsam reacted with laugh emoji
unknownconstant and others added 23 commits January 1, 2021 11:22
With write encryptionn requirement
Write encryption & visible LTK / IRK
Fix timing issue during pairing, add pairing control/status methods
Secure random & reject unknown LTK
Pairing code & IOCaps set via implementation of callbacks
Copy link
Contributor Author

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

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

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.

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

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

Copy link

CLAassistant commented Apr 9, 2021
edited
Loading

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.

Copy link

shemminger commented Apr 9, 2021
edited by per1234
Loading

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

Copy link

shemminger commented Apr 9, 2021
edited by per1234
Loading

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++){
 ~^~~~~~~~

Copy link
Contributor Author

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

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?

Copy link

shemminger commented Apr 26, 2021 via email

On 2021年4月26日 02:51:12 -0700 unknownconstant ***@***.***> wrote: 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?
I was just building with Arduino IDE on Debian 11. It has GCC 10 as back end.

Copy link

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

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.

Copy link
Contributor

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! :)

Copy link
Contributor

eltos commented Aug 2, 2021

@alranel CLA signed ✅

Copy link

@alranel can you merge pull request?

Copy link

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

Copy link
Contributor

@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! :)

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!

Copy link
Contributor

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. :)

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.

Copy link
Contributor

Do we progress or other implementation is needed?

Copy link
Contributor Author

Unsure - AFAIK it's just waiting on being merged

Copy link

Uberflup commented May 12, 2022
edited
Loading

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?

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 😉

Uberflup reacted with heart emoji

@facchinm facchinm merged commit 8f9e785 into arduino-libraries:master May 13, 2022
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.

unknownconstant and Uberflup reacted with thumbs up emoji

@per1234 per1234 added type: enhancement Proposed improvement topic: code Related to content of the project itself labels May 13, 2022
@fpistm fpistm mentioned this pull request Mar 14, 2025
10 tasks
pennam pushed a commit to pennam/ArduinoBLE that referenced this pull request Apr 9, 2025
5 years ago, support for encryption was added by arduino-libraries#156.
Currently, the BLEEncryption tag needs to be added to require proper encrypted bluetooth pairing.
It is possible to add this tag to all other kinds of characteristics except String characteristics.
BLEStringCharacteristic's constructor calls BLECharacteristic's constructor, and BLECharacteristic
uses a uint16_t instead of a uint8_t.
This PR simply changes the properties attribute of BLEStringCharacteristic to match the width of
the permissions attribute of BLECharacteristic (from 8 to 16 bits) to support the BLEPermission
tags in BLEProperty.h.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@alranel alranel alranel approved these changes

Reviewers whose approvals may not affect merge requirements
Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /