Fix leftover from #8236 and make BufferReader/Writer support Span

- Fix leftover nits from @bzbarsky-apple's review of #8236
- In order to add span support cleanly, added Span support to
  Reader and BufferWriter, and fixed all necessary breakage.

Testing done: pass all unit tests and CASE cert tests

src/app/decoder.cpp

@@ -53,7 +53,7 @@ uint16_t extractApsFrame(uint8_t * buffer, uint16_t buf_length, EmberApsFrame *
                                .ReadOctet(&outApsFrame->radius)
                                .StatusCode();
 
-    return err == CHIP_NO_ERROR ? reader.OctetsRead() : 0;
+    return err == CHIP_NO_ERROR ? static_cast<uint16_t>(reader.OctetsRead()) : 0;
 }
 
 void printApsFrame(EmberApsFrame * frame)

src/app/message-reader.h

@@ -51,7 +51,7 @@ class DataModelReader
      * we do less switching back and forth between DataModelReader and raw
      * buffers.
      */
-    uint16_t OctetsRead() const { return mReader.OctetsRead(); }
+    size_t OctetsRead() const { return mReader.OctetsRead(); }
 
     /**
      * The reader status.

src/app/util/attribute-list-byte-span.cpp

@@ -83,7 +83,7 @@ uint16_t GetByteSpanOffsetFromIndex(const uint8_t * buffer, uint16_t bufferLen,
         reader.Skip(entrySize);
     }
 
-    return reader.OctetsRead();
+    return static_cast<uint16_t>(reader.OctetsRead());
 }
 
 } // namespace List

src/ble/BtpEngine.cpp

@@ -292,7 +292,7 @@ CHIP_ERROR BtpEngine::HandleCharacteristicReceived(System::PacketBufferHandle &&
     data->SetDataLength(chip::min(data->DataLength(), mRxFragmentSize));
 
     // Now mark the bytes we consumed as consumed.
-    data->ConsumeHead(reader.OctetsRead());
+    data->ConsumeHead(static_cast<uint16_t>(reader.OctetsRead()));
 
     ChipLogDebugBtpEngine(Ble, ">>> BTP reassembler received data:");
     PrintBufDebug(data);
@@ -312,7 +312,7 @@ CHIP_ERROR BtpEngine::HandleCharacteristicReceived(System::PacketBufferHandle &&
 
         mRxState = kState_InProgress;
 
-        data->ConsumeHead(startReader.OctetsRead());
+        data->ConsumeHead(static_cast<uint16_t>(startReader.OctetsRead()));
 
         // Create a new buffer for use as the Rx re-assembly area.
         mRxBuf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize);

src/crypto/CHIPCryptoPAL.cpp

@@ -25,7 +25,10 @@
 #include <support/BufferReader.h>
 #include <support/BufferWriter.h>
 #include <support/CodeUtils.h>
+#include <support/Span.h>
 
+using chip::ByteSpan;
+using chip::MutableByteSpan;
 using chip::Encoding::BufferWriter;
 using chip::Encoding::LittleEndian::Reader;
 
@@ -35,6 +38,12 @@ 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;
@@ -64,7 +73,14 @@ CHIP_ERROR ReadDerLength(Reader & reader, uint8_t & length)
     }
 }
 
-CHIP_ERROR ReadDerUnsignedIntegerIntoRaw(Reader & reader, uint8_t * raw_integer_out, size_t raw_integer_length)
+/**
+ * @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.
+ * @param[in] reader Reader instance from which the input will be read
+ * @param[out] raw_integer_out Buffer to receive the DER-encoded integer
+ * @return CHIP_ERROR_INVALID_ARGUMENT or CHIP_ERROR_BUFFER_TOO_SMALL on error, CHIP_NO_ERROR otherwise
+ */
+CHIP_ERROR ReadDerUnsignedIntegerIntoRaw(Reader & reader, MutableByteSpan raw_integer_out)
 {
     uint8_t cur_byte = 0;
 
@@ -78,13 +94,13 @@ CHIP_ERROR ReadDerUnsignedIntegerIntoRaw(Reader & reader, uint8_t * raw_integer_
     ReturnErrorOnFailure(ReadDerLength(reader, integer_len));
 
     // Clear the destination buffer, so we can blit the unsigned value into place
-    memset(raw_integer_out, 0, raw_integer_length);
+    memset(raw_integer_out.data(), 0, raw_integer_out.size());
 
     // Check for pseudo-zero to mark unsigned value
     // This means we have too large an integer (should be at most 1 byte too large), it's invalid
-    ReturnErrorCodeIf(integer_len > (raw_integer_length + 1), CHIP_ERROR_INVALID_ARGUMENT);
+    ReturnErrorCodeIf(integer_len > (raw_integer_out.size() + 1), CHIP_ERROR_INVALID_ARGUMENT);
 
-    if (integer_len == (raw_integer_length + 1u))
+    if (integer_len == (raw_integer_out.size() + 1u))
     {
         // Means we had a 0x00 byte stuffed due to MSB being high in original integer
         ReturnErrorOnFailure(reader.Read8(&cur_byte).StatusCode());
@@ -97,25 +113,31 @@ CHIP_ERROR ReadDerUnsignedIntegerIntoRaw(Reader & reader, uint8_t * raw_integer_
     // We now have the rest of the tag that is a "minimal length" unsigned integer.
     // Blit it at the correct offset, since the order we use is MSB first for
     // both ASN.1 and EC curve raw points.
-    size_t offset = raw_integer_length - integer_len;
-    return reader.ReadBytes(&raw_integer_out[offset], integer_len).StatusCode();
+    size_t offset = raw_integer_out.size() - integer_len;
+    return reader.ReadBytes(raw_integer_out.data() + offset, integer_len).StatusCode();
 }
 
-size_t EmitDerIntegerFromRaw(const uint8_t * raw_integer, size_t raw_integer_length_bytes, uint8_t * out_der_integer,
-                             size_t out_der_integer_size)
+/**
+ * @brief Utility to emit a DER-encoded INTEGER given a raw unsigned large integer
+ *        in big-endian order.
+ * @param[in] raw_integer Bytes of a large unsigned integer in big-endian, possibly including leading zeroes
+ * @param[out] out_der_integer Buffer to receive the DER-encoded integer
+ * @return The non-zero length used by DER encoding in `out_der_ingeger`, or 0 on error, such as value too large to fit output.
+ */
+size_t EmitDerIntegerFromRaw(const ByteSpan & raw_integer, MutableByteSpan out_der_integer)
 {
-    if ((raw_integer == nullptr) || (raw_integer_length_bytes == 0))
+    if ((raw_integer.empty()) || (raw_integer.size() == 0))
     {
         return 0;
     }
 
-    Reader reader(raw_integer, static_cast<uint16_t>(raw_integer_length_bytes));
-    BufferWriter writer(out_der_integer, static_cast<uint16_t>(out_der_integer_size));
+    Reader reader(raw_integer);
+    BufferWriter writer(out_der_integer);
 
     bool needs_leading_zero_byte = false;
 
     uint8_t cur_byte = 0;
-    while ((reader.Remaining() > 0) && (reader.Read8(&cur_byte).StatusCode() == CHIP_NO_ERROR) && (cur_byte == 0))
+    while ((reader.Read8(&cur_byte).StatusCode() == CHIP_NO_ERROR) && (cur_byte == 0))
     {
         // Omit all leading zeros
     }
@@ -127,7 +149,7 @@ size_t EmitDerIntegerFromRaw(const uint8_t * raw_integer, size_t raw_integer_len
         needs_leading_zero_byte = true;
     }
 
-    // The + 1 is to account for the last consummed byte of the loop to skip leading zeros
+    // The + 1 is to account for the last consumed byte of the loop to skip leading zeros
     size_t length = reader.Remaining() + 1 + (needs_leading_zero_byte ? 1 : 0);
 
     if (length > 127)
@@ -142,17 +164,17 @@ size_t EmitDerIntegerFromRaw(const uint8_t * raw_integer, size_t raw_integer_len
     // Put length over 1 byte (i.e. MSB clear)
     writer.Put(static_cast<uint8_t>(length));
 
-    // If leading zero or no more bytes remaining, must ensure we start we at least a zero byte
+    // If leading zero or no more bytes remaining, must ensure we start with at least a zero byte
     if (needs_leading_zero_byte)
     {
         writer.Put(static_cast<uint8_t>(0u));
     }
 
-    // Put first consummed byte from last read iteration of leading zero suppression
+    // Put first consumed byte from last read iteration of leading zero suppression
     writer.Put(cur_byte);
 
     // Fill the rest from the input in order
-    while ((reader.Remaining() > 0) && (reader.Read8(&cur_byte).StatusCode() == CHIP_NO_ERROR))
+    while (reader.Read8(&cur_byte).StatusCode() == CHIP_NO_ERROR)
     {
         // Emit all other bytes as-is
         writer.Put(cur_byte);
@@ -473,31 +495,31 @@ CHIP_ERROR Spake2p_P256_SHA256_HKDF_HMAC::KDF(const uint8_t * ikm, const size_t
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR EcdsaRawSignatureToAsn1(size_t fe_length_bytes, const uint8_t * raw_sig, size_t raw_sig_length, uint8_t * out_asn1_sig,
-                                   size_t out_asn1_sig_length, size_t & out_asn1_sig_actual_length)
+CHIP_ERROR EcdsaRawSignatureToAsn1(size_t fe_length_bytes, const ByteSpan & raw_sig, MutableByteSpan out_asn1_sig,
+                                   size_t & out_asn1_sig_actual_length)
 {
     VerifyOrReturnError(fe_length_bytes > 0, CHIP_ERROR_INVALID_ARGUMENT);
-    VerifyOrReturnError(raw_sig != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
-    VerifyOrReturnError(raw_sig_length == (2u * fe_length_bytes), CHIP_ERROR_INVALID_ARGUMENT);
-    VerifyOrReturnError(out_asn1_sig != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
-    VerifyOrReturnError(out_asn1_sig_length >= (raw_sig_length + kMax_ECDSA_X9Dot62_Asn1_Overhead), CHIP_ERROR_BUFFER_TOO_SMALL);
+    VerifyOrReturnError(raw_sig.data() != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(raw_sig.size() == (2u * fe_length_bytes), CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(out_asn1_sig.data() != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(out_asn1_sig.size() >= (raw_sig.size() + kMax_ECDSA_X9Dot62_Asn1_Overhead), CHIP_ERROR_BUFFER_TOO_SMALL);
 
     // Write both R an S integers past the overhead, we will shift them back later if we only needed 2 size bytes.
-    uint8_t * cursor = &out_asn1_sig[kMinSequenceOverhead];
-    size_t remaining = out_asn1_sig_length - kMinSequenceOverhead;
+    uint8_t * cursor = out_asn1_sig.data() + kMinSequenceOverhead;
+    size_t remaining = out_asn1_sig.size() - kMinSequenceOverhead;
 
     size_t integers_length = 0;
 
     // Write R (first `fe_length_bytes` block of raw signature)
-    size_t integer_length = EmitDerIntegerFromRaw(&raw_sig[0], fe_length_bytes, cursor, remaining);
+    size_t integer_length = EmitDerIntegerFromRaw(raw_sig.SubSpan(0, fe_length_bytes), MutableByteSpan{ cursor, remaining });
     VerifyOrReturnError(integer_length != 0, CHIP_ERROR_INTERNAL);
     VerifyOrReturnError(integer_length < remaining, CHIP_ERROR_INTERNAL);
     remaining -= integer_length;
     integers_length += integer_length;
     cursor += integer_length;
 
     // Write S (second `fe_length_bytes` block of raw signature)
-    integer_length = EmitDerIntegerFromRaw(&raw_sig[fe_length_bytes], fe_length_bytes, cursor, remaining);
+    integer_length = EmitDerIntegerFromRaw(raw_sig.SubSpan(fe_length_bytes, fe_length_bytes), MutableByteSpan{ cursor, remaining });
     VerifyOrReturnError(integer_length != 0, CHIP_ERROR_INTERNAL);
     VerifyOrReturnError(integer_length <= remaining, CHIP_ERROR_INTERNAL);
     integers_length += integer_length;
@@ -507,7 +529,7 @@ CHIP_ERROR EcdsaRawSignatureToAsn1(size_t fe_length_bytes, const uint8_t * raw_s
 
     // We now know the length of both variable sized integers in the sequence, so we
     // can write the tag and length.
-    BufferWriter writer(out_asn1_sig, static_cast<uint16_t>(out_asn1_sig_length));
+    BufferWriter writer(out_asn1_sig);
 
     // Put SEQUENCE tag
     writer.Put(kSeqTag);
@@ -528,7 +550,7 @@ CHIP_ERROR EcdsaRawSignatureToAsn1(size_t fe_length_bytes, const uint8_t * raw_s
     // Put the contents of the integers previously serialized in the buffer.
     // The writer.Put is memmove-safe, so the shifting will happen from the read
     // of the same buffer where the write is taking place.
-    writer.Put(&out_asn1_sig[kMinSequenceOverhead], integers_length);
+    writer.Put(out_asn1_sig.data() + kMinSequenceOverhead, integers_length);
 
     size_t actually_written = 0;
     VerifyOrReturnError(writer.Fit(actually_written), CHIP_ERROR_BUFFER_TOO_SMALL);
@@ -537,19 +559,18 @@ CHIP_ERROR EcdsaRawSignatureToAsn1(size_t fe_length_bytes, const uint8_t * raw_s
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR EcdsaAsn1SignatureToRaw(size_t fe_length_bytes, const uint8_t * asn1_sig, size_t asn1_sig_length, uint8_t * out_raw_sig,
-                                   size_t out_raw_sig_length)
+CHIP_ERROR EcdsaAsn1SignatureToRaw(size_t fe_length_bytes, const ByteSpan & asn1_sig, MutableByteSpan out_raw_sig)
 {
     VerifyOrReturnError(fe_length_bytes > 0, CHIP_ERROR_INVALID_ARGUMENT);
-    VerifyOrReturnError(asn1_sig != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(asn1_sig.data() != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
 
-    VerifyOrReturnError(asn1_sig_length > kMinSequenceOverhead, CHIP_ERROR_BUFFER_TOO_SMALL);
+    VerifyOrReturnError(asn1_sig.size() > kMinSequenceOverhead, CHIP_ERROR_BUFFER_TOO_SMALL);
 
     // Output raw signature is <r,s> both of which are of fe_length_bytes (see SEC1).
-    VerifyOrReturnError(out_raw_sig_length >= (2u * fe_length_bytes), CHIP_ERROR_BUFFER_TOO_SMALL);
-    VerifyOrReturnError(out_raw_sig != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(out_raw_sig.data() != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(out_raw_sig.size() >= (2u * fe_length_bytes), CHIP_ERROR_BUFFER_TOO_SMALL);
 
-    chip::Encoding::LittleEndian::Reader reader(asn1_sig, static_cast<uint16_t>(asn1_sig_length));
+    Reader reader(asn1_sig);
 
     // Make sure we have a starting Sequence
     uint8_t tag = 0;
@@ -564,15 +585,15 @@ CHIP_ERROR EcdsaAsn1SignatureToRaw(size_t fe_length_bytes, const uint8_t * asn1_
     VerifyOrReturnError(tag_len == reader.Remaining(), CHIP_ERROR_INVALID_ARGUMENT);
 
     // Can now clear raw signature integers r,s one by one
-    uint8_t * raw_cursor = out_raw_sig;
+    uint8_t * raw_cursor = out_raw_sig.data();
 
     // Read R
-    ReturnErrorOnFailure(ReadDerUnsignedIntegerIntoRaw(reader, raw_cursor, fe_length_bytes));
+    ReturnErrorOnFailure(ReadDerUnsignedIntegerIntoRaw(reader, MutableByteSpan{ raw_cursor, fe_length_bytes }));
 
     raw_cursor += fe_length_bytes;
 
     // Read S
-    ReturnErrorOnFailure(ReadDerUnsignedIntegerIntoRaw(reader, raw_cursor, fe_length_bytes));
+    ReturnErrorOnFailure(ReadDerUnsignedIntegerIntoRaw(reader, MutableByteSpan{ raw_cursor, fe_length_bytes }));
 
     return CHIP_NO_ERROR;
 }

src/crypto/CHIPCryptoPAL.h

@@ -88,9 +88,9 @@ constexpr size_t kMAX_P256Keypair_Context_Size = 512;
 constexpr size_t kMax_ECDSA_X9Dot62_Asn1_Overhead = 9;
 constexpr size_t kMax_ECDSA_Signature_Length_Der  = kMax_ECDSA_Signature_Length + kMax_ECDSA_X9Dot62_Asn1_Overhead;
 
-nlSTATIC_ASSERT_PRINT(kMax_ECDH_Secret_Length >= kP256_FE_Length, "ECDH shared secret is too short for crypto suite");
-nlSTATIC_ASSERT_PRINT(kMax_ECDSA_Signature_Length >= kP256_ECDSA_Signature_Length_Raw,
-                      "ECDSA signature buffer length is too short for crypto suite");
+static_assert(kMax_ECDH_Secret_Length >= kP256_FE_Length, "ECDH shared secret is too short for crypto suite");
+static_assert(kMax_ECDSA_Signature_Length >= kP256_ECDSA_Signature_Length_Raw,
+              "ECDSA signature buffer length is too short for crypto suite");
 
 /**
  * Spake2+ parameters for P256
@@ -362,15 +362,13 @@ class P256Keypair : public ECPKeypair<P256PublicKey, P256ECDHDerivedSecret, P256
  *
  * @param[in] fe_length_bytes Field Element length in bytes (e.g. 32 for P256 curve)
  * @param[in] raw_sig Raw signature of <r,s> concatenated
- * @param[in] raw_sig_length Raw signature length (MUST be 2*`fe_length_bytes` long)
- * @param[out] out_asn1_sig ASN.1 DER signature format output buffer
- * @param[in] out_asn1_sig_length ASN.1 DER signature format output buffer length. Must have space for at least
+ * @param[out] out_asn1_sig ASN.1 DER signature format output buffer. Size must have space for at least
  * kMax_ECDSA_X9Dot62_Asn1_Overhead.
  * @param[out] out_asn1_sig_actual_length Final computed size of signature.
  * @return Returns a CHIP_ERROR on error, CHIP_NO_ERROR otherwise
  */
-CHIP_ERROR EcdsaRawSignatureToAsn1(size_t fe_length_bytes, const uint8_t * raw_sig, size_t raw_sig_length, uint8_t * out_asn1_sig,
-                                   size_t out_asn1_sig_length, size_t & out_asn1_sig_actual_length);
+CHIP_ERROR EcdsaRawSignatureToAsn1(size_t fe_length_bytes, const ByteSpan & raw_sig, MutableByteSpan out_asn1_sig,
+                                   size_t & out_asn1_sig_actual_length);
 
 /**
  * @brief Convert an ASN.1 DER signature (per X9.62) as used by TLS libraries to SEC1 raw format
@@ -383,14 +381,10 @@ CHIP_ERROR EcdsaRawSignatureToAsn1(size_t fe_length_bytes, const uint8_t * raw_s
  *
  * @param[in] fe_length_bytes Field Element length in bytes (e.g. 32 for P256 curve)
  * @param[in] asn1_sig ASN.1 DER signature input
- * @param[in] asn1_sig_length ASN.1 DER signature length
- * @param[out] out_raw_sig Raw signature of <r,s> concatenated format output buffer
- * @param[in] out_raw_sig_length Raw signature output buffer length. Must be at least >= `2 * fe_length_bytes`
+ * @param[out] out_raw_sig Raw signature of <r,s> concatenated format output buffer. Size must be at least >= `2 * fe_length_bytes`
  * @return Returns a CHIP_ERROR on error, CHIP_NO_ERROR otherwise
  */
-
-CHIP_ERROR EcdsaAsn1SignatureToRaw(size_t fe_length_bytes, const uint8_t * asn1_sig, size_t asn1_sig_length, uint8_t * out_raw_sig,
-                                   size_t out_raw_sig_length);
+CHIP_ERROR EcdsaAsn1SignatureToRaw(size_t fe_length_bytes, const ByteSpan & asn1_sig, MutableByteSpan out_raw_sig);
 
 /**
  * @brief A function that implements AES-CCM encryption

src/crypto/tests/CHIPCryptoPALTest.cpp

@@ -625,15 +625,16 @@ static void TestAsn1Conversions(nlTestSuite * inSuite, void * inContext)
         NL_TEST_ASSERT(inSuite, out_asn1_sig);
 
         // Test converstion from ASN.1 ER to raw
-        CHIP_ERROR status = EcdsaAsn1SignatureToRaw(vector->fe_length_bytes, vector->der_version, vector->der_version_length,
-                                                    out_raw_sig.Get(), out_raw_sig_allocated_size);
+        CHIP_ERROR status =
+            EcdsaAsn1SignatureToRaw(vector->fe_length_bytes, ByteSpan{ vector->der_version, vector->der_version_length },
+                                    MutableByteSpan{ out_raw_sig.Get(), out_raw_sig_allocated_size });
         NL_TEST_ASSERT(inSuite, status == CHIP_NO_ERROR);
         NL_TEST_ASSERT(inSuite, (memcmp(out_raw_sig.Get(), vector->raw_version, vector->raw_version_length) == 0));
 
         // Test conversion from raw to ASN.1 DER
         size_t der_size = 0;
-        status          = EcdsaRawSignatureToAsn1(vector->fe_length_bytes, vector->raw_version, vector->raw_version_length,
-                                         out_asn1_sig.Get(), out_asn1_sig_allocated_size, der_size);
+        status = EcdsaRawSignatureToAsn1(vector->fe_length_bytes, ByteSpan{ vector->raw_version, vector->raw_version_length },
+                                         MutableByteSpan{ out_asn1_sig.Get(), out_asn1_sig_allocated_size }, der_size);
 
         NL_TEST_ASSERT(inSuite, status == CHIP_NO_ERROR);
         NL_TEST_ASSERT(inSuite, der_size <= out_asn1_sig_allocated_size);