Fix buffer over-read in HMAC generation for 16-byte group channel keys

[adjackura]

I was doing some testing and was confused why some devices were able to send group messages but not receive, while others had no issues receiving. Turns out there appears to be a bug that can lead to accidentally reading uninitialized memory when working with 16-byte aes-128 keys (channels). Pretty sure most code currently is either getting lucky relying on pre-zeroed out memory or explicitly calling memset for the full 32 bytes of GroupChannel.secret.

In Utils::encryptThenMAC and Utils::MACThenDecrypt, the HMAC key length was hardcoded to PUB_KEY_SIZE (32 bytes) when initializing the SHA256 context. While correct for direct/private messages (which use 32-byte ECDH shared secrets), group channels rely on 16-byte keys.

When encrypting or decrypting a group message, the HMAC function was reading 16 bytes past the end of the intended key array, silently pulling in uninitialized stack memory.

This fix should be 100% backwards compatible (and has been in my testing).

[nextgens]

This does not make sense to me... the definition is:

namespace mesh {

class GroupChannel {
public:
  uint8_t hash[PATH_HASH_SIZE];
  uint8_t secret[PUB_KEY_SIZE];
};
}

You have other parts that make assumptions on the key being 32bytes, like BaseMeshChat::findChannelIdx().

[nextgens]

On a second look, both key sizes are supported, see https://github.com/meshcore-dev/MeshCore/blob/main/src/helpers/BaseChatMesh.cpp#L883

So yeah it's broken beyond belief :/

[nextgens]

I think the better fix is to ensure the array is zero-initialized.

I've asked on the dev channel on discord what's the preferred way forward, will let you know if I hear back.

[adjackura]

I think that's part of the problem, this key is not 32 bytes, its 16. Unless I'm missing something here, group channels use AES-128 keys, which are 16 bytes, DMs (which is what this PUB_KEY_SIZE is referencing) use 32 byte secrets (Curve25519).

[adjackura]

Sorry, responded before I refreshed and saw you latest two. This was also very confusing to me to debug because of assumptions based on a surface view of the code. My main concern with "make sure you zero-initialize the full array" is it makes assumptions on how all future calls of these methods will work essentially leaving it up to callers to always pad the secret.

The good news so far is the HMAC padding has been saving this, [16-byte PSK] + [16-byte Zeros] is equivalent to just [16-byte PSK] because HMAC-SHA256 pads anything smaller than 64 bytes, but you really should only read in the exact length of the provided key.