Skip to content

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

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.

Sign up for GitHub

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

Open
adjackura wants to merge 1 commit into
base: dev
Choose a base branch
from
+15 −15
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions src/Mesh.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ DispatcherAction Mesh::onRecvPacket(Packet* pkt) {

// decrypt, checking MAC is valid
uint8_t data[MAX_PACKET_PAYLOAD];
int len = Utils::MACThenDecrypt(secret, data, macAndData, pkt->payload_len - i);
int len = Utils::MACThenDecrypt(secret, PUB_KEY_SIZE, data, macAndData, pkt->payload_len - i);
if (len > 0) { // success!
if (pkt->getPayloadType() == PAYLOAD_TYPE_PATH) {
int k = 0;
Expand Down Expand Up @@ -200,7 +200,7 @@ DispatcherAction Mesh::onRecvPacket(Packet* pkt) {

// decrypt, checking MAC is valid
uint8_t data[MAX_PACKET_PAYLOAD];
int len = Utils::MACThenDecrypt(secret, data, macAndData, pkt->payload_len - i);
int len = Utils::MACThenDecrypt(secret, PUB_KEY_SIZE, data, macAndData, pkt->payload_len - i);
if (len > 0) { // success!
onAnonDataRecv(pkt, secret, sender, data, len);
pkt->markDoNotRetransmit();
Expand All @@ -226,7 +226,7 @@ DispatcherAction Mesh::onRecvPacket(Packet* pkt) {
for (int j = 0; j < num; j++) {
// decrypt, checking MAC is valid
uint8_t data[MAX_PACKET_PAYLOAD];
int len = Utils::MACThenDecrypt(channels[j].secret, data, macAndData, pkt->payload_len - i);
int len = Utils::MACThenDecrypt(channels[j].secret, 16, data, macAndData, pkt->payload_len - i);
if (len > 0) { // success!
onGroupDataRecv(pkt, pkt->getPayloadType(), channels[j], data, len);
break;
Expand Down Expand Up @@ -463,7 +463,7 @@ Packet* Mesh::createPathReturn(const uint8_t* dest_hash, const uint8_t* secret,
getRNG()->random(&data[data_len], 4); data_len += 4;
}

len += Utils::encryptThenMAC(secret, &packet->payload[len], data, data_len);
len += Utils::encryptThenMAC(secret, PUB_KEY_SIZE, &packet->payload[len], data, data_len);
}

packet->payload_len = len;
Expand All @@ -488,7 +488,7 @@ Packet* Mesh::createDatagram(uint8_t type, const Identity& dest, const uint8_t*
int len = 0;
len += dest.copyHashTo(&packet->payload[len]); // dest hash
len += self_id.copyHashTo(&packet->payload[len]); // src hash
len += Utils::encryptThenMAC(secret, &packet->payload[len], data, data_len);
len += Utils::encryptThenMAC(secret, PUB_KEY_SIZE, &packet->payload[len], data, data_len);

packet->payload_len = len;

Expand Down Expand Up @@ -516,7 +516,7 @@ Packet* Mesh::createAnonDatagram(uint8_t type, const LocalIdentity& sender, cons
} else {
// FUTURE:
}
len += Utils::encryptThenMAC(secret, &packet->payload[len], data, data_len);
len += Utils::encryptThenMAC(secret, PUB_KEY_SIZE, &packet->payload[len], data, data_len);

packet->payload_len = len;

Expand All @@ -536,7 +536,7 @@ Packet* Mesh::createGroupDatagram(uint8_t type, const GroupChannel& channel, con

int len = 0;
memcpy(&packet->payload[len], channel.hash, PATH_HASH_SIZE); len += PATH_HASH_SIZE;
len += Utils::encryptThenMAC(channel.secret, &packet->payload[len], data, data_len);
len += Utils::encryptThenMAC(channel.secret, 16, &packet->payload[len], data, data_len);
Copy link
Copy Markdown
Contributor

@nextgens nextgens May 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Copy Markdown
Contributor

@nextgens nextgens May 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :/

Copy link
Copy Markdown
Contributor

@nextgens nextgens May 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

@adjackura adjackura May 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Author

@adjackura adjackura May 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


packet->payload_len = len;

Expand Down
12 changes: 6 additions & 6 deletions src/Utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,26 +60,26 @@ int Utils::encrypt(const uint8_t* shared_secret, uint8_t* dest, const uint8_t* s
return dp - dest; // will always be multiple of 16
}

int Utils::encryptThenMAC(const uint8_t* shared_secret, uint8_t* dest, const uint8_t* src, int src_len) {
int Utils::encryptThenMAC(const uint8_t* shared_secret, int secret_len, uint8_t* dest, const uint8_t* src, int src_len) {
int enc_len = encrypt(shared_secret, dest + CIPHER_MAC_SIZE, src, src_len);

SHA256 sha;
sha.resetHMAC(shared_secret, PUB_KEY_SIZE);
sha.resetHMAC(shared_secret, secret_len);
sha.update(dest + CIPHER_MAC_SIZE, enc_len);
sha.finalizeHMAC(shared_secret, PUB_KEY_SIZE, dest, CIPHER_MAC_SIZE);
sha.finalizeHMAC(shared_secret, secret_len, dest, CIPHER_MAC_SIZE);

return CIPHER_MAC_SIZE + enc_len;
}

int Utils::MACThenDecrypt(const uint8_t* shared_secret, uint8_t* dest, const uint8_t* src, int src_len) {
int Utils::MACThenDecrypt(const uint8_t* shared_secret, int secret_len, uint8_t* dest, const uint8_t* src, int src_len) {
if (src_len <= CIPHER_MAC_SIZE) return 0; // invalid src bytes

uint8_t hmac[CIPHER_MAC_SIZE];
{
SHA256 sha;
sha.resetHMAC(shared_secret, PUB_KEY_SIZE);
sha.resetHMAC(shared_secret, secret_len);
sha.update(src + CIPHER_MAC_SIZE, src_len - CIPHER_MAC_SIZE);
sha.finalizeHMAC(shared_secret, PUB_KEY_SIZE, hmac, CIPHER_MAC_SIZE);
sha.finalizeHMAC(shared_secret, secret_len, hmac, CIPHER_MAC_SIZE);
}
if (memcmp(hmac, src, CIPHER_MAC_SIZE) == 0) {
return decrypt(shared_secret, dest, src + CIPHER_MAC_SIZE, src_len - CIPHER_MAC_SIZE);
Expand Down
4 changes: 2 additions & 2 deletions src/Utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,13 @@ class Utils {
* \brief encrypts bytes in src, then calculates MAC on ciphertext, inserting into leading bytes of 'dest'.
* \returns total length of bytes in 'dest' (MAC + ciphertext)
*/
static int encryptThenMAC(const uint8_t* shared_secret, uint8_t* dest, const uint8_t* src, int src_len);
static int encryptThenMAC(const uint8_t* shared_secret, int secret_len, uint8_t* dest, const uint8_t* src, int src_len);

/**
* \brief checks the MAC (in leading bytes of 'src'), then if valid, decrypts remaining bytes in src.
* \returns zero if MAC is invalid, otherwise the length of decrypted bytes in 'dest'
*/
static int MACThenDecrypt(const uint8_t* shared_secret, uint8_t* dest, const uint8_t* src, int src_len);
static int MACThenDecrypt(const uint8_t* shared_secret, int secret_len, uint8_t* dest, const uint8_t* src, int src_len);

/**
* \brief converts 'src' bytes with given length to Hex representation, and null terminates.
Expand Down