feat: add check for increasing encryption counter

Bluetooth-Devices / bthome-ble

Parser for BTHome BLE devices

https://bthome.io/

MIT License

64 stars 11 forks source link

feat: add check for increasing encryption counter #92

Closed Ernst79 closed 7 months ago

Ernst79 commented 9 months ago

Add a check to verify that the encryption counter is increasing compared to the previous value #20

codecov[bot] commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (6f9969e) 79.47% compared to head (e2916fb) 79.96%. Report is 1 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #92 +/- ## ========================================== + Coverage 79.47% 79.96% +0.48% ========================================== Files 6 6 Lines 497 509 +12 Branches 74 77 +3 ========================================== + Hits 395 407 +12 Misses 80 80 Partials 22 22 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Ernst79 commented 9 months ago

In the future, I think we should not allow duplicate messages to be delivered to the client though. That could allow the client to operate on a replayed message. This is important for messages that convey an action, like a button press. Allowing the same button press to be received by the client more than once would have unintentional side-effects.

Feedback is always welcome, I do appreciate that. I know that Xiaomi is sending multiple messages that are exactly the same. If the counter is the same, the message is the same, by definition.

In BLE monitor, I programmed it such that if the counter is the same as the counter in the previous message, it skips the message. I think it makes sense to do the same for BTHome. E.g.

if the encryption counter is lower than the previous counter value --> Raise a warning (for now)
if the encryption counter is exactly the same --> Skip the advertisement, no warning, as we assume it is only for improving reliability.

Ernst79 commented 9 months ago

I'll fix the other comments later

Ernst79 commented 9 months ago

ToDo

[x] Fix comments of @esev
[x] if the encryption counter is lower than the previous counter value --> Skip the advertisement + warning
[x] if the encryption counter is equal to the previous counter value and the payload differs --> Skip the advertisement + warning
[x] if the encryption counter and the payload are exactly the same as the previous counter and payload --> Skip the advertisement, only debug log message, as we assume that this is only for improving reliability (e.g. is the first message was missed due to bad reception).

Ernst79 commented 8 months ago

@esev Could you do another check of this PR? I think it is finished.