From 72a06f1bfacf93554cbf04af23f37b6ffdb29cb8 Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Fri, 4 Feb 2022 19:43:58 -0500 Subject: [PATCH] Make Spake2+ output arguments size-safe (#14667) * Make Spake2+ output arguments size-size - Both Spake2p::Mac() and Spake2p::HashFinalize expected properly sized output buffers without ensuring it, leading to possible buffer overruns. - This PR makes it necessary to pass a MutableByteSpan to ensure size is checked, and adds the proper checks throughout Fixes #4189 Testing done: - Updatd unit tests to match and they still pass - Integration tests pass * Restyled by clang-format Co-authored-by: Restyled.io --- src/crypto/CHIPCryptoPAL.cpp | 11 +++++++---- src/crypto/CHIPCryptoPAL.h | 21 +++++++++++---------- src/crypto/CHIPCryptoPALOpenSSL.cpp | 14 ++++++++++---- src/crypto/CHIPCryptoPALmbedTLS.cpp | 14 +++++++++----- src/crypto/tests/CHIPCryptoPALTest.cpp | 5 +++-- 5 files changed, 40 insertions(+), 25 deletions(-) diff --git a/src/crypto/CHIPCryptoPAL.cpp b/src/crypto/CHIPCryptoPAL.cpp index 6e42c1f6df9e47..a1664c554812a5 100644 --- a/src/crypto/CHIPCryptoPAL.cpp +++ b/src/crypto/CHIPCryptoPAL.cpp @@ -345,6 +345,7 @@ CHIP_ERROR Spake2p::ComputeRoundOne(const uint8_t * pab, size_t pab_len, uint8_t CHIP_ERROR Spake2p::ComputeRoundTwo(const uint8_t * in, size_t in_len, uint8_t * out, size_t * out_len) { CHIP_ERROR error = CHIP_ERROR_INTERNAL; + MutableByteSpan out_span{ out, *out_len }; uint8_t point_buffer[kMAX_Point_Length]; void * MN = nullptr; // Choose N if a prover, M if a verifier void * XY = nullptr; // Choose Y if a prover, X if a verifier @@ -406,7 +407,8 @@ CHIP_ERROR Spake2p::ComputeRoundTwo(const uint8_t * in, size_t in_len, uint8_t * SuccessOrExit(error = GenerateKeys()); - SuccessOrExit(error = Mac(Kcaorb, hash_size / 2, in, in_len, out)); + SuccessOrExit(error = Mac(Kcaorb, hash_size / 2, in, in_len, out_span)); + VerifyOrExit(out_span.size() == hash_size, error = CHIP_ERROR_INTERNAL); state = CHIP_SPAKE2P_STATE::R2; error = CHIP_NO_ERROR; @@ -419,7 +421,9 @@ CHIP_ERROR Spake2p::GenerateKeys() { static const uint8_t info_keyconfirm[16] = { 'C', 'o', 'n', 'f', 'i', 'r', 'm', 'a', 't', 'i', 'o', 'n', 'K', 'e', 'y', 's' }; - ReturnErrorOnFailure(HashFinalize(Kae)); + MutableByteSpan Kae_span{ &Kae[0], sizeof(Kae) }; + + ReturnErrorOnFailure(HashFinalize(Kae_span)); ReturnErrorOnFailure(KDF(Ka, hash_size / 2, nullptr, 0, info_keyconfirm, sizeof(info_keyconfirm), Kcab, hash_size)); return CHIP_NO_ERROR; @@ -486,9 +490,8 @@ CHIP_ERROR Spake2p_P256_SHA256_HKDF_HMAC::Hash(const uint8_t * in, size_t in_len return CHIP_NO_ERROR; } -CHIP_ERROR Spake2p_P256_SHA256_HKDF_HMAC::HashFinalize(uint8_t * out) +CHIP_ERROR Spake2p_P256_SHA256_HKDF_HMAC::HashFinalize(MutableByteSpan & out_span) { - MutableByteSpan out_span(out, kSHA256_Hash_Length); ReturnErrorOnFailure(sha256_hash_ctx.Finish(out_span)); return CHIP_NO_ERROR; } diff --git a/src/crypto/CHIPCryptoPAL.h b/src/crypto/CHIPCryptoPAL.h index 618c5676196b81..6367ae2698dce8 100644 --- a/src/crypto/CHIPCryptoPAL.h +++ b/src/crypto/CHIPCryptoPAL.h @@ -1112,24 +1112,25 @@ class Spake2p /** * @brief Return the hash. * - * @param out Output buffer. The size is implicit and is determined by the hash used. + * @param out_span Output buffer. The size available must be >= the hash size. It gets resized + * to hash size on success. * * @return Returns a CHIP_ERROR on error, CHIP_NO_ERROR otherwise **/ - virtual CHIP_ERROR HashFinalize(uint8_t * out) = 0; + virtual CHIP_ERROR HashFinalize(MutableByteSpan & out_span) = 0; /** * @brief Generate a message authentication code. * - * @param key The MAC key buffer. - * @param key_len The size of the MAC key in bytes. - * @param in The input buffer. - * @param in_len The size of the input data to MAC in bytes. - * @param out The output MAC buffer. Size is implicit and is determined by the hash used. + * @param key The MAC key buffer. + * @param key_len The size of the MAC key in bytes. + * @param in The input buffer. + * @param in_len The size of the input data to MAC in bytes. + * @param out_span The output MAC buffer span. Size must be >= the hash_size. Output size is updated to fit on success. * * @return Returns a CHIP_ERROR on error, CHIP_NO_ERROR otherwise **/ - virtual CHIP_ERROR Mac(const uint8_t * key, size_t key_len, const uint8_t * in, size_t in_len, uint8_t * out) = 0; + virtual CHIP_ERROR Mac(const uint8_t * key, size_t key_len, const uint8_t * in, size_t in_len, MutableByteSpan & out_span) = 0; /** * @brief Verify a message authentication code. @@ -1192,7 +1193,7 @@ class Spake2p_P256_SHA256_HKDF_HMAC : public Spake2p ~Spake2p_P256_SHA256_HKDF_HMAC() override { Spake2p_P256_SHA256_HKDF_HMAC::Clear(); } void Clear() override; - CHIP_ERROR Mac(const uint8_t * key, size_t key_len, const uint8_t * in, size_t in_len, uint8_t * out) override; + CHIP_ERROR Mac(const uint8_t * key, size_t key_len, const uint8_t * in, size_t in_len, MutableByteSpan & out_span) override; CHIP_ERROR MacVerify(const uint8_t * key, size_t key_len, const uint8_t * mac, size_t mac_len, const uint8_t * in, size_t in_len) override; CHIP_ERROR FELoad(const uint8_t * in, size_t in_len, void * fe) override; @@ -1212,7 +1213,7 @@ class Spake2p_P256_SHA256_HKDF_HMAC : public Spake2p protected: CHIP_ERROR InitImpl() override; CHIP_ERROR Hash(const uint8_t * in, size_t in_len) override; - CHIP_ERROR HashFinalize(uint8_t * out) override; + CHIP_ERROR HashFinalize(MutableByteSpan & out_span) override; CHIP_ERROR KDF(const uint8_t * secret, size_t secret_length, const uint8_t * salt, size_t salt_length, const uint8_t * info, size_t info_length, uint8_t * out, size_t out_length) override; diff --git a/src/crypto/CHIPCryptoPALOpenSSL.cpp b/src/crypto/CHIPCryptoPALOpenSSL.cpp index ac65c77fd9d8db..6723da3c721771 100644 --- a/src/crypto/CHIPCryptoPALOpenSSL.cpp +++ b/src/crypto/CHIPCryptoPALOpenSSL.cpp @@ -1323,10 +1323,14 @@ void Spake2p_P256_SHA256_HKDF_HMAC::Clear() state = CHIP_SPAKE2P_STATE::PREINIT; } -CHIP_ERROR Spake2p_P256_SHA256_HKDF_HMAC::Mac(const uint8_t * key, size_t key_len, const uint8_t * in, size_t in_len, uint8_t * out) +CHIP_ERROR Spake2p_P256_SHA256_HKDF_HMAC::Mac(const uint8_t * key, size_t key_len, const uint8_t * in, size_t in_len, + MutableByteSpan & out_span) { HMAC_sha hmac; - return hmac.HMAC_SHA256(key, key_len, in, in_len, out, kSHA256_Hash_Length); + VerifyOrReturnError(out_span.size() >= kSHA256_Hash_Length, CHIP_ERROR_BUFFER_TOO_SMALL); + ReturnErrorOnFailure(hmac.HMAC_SHA256(key, key_len, in, in_len, out_span.data(), kSHA256_Hash_Length)); + out_span = out_span.SubSpan(0, kSHA256_Hash_Length); + return CHIP_NO_ERROR; } CHIP_ERROR Spake2p_P256_SHA256_HKDF_HMAC::MacVerify(const uint8_t * key, size_t key_len, const uint8_t * mac, size_t mac_len, @@ -1335,9 +1339,11 @@ CHIP_ERROR Spake2p_P256_SHA256_HKDF_HMAC::MacVerify(const uint8_t * key, size_t VerifyOrReturnError(mac_len == kSHA256_Hash_Length, CHIP_ERROR_INVALID_ARGUMENT); uint8_t computed_mac[kSHA256_Hash_Length]; - ReturnErrorOnFailure(Mac(key, key_len, in, in_len, computed_mac)); + MutableByteSpan computed_mac_span{ computed_mac }; + ReturnErrorOnFailure(Mac(key, key_len, in, in_len, computed_mac_span)); + VerifyOrReturnError(computed_mac_span.size() == mac_len, CHIP_ERROR_INTERNAL); - VerifyOrReturnError(CRYPTO_memcmp(mac, computed_mac, mac_len) == 0, CHIP_ERROR_INTERNAL); + VerifyOrReturnError(CRYPTO_memcmp(mac, computed_mac_span.data(), computed_mac_span.size()) == 0, CHIP_ERROR_INTERNAL); return CHIP_NO_ERROR; } diff --git a/src/crypto/CHIPCryptoPALmbedTLS.cpp b/src/crypto/CHIPCryptoPALmbedTLS.cpp index ec504dc84d041b..b28f5af9816ae0 100644 --- a/src/crypto/CHIPCryptoPALmbedTLS.cpp +++ b/src/crypto/CHIPCryptoPALmbedTLS.cpp @@ -1027,10 +1027,14 @@ void Spake2p_P256_SHA256_HKDF_HMAC::Clear() state = CHIP_SPAKE2P_STATE::PREINIT; } -CHIP_ERROR Spake2p_P256_SHA256_HKDF_HMAC::Mac(const uint8_t * key, size_t key_len, const uint8_t * in, size_t in_len, uint8_t * out) +CHIP_ERROR Spake2p_P256_SHA256_HKDF_HMAC::Mac(const uint8_t * key, size_t key_len, const uint8_t * in, size_t in_len, + MutableByteSpan & out_span) { HMAC_sha hmac; - return hmac.HMAC_SHA256(key, key_len, in, in_len, out, kSHA256_Hash_Length); + VerifyOrReturnError(out_span.size() >= kSHA256_Hash_Length, CHIP_ERROR_BUFFER_TOO_SMALL); + ReturnErrorOnFailure(hmac.HMAC_SHA256(key, key_len, in, in_len, out_span.data(), kSHA256_Hash_Length)); + out_span = out_span.SubSpan(0, kSHA256_Hash_Length); + return CHIP_NO_ERROR; } /** @@ -1058,11 +1062,11 @@ CHIP_ERROR Spake2p_P256_SHA256_HKDF_HMAC::MacVerify(const uint8_t * key, size_t int result = 0; uint8_t computed_mac[kSHA256_Hash_Length]; - + MutableByteSpan computed_mac_span{ computed_mac }; VerifyOrExit(mac_len == kSHA256_Hash_Length, error = CHIP_ERROR_INVALID_ARGUMENT); - error = Mac(key, key_len, in, in_len, computed_mac); - SuccessOrExit(error); + SuccessOrExit(error = Mac(key, key_len, in, in_len, computed_mac_span)); + VerifyOrExit(computed_mac_span.size() == mac_len, error = CHIP_ERROR_INTERNAL); VerifyOrExit(constant_time_memcmp(mac, computed_mac, kSHA256_Hash_Length) == 0, error = CHIP_ERROR_INTERNAL); diff --git a/src/crypto/tests/CHIPCryptoPALTest.cpp b/src/crypto/tests/CHIPCryptoPALTest.cpp index 659f9884ef3782..f34e18267210be 100644 --- a/src/crypto/tests/CHIPCryptoPALTest.cpp +++ b/src/crypto/tests/CHIPCryptoPALTest.cpp @@ -1414,6 +1414,7 @@ static void TestSPAKE2P_spake2p_Mac(nlTestSuite * inSuite, void * inContext) { HeapChecker heapChecker(inSuite); uint8_t mac[kMAX_Hash_Length]; + MutableByteSpan mac_span{ mac }; int numOfTestVectors = ArraySize(hmac_tvs); int numOfTestsRan = 0; @@ -1426,10 +1427,10 @@ static void TestSPAKE2P_spake2p_Mac(nlTestSuite * inSuite, void * inContext) CHIP_ERROR err = spake2p.Init(nullptr, 0); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); - err = spake2p.Mac(vector->key, vector->key_len, vector->input, vector->input_len, mac); + err = spake2p.Mac(vector->key, vector->key_len, vector->input, vector->input_len, mac_span); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); - NL_TEST_ASSERT(inSuite, memcmp(mac, vector->output, vector->output_len) == 0); + NL_TEST_ASSERT(inSuite, memcmp(mac_span.data(), vector->output, vector->output_len) == 0); err = spake2p.MacVerify(vector->key, vector->key_len, vector->output, vector->output_len, vector->input, vector->input_len); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);