Implement AES in CTR mode instead of ECB
bezmi opened this issue · 3 comments
The current security implementation appears to be using AES in ECB mode. This is not considered secure if encrypting more than 1 block, as identical blocks of data will result in the same cipher text - not ideal when transmitting keystrokes.
As a bear minimum for confidentiality, the security implementation should use encryption in the CTR mode. To avoid synchronisation issues, the counter can be prefixed to the packet in plaintext.
This can be taken further by including some form of message authentication, but it is debatable whether that is necessary for a wireless keyboard.
The encryption protocol used is ECB, however the packet format used insures that each packet sent is unique. The packet format is:
aes128_block: { // 16 byte AES block
payload: 11 bytes;
dev_id: 1 byte: // device ID
uid: 4 bytes; // unique ID, implemented as a counter
}
The reason why you don't see this kind of protocol much in practice is because it wastes about 30% of the AES block, but we don't need much throughput for a keyboard. This is protocol is essentially the same as CTR mode, but it has one advantage I'll explain below.
This can be taken further by including some form of message authentication, but it is debatable whether that is necessary for a wireless keyboard.
If using CTR mode, you definitely would need a MAC (message authentication code). Otherwise an attacker can potentially inject keystrokes via XORing bits in the cipher text. However, the current protocol doesn't need a MAC because flipping any bits in the cipher text will cause it to decrypt to garbage. So we can just check the dev_id and uid to verify the integrity of the packet.
If you have any other concerns feel free to ask.
I looked through the crypto code without actually taking a look at the packet format. With the UID and key for each peripheral, the encrypted packet will definitely be unique.
So we can just check the
dev_idanduidto verify the integrity of the packet.
How would this work when dealing with a loss of synchronisation if the receiver misses some packets? The uid would not be the expected value, so you'd need some way of resynchronising the counter.