bluerange-io / bluerange-mesh

BlueRange Mesh (formerly FruityMesh) - The first completely connection-based open source mesh on top of Bluetooth Low Energy (4.1/5.0 or higher)
https://bluerange.io/
Other
287 stars 109 forks source link

Question regarding encryption and packet size #149

Closed jjduhamel closed 3 years ago

jjduhamel commented 3 years ago

Hi,

I'm getting the following error after sending an snonce handshake packet in encrypted form.

00> [MeshAccessConnection.cpp@1025 WARNING]: Wrong handshake packet

In plain form, the snonce packet should be 13 bytes long. However, once I encrypt the packet, I get 16 bytes. For clarity , I added some logging to verify that the data is decrypted properly. However, although I'm receiving the correct data, the check at this line is causing the handshake to fail, because sendData->dataLength is 16 rather than 13.

src/mesh/MeshAccessConnection.cpp

        if(sendData->dataLength == SIZEOF_CONN_PACKET_ENCRYPT_CUSTOM_ANONCE && packetHeader->messageType == MessageType::ENCRYPT_CUSTOM_ANONCE){
            OnANonceReceived((connPacketEncryptCustomANonce const *) data);
        }
        else if(**sendData->dataLength == SIZEOF_CONN_PACKET_ENCRYPT_CUSTOM_SNONCE** && packetHeader->messageType == MessageType::ENCRYPT_CUSTOM_SNONCE){
            OnSNonceReceived((connPacketEncryptCustomSNonce const *) data);
        }

Here, it looks like we simply subtract 4 from the payload size. Shouldn't the AES128 algorithm produce a 16 byte (128-bit) blob when encrypting any packet up the the maximum 16 bytes? How is sendData->dataLength supposed to be adjusted downward in the case where you're encrypting a 13 byte snonce packet?

Fruitymesh Logs:

00> [MeshAccessConnection.cpp@699 MACONN]: Decrypting 87:95:CD:B1:AD:99:56:8B:79:90:5E:53:64:85:21:8C:18:C5:76:7D (20) with nonce 541230638
00> [MeshAccessConnection.cpp@705 ERROR]:   - nonce: 541230638

00> [MeshAccessConnection.cpp@708 ERROR]:   - nonce: 541230639

00> [MeshAccessConnection.cpp@721 ERROR]:   - keystream1: FF:57:A9:C3:09:70:D1:7E:75:73:FE:03:13:A2:8A:3D

00> [MeshAccessConnection.cpp@736 ERROR]:   - cleartext1: 78:C2:64:72:A4:E9:87:F5:0C:E3:A0:50:77:27:AB:B1

00> [MeshAccessConnection.cpp@740 ERROR]:   - keystream2: 18:C5:76:7D:49:34:B8:94:17:3D:5E:E8:72:1D:34:6B

00> [MeshAccessConnection.cpp@747 ERROR]:   - mic: 18:C5:76:7D

00> [MeshAccessConnection.cpp@750 ERROR]:   - nonce: 541230638

00> [MeshAccessConnection.cpp@764 ERROR]:   - keystream3: 9C:95:B0:F5:AD:99:56:8B:79:90:5E:53:64:85:21:8C

00> [MeshAccessConnection.cpp@775 ERROR]:   - cleartext2: F1:12:37:3D:2E:86:42:20:00:00:00:00:00:00:00:00

00> [MeshAccessConnection.cpp@779 ERROR]:   - nonce: 541230640

00> [MeshAccessConnection.cpp@790 MACONN]: **decryptedOut: 1B:00:7D:44:00:00:00:00:00:00:00:00:00:00:00:00*
*
00> [MeshAccessConnection.cpp@791 MACONN]: Decrypted as 87:95:CD:B1:AD:99:56:8B:79:90:5E:53:64:85:21:8C (16) micValid 1
mariusheil commented 3 years ago

Hi,

did you already take a look at this documentation that explains the handshake in a lot of detail? https://www.bluerange.io/docs/fruitymesh/MeshAccessConnection.html

In short, if the packet should only have 13 byte, you simply do not send the last three bytes. You are adding 0 padding to the cleartext packet and then xoring it with the keystream. Xoring will not scramble the bytes of the whole message and therefore you can simly cut off the last three bytes before sending the message.

But actually what you have found looks a bit like a bug as well because it will not allow us to extend the message easily in the future. But either way you must not send more bytes than expected, otherwhise that could lead to undefined behavior in the future

Marius

jjduhamel commented 3 years ago

Yep that worked. Thanks for the quick response. If it's all right to ask a question before you close this, I need to increment both the encryption and decryption nonces by 2 after I send or receive a packet. Why increment by 2 rather than 1?

Also, this spec says that the handshake packets are padded with zeros to be 16 bytes long, but the code linked above seems to require you not to send the trailing zeros. Is this a typo in the docs or a planned change?

mariusheil commented 3 years ago

Hi,

it is incremented by 1 to generate the encryption keystream and incremented again to generate the MIC as the same keystream cannot be used again for security reasons. So in total every packet needs 2 nonces.

The packets need to be padded with 0s so that it can be encrypted wit the AES block cipher. At one point it states "we can omit the last bytes that were only padded with zeros and only need to send the part that contains data". But I can see that it is not clearly written in the packet description. I will clarify this in the spec. Zero padded encrypted bytes must not be sent.

Marius

jjduhamel commented 3 years ago

Ok I have it working now. There was a bug in my code related to packet splitting, and my phone was disconnecting after like 30 seconds of the mic check failing. Is this expected? I'm wondering if there's a more graceful way to recover such as retrying with different nonces (+1, -1, etc.).