Skip to content

Commit

Permalink
Remove NOCSR Request size limits and fix ReadDerLength bugs (#28899)
Browse files Browse the repository at this point in the history 
* 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 <[email protected]>

* Restyled by clang-format

---------

Co-authored-by: Restyled.io <[email protected]>
Co-authored-by: Boris Zbarsky <[email protected]>
  • Loading branch information
@tcarmelveilleux @restyled-commits
@bzbarsky-apple @pull
3 people authored and pull[bot] committed Oct 5, 2023
1 parent cae9599 commit 3522407
Show file tree
Hide file tree
Showing 15 changed files with 257 additions and 92 deletions.
4 changes: 2 additions & 2 deletions src/app/clusters/operational-credentials-server/operational-credentials-server.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<uint8_t> csr;
Expand All @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion src/credentials/FabricTable.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1540,7 +1540,7 @@ CHIP_ERROR FabricTable::AllocatePendingOperationalKey(Optional<FabricIndex> 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;
Expand Down
2 changes: 1 addition & 1 deletion src/credentials/FabricTable.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
42 changes: 21 additions & 21 deletions src/credentials/tests/TestFabricTable.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));

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

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

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

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

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

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

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

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

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

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

Expand Down Expand Up @@ -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<FabricIndex>(1)), csrSpan));
Expand Down Expand Up @@ -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));

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

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

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

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

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

Expand Down
101 changes: 59 additions & 42 deletions src/crypto/CHIPCryptoPAL.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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());
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);

Expand All @@ -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;
Expand Down
Loading

0 comments on commit 3522407

Please sign in to comment.