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 <bzbarsky@apple.com>

* Restyled by clang-format

---------

Co-authored-by: Restyled.io <commits@restyled.io>
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

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<uint8_t> 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;
         }

src/credentials/FabricTable.cpp

@@ -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;

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

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<FabricIndex>(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));
 

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;