Skip to content

Commit

Permalink
Make Spake2+ output arguments size-safe (#14667)
Browse files Browse the repository at this point in the history 
* 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 <[email protected]>
  • Loading branch information
@tcarmelveilleux @restyled-commits @pull
2 people authored and pull[bot] committed Dec 13, 2023
1 parent 50ca1eb commit 72a06f1
Show file tree
Hide file tree
Showing 5 changed files with 40 additions and 25 deletions.
11 changes: 7 additions & 4 deletions src/crypto/CHIPCryptoPAL.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down
21 changes: 11 additions & 10 deletions src/crypto/CHIPCryptoPAL.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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;
Expand All @@ -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;

Expand Down
14 changes: 10 additions & 4 deletions src/crypto/CHIPCryptoPALOpenSSL.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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;
}
Expand Down
14 changes: 9 additions & 5 deletions src/crypto/CHIPCryptoPALmbedTLS.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand Down Expand Up @@ -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);

Expand Down
5 changes: 3 additions & 2 deletions src/crypto/tests/CHIPCryptoPALTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
Expand Down

0 comments on commit 72a06f1

Please sign in to comment.