Remove duplicate P256Keypair::ECDSA_sign_hash code (#20078)

* Remove duplicate P256Keypair::ECDSA_sign_hash code

- The ECDSA_sign_hash method is a near identical copy of of ECDSA_sign_msg,
  that takes a raw hash.
- This is problematic since some platforms, like Android, cannot directly sign
  a pre-computed hash with OS-aided APIs, and overall this is not consistent
  with signature APIs that work on messages, and where a digest is an internal
  implementation detail.
- Overall, the method adds little value and prevents easy transition to different
  signing algorithms over time if the hash assumption is kept

Fixes #18430

This PR:

- Removes the sign_hash API
- Replaces its usage throughout the SDK
- Updates all tests
- Leaves the ECDSA_verify_hash_signature (since it's only used in one place,
  already in native code, and always against raw public keys)

Testing done:

- Cert tests still pass, including device attestation during commissioning
- Unit tests still pass including updated unit tests

* Restyled by clang-format

* Remove missed removals

* Apply review comments

Co-authored-by: Restyled.io <commits@restyled.io>

examples/platform/nxp/se05x/DeviceAttestationSe05xCredsExample.cpp

@@ -48,7 +48,7 @@ class ExampleSe05xDACProvider : public DeviceAttestationCredentialsProvider
     CHIP_ERROR GetFirmwareInformation(MutableByteSpan & out_firmware_info_buffer) override;
     CHIP_ERROR GetDeviceAttestationCert(MutableByteSpan & out_dac_buffer) override;
     CHIP_ERROR GetProductAttestationIntermediateCert(MutableByteSpan & out_pai_buffer) override;
-    CHIP_ERROR SignWithDeviceAttestationKey(const ByteSpan & digest_to_sign, MutableByteSpan & out_signature_buffer) override;
+    CHIP_ERROR SignWithDeviceAttestationKey(const ByteSpan & message_to_sign, MutableByteSpan & out_signature_buffer) override;
 };
 
 CHIP_ERROR ExampleSe05xDACProvider::GetDeviceAttestationCert(MutableByteSpan & out_dac_buffer)
