Skip to content

Commit

Permalink
Group Cryptography: Bugs fixed (#14796)
Browse files Browse the repository at this point in the history 
* Group Cryptography: Bugs fixed
* Keys stored incorrectly
* LWIP decryption failure

* Group Cryptography: Bugs fixed: Review comments applied.
  • Loading branch information
@rcasallas-silabs @pull
rcasallas-silabs authored and pull[bot] committed Sep 29, 2023
1 parent 276c786 commit 8482658
Show file tree
Hide file tree
Showing 5 changed files with 44 additions and 41 deletions.
23 changes: 11 additions & 12 deletions src/credentials/GroupDataProviderImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1606,12 +1606,10 @@ CHIP_ERROR GroupDataProviderImpl::SetKeySet(chip::FabricIndex fabric_index, cons
for (size_t i = 0; i < in_keyset.num_keys_used; ++i)
{
ByteSpan epoch_key(in_keyset.epoch_keys[i].key, Crypto::CHIP_CRYPTO_SYMMETRIC_KEY_LENGTH_BYTES);
uint8_t key[Crypto::CHIP_CRYPTO_SYMMETRIC_KEY_LENGTH_BYTES];
MutableByteSpan key_span(key, sizeof(key));
MutableByteSpan key_span(keyset.operational_keys[i].value);
ReturnErrorOnFailure(Crypto::DeriveGroupOperationalKey(epoch_key, key_span));
ReturnErrorOnFailure(Crypto::DeriveGroupSessionId(ByteSpan(key, sizeof(key)), keyset.operational_keys[i].hash));
ReturnErrorOnFailure(Crypto::DeriveGroupSessionId(key_span, keyset.operational_keys[i].hash));
}

if (found)
{
// Update existing keyset info, keep next
Expand Down Expand Up @@ -1817,19 +1815,20 @@ void GroupDataProviderImpl::GroupKeyContext::Release()
mProvider.mKeyContexPool.ReleaseObject(this);
}

CHIP_ERROR GroupDataProviderImpl::GroupKeyContext::EncryptMessage(MutableByteSpan & plaintext, const ByteSpan & aad,
const ByteSpan & nonce, MutableByteSpan & out_mic) const
CHIP_ERROR GroupDataProviderImpl::GroupKeyContext::EncryptMessage(const ByteSpan & plaintext, const ByteSpan & aad,
const ByteSpan & nonce, MutableByteSpan & mic,
MutableByteSpan & ciphertext) const
{
uint8_t * output = plaintext.data();
uint8_t * output = ciphertext.data();
return Crypto::AES_CCM_encrypt(plaintext.data(), plaintext.size(), aad.data(), aad.size(), mKeyValue,
Crypto::kAES_CCM128_Key_Length, nonce.data(), nonce.size(), output, out_mic.data(),
out_mic.size());
Crypto::kAES_CCM128_Key_Length, nonce.data(), nonce.size(), output, mic.data(), mic.size());
}

CHIP_ERROR GroupDataProviderImpl::GroupKeyContext::DecryptMessage(MutableByteSpan & ciphertext, const ByteSpan & aad,
const ByteSpan & nonce, const ByteSpan & mic) const
CHIP_ERROR GroupDataProviderImpl::GroupKeyContext::DecryptMessage(const ByteSpan & ciphertext, const ByteSpan & aad,
const ByteSpan & nonce, const ByteSpan & mic,
MutableByteSpan & plaintext) const
{
uint8_t * output = ciphertext.data();
uint8_t * output = plaintext.data();
return Crypto::AES_CCM_decrypt(ciphertext.data(), ciphertext.size(), aad.data(), aad.size(), mic.data(), mic.size(), mKeyValue,
Crypto::kAES_CCM128_Key_Length, nonce.data(), nonce.size(), output);
}
Expand Down
8 changes: 4 additions & 4 deletions src/credentials/GroupDataProviderImpl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,10 +158,10 @@ class GroupDataProviderImpl : public GroupDataProvider

uint16_t GetKeyHash() override { return mKeyHash; }

CHIP_ERROR EncryptMessage(MutableByteSpan & plaintext, const ByteSpan & aad, const ByteSpan & nonce,
MutableByteSpan & out_mic) const override;
CHIP_ERROR DecryptMessage(MutableByteSpan & ciphertext, const ByteSpan & aad, const ByteSpan & nonce,
const ByteSpan & mic) const override;
CHIP_ERROR EncryptMessage(const ByteSpan & plaintext, const ByteSpan & aad, const ByteSpan & nonce, MutableByteSpan & mic,
MutableByteSpan & ciphertext) const override;
CHIP_ERROR DecryptMessage(const ByteSpan & ciphertext, const ByteSpan & aad, const ByteSpan & nonce, const ByteSpan & mic,
MutableByteSpan & plaintext) const override;
CHIP_ERROR EncryptPrivacy(MutableByteSpan & header, uint16_t session_id, const ByteSpan & payload,
const ByteSpan & mic) const override;
CHIP_ERROR DecryptPrivacy(MutableByteSpan & header, uint16_t session_id, const ByteSpan & payload,
Expand Down
20 changes: 9 additions & 11 deletions src/credentials/tests/TestGroupDataProvider.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1050,17 +1050,18 @@ void TestGroupDecryption(nlTestSuite * apSuite, void * apContext)
//

// Load the plaintext to encrypt
memcpy(ciphertext_buffer, kMessage, sizeof(kMessage));
memcpy(plaintext_buffer, kMessage, sizeof(kMessage));

// Get the key context
Crypto::SymmetricKeyContext * key_context = provider->GetKeyContext(kFabric2, kGroup2);
NL_TEST_ASSERT(apSuite, nullptr != key_context);
uint16_t session_id = key_context->GetKeyHash();

// Encrypt the message
NL_TEST_ASSERT(apSuite,
CHIP_NO_ERROR ==
key_context->EncryptMessage(ciphertext, ByteSpan(aad, sizeof(aad)), ByteSpan(nonce, sizeof(nonce)), tag));
NL_TEST_ASSERT(
apSuite,
CHIP_NO_ERROR ==
key_context->EncryptMessage(plaintext, ByteSpan(aad, sizeof(aad)), ByteSpan(nonce, sizeof(nonce)), tag, ciphertext));

// The ciphertext must be different to the original message
NL_TEST_ASSERT(apSuite, memcmp(ciphertext.data(), kMessage, sizeof(kMessage)));
Expand Down Expand Up @@ -1088,14 +1089,11 @@ void TestGroupDecryption(nlTestSuite * apSuite, void * apContext)
NL_TEST_ASSERT(apSuite, expected.count(found) > 0);
NL_TEST_ASSERT(apSuite, session.key != nullptr);

// Load ciphertext to decrypt
memcpy(plaintext_buffer, ciphertext_buffer, sizeof(plaintext_buffer));

// Decrypt de ciphertext
NL_TEST_ASSERT(
apSuite,
CHIP_NO_ERROR ==
session.key->DecryptMessage(plaintext, ByteSpan(aad, sizeof(aad)), ByteSpan(nonce, sizeof(nonce)), tag));
NL_TEST_ASSERT(apSuite,
CHIP_NO_ERROR ==
session.key->DecryptMessage(ciphertext, ByteSpan(aad, sizeof(aad)), ByteSpan(nonce, sizeof(nonce)),
tag, plaintext));

// The new plaintext must match the original message
NL_TEST_ASSERT(apSuite, 0 == memcmp(plaintext.data(), kMessage, sizeof(kMessage)));
Expand Down
24 changes: 14 additions & 10 deletions src/crypto/CHIPCryptoPAL.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1356,24 +1356,28 @@ class SymmetricKeyContext
virtual ~SymmetricKeyContext() = default;
/**
* @brief Perform the message encryption as described in 4.7.2. (Security Processing of Outgoing Messages)
* @param[in,out] plaintext Outgoing message payload.
* @param[in] plaintext Outgoing message payload.
* @param[in] aad Additional data (message header contents)
* @param[in] nonce Nonce (Security Flags | Message Counter | Source Node ID)
* @param[out] out_mic Outgoing Message Integrity Check
* @param[out] mic Outgoing Message Integrity Check
* @param[out] ciphertext Outgoing encrypted payload. Must be at least as big as plaintext. The same buffer may be used both
* for ciphertext, and plaintext.
* @return CHIP_ERROR
*/
virtual CHIP_ERROR EncryptMessage(MutableByteSpan & plaintext, const ByteSpan & aad, const ByteSpan & nonce,
MutableByteSpan & out_mic) const = 0;
virtual CHIP_ERROR EncryptMessage(const ByteSpan & plaintext, const ByteSpan & aad, const ByteSpan & nonce,
MutableByteSpan & mic, MutableByteSpan & ciphertext) const = 0;
/**
* @brief Perform the message decryption as described in 4.7.3.(Security Processing of Incoming Messages)
* @param[in,out] ciphertext Incoming encrypted payload
* @param[in] aad Additional data (message header contents)
* @param[in] nonce Nonce (Security Flags | Message Counter | Source Node ID)
* @param[in] mic Incoming Message Integrity Check
* @param[in] ciphertext Incoming encrypted payload
* @param[in] aad Additional data (message header contents)
* @param[in] nonce Nonce (Security Flags | Message Counter | Source Node ID)
* @param[in] mic Incoming Message Integrity Check
* @param[out] plaintext Incoming message payload. Must be at least as big as ciphertext. The same buffer may be used both
* for plaintext, and ciphertext.
* @return CHIP_ERROR
*/
virtual CHIP_ERROR DecryptMessage(MutableByteSpan & ciphertext, const ByteSpan & aad, const ByteSpan & nonce,
const ByteSpan & mic) const = 0;
virtual CHIP_ERROR DecryptMessage(const ByteSpan & ciphertext, const ByteSpan & aad, const ByteSpan & nonce,
const ByteSpan & mic, MutableByteSpan & plaintext) const = 0;

/**
* @brief Perform privacy encoding as described in 4.8.2. (Privacy Processing of Outgoing Messages)
Expand Down
10 changes: 6 additions & 4 deletions src/transport/CryptoContext.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,10 +183,11 @@ CHIP_ERROR CryptoContext::Encrypt(const uint8_t * input, size_t input_length, ui

if (mKeyContext)
{
MutableByteSpan plaintext(output, input_length); // WARNING: ASSUMES IN-PLACE ENCRYPTION
ByteSpan plaintext(input, input_length);
MutableByteSpan ciphertext(output, input_length);
MutableByteSpan mic(tag, taglen);

ReturnErrorOnFailure(mKeyContext->EncryptMessage(plaintext, ByteSpan(AAD, aadLen), ByteSpan(IV), mic));
ReturnErrorOnFailure(mKeyContext->EncryptMessage(plaintext, ByteSpan(AAD, aadLen), ByteSpan(IV), mic, ciphertext));
}
else
{
Expand Down Expand Up @@ -228,10 +229,11 @@ CHIP_ERROR CryptoContext::Decrypt(const uint8_t * input, size_t input_length, ui

if (nullptr != mKeyContext)
{
MutableByteSpan ciphertext(output, input_length); // WARNING: ASSUMES input == output
ByteSpan ciphertext(input, input_length);
MutableByteSpan plaintext(output, input_length);
ByteSpan mic(tag, taglen);

CHIP_ERROR err = mKeyContext->DecryptMessage(ciphertext, ByteSpan(AAD, aadLen), ByteSpan(IV), mic);
CHIP_ERROR err = mKeyContext->DecryptMessage(ciphertext, ByteSpan(AAD, aadLen), ByteSpan(IV), mic, plaintext);
ReturnErrorOnFailure(err);
}
else
Expand Down

0 comments on commit 8482658

Please sign in to comment.