search

InfiniTimeOrg / InfiniTime

Firmware for Pinetime smartwatch written in C++ and based on FreeRTOS
GNU General Public License v3.0
2.71k stars 926 forks source link

Breaking change - Secure/BLE pairing #880

Closed evergreen22 closed 2 years ago

evergreen22 commented 2 years ago

This applies to a 1.7.1 develop build that includes secure pairing PR #796.

While chatting about whether or not to persist the airplane mode setting, @kieranc asked why not. After explaining the potential issue with recovery, the question was asked about how secure pairing might impact recovery. It appeared on my screen at the same moment I realized it is also a potential issue. Thanks @kieranc !

Good news - when using the recovery firmware from a NOT bonded companion everything works (tested with GB and nRFConnect).

However, when using the recovery firmware from a bonded companion you will be unable to establish a connection until the bond is deleted from the companion.

This is because the recovery firmware installed on watches is too old to understand secure connections.

This could be addressed at least three ways:

  1. Clearly document that the bond must be deleted from the companion to use the recovery firmware.

  2. Update the recovery firmware to handle secure and non-secure connections. This only solves the problem for people who update their recovery firmware.

  3. Do not persist bonds on the watch. Recovery firmware works for everyone, but you'll have to re-pair your watch on every boot.

In a related note, this means that saving the airplane mode setting doesn't impact recovery today. It might in the future though if people install a recovery firmware that enforces airplane mode.

Finally, should we consider separating the recovery firmware and making it a standalone repo similar to mcuboot?

evergreen22 commented 2 years ago

I'm migrating this conversation to an issue in order to organize the discussion as suggested by @geekbozu .

FWIW, the consensus to date is option (1)

geekbozu commented 2 years ago

I really think we should just have a check for recovery to bluntly ignore all loading from flash. Incase it gets damaged or similar. Recovery should be as standalone as possible. Even if that makes it an inconvenience. IT working reliably is More important then all.

evergreen22 commented 2 years ago

That was one of my initial thoughts as well. However, that will mean updating the recovery firmware and getting everyone to install the updated version.

I think we are stuck with some period of time where folks will have the current 'basic/dumb' recovery firmware and a new recovery-aware version.

evergreen22 commented 2 years ago

Also, your argument @geekbozu clearly articulates why I think the recovery firmware should be separated from the feature firmware.

geekbozu commented 2 years ago

That was one of my initial thoughts as well. However, that will mean updating the recovery firmware and getting everyone to install the updated version.

I think we are stuck with some period of time where folks will have the current 'basic/dumb' recovery firmware and a new recovery-aware version.

While this is a truth, I think that asking our user base to update is not a huge concern.

And yes the recovery firmware at some point should be forked and cleaned up/final/stabilized pending major bugs.

@JF002 does the current release (1.0.0) of the recovery firmware load settings flash do you know?

As a lot of these concerns also are only relevant to when we update the recovery firmware next

evergreen22 commented 2 years ago

We should also remember that even if the recovery firmware does not use the stored settings and does not restore the bond (like the current 'basic/dumb' recovery firmware), we still will have the issue of needing to delete the bond from the companion app when we were previously bonded.

The only way to address the stored bond on the companion that I can think of is to have recovery firmware that uses a different ble address so the companion starts a fresh pairing cycle.

geekbozu commented 2 years ago

Yeah the reason I'm confirming Is i find that to be a "non" issue. As if your borked enough to be in recovery, You probably don't want that bond anymore anyway.

evergreen22 commented 2 years ago

It sounds like we all agree that recovery firmware should be kept as simple as possible (i.e. like it is today - no secure pairing and ignore all settings).

Since we build the recovery firmware from the same sources as the feature build today, we are at risk of breaking the recovery firmware with a new build. It also sounds like we agree that the sources that the current recovery firmware was built from should be copied into a separate folder to avoid this problem going forward.

geekbozu commented 2 years ago

Thats not my exact stance. My current stance is. "We have no intention of releasing a new recovery. Lets cross this bridge when we get there. :)

On Sun, Dec 19, 2021 at 2:14 PM James A. Jerkins @.***> wrote:

It sounds like we all agree that recovery firmware should be kept as simple as possible (i.e. like it is today - no secure pairing and ignore all settings).

Since we build the recovery firmware from the same sources as the feature build today, we are at risk of breaking the recovery firmware with a new build. It also sounds like we agree that the sources that the current recovery firmware was built from should be copied into a separate folder to avoid this problem going forward.

— Reply to this email directly, view it on GitHub https://github.com/InfiniTimeOrg/InfiniTime/issues/880#issuecomment-997445455, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA7I5COEIEIQBNSTF6PH3Y3URYVIZANCNFSM5KJUDZUA . You are receiving this because you were mentioned.Message ID: @.***>

trman commented 2 years ago

We should also remember that even if the recovery firmware does not use the stored settings and does not restore the bond (like the current 'basic/dumb' recovery firmware), we still will have the issue of needing to delete the bond from the companion app when we were previously bonded.

i think that this issue could be adressed by giving another ssid than infintime (like infinitme - recovery) maybe it could help otherwise you could simply request a new passkey after the recovery flashing the firmware to recreate the bonds @evergreen22

JF002 commented 2 years ago

I agree with the consensus regarding the recovery firmware : it should be kept as simple and as reliable as possible : it should not read any setting from the filesystem, and should not enable the secure pairing functionality. It could also expose another MAC address and/or name so that we can easily differentiate the recovery firmware from InfiniTime.

