-
Notifications
You must be signed in to change notification settings - Fork 964
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement public/private key encryption for direct messages #1509
Conversation
Looks awesome! Have you seen the ham mode settings? Are we able to easily disable the encryption / use the default psk? We need for encryption to be "off" when the ham mode is on. |
Good catch! Does ham mode cause the |
FYI - perhapsDecode and perhapsEncode is also what does the text message compression. |
@@ -24,9 +25,21 @@ class CryptoEngine | |||
|
|||
CryptoKey key = {}; | |||
|
|||
uint8_t private_key[32]; | |||
bool keyPairSet; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please initialize private_key, public_key and keyPairSet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comments. cpp_check wants the keys to be initialized and the key in user struct is blowing up memory usage.
@@ -159,7 +159,7 @@ extern const pb_msgdesc_t OEMStore_msg; | |||
|
|||
/* Maximum encoded size of messages (where known) */ | |||
#define ChannelFile_size 624 | |||
#define DeviceState_size 23728 | |||
#define DeviceState_size 26598 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ouch ... RAM usage...
Nice work. However, right now direct messages are not rebroadcasted as mentioned in issue #1459. With this PR in, I think direct messages can be handled just like broadcast messages, except that every node can stop flooding once the intended recipient has ACK-ed. I think then two changes are needed as I mentioned in the issue, namely setting the hop limit to HOP_RELIABLE for direct messages as well and letting the FloodingRouter rebroadcast every packet with an address not equal to its own ID. Maybe this is a good time to test this as well? |
@@ -239,6 +244,7 @@ ErrorCode Router::send(MeshPacket *p) | |||
#endif | |||
|
|||
auto encodeResult = perhapsEncode(p); | |||
if (isDirectMessage(p)) p->want_ack = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think isDirectMessage(p) will never be true if p->want_ack is not true already before, so this line will not do anything.
Closing. It's a great idea, but any future efforts at a similar solution to enhanced privacy need to take into account the limited resources we have to work with on these devices. |
I'd like to keep this open, as it's a base framework that can be built upon. I see 2 possibilities to make this happen.
|
By the way, I've done some performance comparisons on nRF52 RAK 4631 platform between
|
This is very cool! Has there been any progress on this? |
Wondering as well ? |
@kravietz Did you use the hardware AES accelerator on the nrf boards? I bet those would be faster and more energy-efficient. |
@kokroo Interesting, I didn't know the platform has it! We'd need to check the nRF SDK to see how this could be implemented, which shouldn't be that difficult granted that the SD has examples for both AES and ChaCha-Poly RAK4631 is based on nRF52840, which seems to have the following hardware cryptography features:
https://infocenter.nordicsemi.com/index.jsp?topic=%2Fstruct_nrf52%2Fstruct%2Fnrf52840.html These are listed as part of ARM® TrustZone® Cryptocell 310 security subsystem, I'll have a look how these are handled in the SDK. Relying on hardware crypto creates some portability challenges, but the fallback across all platforms is software implementation which will be faster for ChaCha than for AES in any case. |
@kravietz We can easily detect platforms and swap out encryption functions across platforms. This is something programmers deal with all the time. |
I know, this is about PK crypto, but let me first talk about the symmetric encryption, which is important for PK too. Currently AES in CTR mode is used. This is a stream cipher which is XORed with the data and in a proper CTR mode implementation the counter starts at 0, is then for each block incremented by 1 and most importantly is never reused. This allows to use the full counter range without any reuse happening. If a counter is reused, then two encrypted blocks can be XORed to eliminate the encryption and the result is the XOR of the data of two blocks. When the data has some structure (which is basically always the case) it can be used to get information of the messages, e.g. when a chat message is XORed with a more static node info or telemetry message. The same is the case with OFB and CFB. On yt is a video why DMR encryption with Hytera is so good. It is because it uses a random IV for OFB. When using a random IV the birthday paradox applies: https://en.wikipedia.org/wiki/Birthday_problem So after about a square of the IV number space IV duplicates can be expected. Because the IV there is only 32 bits long this means after about 64k of frames. (There is also a LFSR involved, which can also create issues, but I don't know any details.) 64k frames is about 6.5 hours talking instead of the 49 years presented in the video. So small mistake and it basically falls apart. This is why I avoid CTR/OFB/CFB if possible and get skeptical when something is using one of these modes. Writing marketing material, that AES is military security, is easy. Getting that security to the road is difficult. Currently Meshtastic uses a random value as a start value after a boot and then counts from this value. This is a mix between the proper CTR mode and a random value giving us the birthday problem. The more often the node reboots the more birthday problem there is. The counter in the IV is also 32 bits long and is AFAIK not incremented per packet, but per block. So reuse occurs after about 64k to 4G blocks. What I would do instead is to use the CBC (Edit: typo) mode with ciphertext stealing: https://en.wikipedia.org/wiki/Ciphertext_stealing It is a bit more difficult to implement than CTR, but it has otherwise very similar relevant properties than CTR. It requires only a tiny little bit more processing power and most of all it does also not increase the size of the message. When an IV is reused an attacker gets the info if the same or different data is encrypted, but an XOR of two blocks stays the XOR of two ciphertexts which doesn't help at all. So when the chance arises to change crypto I do suggest to look into this topic. I am not an expert, just an cryptographically interested user for 30 years. So there might be better solutions than mine and therefore it would be advisable to ask real experts if possible. Will now think about PK crypto. |
@Woomeico Wow, i never even thought about this. You've made excellent points there. Do you mind if I discuss a few ideas with you over email? You can mail me at [email protected] |
Should this be added to Meshtastic 3.0 goals? |
co-authored-by: edinnen <[email protected]>
* Re-implement PKI from #1509 co-authored-by: edinnen <[email protected]> * Set the key lengnth to actually make PKI work. * Remove unused variable and initialize keys to null * move printBytes() to meshUtils * Don't reset PKI key son reboot unless needed. * Remove double encryption for PKI messages * Cleanup encrypt logic * Add the MESHTASTIC_EXCLUDE_PKI option, and set it for minimal builds. Required for STM32 targets for now. * Use SHA-256 for PKI key hashing, and add MESHTASTIC_EXCLUDE_PKI_KEYGEN for STM32 * Fix a crash when node is null * Don't send PKI encrypted packets while licensed * use chIndex 8 for PKI * Don't be so clever, that you corrupt incoming packets * Pass on channel 8 for now * Typo * Lock keys once non-zero * We in fact need 2 scratch buffers, to store the encrypted bytes, unencrypted bytes, and decoded protobuf. * Lighter approach to retaining known key * Attach the public key to PKI decrypted packets in device memory * Turn PKI back off for STM32 :( * Don't just memcp over a protobuf * Don't PKI encrypt nodeinfo packets * Add a bit more memory logging around nodeDB * Use the proper macro to refer to NODENUM_BROADCAST * Typo fix * Don't PKI encrypt ROUTING (naks and acks) * Adds SecurityConfig protobuf * Add admin messages over PKI * Disable PKI for the WIO-e5 * Add MINIMUM_SAFE_FREE_HEAP macro and set to safe 1.5k * Add missed "has_security" * Add the admin_channel_enabled option * STM32 again * add missed configuration.h at the top of files * Add EXCLUDE_TZ and RTC * Enable PKI build on STM32 once again * Attempt 1 at moving PKI to aes-ccm * Fix buffers for encrypt/decrypt * Eliminate unused aes variable * Add debugging lines * Set hash to 0 for PKI * Fix debug lines so they don't print pointers. * logic fix and more debug * Rather important typo * Check for short packets before attempting decrypt * Don't forget to give cryptoEngine the keys! * Use the right scratch buffer * Cleanup * moar cleanups * Minor hardening * Remove some in-progress stuff * Turn PKI back off on STM32 * Return false * 2.5 protos * Sync up protos * Add initial cryptography test vector tests * re-add MINIMUM_SAFE_FREE_HEAP * Housekeeping and comment fixes * Add explanatory comment about weak dh25519 keys --------- Co-authored-by: Ben Meadors <[email protected]>
* Re-implement PKI from #1509 co-authored-by: edinnen <[email protected]> * Set the key lengnth to actually make PKI work. * Remove unused variable and initialize keys to null * move printBytes() to meshUtils * Don't reset PKI key son reboot unless needed. * Remove double encryption for PKI messages * Cleanup encrypt logic * Add the MESHTASTIC_EXCLUDE_PKI option, and set it for minimal builds. Required for STM32 targets for now. * Use SHA-256 for PKI key hashing, and add MESHTASTIC_EXCLUDE_PKI_KEYGEN for STM32 * Fix a crash when node is null * Don't send PKI encrypted packets while licensed * use chIndex 8 for PKI * Don't be so clever, that you corrupt incoming packets * Pass on channel 8 for now * Typo * Lock keys once non-zero * We in fact need 2 scratch buffers, to store the encrypted bytes, unencrypted bytes, and decoded protobuf. * Lighter approach to retaining known key * Attach the public key to PKI decrypted packets in device memory * Turn PKI back off for STM32 :( * Don't just memcp over a protobuf * Don't PKI encrypt nodeinfo packets * Add a bit more memory logging around nodeDB * Use the proper macro to refer to NODENUM_BROADCAST * Typo fix * Don't PKI encrypt ROUTING (naks and acks) * Adds SecurityConfig protobuf * Add admin messages over PKI * Disable PKI for the WIO-e5 * Add MINIMUM_SAFE_FREE_HEAP macro and set to safe 1.5k * Add missed "has_security" * Add the admin_channel_enabled option * STM32 again * add missed configuration.h at the top of files * Add EXCLUDE_TZ and RTC * Enable PKI build on STM32 once again * Attempt 1 at moving PKI to aes-ccm * Fix buffers for encrypt/decrypt * Eliminate unused aes variable * Add debugging lines * Set hash to 0 for PKI * Fix debug lines so they don't print pointers. * logic fix and more debug * Rather important typo * Check for short packets before attempting decrypt * Don't forget to give cryptoEngine the keys! * Use the right scratch buffer * Cleanup * moar cleanups * Minor hardening * Remove some in-progress stuff * Turn PKI back off on STM32 * Return false * 2.5 protos * Sync up protos * Add initial cryptography test vector tests * re-add MINIMUM_SAFE_FREE_HEAP * Housekeeping and comment fixes * Add explanatory comment about weak dh25519 keys --------- Co-authored-by: Ben Meadors <[email protected]>
This was implemented in v 2.5. |
Modified version now merged will release as 2.5 |
Direct message encryption over a shared channel
The goal of this PR is to prevent the interception of direct message payloads by non-recipient nodes over a shared channel on the mesh while simultaneously allowing non-recipient nodes to repeat messages across the mesh.
Public/private key pairs are generated on the Curve25519 elliptic-curve. We then use the elliptic curve Diffie–Hellman key agreement protocol to compute a shared key which we can use to encrypt data in a way that can only be decrypted by the sender or the recipient. This allows us to send secured messages over the "insecure" shared channel.
Direct Message Encryption Steps
CryptoEngine
AES key to the shared keydecoded.payload
Direct Message Decryption Steps
CryptoEngine
AES key to the shared keydecoded.payload
Changes
rweather/Crypto
libpublic_key
toUser
protobufprivate_key
toMyNodeInfo
protobufCaveats
In order to properly encrypt/decrypt messages both nodes must store the current public key for the other party. As the public/private keys are cycled every boot other nodes on the network could have outdated public keys for another node. This state will persist until a recently booted node sends an announce message with its new public key to the network. If the keys are out of date messages will be gibberish until the keys are properly announced.