meshtastic / firmware

Meshtastic device firmware
https://meshtastic.org
GNU General Public License v3.0
3.35k stars 816 forks source link

[Bug]: RP2040 - Decrypt or Encrypt (during remote admin for example) crashes the device #4855

Closed TheMalkavien closed 1 day ago

TheMalkavien commented 3 days ago

Category

Hardware Compatibility

Hardware

Other

Firmware Version

2.5 +

Description

Hi,

Since 2.5+, my rp2040-lora was unusable. When trying to remote admin, the devices just crash. I finally took time to do some investigations and found out it crashes just after the "Attempting PKI decryption" message.

I found out that there was some pointer dereference on "extraNonce" on the "encryptCurve25519" and "decryptCurve25519" functions causing this crash. The pointer itself is not bad (valid address) but was not word-aligned on the memory. While it may work on most hardware (with non-guaranted results), it makes rp2040 crash.

Good news is I have a fix, which you may use or not, depending on how you want to handle this situation.

Firstly, I replaced the dereferences "*extraNonce" with a memcpy, like that on 2 or 3 places :

*extraNonce = extraNonceTmp; ==> memcpy(extraNonce, &extraNonceTmp, 4); // do not use dereference on non aligned pointers

I tested, and it worked further until another crash. I again found out a lot of pointer dereference into the byte buffer in the aes code, which is not very good news (you can't replace every dereference, and don't want to touch the aes code !), but can be fixed in this case with a simple alignment directive on the buffer itself during declaration (the buffer was not aligned at all !) :

static uint8_t bytes[MAX_RHPACKETLEN] __attribute__ ((__aligned__(4)));
static uint8_t ScratchEncrypted[MAX_RHPACKETLEN] __attribute__ ((__aligned__(4)));

With these 2 tricks, my rp2040 is finally working fine on 2.5+ on decrypt AND crypt (tested on 2.5.1). The directive is not sufficient, because the code use as nonce the last 4 bytes of the buffer, and this buffer does not always have a size modulo 4. But you can easily change by using another way to get a nonce.

Also, it may explain arbitrary crashes on other devices.

I'm not sure I explained it right, but I can answer questions :)

Thanks !

Relevant log output

No response

TheMalkavien commented 3 days ago

UPDATE : I also have some heltec devices.

I Had a PKI_FAILED message by trying to remote admin a 2.5.1 htv3, I checked the keys, everything were right. So, I updated with my patched 2.5.1 (fix alignment issues), and now everything is working fine.

I'm not a professional developer, I may understand thing the wrong way, but it seems that my changes are fixing other issues related to encryption.

Hope this helps.

jp-bennett commented 3 days ago

I could hug you for this. Such excellent work. Memory alignment is something we don't have to worry about much with X86 code, so I didn't even consider it here.

jp-bennett commented 3 days ago

@TheMalkavien Do you want to go ahead and make a PR with these fixes?

TheMalkavien commented 2 days ago

Hi, of course ! I will do that as son as I can, maybe today or tomorrow. Anyway, this should be considered more as a quick fix, an inspiration, than a definitive way to fix all memory alignment problems. I feel that some compiler flags may be used to align things depending on hardware, and of course, some coding rules.