Slightly refactor and fix CTKD #339
Conversation
bumble/smp.py
Outdated
| crypto.h7(salt=SMP_CTKD_H7_BRLE_SALT, w=link_key) | ||
| if self.ct2 | ||
| else crypto.h6(link_key, b'tmp2') | ||
| crypto.h7(salt=SMP_CTKD_H7_BRLE_SALT, w=link_key_big_endian) |
There was a problem hiding this comment.
I would prefer to update h6 and h7 to be implemented the same way the other bt crypto functions are done in crypto.py, where the inputs and outputs are in little-endian, and the byte reversing is done internally, as appropriate, when calling internal crypto primitives that are not BT specific (like aes_cmac). See crypto.l6 for example.
Also, I realize that crypto.py uses the expression bytes(reversed(xyz)) a lot. Since you're editing that part of the code, it might be a good opportunity to use xxx[::-1] as a faster way to reverse a bytes value, or maybe even make that a local function def reverse(x: bytes) -> bytes: return x[::-1] in crypto.py for clarity.
There was a problem hiding this comment.
One problem here is that the spec provides a sample data for the derive procedure and I would like to test it. However, it would be strange if we just reimplement the same code in test.
There was a problem hiding this comment.
The other tests in smp_tests.py also use the test vectors from the spec, exactly as they are given (i.e big-endian), and reverses the bytes before calling the crypto functions (for example, look at test_f6, which uses the test vectors from D.4 f6 LE SC CHECK VALUE GENERATION FUNCTION in the spec. By having the crypto.xxx functions take their inputs and outputs in little-endian, it makes the code in smp.py simpler because it doesn't have to do the byte reversing each time before and after calling the crypto functions (see smp.py line 1711 and 1712 for example where crypto.f6 is called.
There was a problem hiding this comment.
Okay, so should we keep a standalone derive_[ltk/lk] method? Also shall we move them to crypto?
There was a problem hiding this comment.
I think the derive_[ltk_lk] methods are fine as is.
zxzxwu
force-pushed
the
enc
branch
4 times, most recently
from
0beabc4 to
3abfa65
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
zxzxwu
force-pushed
the
enc
branch
2 times, most recently
from
8830c2c to
0d9249f
Compare
bumble/smp.py
Outdated
| async def get_link_key_and_derive_ltk(self) -> None: | ||
| '''Retrieves BR/EDR Link Key from storage and derive it to LE LTK.''' | ||
| link_key = await self.manager.device.get_link_key(self.connection.peer_address) | ||
| assert link_key is not None |
There was a problem hiding this comment.
Could we fail cleanly without an assert? The link key may not exist in the keystore, or there may be no keystore (that's always possible, for stateless use cases, where pairing is done without bonding; even though we do default the keystore to a memory key store, if a user of the API really insists on having no keystore, this shouldn't break)
There was a problem hiding this comment.
Then it needs to be judged before start/reply pairing request, because here it's an async subtask so we don't have a proper way to propagate the failure to the main task.
bumble/smp.py
Outdated
| crypto.h7(salt=SMP_CTKD_H7_BRLE_SALT, w=link_key) | ||
| if self.ct2 | ||
| else crypto.h6(link_key, b'tmp2') | ||
| crypto.h7(salt=SMP_CTKD_H7_BRLE_SALT, w=link_key_big_endian) |
There was a problem hiding this comment.
I think the derive_[ltk_lk] methods are fine as is.
It seems sample input data provided in the spec is big-endian (just like other AES-CMAC-based functions), but all keys are in little-endian( HCI standard), so they need to be reverse before and after applying AES-CMAC.
It seems sample input data provided in the spec is big-endian (just like other AES-CMAC-based functions), but all keys are in little-endian( HCI standard), so they need to be reverse before and after applying AES-CMAC.