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

bluetooth: host: Add workaround for USB HCI controllers #22334

Merged

Conversation

fnde-ot
Copy link
Collaborator

@fnde-ot fnde-ot commented Jan 30, 2020

This commit adds a new option CONFIG_BT_SMP_USB_HCI_CTLR_WORKAROUND
to support USB HCI controllers that sometimes send out-of-order HCI
events and ACL Data due to using different USB endpoints.

Enabling this option will make the master role not require the
encryption-change event to be received before accepting
key-distribution data.

It opens up for a potential vulnerability as the master cannot detect
if the keys are distributed over an encrypted link.

Fixes: #22086

Signed-off-by: François Delawarde fnde@oticon.com

@jhedberg jhedberg requested a review from sjanc January 30, 2020 07:53
@aescolar aescolar added the bug The issue is a bug, or the PR is fixing a bug label Jan 30, 2020
@joerchan
Copy link
Contributor

@jhedberg @fnde-ot From the discussion in the issue I assumed we weren't gonna do this?

In the case that we allow it I think we should only accept Identity Information and Identity Address Information. This would be the first thing the slave sends in LESC. (In legacy it is the LTK being sent first). If the peer decides to send it's identity information on an unencrypted link then the peer has messed up it's own privacy, but shouldn't affect us unless we need network privacy.

@jhedberg
Copy link
Member

@jhedberg @fnde-ot From the discussion in the issue I assumed we weren't gonna do this?

That's what I assumed as well, particularly since (to my knowledge) this isn't preventing any critical (product) use case. So my first choice would be not to add anything like this.

If there really is a critical need I'd request at least the following:

  • Make it depend on CONFIG_BT_TESTING
  • Throw a big fat warning to the logs when initialising Bluetooth that there is an intentional security vulnerability enabled

@fnde-ot
Copy link
Collaborator Author

fnde-ot commented Jan 30, 2020

@jhedberg @fnde-ot From the discussion in the issue I assumed we weren't gonna do this?

I was thinking that maybe a workaround (disabled by default) could be acceptable for us to be able to use the official PTS dongles with Zephyr host in native_posix. We are using that heavily for testing purposes. Although it clearly states that it opens up for a potential vulnerability if the slave is broken, maybe we could make this Kconfig option depend on BT_TESTING to make sure no one enables this in production builds?

In the case that we allow it I think we should only accept Identity Information and Identity Address Information. This would be the first thing the slave sends in LESC. (In legacy it is the LTK being sent first). If the peer decides to send it's identity information on an unencrypted link then the peer has messed up it's own privacy, but shouldn't affect us unless we need network privacy.

Working with the PTS dongle in legacy mode, we sometimes get the LTK before receiving the encryption_change event, so this patch also fixes that case.

@fnde-ot
Copy link
Collaborator Author

fnde-ot commented Jan 30, 2020

If there really is a critical need I'd request at least the following:
* Make it depend on CONFIG_BT_TESTING
* Throw a big fat warning to the logs when initialising Bluetooth that there is an intentional security vulnerability enabled

Looks like our messages crossed paths, I can certainly update the patch to do that!

@joerchan
Copy link
Contributor

@fnde-ot Ok. then BT_TESTING and the BT_WARN.

@fnde-ot fnde-ot force-pushed the usb_hci_ctlr_workaround branch from 76d1a31 to 477ed6c Compare January 31, 2020 14:55
@fnde-ot
Copy link
Collaborator Author

fnde-ot commented Jan 31, 2020

@fnde-ot Ok. then BT_TESTING and the BT_WARN.

Pushed!!

This commit adds a new option CONFIG_BT_SMP_USB_HCI_CTLR_WORKAROUND
to support USB HCI controllers that sometimes send out-of-order HCI
events and ACL Data due to using different USB endpoints.

Enabling this option will make the master role not require the
encryption-change event to be received before accepting
key-distribution data.

It opens up for a potential vulnerability as the master cannot detect
if the keys are distributed over an encrypted link.

Fixes: zephyrproject-rtos#22086

Signed-off-by: François Delawarde <fnde@oticon.com>
@fnde-ot fnde-ot force-pushed the usb_hci_ctlr_workaround branch from 477ed6c to 7d1a615 Compare January 31, 2020 17:20
@jhedberg
Copy link
Member

jhedberg commented Feb 3, 2020

@joerchan are you ok with this now?

@jhedberg jhedberg added Enhancement Changes/Updates/Additions to existing features and removed bug The issue is a bug, or the PR is fixing a bug labels Feb 3, 2020
@jhedberg jhedberg merged commit 9d2e34e into zephyrproject-rtos:master Feb 3, 2020
@fnde-ot fnde-ot deleted the usb_hci_ctlr_workaround branch October 19, 2020 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Bluetooth Enhancement Changes/Updates/Additions to existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

L2CAP/SMP: Race condition possible in native posix central when bonding.
5 participants