@@ -130,7 +130,7 @@ CHIP_ERROR ExampleSe05xDACProvider::GetFirmwareInformation(MutableByteSpan & out
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR ExampleSe05xDACProvider::SignWithDeviceAttestationKey(const ByteSpan & digest_to_sign,
+CHIP_ERROR ExampleSe05xDACProvider::SignWithDeviceAttestationKey(const ByteSpan & message_to_sign,
                                                                  MutableByteSpan & out_signature_buffer)
 {
     Crypto::P256ECDSASignature signature;
@@ -139,14 +139,14 @@ CHIP_ERROR ExampleSe05xDACProvider::SignWithDeviceAttestationKey(const ByteSpan
     ChipLogDetail(Crypto, "Sign using DA key from se05x");
 
     VerifyOrReturnError(IsSpanUsable(out_signature_buffer), CHIP_ERROR_INVALID_ARGUMENT);
-    VerifyOrReturnError(IsSpanUsable(digest_to_sign), CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(IsSpanUsable(message_to_sign), CHIP_ERROR_INVALID_ARGUMENT);
     VerifyOrReturnError(out_signature_buffer.size() >= signature.Capacity(), CHIP_ERROR_BUFFER_TOO_SMALL);
 
     keypair.SetKeyId(DEV_ATTESTATION_KEY_ID);
     keypair.provisioned_key = true;
     keypair.Initialize();
 
-    ReturnErrorOnFailure(keypair.ECDSA_sign_hash(digest_to_sign.data(), digest_to_sign.size(), signature));
+    ReturnErrorOnFailure(keypair.ECDSA_sign_msg(message_to_sign.data(), message_to_sign.size(), signature));
 
     return CopySpanToMutableSpan(ByteSpan{ signature.ConstBytes(), signature.Length() }, out_signature_buffer);
 }

examples/tv-app/android/java/JNIDACProvider.cpp

@@ -145,14 +145,14 @@ CHIP_ERROR LoadKeypairFromRaw(ByteSpan private_key, ByteSpan public_key, Crypto:
     return keypair.Deserialize(serialized_keypair);
 }
 
-CHIP_ERROR JNIDACProvider::SignWithDeviceAttestationKey(const ByteSpan & digest_to_sign, MutableByteSpan & out_signature_buffer)
+CHIP_ERROR JNIDACProvider::SignWithDeviceAttestationKey(const ByteSpan & message_to_sign, MutableByteSpan & out_signature_buffer)
 {
     ChipLogProgress(Zcl, "Received SignWithDeviceAttestationKey");
     Crypto::P256ECDSASignature signature;
     Crypto::P256Keypair keypair;
 
     VerifyOrReturnError(IsSpanUsable(out_signature_buffer), CHIP_ERROR_INVALID_ARGUMENT);
-    VerifyOrReturnError(IsSpanUsable(digest_to_sign), CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(IsSpanUsable(message_to_sign), CHIP_ERROR_INVALID_ARGUMENT);
     VerifyOrReturnError(out_signature_buffer.size() >= signature.Capacity(), CHIP_ERROR_BUFFER_TOO_SMALL);
 
     uint8_t privateKeyBuf[Crypto::kP256_PrivateKey_Length];
@@ -166,7 +166,7 @@ CHIP_ERROR JNIDACProvider::SignWithDeviceAttestationKey(const ByteSpan & digest_
     // In a non-exemplary implementation, the public key is not needed here. It is used here merely because
     // Crypto::P256Keypair is only (currently) constructable from raw keys if both private/public keys are present.
     ReturnErrorOnFailure(LoadKeypairFromRaw(privateKeyBufSpan, publicKeyBufSpan, keypair));
-    ReturnErrorOnFailure(keypair.ECDSA_sign_hash(digest_to_sign.data(), digest_to_sign.size(), signature));
+    ReturnErrorOnFailure(keypair.ECDSA_sign_msg(message_to_sign.data(), message_to_sign.size(), signature));
 
     return CopySpanToMutableSpan(ByteSpan{ signature.ConstBytes(), signature.Length() }, out_signature_buffer);
 }

examples/tv-app/android/java/JNIDACProvider.h

@@ -28,7 +28,7 @@ class JNIDACProvider : public chip::Credentials::DeviceAttestationCredentialsPro
     CHIP_ERROR GetFirmwareInformation(chip::MutableByteSpan & out_firmware_info_buffer) override;
     CHIP_ERROR GetDeviceAttestationCert(chip::MutableByteSpan & out_dac_buffer) override;
     CHIP_ERROR GetProductAttestationIntermediateCert(chip::MutableByteSpan & out_pai_buffer) override;
-    CHIP_ERROR SignWithDeviceAttestationKey(const chip::ByteSpan & digest_to_sign,
+    CHIP_ERROR SignWithDeviceAttestationKey(const chip::ByteSpan & message_to_sign,
                                             chip::MutableByteSpan & out_signature_buffer) override;
 
 private:

examples/tv-casting-app/android/App/app/src/main/jni/cpp/JNIDACProvider.cpp

@@ -145,14 +145,14 @@ CHIP_ERROR LoadKeypairFromRaw(ByteSpan private_key, ByteSpan public_key, Crypto:
     return keypair.Deserialize(serialized_keypair);
 }
 
-CHIP_ERROR JNIDACProvider::SignWithDeviceAttestationKey(const ByteSpan & digest_to_sign, MutableByteSpan & out_signature_buffer)
+CHIP_ERROR JNIDACProvider::SignWithDeviceAttestationKey(const ByteSpan & message_to_sign, MutableByteSpan & out_signature_buffer)
 {
     ChipLogProgress(Zcl, "Received SignWithDeviceAttestationKey");
     Crypto::P256ECDSASignature signature;
     Crypto::P256Keypair keypair;
 
     VerifyOrReturnError(IsSpanUsable(out_signature_buffer), CHIP_ERROR_INVALID_ARGUMENT);
-    VerifyOrReturnError(IsSpanUsable(digest_to_sign), CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(IsSpanUsable(message_to_sign), CHIP_ERROR_INVALID_ARGUMENT);
     VerifyOrReturnError(out_signature_buffer.size() >= signature.Capacity(), CHIP_ERROR_BUFFER_TOO_SMALL);
 
     uint8_t privateKeyBuf[Crypto::kP256_PrivateKey_Length];
@@ -166,7 +166,7 @@ CHIP_ERROR JNIDACProvider::SignWithDeviceAttestationKey(const ByteSpan & digest_
     // In a non-exemplary implementation, the public key is not needed here. It is used here merely because
     // Crypto::P256Keypair is only (currently) constructable from raw keys if both private/public keys are present.
     ReturnErrorOnFailure(LoadKeypairFromRaw(privateKeyBufSpan, publicKeyBufSpan, keypair));
-    ReturnErrorOnFailure(keypair.ECDSA_sign_hash(digest_to_sign.data(), digest_to_sign.size(), signature));
+    ReturnErrorOnFailure(keypair.ECDSA_sign_msg(message_to_sign.data(), message_to_sign.size(), signature));
 
     return CopySpanToMutableSpan(ByteSpan{ signature.ConstBytes(), signature.Length() }, out_signature_buffer);
 }

examples/tv-casting-app/android/App/app/src/main/jni/cpp/JNIDACProvider.h

@@ -28,7 +28,7 @@ class JNIDACProvider : public chip::Credentials::DeviceAttestationCredentialsPro
     CHIP_ERROR GetFirmwareInformation(chip::MutableByteSpan & out_firmware_info_buffer) override;
     CHIP_ERROR GetDeviceAttestationCert(chip::MutableByteSpan & out_dac_buffer) override;
     CHIP_ERROR GetProductAttestationIntermediateCert(chip::MutableByteSpan & out_pai_buffer) override;
-    CHIP_ERROR SignWithDeviceAttestationKey(const chip::ByteSpan & digest_to_sign,
+    CHIP_ERROR SignWithDeviceAttestationKey(const chip::ByteSpan & message_to_sign,
                                             chip::MutableByteSpan & out_signature_buffer) override;
 
 private:

src/app/clusters/operational-credentials-server/operational-credentials-server.cpp

@@ -247,33 +247,6 @@ CHIP_ERROR OperationalCredentialsAttrAccess::Read(const ConcreteReadAttributePat
     return CHIP_NO_ERROR;
 }
 
-// Utility to compute Attestation signature for NOCSRResponse and AttestationResponse
-CHIP_ERROR ComputeAttestationSignature(app::CommandHandler * commandObj,
-                                       Credentials::DeviceAttestationCredentialsProvider * dacProvider, const ByteSpan & payload,
-                                       MutableByteSpan & signatureSpan)
-{
-    uint8_t md[Crypto::kSHA256_Hash_Length];
-    MutableByteSpan messageDigestSpan(md);
-
-    VerifyOrReturnError(signatureSpan.size() >= Crypto::P256ECDSASignature::Capacity(), CHIP_ERROR_INVALID_ARGUMENT);
-
-    // TODO: Create an alternative way to retrieve the Attestation Challenge without this huge amount of calls.
-    // Retrieve attestation challenge
-    ByteSpan attestationChallenge =
-        commandObj->GetExchangeContext()->GetSessionHandle()->AsSecureSession()->GetCryptoContext().GetAttestationChallenge();
-
-    Hash_SHA256_stream hashStream;
-    ReturnErrorOnFailure(hashStream.Begin());
-    ReturnErrorOnFailure(hashStream.AddData(payload));
-    ReturnErrorOnFailure(hashStream.AddData(attestationChallenge));
-    ReturnErrorOnFailure(hashStream.Finish(messageDigestSpan));
-
-    ReturnErrorOnFailure(dacProvider->SignWithDeviceAttestationKey(messageDigestSpan, signatureSpan));
-    VerifyOrReturnError(signatureSpan.size() == Crypto::P256ECDSASignature::Capacity(), CHIP_ERROR_INTERNAL);
-
-    return CHIP_NO_ERROR;
-}
-
 FabricInfo * RetrieveCurrentFabric(CommandHandler * aCommandHandler)
 {
     FabricIndex index = aCommandHandler->GetAccessingFabricIndex();
@@ -930,6 +903,7 @@ bool emberAfOperationalCredentialsClusterAttestationRequestCallback(app::Command
 
     auto finalStatus = Status::Failure;
     CHIP_ERROR err   = CHIP_ERROR_INVALID_ARGUMENT;
+    ByteSpan tbsSpan;
 
     Platform::ScopedMemoryBuffer<uint8_t> attestationElements;
     size_t attestationElementsLen = 0;
@@ -938,6 +912,11 @@ bool emberAfOperationalCredentialsClusterAttestationRequestCallback(app::Command
                                                               // See DeviceAttestationCredsExample
     MutableByteSpan certDeclSpan(certDeclBuf);
 
+    // TODO: Create an alternative way to retrieve the Attestation Challenge without this huge amount of calls.
+    // Retrieve attestation challenge
+    ByteSpan attestationChallenge =
+        commandObj->GetExchangeContext()->GetSessionHandle()->AsSecureSession()->GetCryptoContext().GetAttestationChallenge();
+
     // TODO: in future versions, retrieve vendor information to populate the fields below.
     uint32_t timestamp = 0;
     Credentials::DeviceAttestationVendorReservedConstructor emptyVendorReserved(nullptr, 0);
@@ -961,7 +940,7 @@ bool emberAfOperationalCredentialsClusterAttestationRequestCallback(app::Command
 
     attestationElementsLen = TLV::EstimateStructOverhead(certDeclSpan.size(), attestationNonce.size(), sizeof(uint64_t) * 8);
 
-    if (!attestationElements.Alloc(attestationElementsLen))
+    if (!attestationElements.Alloc(attestationElementsLen + attestationChallenge.size()))
     {
         err = CHIP_ERROR_NO_MEMORY;
         VerifyOrExit(err == CHIP_NO_ERROR, finalStatus = Status::ResourceExhausted);
@@ -972,14 +951,21 @@ bool emberAfOperationalCredentialsClusterAttestationRequestCallback(app::Command
                                                     emptyVendorReserved, attestationElementsSpan);
     VerifyOrExit((err == CHIP_NO_ERROR) && (attestationElementsSpan.size() <= kMaxRspLen), finalStatus = Status::Failure);
 
-    // Prepare response payload with signature
-    {
-        Commands::AttestationResponse::Type response;
+    // Append attestation challenge in the back of the reserved space for the signature
+    memcpy(attestationElements.Get() + attestationElementsSpan.size(), attestationChallenge.data(), attestationChallenge.size());
+    tbsSpan = ByteSpan{ attestationElements.Get(), attestationElementsSpan.size() + attestationChallenge.size() };
 
+    {
         Crypto::P256ECDSASignature signature;
         MutableByteSpan signatureSpan{ signature.Bytes(), signature.Capacity() };
-        err = ComputeAttestationSignature(commandObj, dacProvider, attestationElementsSpan, signatureSpan);
+
+        // Getnerate attestation signature
+        err = dacProvider->SignWithDeviceAttestationKey(tbsSpan, signatureSpan);
+        ClearSecretData(attestationElements.Get() + attestationElementsSpan.size(), attestationChallenge.size());
         VerifyOrExit(err == CHIP_NO_ERROR, finalStatus = Status::Failure);
+        VerifyOrExit(signatureSpan.size() == Crypto::P256ECDSASignature::Capacity(), finalStatus = Status::Failure);
+
+        Commands::AttestationResponse::Type response;
 
         response.attestationElements = attestationElementsSpan;
         response.signature           = signatureSpan;
@@ -1010,6 +996,7 @@ bool emberAfOperationalCredentialsClusterCSRRequestCallback(app::CommandHandler
     chip::Platform::ScopedMemoryBuffer<uint8_t> nocsrElements;
     MutableByteSpan nocsrElementsSpan;
     auto finalStatus = Status::Failure;
+    ByteSpan tbsSpan;
 
     // Start with CHIP_ERROR_INVALID_ARGUMENT so that cascading errors yield correct
     // logs by the end. We use finalStatus as our overall success marker, not error
@@ -1021,6 +1008,11 @@ bool emberAfOperationalCredentialsClusterCSRRequestCallback(app::CommandHandler
     auto & CSRNonce     = commandData.CSRNonce;
     bool isForUpdateNoc = commandData.isForUpdateNOC.ValueOr(false);
 
+    // TODO: Create an alternative way to retrieve the Attestation Challenge without this huge amount of calls.
+    // Retrieve attestation challenge
+    ByteSpan attestationChallenge =
+        commandObj->GetExchangeContext()->GetSessionHandle()->AsSecureSession()->GetCryptoContext().GetAttestationChallenge();
+
     failSafeContext.SetCsrRequestForUpdateNoc(isForUpdateNoc);
     FabricInfo * fabricInfo = RetrieveCurrentFabric(commandObj);
 
@@ -1075,7 +1067,7 @@ bool emberAfOperationalCredentialsClusterCSRRequestCallback(app::CommandHandler
                                                           0u               // no vendor reserved data
         );
 
-        if (!nocsrElements.Alloc(nocsrLengthEstimate))
+        if (!nocsrElements.Alloc(nocsrLengthEstimate + attestationChallenge.size()))
         {
             err = CHIP_ERROR_NO_MEMORY;
             VerifyOrExit(err == CHIP_NO_ERROR, finalStatus = Status::ResourceExhausted);
@@ -1086,26 +1078,32 @@ bool emberAfOperationalCredentialsClusterCSRRequestCallback(app::CommandHandler
         err = Credentials::ConstructNOCSRElements(ByteSpan{ csrSpan.data(), csrSpan.size() }, CSRNonce, kNoVendorReserved,
                                                   kNoVendorReserved, kNoVendorReserved, nocsrElementsSpan);
         VerifyOrExit((err == CHIP_NO_ERROR) && (nocsrElementsSpan.size() <= kMaxRspLen), finalStatus = Status::Failure);
-    }
 
-    // Prepare response payload with signature
-    {
-        Commands::CSRResponse::Type response;
-
-        Credentials::DeviceAttestationCredentialsProvider * dacProvider = Credentials::GetDeviceAttestationCredentialsProvider();
+        // Append attestation challenge in the back of the reserved space for the signature
+        memcpy(nocsrElements.Get() + nocsrElementsSpan.size(), attestationChallenge.data(), attestationChallenge.size());
+        tbsSpan = ByteSpan{ nocsrElements.Get(), nocsrElementsSpan.size() + attestationChallenge.size() };
 
-        Crypto::P256ECDSASignature signature;
-        MutableByteSpan signatureSpan{ signature.Bytes(), signature.Capacity() };
+        {
+            Credentials::DeviceAttestationCredentialsProvider * dacProvider =
+                Credentials::GetDeviceAttestationCredentialsProvider();
+            Crypto::P256ECDSASignature signature;
+            MutableByteSpan signatureSpan{ signature.Bytes(), signature.Capacity() };
+
+            // Getnerate attestation signature
+            err = dacProvider->SignWithDeviceAttestationKey(tbsSpan, signatureSpan);
+            ClearSecretData(nocsrElements.Get() + nocsrElementsSpan.size(), attestationChallenge.size());
+            VerifyOrExit(err == CHIP_NO_ERROR, finalStatus = Status::Failure);
+            VerifyOrExit(signatureSpan.size() == Crypto::P256ECDSASignature::Capacity(), finalStatus = Status::Failure);
 
-        err = ComputeAttestationSignature(commandObj, dacProvider, nocsrElementsSpan, signatureSpan);
-        VerifyOrExit(err == CHIP_NO_ERROR, finalStatus = Status::Failure);
+            Commands::CSRResponse::Type response;
 
-        response.NOCSRElements        = nocsrElementsSpan;
-        response.attestationSignature = signatureSpan;
+            response.NOCSRElements        = nocsrElementsSpan;
+            response.attestationSignature = signatureSpan;
 
-        ChipLogProgress(Zcl, "OpCreds: CSRRequest successful.");
-        finalStatus = Status::Success;
-        commandObj->AddResponse(commandPath, response);
+            ChipLogProgress(Zcl, "OpCreds: CSRRequest successful.");
+            finalStatus = Status::Success;
+            commandObj->AddResponse(commandPath, response);
+        }
     }
 
 exit:

src/app/tests/suites/credentials/TestHarnessDACProvider.cpp

@@ -209,20 +209,20 @@ CHIP_ERROR TestHarnessDACProvider::GetFirmwareInformation(MutableByteSpan & out_
     return CopySpanToMutableSpan(mFirmwareInformation, out_firmware_info_buffer);
 }
 
-CHIP_ERROR TestHarnessDACProvider::SignWithDeviceAttestationKey(const ByteSpan & digest_to_sign,
+CHIP_ERROR TestHarnessDACProvider::SignWithDeviceAttestationKey(const ByteSpan & message_to_sign,
                                                                 MutableByteSpan & out_signature_buffer)
 {
     Crypto::P256ECDSASignature signature;
     Crypto::P256Keypair keypair;
 
     VerifyOrReturnError(IsSpanUsable(out_signature_buffer), CHIP_ERROR_INVALID_ARGUMENT);
-    VerifyOrReturnError(IsSpanUsable(digest_to_sign), CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(IsSpanUsable(message_to_sign), CHIP_ERROR_INVALID_ARGUMENT);
     VerifyOrReturnError(out_signature_buffer.size() >= signature.Capacity(), CHIP_ERROR_BUFFER_TOO_SMALL);
 
     // In a non-exemplary implementation, the public key is not needed here. It is used here merely because
     // Crypto::P256Keypair is only (currently) constructable from raw keys if both private/public keys are present.
     ReturnErrorOnFailure(LoadKeypairFromRaw(mDacPrivateKey, mDacPublicKey, keypair));
-    ReturnErrorOnFailure(keypair.ECDSA_sign_hash(digest_to_sign.data(), digest_to_sign.size(), signature));
+    ReturnErrorOnFailure(keypair.ECDSA_sign_msg(message_to_sign.data(), message_to_sign.size(), signature));
 
     return CopySpanToMutableSpan(ByteSpan{ signature.ConstBytes(), signature.Length() }, out_signature_buffer);
 }

src/app/tests/suites/credentials/TestHarnessDACProvider.h

@@ -42,7 +42,7 @@ class TestHarnessDACProvider : public DeviceAttestationCredentialsProvider
     CHIP_ERROR GetFirmwareInformation(MutableByteSpan & out_firmware_info_buffer) override;
     CHIP_ERROR GetDeviceAttestationCert(MutableByteSpan & out_dac_buffer) override;
     CHIP_ERROR GetProductAttestationIntermediateCert(MutableByteSpan & out_pai_buffer) override;
-    CHIP_ERROR SignWithDeviceAttestationKey(const ByteSpan & digest_to_sign, MutableByteSpan & out_signature_buffer) override;
+    CHIP_ERROR SignWithDeviceAttestationKey(const ByteSpan & message_to_sign, MutableByteSpan & out_signature_buffer) override;
 
     void Init(const char * filepath);
     void Init(const TestHarnessDACProviderData & data);

src/credentials/DeviceAttestationCredsProvider.cpp

@@ -50,9 +50,9 @@ class UnimplementedDACProvider : public DeviceAttestationCredentialsProvider
         return CHIP_ERROR_NOT_IMPLEMENTED;
     }
 
-    CHIP_ERROR SignWithDeviceAttestationKey(const ByteSpan & digest_to_sign, MutableByteSpan & out_signature_buffer) override
+    CHIP_ERROR SignWithDeviceAttestationKey(const ByteSpan & message_to_sign, MutableByteSpan & out_signature_buffer) override
     {
-        (void) digest_to_sign;
+        (void) message_to_sign;
         (void) out_signature_buffer;
         return CHIP_ERROR_NOT_IMPLEMENTED;
     }