If we keep the project as it is right now (InfiniTime and recovery share the same codebase), we'll probably need to make the code more configurable : enable all features for InfiniTime, disable most of them (settings, secure pairing, optional BLE services,...) for the recovery firmware.

But anyway, for now, the only "official" release of the recovery firmware is the 0.14.1. Since then, we haven't checked if the recovery firmware is still working as expected and we haven't released a new version. That would be nice if we could maintain the recovery firmware and provide updates from time to time, though...

I re-opened this discussion to talk about how we enable the secure pairing feature. Currently, secure pairing is "triggered" by InfiniTime when the central reads the battery level. This looks like a good idea because most of the companion apps (and even BLE widgets on desktop linux) automatically read the battery level when connecting to the device.

But the more I think about this, the more I think that secure pairing being a by-product of a read request on the battery level is not the best solution :

So, in this current state, we cannot release InfiniTime with secure bonding until all (most of) the companion app implement/integrate the secure bonding feature. If we release too soon, some users won't be able to connect with their PineTime.

I was wondering if we could change the way the secure bonding feature is activated. Instead of requesting the encrypted connection when the central reads the battery level, would it be possible to let the companion app initiate the secure bond if they support it? This way, every companion app would still be able to work as they used to, and will be able to enable the secure bonding if/when their developers implements it in their app.

Here's an example with bluetoothctl on Linux:

In a later step, when the ecosystem of companion apps is ready, we will be able to enforce encryption on some of the characteristics/services to provides more security and privacy, (or find any better way to enable the secure pairing) of course.

I don't know if what I'm writing makes any sense... Basically, I'm wondering if the companion apps are able to initiate a "simple" connection or a secure pairing. That way, we'll be able to remove the encryption constraint on the battery level so we do not break existing companion apps. In the future, when companion app are ready, we can enable the security on some/all services.

I also don't know the "state of the art" regarding BLE connection/bonding and if what I'm describing here is even possible. Any opinion?

evergreen22 commented 2 years ago

So, in this current state, we cannot release InfiniTime with secure bonding until all (most of) the companion app implement/integrate the secure bonding feature. If we release too soon, some users won't be able to connect with their PineTime.

OK.

I was wondering if we could change the way the secure bonding feature is activated. Instead of requesting the encrypted connection when the central reads the battery level, would it be possible to let the companion app initiate the secure bond if they support it?

Yes, companion apps can initiate secure pairing according to the spec. "We" cannot change the companion apps obviously, but we can choose not include this PR which forces companion apps to pair securely or fail.

Would you like me to push a revert to the PR branch, or it is easier for you to handle that in the develop branch?

This way, every companion app would still be able to work as they used to, and will be able to enable the secure bonding if/when their developers implements it in their app.

OK.

JF002 commented 2 years ago

Yes, companion apps can initiate secure pairing according to the spec.

Great!

"We" cannot change the companion apps obviously

Yes, of course, but we can communicate with companion app developers to let them know they now have the possibility to enable securing pairing (and that we strongly encourage them to enable it) :-)

but we can choose not include this PR which forces companion apps to pair securely or fail.

So the whole of text I wrote yesterday makes sense? Awesome!

Would you like me to push a revert to the PR branch, or it is easier for you to handle that in the develop branch?

I would be grateful if you could create a PR for this? How will you do it? Just remove the enc/auth requirement for the battery level characteristic?

lman0 commented 2 years ago

@JF002 @evergreen22 if i understand right , you will remove the secure pairing altogether instead of make it optional ?

if feel it's a bad move because due to chicken egg problem , siglo will not implement since they are not forced by you enforcing it and you will not merge it because they not implementing it: it a deadlock a this pr will never be merged agzin ...

at least please make a pr that allow people to activate the secure pairing with an option in code or a button so people could have their own branch with the secure pair that is life saving for gadget bridge user like me

lman0 commented 2 years ago

or better add an option that allow to select if the secure bonding is activated or not , either for a specific device (that would be perfect , like in wifi were it possible to have no password for specifc ssid)

or always (an button for activatcting secure bonding like was for airplane mode) @JF002 @evergreen22

JF002 commented 2 years ago

@lman0 The idea is not to remove it completely, but just to make it optional : if the application app initiates a secure pairing process, InfiniTime will accept it. If the companion app simply connects without pairing, InfiniTime will also accept the connection.

lman0 commented 2 years ago

@lman0 The idea is not to remove it completely, but just to make it optional : if the application app initiates a secure pairing process, InfiniTime will accept it. If the companion app simply connects without pairing, InfiniTime will also accept the connection.

for what i've read of @evergreen22 response @JF002 , the pr that evergren talk about is a pr that remove (by reverting the pr code) the secure bonding not making optional

as explained by evergreen here :

Would you like me to push a revert to the PR branch, or it is easier for you to handle that in the develop branch?

lman0 commented 2 years ago

he talk to you @JF002 reverting the merge of his pr related to secure bonding ....in develop

so in final , the final code of infitime would not contain anymore anything related to secure bonding

Avamander commented 2 years ago

@lman0 A PR would have to be merged for it to have any effect. If it's not requested and necessary, then it is not planned.

JF002 commented 2 years ago

My understanding is that @evergreen22 will revert the part of the PR that forces the secure pairing for all companion apps. Let's wait for evergreen22's feedback before drawing any conclusions :-)

lman0 commented 2 years ago

as long as the secure bonding remain possible in the final code /version i'am good

evergreen22 commented 2 years ago

PR #886

evergreen22 commented 2 years ago

Closed by #886