From deca7f78651f1588a2a5a1c14b6df8438625865f Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Thu, 24 Aug 2023 13:54:18 -0400 Subject: [PATCH 1/5] Increase available size for NOCSR Request - Some implementers ran out of space when adding even a single extension. - 400 is the same size as the max compressed cert format, which we use many places. The total increase is thus small, for a transient buffer. - ReadDerLength was private and insufficiently tested. Some issues were found by @bluebin14. Testing done: - Existing test coverage passes - Added exhaustive unit tests for ReadDerLength, covering all failing cases, and expanding it usability beyond 8 bits of range. --- src/crypto/CHIPCryptoPAL.cpp | 92 +++++++------ src/crypto/CHIPCryptoPAL.h | 15 ++- src/crypto/tests/CHIPCryptoPALTest.cpp | 135 +++++++++++++++++++ src/lib/support/UnitTestExtendedAssertions.h | 33 +++-- 4 files changed, 218 insertions(+), 57 deletions(-) diff --git a/src/crypto/CHIPCryptoPAL.cpp b/src/crypto/CHIPCryptoPAL.cpp index 8d42fd49726156..488fcf3e95b662 100644 --- a/src/crypto/CHIPCryptoPAL.cpp +++ b/src/crypto/CHIPCryptoPAL.cpp @@ -44,39 +44,6 @@ constexpr uint8_t kIntegerTag = 0x02u; constexpr uint8_t kSeqTag = 0x30u; constexpr size_t kMinSequenceOverhead = 1 /* tag */ + 1 /* length */ + 1 /* actual data or second length byte*/; -/** - * @brief Utility to read a length field after a tag in a DER-encoded stream. - * @param[in] reader Reader instance from which the input will be read - * @param[out] length Length of the following element read from the stream - * @return CHIP_ERROR_INVALID_ARGUMENT or CHIP_ERROR_BUFFER_TOO_SMALL on error, CHIP_NO_ERROR otherwise - */ -CHIP_ERROR ReadDerLength(Reader & reader, uint8_t & length) -{ - length = 0; - - uint8_t cur_byte = 0; - ReturnErrorOnFailure(reader.Read8(&cur_byte).StatusCode()); - - if ((cur_byte & (1u << 7)) == 0) - { - // 7 bit length, the rest of the byte is the length. - length = cur_byte & 0x7Fu; - return CHIP_NO_ERROR; - } - - // Did not early return: > 7 bit length, the number of bytes of the length is provided next. - uint8_t length_bytes = cur_byte & 0x7Fu; - - if ((length_bytes > 1) || !reader.HasAtLeast(length_bytes)) - { - // We only support lengths of 0..255 over 2 bytes - return CHIP_ERROR_INVALID_ARGUMENT; - } - - // Next byte has length 0..255. - return reader.Read8(&length).StatusCode(); -} - /** * @brief Utility to convert DER-encoded INTEGER into a raw integer buffer in big-endian order * with leading zeroes if the output buffer is larger than needed. @@ -94,8 +61,8 @@ CHIP_ERROR ReadDerUnsignedIntegerIntoRaw(Reader & reader, MutableByteSpan raw_in VerifyOrReturnError(cur_byte == kIntegerTag, CHIP_ERROR_INVALID_ARGUMENT); // Read the length - uint8_t integer_len = 0; - ReturnErrorOnFailure(ReadDerLength(reader, integer_len)); + size_t integer_len = 0; + ReturnErrorOnFailure(chip::Crypto::ReadDerLength(reader, integer_len)); // Clear the destination buffer, so we can blit the unsigned value into place memset(raw_integer_out.data(), 0, raw_integer_out.size()); @@ -580,6 +547,55 @@ CHIP_ERROR Spake2pVerifier::ComputeWS(uint32_t pbkdf2IterCount, const ByteSpan & pbkdf2IterCount, ws_len, ws); } +CHIP_ERROR ReadDerLength(Reader & reader, size_t & length) +{ + length = 0; + + uint8_t cur_byte = 0; + ReturnErrorOnFailure(reader.Read8(&cur_byte).StatusCode()); + + if ((cur_byte & (1u << 7)) == 0) + { + // 7 bit length, the rest of the byte is the length. + length = cur_byte & 0x7Fu; + return CHIP_NO_ERROR; + } + + CHIP_ERROR err = CHIP_ERROR_INVALID_ARGUMENT; + + // Did not early return: > 7 bit length, the number of bytes of the length is provided next. + uint8_t length_bytes = cur_byte & 0x7Fu; + VerifyOrReturnError((length_bytes >= 1) && (length_bytes <= sizeof(size_t)), CHIP_ERROR_INVALID_ARGUMENT); + VerifyOrReturnError(reader.HasAtLeast(length_bytes), CHIP_ERROR_BUFFER_TOO_SMALL); + + for (uint8_t i = 0; i < length_bytes; i++) + { + uint8_t cur_length_byte = 0; + err = reader.Read8(&cur_length_byte).StatusCode(); + if (err != CHIP_NO_ERROR) + break; + + // Cannot have zero padding on multi-bytes lengths in DER, so first + // byte must always be > 0. + if ((i == 0) && (cur_length_byte == 0)) + { + return CHIP_ERROR_INVALID_ARGUMENT; + } + + length <<= 8; + length |= cur_length_byte; + } + + // Single-byte long length cannot be < 128: DER always encodes on smallest size + // possible, so length zero should have been a single byte short length. + if ((length_bytes == 1) && (length < 128)) + { + return CHIP_ERROR_INVALID_ARGUMENT; + } + + return CHIP_NO_ERROR; +} + CHIP_ERROR ConvertIntegerRawToDerWithoutTag(const ByteSpan & raw_integer, MutableByteSpan & out_der_integer) { return ConvertIntegerRawToDerInternal(raw_integer, out_der_integer, /* include_tag_and_length = */ false); @@ -672,7 +688,7 @@ CHIP_ERROR EcdsaAsn1SignatureToRaw(size_t fe_length_bytes, const ByteSpan & asn1 VerifyOrReturnError(tag == kSeqTag, CHIP_ERROR_INVALID_ARGUMENT); // Read length of sequence - uint8_t tag_len = 0; + size_t tag_len = 0; ReturnErrorOnFailure(ReadDerLength(reader, tag_len)); // Length of sequence must match what is left of signature @@ -1102,12 +1118,12 @@ CHIP_ERROR VerifyCertificateSigningRequestFormat(const uint8_t * csr, size_t csr ReturnErrorOnFailure(reader.Read8(&seq_header).StatusCode()); VerifyOrReturnError(seq_header == kSeqTag, CHIP_ERROR_UNSUPPORTED_CERT_FORMAT); - uint8_t seq_length = 0; + size_t seq_length = 0; VerifyOrReturnError(ReadDerLength(reader, seq_length) == CHIP_NO_ERROR, CHIP_ERROR_UNSUPPORTED_CERT_FORMAT); // Ensure that outer length matches sequence length + tag overhead, otherwise // we have trailing garbage - size_t header_overhead = (seq_length <= 127) ? 2 : 3; + size_t header_overhead = (seq_length <= 127) ? 2 : ((seq_length <= 255) ? 3 : 4); VerifyOrReturnError(csr_length == (seq_length + header_overhead), CHIP_ERROR_UNSUPPORTED_CERT_FORMAT); return CHIP_NO_ERROR; diff --git a/src/crypto/CHIPCryptoPAL.h b/src/crypto/CHIPCryptoPAL.h index 65511668b1d5fd..952f909577282e 100644 --- a/src/crypto/CHIPCryptoPAL.h +++ b/src/crypto/CHIPCryptoPAL.h @@ -31,6 +31,7 @@ #include #include #include +#include #include #include #include @@ -70,8 +71,10 @@ constexpr size_t kMAX_Point_Length = kP256_Point_Length; constexpr size_t kMAX_Hash_Length = kSHA256_Hash_Length; // Max CSR length should be relatively small since it's a single P256 key and -// no metadata is expected to be honored by the CA. -constexpr size_t kMAX_CSR_Length = 255; +// no metadata is expected to be honored by the CA. This size is an implementation +// compromise and is not spec-mandated. It is sufficiently large for a PKCS#10 +// CSR that contains approximately 150 bytes of ASN.1 extensions. +constexpr size_t kMAX_CSR_Length = 400; constexpr size_t CHIP_CRYPTO_HASH_LEN_BYTES = kSHA256_Hash_Length; @@ -639,6 +642,14 @@ CHIP_ERROR EcdsaRawSignatureToAsn1(size_t fe_length_bytes, const ByteSpan & raw_ */ CHIP_ERROR EcdsaAsn1SignatureToRaw(size_t fe_length_bytes, const ByteSpan & asn1_sig, MutableByteSpan & out_raw_sig); +/** + * @brief Utility to read a length field after a tag in a DER-encoded stream. + * @param[in] reader Reader instance from which the input will be read + * @param[out] length Length of the following element read from the stream + * @return CHIP_ERROR_INVALID_ARGUMENT or CHIP_ERROR_BUFFER_TOO_SMALL on error, CHIP_NO_ERROR otherwise + */ +CHIP_ERROR ReadDerLength(chip::Encoding::LittleEndian::Reader & reader, size_t & length); + /** * @brief Utility to emit a DER-encoded INTEGER given a raw unsigned large integer * in big-endian order. The `out_der_integer` span is updated to reflect the final diff --git a/src/crypto/tests/CHIPCryptoPALTest.cpp b/src/crypto/tests/CHIPCryptoPALTest.cpp index bcf858d8b53e10..d8ba8087f29732 100644 --- a/src/crypto/tests/CHIPCryptoPALTest.cpp +++ b/src/crypto/tests/CHIPCryptoPALTest.cpp @@ -40,6 +40,7 @@ #include #include #include +#include #include #include @@ -609,6 +610,138 @@ static void TestRawIntegerToDerInvalidCases(nlTestSuite * inSuite, void * inCont } } +static void TestReadDerLengthValidCases(nlTestSuite * inSuite, void * inContext) +{ + const uint8_t short_zero_length[] = {0x00}; + ByteSpan short_zero_length_buf(short_zero_length); + + const uint8_t short_length[] = {0x15}; + ByteSpan short_length_buf(short_length); + + const uint8_t single_byte_length[] = {0x81, 0x80}; + ByteSpan single_byte_length_buf(single_byte_length); + + const uint8_t single_byte_length_large[] = {0x81, 0xFF}; + ByteSpan single_byte_length_large_buf(single_byte_length_large); + + const uint8_t two_byte_length[] = {0x82, 0xFF, 0x01}; + ByteSpan two_byte_length_buf(two_byte_length); + + const uint8_t three_byte_length[] = {0x83, 0xFF, 0x00, 0xAA}; + ByteSpan three_byte_length_buf(three_byte_length); + + const uint8_t four_byte_length[] = {0x84, 0x01, 0x02, 0x03, 0x04}; + ByteSpan four_byte_length_buf(four_byte_length); + + const uint8_t four_byte_length_large[] = {0x84, 0xFF, 0xFF, 0xFF, 0xFF}; + ByteSpan four_byte_length_large_buf(four_byte_length_large); + + uint8_t max_byte_length_large[1 + sizeof(size_t)]; + ByteSpan max_byte_length_large_buf(max_byte_length_large); + + // We build a DER length value of SIZE_MAX programmatically. + max_byte_length_large[0] = 0x80 | sizeof(size_t); + memset(&max_byte_length_large[1], 0xFF, sizeof(size_t)); + + struct SuccessCase + { + const ByteSpan & input_buf; + const size_t expected_length; + }; + + const SuccessCase cases[] = { + { .input_buf = short_zero_length_buf, .expected_length = static_cast(0x00) }, + { .input_buf = short_length_buf, .expected_length = static_cast(0x15) }, + { .input_buf = single_byte_length_buf, .expected_length = static_cast(0x80) }, + { .input_buf = single_byte_length_large_buf, .expected_length = static_cast(0xFF) }, + { .input_buf = two_byte_length_buf, .expected_length = static_cast(0xFF01) }, + { .input_buf = three_byte_length_buf, .expected_length = static_cast(0xFF00AAUL) }, + { .input_buf = four_byte_length_buf, .expected_length = static_cast(0x01020304UL) }, + { .input_buf = four_byte_length_large_buf, .expected_length = static_cast(0xFFFFFFFFUL) }, + { .input_buf = max_byte_length_large_buf, .expected_length = SIZE_MAX }, + }; + + int case_idx = 0; + for (const SuccessCase & v : cases) + { + size_t output_length = SIZE_MAX - 1; + chip::Encoding::LittleEndian::Reader input_reader{v.input_buf}; + CHIP_ERROR status = ReadDerLength(input_reader, output_length); + if ((status != CHIP_NO_ERROR) || (v.expected_length != output_length)) + { + ChipLogError(Crypto, "Failed TestReadDerLengthValidCases sub-case %d", case_idx); + NL_TEST_ASSERT_EQUALS(inSuite, output_length, v.expected_length); + NL_TEST_ASSERT_SUCCESS(inSuite, status); + } + ++case_idx; + } +} + +static void TestReadDerLengthInvalidCases(nlTestSuite * inSuite, void * inContext) +{ + uint8_t placeholder[1]; + + ByteSpan bad_buffer_nullptr(nullptr, sizeof(placeholder)); + ByteSpan bad_buffer_empty(placeholder, 0); + + const uint8_t zero_multi_byte_length[] = {0x80}; + ByteSpan zero_multi_byte_length_buf(zero_multi_byte_length); + + const uint8_t single_byte_length_zero[] = {0x81, 0x00}; + ByteSpan single_byte_length_zero_buf(single_byte_length_zero); + + const uint8_t single_byte_length_too_small[] = {0x81, 0x7F}; + ByteSpan single_byte_length_too_small_buf(single_byte_length_too_small); + + const uint8_t multiple_byte_length_zero_padded[] = {0x82, 0x00, 0xFF}; + ByteSpan multiple_byte_length_zero_padded_buf(multiple_byte_length_zero_padded); + + const uint8_t multiple_byte_length_insufficient_bytes[] = {0x84, 0xFF, 0xAA, 0x01}; + ByteSpan multiple_byte_length_insufficient_bytes_buf(multiple_byte_length_insufficient_bytes); + + const uint8_t multiple_byte_length_insufficient_bytes2[] = {0x83}; + ByteSpan multiple_byte_length_insufficient_bytes2_buf(multiple_byte_length_insufficient_bytes2); + + uint8_t max_byte_length_large_insufficient_bytes[1 + sizeof(size_t) - 1]; + ByteSpan max_byte_length_large_insufficient_bytes_buf(max_byte_length_large_insufficient_bytes); + + // We build a DER length value of SIZE_MAX programmatically, with one byte too few. + max_byte_length_large_insufficient_bytes[0] = 0x80 | sizeof(size_t); + memset(&max_byte_length_large_insufficient_bytes[1], 0xFF, sizeof(max_byte_length_large_insufficient_bytes) - 1); + + struct ErrorCase + { + const ByteSpan & input_buf; + CHIP_ERROR expected_status; + }; + + const ErrorCase error_cases[] = { + { .input_buf = bad_buffer_nullptr, .expected_status = CHIP_ERROR_BUFFER_TOO_SMALL }, + { .input_buf = bad_buffer_empty, .expected_status = CHIP_ERROR_BUFFER_TOO_SMALL }, + { .input_buf = zero_multi_byte_length_buf,.expected_status = CHIP_ERROR_INVALID_ARGUMENT }, + { .input_buf = single_byte_length_zero_buf,.expected_status = CHIP_ERROR_INVALID_ARGUMENT }, + { .input_buf = single_byte_length_too_small_buf, .expected_status = CHIP_ERROR_INVALID_ARGUMENT }, + { .input_buf = multiple_byte_length_zero_padded_buf, .expected_status = CHIP_ERROR_INVALID_ARGUMENT }, + { .input_buf = multiple_byte_length_insufficient_bytes_buf, .expected_status = CHIP_ERROR_BUFFER_TOO_SMALL }, + { .input_buf = multiple_byte_length_insufficient_bytes2_buf, .expected_status = CHIP_ERROR_BUFFER_TOO_SMALL }, + { .input_buf = max_byte_length_large_insufficient_bytes_buf, .expected_status = CHIP_ERROR_BUFFER_TOO_SMALL }, + }; + + int case_idx = 0; + for (const ErrorCase & v : error_cases) + { + size_t output_length = SIZE_MAX; + chip::Encoding::LittleEndian::Reader input_reader{v.input_buf}; + CHIP_ERROR status = ReadDerLength(input_reader, output_length); + if (status != v.expected_status) + { + ChipLogError(Crypto, "Failed TestReadDerLengthInvalidCases sub-case %d", case_idx); + NL_TEST_ASSERT_EQUALS(inSuite, v.expected_status, status); + } + ++case_idx; + } +} + static void TestHash_SHA256(nlTestSuite * inSuite, void * inContext) { HeapChecker heapChecker(inSuite); @@ -2781,6 +2914,8 @@ static const nlTest sTests[] = { NL_TEST_DEF("Test decrypting AES-CCM-128 invalid nonce", TestAES_CCM_128DecryptInvalidNonceLen), NL_TEST_DEF("Test encrypt/decrypt AES-CTR-128 test vectors", TestAES_CTR_128CryptTestVectors), NL_TEST_DEF("Test ASN.1 signature conversion routines", TestAsn1Conversions), + NL_TEST_DEF("Test reading a length from ASN.1 DER stream success cases", TestReadDerLengthValidCases), + NL_TEST_DEF("Test reading a length from ASN.1 DER stream error cases", TestReadDerLengthInvalidCases), NL_TEST_DEF("Test Integer to ASN.1 DER conversion", TestRawIntegerToDerValidCases), NL_TEST_DEF("Test Integer to ASN.1 DER conversion error cases", TestRawIntegerToDerInvalidCases), NL_TEST_DEF("Test ECDSA signing and validation message using SHA256", TestECDSA_Signing_SHA256_Msg), diff --git a/src/lib/support/UnitTestExtendedAssertions.h b/src/lib/support/UnitTestExtendedAssertions.h index 9bf1d6f9b0e600..42cd3e9553d6ef 100644 --- a/src/lib/support/UnitTestExtendedAssertions.h +++ b/src/lib/support/UnitTestExtendedAssertions.h @@ -50,28 +50,27 @@ } while (0) /** - * @def NL_TEST_ASSERT_SUCCESS(inSuite, expression) + * @def NL_TEST_ASSERT_EQUALS(inSuite, inExpr1,, inExpr2) * * @brief - * This is used to assert that an expression is equal to CHIP_NO_ERROR - * throughout a test in a test suite. + * This is used to assert that two expressions are equal, and to print both sides on failure. This + * does not attempt to print the value of variables. It only prints the expressions. * * @param[in] inSuite A pointer to the test suite the assertion * should be accounted against. - * @param[in] inExpression Expression to be checked for equality to CHIP_NO_ERROR. - * If the expression is different than CHIP_NO_ERROR, the - * assertion fails. + * @param[in] inExpr1 Left hand-side to check + * @param[in] inExpr2 Right hand-side to check * */ -#define NL_TEST_ASSERT_EQUALS(inSuite, inExpr1, inExpr2) \ - do \ - { \ - (inSuite)->performedAssertions += 1; \ - \ - if ((inExpr1) != (inExpr2)) \ - { \ - printf("%s:%u: assertion failed: %s == %s\n", __FILE__, __LINE__, #inExpr1, #inExpr2); \ - (inSuite)->failedAssertions += 1; \ - (inSuite)->flagError = true; \ - } \ +#define NL_TEST_ASSERT_EQUALS(inSuite, inExpr1, inExpr2) \ + do \ + { \ + (inSuite)->performedAssertions += 1; \ + \ + if (!((inExpr1) == (inExpr2))) \ + { \ + printf("%s:%u: assertion failed: %s == %s\n", __FILE__, __LINE__, #inExpr1, #inExpr2); \ + (inSuite)->failedAssertions += 1; \ + (inSuite)->flagError = true; \ + } \ } while (0) From 37a462aae0546d4f15479d9593ee7b668a8eefe5 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Fri, 25 Aug 2023 18:40:56 +0000 Subject: [PATCH 2/5] Restyled by clang-format --- src/crypto/CHIPCryptoPAL.cpp | 2 +- src/crypto/tests/CHIPCryptoPALTest.cpp | 36 ++++++++++---------- src/lib/support/UnitTestExtendedAssertions.h | 22 ++++++------ 3 files changed, 30 insertions(+), 30 deletions(-) diff --git a/src/crypto/CHIPCryptoPAL.cpp b/src/crypto/CHIPCryptoPAL.cpp index 488fcf3e95b662..f854eee067d5df 100644 --- a/src/crypto/CHIPCryptoPAL.cpp +++ b/src/crypto/CHIPCryptoPAL.cpp @@ -571,7 +571,7 @@ CHIP_ERROR ReadDerLength(Reader & reader, size_t & length) for (uint8_t i = 0; i < length_bytes; i++) { uint8_t cur_length_byte = 0; - err = reader.Read8(&cur_length_byte).StatusCode(); + err = reader.Read8(&cur_length_byte).StatusCode(); if (err != CHIP_NO_ERROR) break; diff --git a/src/crypto/tests/CHIPCryptoPALTest.cpp b/src/crypto/tests/CHIPCryptoPALTest.cpp index d8ba8087f29732..d2167c20d6fbf2 100644 --- a/src/crypto/tests/CHIPCryptoPALTest.cpp +++ b/src/crypto/tests/CHIPCryptoPALTest.cpp @@ -612,28 +612,28 @@ static void TestRawIntegerToDerInvalidCases(nlTestSuite * inSuite, void * inCont static void TestReadDerLengthValidCases(nlTestSuite * inSuite, void * inContext) { - const uint8_t short_zero_length[] = {0x00}; + const uint8_t short_zero_length[] = { 0x00 }; ByteSpan short_zero_length_buf(short_zero_length); - const uint8_t short_length[] = {0x15}; + const uint8_t short_length[] = { 0x15 }; ByteSpan short_length_buf(short_length); - const uint8_t single_byte_length[] = {0x81, 0x80}; + const uint8_t single_byte_length[] = { 0x81, 0x80 }; ByteSpan single_byte_length_buf(single_byte_length); - const uint8_t single_byte_length_large[] = {0x81, 0xFF}; + const uint8_t single_byte_length_large[] = { 0x81, 0xFF }; ByteSpan single_byte_length_large_buf(single_byte_length_large); - const uint8_t two_byte_length[] = {0x82, 0xFF, 0x01}; + const uint8_t two_byte_length[] = { 0x82, 0xFF, 0x01 }; ByteSpan two_byte_length_buf(two_byte_length); - const uint8_t three_byte_length[] = {0x83, 0xFF, 0x00, 0xAA}; + const uint8_t three_byte_length[] = { 0x83, 0xFF, 0x00, 0xAA }; ByteSpan three_byte_length_buf(three_byte_length); - const uint8_t four_byte_length[] = {0x84, 0x01, 0x02, 0x03, 0x04}; + const uint8_t four_byte_length[] = { 0x84, 0x01, 0x02, 0x03, 0x04 }; ByteSpan four_byte_length_buf(four_byte_length); - const uint8_t four_byte_length_large[] = {0x84, 0xFF, 0xFF, 0xFF, 0xFF}; + const uint8_t four_byte_length_large[] = { 0x84, 0xFF, 0xFF, 0xFF, 0xFF }; ByteSpan four_byte_length_large_buf(four_byte_length_large); uint8_t max_byte_length_large[1 + sizeof(size_t)]; @@ -665,7 +665,7 @@ static void TestReadDerLengthValidCases(nlTestSuite * inSuite, void * inContext) for (const SuccessCase & v : cases) { size_t output_length = SIZE_MAX - 1; - chip::Encoding::LittleEndian::Reader input_reader{v.input_buf}; + chip::Encoding::LittleEndian::Reader input_reader{ v.input_buf }; CHIP_ERROR status = ReadDerLength(input_reader, output_length); if ((status != CHIP_NO_ERROR) || (v.expected_length != output_length)) { @@ -684,22 +684,22 @@ static void TestReadDerLengthInvalidCases(nlTestSuite * inSuite, void * inContex ByteSpan bad_buffer_nullptr(nullptr, sizeof(placeholder)); ByteSpan bad_buffer_empty(placeholder, 0); - const uint8_t zero_multi_byte_length[] = {0x80}; + const uint8_t zero_multi_byte_length[] = { 0x80 }; ByteSpan zero_multi_byte_length_buf(zero_multi_byte_length); - const uint8_t single_byte_length_zero[] = {0x81, 0x00}; + const uint8_t single_byte_length_zero[] = { 0x81, 0x00 }; ByteSpan single_byte_length_zero_buf(single_byte_length_zero); - const uint8_t single_byte_length_too_small[] = {0x81, 0x7F}; + const uint8_t single_byte_length_too_small[] = { 0x81, 0x7F }; ByteSpan single_byte_length_too_small_buf(single_byte_length_too_small); - const uint8_t multiple_byte_length_zero_padded[] = {0x82, 0x00, 0xFF}; + const uint8_t multiple_byte_length_zero_padded[] = { 0x82, 0x00, 0xFF }; ByteSpan multiple_byte_length_zero_padded_buf(multiple_byte_length_zero_padded); - const uint8_t multiple_byte_length_insufficient_bytes[] = {0x84, 0xFF, 0xAA, 0x01}; + const uint8_t multiple_byte_length_insufficient_bytes[] = { 0x84, 0xFF, 0xAA, 0x01 }; ByteSpan multiple_byte_length_insufficient_bytes_buf(multiple_byte_length_insufficient_bytes); - const uint8_t multiple_byte_length_insufficient_bytes2[] = {0x83}; + const uint8_t multiple_byte_length_insufficient_bytes2[] = { 0x83 }; ByteSpan multiple_byte_length_insufficient_bytes2_buf(multiple_byte_length_insufficient_bytes2); uint8_t max_byte_length_large_insufficient_bytes[1 + sizeof(size_t) - 1]; @@ -718,8 +718,8 @@ static void TestReadDerLengthInvalidCases(nlTestSuite * inSuite, void * inContex const ErrorCase error_cases[] = { { .input_buf = bad_buffer_nullptr, .expected_status = CHIP_ERROR_BUFFER_TOO_SMALL }, { .input_buf = bad_buffer_empty, .expected_status = CHIP_ERROR_BUFFER_TOO_SMALL }, - { .input_buf = zero_multi_byte_length_buf,.expected_status = CHIP_ERROR_INVALID_ARGUMENT }, - { .input_buf = single_byte_length_zero_buf,.expected_status = CHIP_ERROR_INVALID_ARGUMENT }, + { .input_buf = zero_multi_byte_length_buf, .expected_status = CHIP_ERROR_INVALID_ARGUMENT }, + { .input_buf = single_byte_length_zero_buf, .expected_status = CHIP_ERROR_INVALID_ARGUMENT }, { .input_buf = single_byte_length_too_small_buf, .expected_status = CHIP_ERROR_INVALID_ARGUMENT }, { .input_buf = multiple_byte_length_zero_padded_buf, .expected_status = CHIP_ERROR_INVALID_ARGUMENT }, { .input_buf = multiple_byte_length_insufficient_bytes_buf, .expected_status = CHIP_ERROR_BUFFER_TOO_SMALL }, @@ -731,7 +731,7 @@ static void TestReadDerLengthInvalidCases(nlTestSuite * inSuite, void * inContex for (const ErrorCase & v : error_cases) { size_t output_length = SIZE_MAX; - chip::Encoding::LittleEndian::Reader input_reader{v.input_buf}; + chip::Encoding::LittleEndian::Reader input_reader{ v.input_buf }; CHIP_ERROR status = ReadDerLength(input_reader, output_length); if (status != v.expected_status) { diff --git a/src/lib/support/UnitTestExtendedAssertions.h b/src/lib/support/UnitTestExtendedAssertions.h index 42cd3e9553d6ef..1552648bed8ac6 100644 --- a/src/lib/support/UnitTestExtendedAssertions.h +++ b/src/lib/support/UnitTestExtendedAssertions.h @@ -62,15 +62,15 @@ * @param[in] inExpr2 Right hand-side to check * */ -#define NL_TEST_ASSERT_EQUALS(inSuite, inExpr1, inExpr2) \ - do \ - { \ - (inSuite)->performedAssertions += 1; \ - \ - if (!((inExpr1) == (inExpr2))) \ - { \ - printf("%s:%u: assertion failed: %s == %s\n", __FILE__, __LINE__, #inExpr1, #inExpr2); \ - (inSuite)->failedAssertions += 1; \ - (inSuite)->flagError = true; \ - } \ +#define NL_TEST_ASSERT_EQUALS(inSuite, inExpr1, inExpr2) \ + do \ + { \ + (inSuite)->performedAssertions += 1; \ + \ + if (!((inExpr1) == (inExpr2))) \ + { \ + printf("%s:%u: assertion failed: %s == %s\n", __FILE__, __LINE__, #inExpr1, #inExpr2); \ + (inSuite)->failedAssertions += 1; \ + (inSuite)->flagError = true; \ + } \ } while (0) From f835837bafcca8b2631455708adcd19c5f53c7b7 Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Sat, 26 Aug 2023 11:03:31 -0400 Subject: [PATCH 3/5] Replace Max CSR size with a Min CSR size. - Works in every situation. - Keeps existing constant as deprecated since main legacy usage will still work. - Remove a needless verification of size that was the root cause of issues with the constant before. --- .../operational-credentials-server.cpp | 4 +- src/credentials/FabricTable.cpp | 2 +- src/credentials/FabricTable.h | 2 +- src/credentials/tests/TestFabricTable.cpp | 42 +++++++++---------- src/crypto/CHIPCryptoPAL.cpp | 9 ++-- src/crypto/CHIPCryptoPAL.h | 16 +++---- src/crypto/OperationalKeystore.h | 2 +- .../PersistentStorageOperationalKeystore.cpp | 2 +- src/crypto/tests/CHIPCryptoPALTest.cpp | 6 +-- src/crypto/tests/TestPSAOpKeyStore.cpp | 4 +- .../tests/TestPersistentStorageOpKeyStore.cpp | 6 +-- src/darwin/Framework/CHIP/MTRCertificates.mm | 2 +- .../Framework/CHIP/MTRDeviceController.mm | 2 +- .../efr32/Efr32PsaOperationalKeystore.cpp | 2 +- 14 files changed, 52 insertions(+), 49 deletions(-) diff --git a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp index 4340f683b0d666..04a01371af92f3 100644 --- a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp +++ b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp @@ -1038,7 +1038,7 @@ bool emberAfOperationalCredentialsClusterCSRRequestCallback(app::CommandHandler // Prepare NOCSRElements structure { - constexpr size_t csrLength = Crypto::kMAX_CSR_Length; + constexpr size_t csrLength = Crypto::kMIN_CSR_Buffer_Size; size_t nocsrLengthEstimate = 0; ByteSpan kNoVendorReserved; Platform::ScopedMemoryBuffer csr; @@ -1060,7 +1060,7 @@ bool emberAfOperationalCredentialsClusterCSRRequestCallback(app::CommandHandler err = fabricTable.AllocatePendingOperationalKey(fabricIndexForCsr, csrSpan); - if (csrSpan.size() > Crypto::kMAX_CSR_Length) + if (csrSpan.size() > csrLength) { err = CHIP_ERROR_INTERNAL; } diff --git a/src/credentials/FabricTable.cpp b/src/credentials/FabricTable.cpp index f9b7503a7bad9e..474d67959db1f2 100644 --- a/src/credentials/FabricTable.cpp +++ b/src/credentials/FabricTable.cpp @@ -1540,7 +1540,7 @@ CHIP_ERROR FabricTable::AllocatePendingOperationalKey(Optional fabr // We can only allocate a pending key if no pending state (NOC, ICAC) already present, // since there can only be one pending state per fail-safe. VerifyOrReturnError(!mStateFlags.Has(StateFlags::kIsPendingFabricDataPresent), CHIP_ERROR_INCORRECT_STATE); - VerifyOrReturnError(outputCsr.size() >= Crypto::kMAX_CSR_Length, CHIP_ERROR_BUFFER_TOO_SMALL); + VerifyOrReturnError(outputCsr.size() >= Crypto::kMIN_CSR_Buffer_Size, CHIP_ERROR_BUFFER_TOO_SMALL); EnsureNextAvailableFabricIndexUpdated(); FabricIndex fabricIndexToUse = kUndefinedFabricIndex; diff --git a/src/credentials/FabricTable.h b/src/credentials/FabricTable.h index 96b5c415f7e237..621ec354e50084 100644 --- a/src/credentials/FabricTable.h +++ b/src/credentials/FabricTable.h @@ -712,7 +712,7 @@ class DLL_EXPORT FabricTable * @param fabricIndex - Existing FabricIndex for which a new keypair must be made available. If it * doesn't have a value, the key will be marked pending for the next available * fabric index that would apply for `AddNewFabric`. - * @param outputCsr - Buffer to contain the CSR. Must be at least `kMAX_CSR_Length` large. + * @param outputCsr - Buffer to contain the CSR. Must be at least `kMIN_CSR_Buffer_Size` large. * * @retval CHIP_NO_ERROR on success * @retval CHIP_ERROR_BUFFER_TOO_SMALL if `outputCsr` buffer is too small diff --git a/src/credentials/tests/TestFabricTable.cpp b/src/credentials/tests/TestFabricTable.cpp index 0ed6db832f3b17..bb7cfb910c8f73 100644 --- a/src/credentials/tests/TestFabricTable.cpp +++ b/src/credentials/tests/TestFabricTable.cpp @@ -690,7 +690,7 @@ void TestBasicAddNocUpdateNocFlow(nlTestSuite * inSuite, void * inContext) FabricId fabricId = 44; NodeId nodeId = 999; - uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length]; + uint8_t csrBuf[chip::Crypto::kMIN_CSR_Buffer_Size]; MutableByteSpan csrSpan{ csrBuf }; NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan)); @@ -810,7 +810,7 @@ void TestBasicAddNocUpdateNocFlow(nlTestSuite * inSuite, void * inContext) NodeId nodeId = 1000; FabricIndex fabricIndex = 2; - uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length]; + uint8_t csrBuf[chip::Crypto::kMIN_CSR_Buffer_Size]; MutableByteSpan csrSpan{ csrBuf }; // Make sure to tag fabric index to pending opkey: otherwise the UpdateNOC fails @@ -1070,7 +1070,7 @@ void TestAddMultipleSameRootDifferentFabricId(nlTestSuite * inSuite, void * inCo FabricId fabricId = 1111; NodeId nodeId = 55; - uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length]; + uint8_t csrBuf[chip::Crypto::kMIN_CSR_Buffer_Size]; MutableByteSpan csrSpan{ csrBuf }; NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan)); @@ -1113,7 +1113,7 @@ void TestAddMultipleSameRootDifferentFabricId(nlTestSuite * inSuite, void * inCo FabricId fabricId = 2222; NodeId nodeId = 66; - uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length]; + uint8_t csrBuf[chip::Crypto::kMIN_CSR_Buffer_Size]; MutableByteSpan csrSpan{ csrBuf }; NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan)); @@ -1177,7 +1177,7 @@ void TestAddMultipleSameFabricIdDifferentRoot(nlTestSuite * inSuite, void * inCo FabricId fabricId = 1111; NodeId nodeId = 55; - uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length]; + uint8_t csrBuf[chip::Crypto::kMIN_CSR_Buffer_Size]; MutableByteSpan csrSpan{ csrBuf }; NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan)); @@ -1220,7 +1220,7 @@ void TestAddMultipleSameFabricIdDifferentRoot(nlTestSuite * inSuite, void * inCo FabricId fabricId = 1111; NodeId nodeId = 66; - uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length]; + uint8_t csrBuf[chip::Crypto::kMIN_CSR_Buffer_Size]; MutableByteSpan csrSpan{ csrBuf }; NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan)); @@ -1302,7 +1302,7 @@ void TestPersistence(nlTestSuite * inSuite, void * inContext) FabricId fabricId = 1111; NodeId nodeId = 55; - uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length]; + uint8_t csrBuf[chip::Crypto::kMIN_CSR_Buffer_Size]; MutableByteSpan csrSpan{ csrBuf }; NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan)); @@ -1361,7 +1361,7 @@ void TestPersistence(nlTestSuite * inSuite, void * inContext) FabricId fabricId = 2222; NodeId nodeId = 66; - uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length]; + uint8_t csrBuf[chip::Crypto::kMIN_CSR_Buffer_Size]; MutableByteSpan csrSpan{ csrBuf }; NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan)); @@ -1614,7 +1614,7 @@ void TestAddNocFailSafe(nlTestSuite * inSuite, void * inContext) FabricId fabricId = 11; NodeId nodeId = 55; - uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length]; + uint8_t csrBuf[chip::Crypto::kMIN_CSR_Buffer_Size]; MutableByteSpan csrSpan{ csrBuf }; NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan)); @@ -1700,7 +1700,7 @@ void TestAddNocFailSafe(nlTestSuite * inSuite, void * inContext) FabricId fabricId = 44; NodeId nodeId = 999; - uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length]; + uint8_t csrBuf[chip::Crypto::kMIN_CSR_Buffer_Size]; MutableByteSpan csrSpan{ csrBuf }; NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan)); @@ -1846,7 +1846,7 @@ void TestUpdateNocFailSafe(nlTestSuite * inSuite, void * inContext) FabricId fabricId = 44; NodeId nodeId = 999; - uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length]; + uint8_t csrBuf[chip::Crypto::kMIN_CSR_Buffer_Size]; MutableByteSpan csrSpan{ csrBuf }; NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan)); @@ -1937,7 +1937,7 @@ void TestUpdateNocFailSafe(nlTestSuite * inSuite, void * inContext) NodeId nodeId = 1000; FabricIndex fabricIndex = 1; - uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length]; + uint8_t csrBuf[chip::Crypto::kMIN_CSR_Buffer_Size]; MutableByteSpan csrSpan{ csrBuf }; // Make sure to tag fabric index to pending opkey: otherwise the UpdateNOC fails @@ -2049,7 +2049,7 @@ void TestUpdateNocFailSafe(nlTestSuite * inSuite, void * inContext) NodeId nodeId = 1001; FabricIndex fabricIndex = 1; - uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length]; + uint8_t csrBuf[chip::Crypto::kMIN_CSR_Buffer_Size]; MutableByteSpan csrSpan{ csrBuf }; // Make sure to tag fabric index to pending opkey: otherwise the UpdateNOC fails @@ -2220,7 +2220,7 @@ void TestFabricLabelChange(nlTestSuite * inSuite, void * inContext) FabricId fabricId = 1111; NodeId nodeId = 55; - uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length]; + uint8_t csrBuf[chip::Crypto::kMIN_CSR_Buffer_Size]; MutableByteSpan csrSpan{ csrBuf }; NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan)); @@ -2291,7 +2291,7 @@ void TestFabricLabelChange(nlTestSuite * inSuite, void * inContext) FabricId fabricId = 1111; NodeId nodeId = 66; // Update node ID from 55 to 66 - uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length]; + uint8_t csrBuf[chip::Crypto::kMIN_CSR_Buffer_Size]; MutableByteSpan csrSpan{ csrBuf }; NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::MakeOptional(static_cast(1)), csrSpan)); @@ -2449,7 +2449,7 @@ void TestAddNocRootCollision(nlTestSuite * inSuite, void * inContext) FabricId fabricId = 1111; NodeId nodeId = 55; - uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length]; + uint8_t csrBuf[chip::Crypto::kMIN_CSR_Buffer_Size]; MutableByteSpan csrSpan{ csrBuf }; NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan)); @@ -2502,7 +2502,7 @@ void TestAddNocRootCollision(nlTestSuite * inSuite, void * inContext) FabricId fabricId = 1111; NodeId nodeId = 55; - uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length]; + uint8_t csrBuf[chip::Crypto::kMIN_CSR_Buffer_Size]; MutableByteSpan csrSpan{ csrBuf }; NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan)); @@ -2556,7 +2556,7 @@ void TestAddNocRootCollision(nlTestSuite * inSuite, void * inContext) FabricId fabricId = 2222; NodeId nodeId = 55; - uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length]; + uint8_t csrBuf[chip::Crypto::kMIN_CSR_Buffer_Size]; MutableByteSpan csrSpan{ csrBuf }; NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan)); @@ -2629,7 +2629,7 @@ void TestInvalidChaining(nlTestSuite * inSuite, void * inContext) FabricId fabricId = 1111; NodeId nodeId = 55; - uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length]; + uint8_t csrBuf[chip::Crypto::kMIN_CSR_Buffer_Size]; MutableByteSpan csrSpan{ csrBuf }; NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan)); @@ -2816,7 +2816,7 @@ void TestCommitMarker(nlTestSuite * inSuite, void * inContext) FabricId fabricId = 1111; NodeId nodeId = 55; - uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length]; + uint8_t csrBuf[chip::Crypto::kMIN_CSR_Buffer_Size]; MutableByteSpan csrSpan{ csrBuf }; NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan)); @@ -2882,7 +2882,7 @@ void TestCommitMarker(nlTestSuite * inSuite, void * inContext) FabricId fabricId = 2222; NodeId nodeId = 66; - uint8_t csrBuf[chip::Crypto::kMAX_CSR_Length]; + uint8_t csrBuf[chip::Crypto::kMIN_CSR_Buffer_Size]; MutableByteSpan csrSpan{ csrBuf }; NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.AllocatePendingOperationalKey(chip::NullOptional, csrSpan)); diff --git a/src/crypto/CHIPCryptoPAL.cpp b/src/crypto/CHIPCryptoPAL.cpp index f854eee067d5df..747cd2b5b6105d 100644 --- a/src/crypto/CHIPCryptoPAL.cpp +++ b/src/crypto/CHIPCryptoPAL.cpp @@ -1001,7 +1001,7 @@ static CHIP_ERROR GenerateCertificationRequestInformation(ASN1Writer & writer, c CHIP_ERROR GenerateCertificateSigningRequest(const P256Keypair * keypair, MutableByteSpan & csr_span) { VerifyOrReturnError(keypair != nullptr, CHIP_ERROR_INVALID_ARGUMENT); - VerifyOrReturnError(csr_span.size() >= kMAX_CSR_Length, CHIP_ERROR_BUFFER_TOO_SMALL); + VerifyOrReturnError(csr_span.size() >= kMIN_CSR_Buffer_Size, CHIP_ERROR_BUFFER_TOO_SMALL); // First pass: Generate the CertificatioRequestInformation inner // encoding one time, to sign it, before re-generating it within the @@ -1108,8 +1108,10 @@ CHIP_ERROR GenerateCertificateSigningRequest(const P256Keypair * keypair, Mutabl CHIP_ERROR VerifyCertificateSigningRequestFormat(const uint8_t * csr, size_t csr_length) { - // Ensure we have enough size to validate header - VerifyOrReturnError((csr_length >= 16) && (csr_length <= kMAX_CSR_Length), CHIP_ERROR_UNSUPPORTED_CERT_FORMAT); + // Ensure we have enough size to validate header, and that our assumptions are met + // for some tag computations below. A csr_length > 65535 would never be seen in + // practice. + VerifyOrReturnError((csr_length >= 16) && (csr_length <= 65535), CHIP_ERROR_UNSUPPORTED_CERT_FORMAT); Reader reader(csr, csr_length); @@ -1120,7 +1122,6 @@ CHIP_ERROR VerifyCertificateSigningRequestFormat(const uint8_t * csr, size_t csr size_t seq_length = 0; VerifyOrReturnError(ReadDerLength(reader, seq_length) == CHIP_NO_ERROR, CHIP_ERROR_UNSUPPORTED_CERT_FORMAT); - // Ensure that outer length matches sequence length + tag overhead, otherwise // we have trailing garbage size_t header_overhead = (seq_length <= 127) ? 2 : ((seq_length <= 255) ? 3 : 4); diff --git a/src/crypto/CHIPCryptoPAL.h b/src/crypto/CHIPCryptoPAL.h index 952f909577282e..7f13ba30c13073 100644 --- a/src/crypto/CHIPCryptoPAL.h +++ b/src/crypto/CHIPCryptoPAL.h @@ -70,11 +70,12 @@ constexpr size_t kMAX_FE_Length = kP256_FE_Length; constexpr size_t kMAX_Point_Length = kP256_Point_Length; constexpr size_t kMAX_Hash_Length = kSHA256_Hash_Length; -// Max CSR length should be relatively small since it's a single P256 key and -// no metadata is expected to be honored by the CA. This size is an implementation -// compromise and is not spec-mandated. It is sufficiently large for a PKCS#10 -// CSR that contains approximately 150 bytes of ASN.1 extensions. -constexpr size_t kMAX_CSR_Length = 400; +// Minimum required CSR length buffer length is relatively small since it's a single +// P256 key and no metadata/extensions are expected to be honored by the CA. +constexpr size_t kMIN_CSR_Buffer_Size = 255; + +[[deprecated("This constant is no longer used by common code and should be replaced by kMIN_CSR_Buffer_Size. Checks that a CSR is <= kMAX_CSR_Buffer_size must be updated. This remains to keep valid buffers working from previous public API usage.")]] +constexpr size_t kMAX_CSR_Buffer_Size = 255; constexpr size_t CHIP_CRYPTO_HASH_LEN_BYTES = kSHA256_Hash_Length; @@ -753,8 +754,9 @@ CHIP_ERROR AES_CTR_crypt(const uint8_t * input, size_t input_length, const Aes12 * be configured to ignore CSR requested subject. * * @param keypair The key pair for which a CSR should be generated. Must not be null. - * @param csr_span Span to hold the resulting CSR. Must be at least kMAX_CSR_Length. Otherwise returns CHIP_ERROR_BUFFER_TOO_SMALL. - * It will get resized to actual size needed on success. + * @param csr_span Span to hold the resulting CSR. Must be at least kMIN_CSR_Buffer_Size. + * Otherwise returns CHIP_ERROR_BUFFER_TOO_SMALL. It will get resized to + * actual size needed on success. * @return Returns a CHIP_ERROR from P256Keypair or ASN.1 backend on error, CHIP_NO_ERROR otherwise **/ diff --git a/src/crypto/OperationalKeystore.h b/src/crypto/OperationalKeystore.h index 6af92629174810..58eba784281786 100644 --- a/src/crypto/OperationalKeystore.h +++ b/src/crypto/OperationalKeystore.h @@ -67,7 +67,7 @@ class OperationalKeystore * Only one pending operational keypair is supported at a time. * * @param fabricIndex - FabricIndex for which a new keypair must be made available - * @param outCertificateSigningRequest - Buffer to contain the CSR. Must be at least `kMAX_CSR_Length` large. + * @param outCertificateSigningRequest - Buffer to contain the CSR. Must be at least `kMIN_CSR_Buffer_Size` large. * * @retval CHIP_NO_ERROR on success * @retval CHIP_ERROR_BUFFER_TOO_SMALL if `outCertificateSigningRequest` buffer is too small diff --git a/src/crypto/PersistentStorageOperationalKeystore.cpp b/src/crypto/PersistentStorageOperationalKeystore.cpp index d1d5b2f725c40d..80eee2d9742006 100644 --- a/src/crypto/PersistentStorageOperationalKeystore.cpp +++ b/src/crypto/PersistentStorageOperationalKeystore.cpp @@ -197,7 +197,7 @@ CHIP_ERROR PersistentStorageOperationalKeystore::NewOpKeypairForFabric(FabricInd { return CHIP_ERROR_INVALID_FABRIC_INDEX; } - VerifyOrReturnError(outCertificateSigningRequest.size() >= Crypto::kMAX_CSR_Length, CHIP_ERROR_BUFFER_TOO_SMALL); + VerifyOrReturnError(outCertificateSigningRequest.size() >= Crypto::kMIN_CSR_Buffer_Size, CHIP_ERROR_BUFFER_TOO_SMALL); // Replace previous pending keypair, if any was previously allocated ResetPendingKey(); diff --git a/src/crypto/tests/CHIPCryptoPALTest.cpp b/src/crypto/tests/CHIPCryptoPALTest.cpp index d2167c20d6fbf2..7ba0ab1a088f12 100644 --- a/src/crypto/tests/CHIPCryptoPALTest.cpp +++ b/src/crypto/tests/CHIPCryptoPALTest.cpp @@ -1423,7 +1423,7 @@ void TestCSR_Verify(nlTestSuite * inSuite, void * inContext) void TestCSR_GenDirect(nlTestSuite * inSuite, void * inContext) { - uint8_t csrBuf[kMAX_CSR_Length]; + uint8_t csrBuf[kMIN_CSR_Buffer_Size]; ClearSecretData(csrBuf); MutableByteSpan csrSpan(csrBuf); @@ -1432,7 +1432,7 @@ void TestCSR_GenDirect(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, keypair.Initialize(ECPKeyTarget::ECDSA) == CHIP_NO_ERROR); // Validate case of buffer too small - uint8_t csrBufTooSmall[kMAX_CSR_Length - 1]; + uint8_t csrBufTooSmall[kMIN_CSR_Buffer_Size - 1]; MutableByteSpan csrSpanTooSmall(csrBufTooSmall); NL_TEST_ASSERT(inSuite, GenerateCertificateSigningRequest(&keypair, csrSpanTooSmall) == CHIP_ERROR_BUFFER_TOO_SMALL); @@ -1468,7 +1468,7 @@ void TestCSR_GenDirect(nlTestSuite * inSuite, void * inContext) static void TestCSR_GenByKeypair(nlTestSuite * inSuite, void * inContext) { HeapChecker heapChecker(inSuite); - uint8_t csr[kMAX_CSR_Length]; + uint8_t csr[kMIN_CSR_Buffer_Size]; size_t length = sizeof(csr); Test_P256Keypair keypair; diff --git a/src/crypto/tests/TestPSAOpKeyStore.cpp b/src/crypto/tests/TestPSAOpKeyStore.cpp index c8aef046010196..f638663e464d3f 100644 --- a/src/crypto/tests/TestPSAOpKeyStore.cpp +++ b/src/crypto/tests/TestPSAOpKeyStore.cpp @@ -39,7 +39,7 @@ void TestBasicLifeCycle(nlTestSuite * inSuite, void * inContext) FabricIndex kBadFabricIndex = static_cast(kFabricIndex + 10u); // Can generate a key and get a CSR - uint8_t csrBuf[kMAX_CSR_Length]; + uint8_t csrBuf[kMIN_CSR_Buffer_Size]; MutableByteSpan csrSpan{ csrBuf }; CHIP_ERROR err = opKeystore.NewOpKeypairForFabric(kFabricIndex, csrSpan); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); @@ -62,7 +62,7 @@ void TestBasicLifeCycle(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, !csrPublicKey1.Matches(csrPublicKey2)); // Cannot NewOpKeypair for a different fabric if one already pending - uint8_t badCsrBuf[kMAX_CSR_Length]; + uint8_t badCsrBuf[kMIN_CSR_Buffer_Size]; MutableByteSpan badCsrSpan{ badCsrBuf }; err = opKeystore.NewOpKeypairForFabric(kBadFabricIndex, badCsrSpan); NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_INVALID_FABRIC_INDEX); diff --git a/src/crypto/tests/TestPersistentStorageOpKeyStore.cpp b/src/crypto/tests/TestPersistentStorageOpKeyStore.cpp index 82eebeab9d6900..c3fc35a453aae7 100644 --- a/src/crypto/tests/TestPersistentStorageOpKeyStore.cpp +++ b/src/crypto/tests/TestPersistentStorageOpKeyStore.cpp @@ -48,7 +48,7 @@ void TestBasicLifeCycle(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, storageDelegate.GetNumKeys() == 0); // Failure before Init of NewOpKeypairForFabric - uint8_t unusedCsrBuf[kMAX_CSR_Length]; + uint8_t unusedCsrBuf[kMIN_CSR_Buffer_Size]; MutableByteSpan unusedCsrSpan{ unusedCsrBuf }; err = opKeystore.NewOpKeypairForFabric(kFabricIndex, unusedCsrSpan); NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_INCORRECT_STATE); @@ -66,7 +66,7 @@ void TestBasicLifeCycle(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); // Can generate a key and get a CSR - uint8_t csrBuf[kMAX_CSR_Length]; + uint8_t csrBuf[kMIN_CSR_Buffer_Size]; MutableByteSpan csrSpan{ csrBuf }; err = opKeystore.NewOpKeypairForFabric(kFabricIndex, csrSpan); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); @@ -85,7 +85,7 @@ void TestBasicLifeCycle(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, opKeystore.HasPendingOpKeypair() == true); // Cannot NewOpKeypair for a different fabric if one already pending - uint8_t badCsrBuf[kMAX_CSR_Length]; + uint8_t badCsrBuf[kMIN_CSR_Buffer_Size]; MutableByteSpan badCsrSpan = MutableByteSpan{ badCsrBuf }; err = opKeystore.NewOpKeypairForFabric(kBadFabricIndex, badCsrSpan); NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_INVALID_FABRIC_INDEX); diff --git a/src/darwin/Framework/CHIP/MTRCertificates.mm b/src/darwin/Framework/CHIP/MTRCertificates.mm index e2967faba8cac2..9444ae920cc44c 100644 --- a/src/darwin/Framework/CHIP/MTRCertificates.mm +++ b/src/darwin/Framework/CHIP/MTRCertificates.mm @@ -220,7 +220,7 @@ + (NSData * _Nullable)createCertificateSigningRequest:(id)keypair break; } - uint8_t buf[kMAX_CSR_Length]; + uint8_t buf[kMIN_CSR_Buffer_Size]; MutableByteSpan csr(buf); err = GenerateCertificateSigningRequest(&keypairBridge, csr); if (err != CHIP_NO_ERROR) { diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index 21ec9e56f737e9..4b2c32d9c2bf0d 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -335,7 +335,7 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams } } else { // Generate a new random keypair. - uint8_t csrBuffer[chip::Crypto::kMAX_CSR_Length]; + uint8_t csrBuffer[chip::Crypto::kMIN_CSR_Buffer_Size]; chip::MutableByteSpan csr(csrBuffer); errorCode = startupParams.fabricTable->AllocatePendingOperationalKey(startupParams.fabricIndex, csr); if ([self checkForStartError:errorCode logMsg:kErrorKeyAllocation]) { diff --git a/src/platform/silabs/efr32/Efr32PsaOperationalKeystore.cpp b/src/platform/silabs/efr32/Efr32PsaOperationalKeystore.cpp index 14b10db2820c43..314c0ad4d3f54b 100644 --- a/src/platform/silabs/efr32/Efr32PsaOperationalKeystore.cpp +++ b/src/platform/silabs/efr32/Efr32PsaOperationalKeystore.cpp @@ -194,7 +194,7 @@ CHIP_ERROR Efr32PsaOperationalKeystore::NewOpKeypairForFabric(FabricIndex fabric return CHIP_ERROR_INVALID_FABRIC_INDEX; } - VerifyOrReturnError(outCertificateSigningRequest.size() >= Crypto::kMAX_CSR_Length, CHIP_ERROR_BUFFER_TOO_SMALL); + VerifyOrReturnError(outCertificateSigningRequest.size() >= Crypto::kMIN_CSR_Buffer_Size, CHIP_ERROR_BUFFER_TOO_SMALL); // Generate new key EFR32OpaqueKeyId id = kEFR32OpaqueKeyIdUnknown; From 6a52fdd5de0f6f00557874d0fa8500a0861f56ea Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Sat, 26 Aug 2023 11:06:26 -0400 Subject: [PATCH 4/5] Update src/crypto/CHIPCryptoPAL.cpp Co-authored-by: Boris Zbarsky --- src/crypto/CHIPCryptoPAL.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/crypto/CHIPCryptoPAL.cpp b/src/crypto/CHIPCryptoPAL.cpp index 747cd2b5b6105d..57410c8b74aff2 100644 --- a/src/crypto/CHIPCryptoPAL.cpp +++ b/src/crypto/CHIPCryptoPAL.cpp @@ -575,7 +575,7 @@ CHIP_ERROR ReadDerLength(Reader & reader, size_t & length) if (err != CHIP_NO_ERROR) break; - // Cannot have zero padding on multi-bytes lengths in DER, so first + // Cannot have zero padding on multi-byte lengths in DER, so first // byte must always be > 0. if ((i == 0) && (cur_length_byte == 0)) { From dac0e5b252186840bf444a6bbba436f8cf829d1d Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Sat, 26 Aug 2023 15:06:42 +0000 Subject: [PATCH 5/5] Restyled by clang-format --- src/crypto/CHIPCryptoPAL.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/crypto/CHIPCryptoPAL.h b/src/crypto/CHIPCryptoPAL.h index 7f13ba30c13073..9fba6d989cddbb 100644 --- a/src/crypto/CHIPCryptoPAL.h +++ b/src/crypto/CHIPCryptoPAL.h @@ -74,8 +74,9 @@ constexpr size_t kMAX_Hash_Length = kSHA256_Hash_Length; // P256 key and no metadata/extensions are expected to be honored by the CA. constexpr size_t kMIN_CSR_Buffer_Size = 255; -[[deprecated("This constant is no longer used by common code and should be replaced by kMIN_CSR_Buffer_Size. Checks that a CSR is <= kMAX_CSR_Buffer_size must be updated. This remains to keep valid buffers working from previous public API usage.")]] -constexpr size_t kMAX_CSR_Buffer_Size = 255; +[[deprecated("This constant is no longer used by common code and should be replaced by kMIN_CSR_Buffer_Size. Checks that a CSR is " + "<= kMAX_CSR_Buffer_size must be updated. This remains to keep valid buffers working from previous public API " + "usage.")]] constexpr size_t kMAX_CSR_Buffer_Size = 255; constexpr size_t CHIP_CRYPTO_HASH_LEN_BYTES = kSHA256_Hash_Length;