From 1793868f4eae3e2cc1a731ffd3366943e0058fea Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Mon, 28 Aug 2023 15:16:23 -0400 Subject: [PATCH] Remove NOCSR Request size limits and fix ReadDerLength bugs (#28899) * 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. * Restyled by clang-format * 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. * Update src/crypto/CHIPCryptoPAL.cpp Co-authored-by: Boris Zbarsky * Restyled by clang-format --------- Co-authored-by: Restyled.io Co-authored-by: Boris Zbarsky --- .../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 | 101 +++++++------ src/crypto/CHIPCryptoPAL.h | 24 ++- src/crypto/OperationalKeystore.h | 2 +- .../PersistentStorageOperationalKeystore.cpp | 2 +- src/crypto/tests/CHIPCryptoPALTest.cpp | 141 +++++++++++++++++- src/crypto/tests/TestPSAOpKeyStore.cpp | 4 +- .../tests/TestPersistentStorageOpKeyStore.cpp | 6 +- src/darwin/Framework/CHIP/MTRCertificates.mm | 2 +- .../Framework/CHIP/MTRDeviceController.mm | 2 +- src/lib/support/UnitTestExtendedAssertions.h | 13 +- .../efr32/Efr32PsaOperationalKeystore.cpp | 2 +- 15 files changed, 257 insertions(+), 92 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 8d42fd49726156..57410c8b74aff2 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-byte 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 @@ -985,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 @@ -1092,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); @@ -1102,12 +1120,11 @@ 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..9fba6d989cddbb 100644 --- a/src/crypto/CHIPCryptoPAL.h +++ b/src/crypto/CHIPCryptoPAL.h @@ -31,6 +31,7 @@ #include #include #include +#include #include #include #include @@ -69,9 +70,13 @@ 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. -constexpr size_t kMAX_CSR_Length = 255; +// 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; @@ -639,6 +644,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 @@ -742,8 +755,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 bcf858d8b53e10..7ba0ab1a088f12 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); @@ -1290,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); @@ -1299,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); @@ -1335,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; @@ -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/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/lib/support/UnitTestExtendedAssertions.h b/src/lib/support/UnitTestExtendedAssertions.h index 9bf1d6f9b0e600..1552648bed8ac6 100644 --- a/src/lib/support/UnitTestExtendedAssertions.h +++ b/src/lib/support/UnitTestExtendedAssertions.h @@ -50,17 +50,16 @@ } 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) \ @@ -68,7 +67,7 @@ { \ (inSuite)->performedAssertions += 1; \ \ - if ((inExpr1) != (inExpr2)) \ + if (!((inExpr1) == (inExpr2))) \ { \ printf("%s:%u: assertion failed: %s == %s\n", __FILE__, __LINE__, #inExpr1, #inExpr2); \ (inSuite)->failedAssertions += 1; \ 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;