project-chip · mergify · Oct 27, 2023 · Oct 26, 2023 · Oct 26, 2023 · Oct 26, 2023
diff --git a/src/credentials/CHIPCert.cpp b/src/credentials/CHIPCert.cpp
@@ -54,9 +54,6 @@ using namespace chip::TLV;
 using namespace chip::Protocols;
 using namespace chip::Crypto;
 
-extern CHIP_ERROR DecodeConvertTBSCert(TLVReader & reader, ASN1Writer & writer, ChipCertificateData & certData);
-extern CHIP_ERROR DecodeECDSASignature(TLVReader & reader, ChipCertificateData & certData);
-
 ChipCertificateSet::ChipCertificateSet()
 {
     mCerts               = nullptr;
@@ -137,76 +134,21 @@ CHIP_ERROR ChipCertificateSet::LoadCert(const ByteSpan chipCert, BitFlags<CertDe
     TLVReader reader;
 
     reader.Init(chipCert);
-
-    ReturnErrorOnFailure(reader.Next(kTLVType_Structure, AnonymousTag()));
-
     return LoadCert(reader, decodeFlags, chipCert);
 }
 
 CHIP_ERROR ChipCertificateSet::LoadCert(TLVReader & reader, BitFlags<CertDecodeFlags> decodeFlags, ByteSpan chipCert)
 {
-    ASN1Writer writer; // ASN1Writer is used to encode TBS portion of the certificate for the purpose of signature
-                       // validation, which should be performed on the TBS data encoded in ASN.1 DER form.
     ChipCertificateData cert;
-    cert.Clear();
-
-    // Must be positioned on the structure element representing the certificate.
-    VerifyOrReturnError(reader.GetType() == kTLVType_Structure, CHIP_ERROR_INVALID_ARGUMENT);
-
-    cert.mCertificate = chipCert;
-
-    {
-        TLVType containerType;
+    ReturnErrorOnFailure(DecodeChipCert(reader, cert, decodeFlags));
 
-        // Enter the certificate structure...
-        ReturnErrorOnFailure(reader.EnterContainer(containerType));
+    // Verify the cert has both the Subject Key Id and Authority Key Id extensions present.
+    // Only certs with both these extensions are supported for the purposes of certificate validation.
+    VerifyOrReturnError(cert.mCertFlags.HasAll(CertFlags::kExtPresent_SubjectKeyId, CertFlags::kExtPresent_AuthKeyId),
+                        CHIP_ERROR_UNSUPPORTED_CERT_FORMAT);
 
-        // If requested to generate the TBSHash.
-        if (decodeFlags.Has(CertDecodeFlags::kGenerateTBSHash))
-        {
-            chip::Platform::ScopedMemoryBuffer<uint8_t> asn1TBSBuf;
-            ReturnErrorCodeIf(!asn1TBSBuf.Alloc(kMaxCHIPCertDecodeBufLength), CHIP_ERROR_NO_MEMORY);
-
-            // Initialize an ASN1Writer and convert the TBS (to-be-signed) portion of the certificate to ASN.1 DER
-            // encoding. At the same time, parse various components within the certificate and set the corresponding
-            // fields in the CertificateData object.
-            writer.Init(asn1TBSBuf.Get(), kMaxCHIPCertDecodeBufLength);
-            ReturnErrorOnFailure(DecodeConvertTBSCert(reader, writer, cert));
-
-            // Generate a SHA hash of the encoded TBS certificate.
-            chip::Crypto::Hash_SHA256(asn1TBSBuf.Get(), writer.GetLengthWritten(), cert.mTBSHash);
-
-            cert.mCertFlags.Set(CertFlags::kTBSHashPresent);
-        }
-        else
-        {
-            // Initialize an ASN1Writer as a NullWriter.
-            writer.InitNullWriter();
-            ReturnErrorOnFailure(DecodeConvertTBSCert(reader, writer, cert));
-        }
-
-        // Verify the cert has both the Subject Key Id and Authority Key Id extensions present.
-        // Only certs with both these extensions are supported for the purposes of certificate validation.
-        VerifyOrReturnError(cert.mCertFlags.HasAll(CertFlags::kExtPresent_SubjectKeyId, CertFlags::kExtPresent_AuthKeyId),
-                            CHIP_ERROR_UNSUPPORTED_CERT_FORMAT);
-
-        // Verify the cert was signed with ECDSA-SHA256. This is the only signature algorithm currently supported.
-        VerifyOrReturnError(cert.mSigAlgoOID == kOID_SigAlgo_ECDSAWithSHA256, CHIP_ERROR_UNSUPPORTED_SIGNATURE_TYPE);
-
-        // Decode the certificate's signature...
-        ReturnErrorOnFailure(DecodeECDSASignature(reader, cert));
-
-        // Verify no more elements in the certificate.
-        ReturnErrorOnFailure(reader.VerifyEndOfContainer());
-
-        ReturnErrorOnFailure(reader.ExitContainer(containerType));
-    }
-
-    // If requested by the caller, mark the certificate as trusted.
-    if (decodeFlags.Has(CertDecodeFlags::kIsTrustAnchor))
-    {
-        cert.mCertFlags.Set(CertFlags::kIsTrustAnchor);
-    }
+    // Verify the cert was signed with ECDSA-SHA256. This is the only signature algorithm currently supported.
+    VerifyOrReturnError(cert.mSigAlgoOID == kOID_SigAlgo_ECDSAWithSHA256, CHIP_ERROR_UNSUPPORTED_SIGNATURE_TYPE);
 
     // Check if this cert matches any currently loaded certificates
     for (uint32_t i = 0; i < mCertCount; i++)

diff --git a/src/credentials/CHIPCert.h b/src/credentials/CHIPCert.h
@@ -458,7 +458,7 @@ struct ChipCertificateData
  *
  * @return Returns a CHIP_ERROR on error, CHIP_NO_ERROR otherwise
  **/
-CHIP_ERROR DecodeChipCert(const ByteSpan chipCert, ChipCertificateData & certData);
+CHIP_ERROR DecodeChipCert(const ByteSpan chipCert, ChipCertificateData & certData, BitFlags<CertDecodeFlags> decodeFlags = {});
 
 /**
  * @brief Decode CHIP certificate.
@@ -470,7 +470,8 @@ CHIP_ERROR DecodeChipCert(const ByteSpan chipCert, ChipCertificateData & certDat
  *
  * @return Returns a CHIP_ERROR on error, CHIP_NO_ERROR otherwise
  **/
-CHIP_ERROR DecodeChipCert(chip::TLV::TLVReader & reader, ChipCertificateData & certData);
+CHIP_ERROR DecodeChipCert(chip::TLV::TLVReader & reader, ChipCertificateData & certData,
+                          BitFlags<CertDecodeFlags> decodeFlags = {});
 
 /**
  * @brief Decode CHIP Distinguished Name (DN).

diff --git a/src/credentials/CHIPCertToX509.cpp b/src/credentials/CHIPCertToX509.cpp
@@ -454,7 +454,7 @@ static CHIP_ERROR DecodeConvertExtensions(TLVReader & reader, ASN1Writer & write
     return err;
 }
 
-CHIP_ERROR DecodeECDSASignature(TLVReader & reader, ChipCertificateData & certData)
+static CHIP_ERROR DecodeECDSASignature(TLVReader & reader, ChipCertificateData & certData)
 {
     ReturnErrorOnFailure(reader.Next(kTLVType_ByteString, ContextTag(kTag_ECDSASignature)));
     ReturnErrorOnFailure(reader.Get(certData.mSignature));
@@ -467,6 +467,9 @@ static CHIP_ERROR DecodeConvertECDSASignature(TLVReader & reader, ASN1Writer & w
 
     ReturnErrorOnFailure(DecodeECDSASignature(reader, certData));
 
+    // Converting the signature is a bit of work, so explicitly check if we have a null writer
+    ReturnErrorCodeIf(writer.IsNullWriter(), CHIP_NO_ERROR);
+
     // signatureValue BIT STRING
     // Per RFC3279, the ECDSA signature value is encoded in DER encapsulated in the signatureValue BIT STRING.
     ASN1_START_BIT_STRING_ENCAPSULATED
@@ -492,7 +495,7 @@ static CHIP_ERROR DecodeConvertECDSASignature(TLVReader & reader, ASN1Writer & w
  *
  * @return Returns a CHIP_ERROR on error, CHIP_NO_ERROR otherwise
  **/
-CHIP_ERROR DecodeConvertTBSCert(TLVReader & reader, ASN1Writer & writer, ChipCertificateData & certData)
+static CHIP_ERROR DecodeConvertTBSCert(TLVReader & reader, ASN1Writer & writer, ChipCertificateData & certData)
 {
     CHIP_ERROR err;
 
@@ -550,7 +553,18 @@ CHIP_ERROR DecodeConvertTBSCert(TLVReader & reader, ASN1Writer & writer, ChipCer
     return err;
 }
 
-static CHIP_ERROR DecodeConvertCert(TLVReader & reader, ASN1Writer & writer, ChipCertificateData & certData)
+/**
+ * Decode a CHIP TLV certificate and convert it to X.509 DER form.
+ *
+ * This helper function takes separate ASN1Writers for the whole Certificate
+ * and the TBSCertificate, to allow the caller to control which part (if any)
+ * to capture.
+ *
+ * If `writer` is NOT a null writer, then `tbsWriter` MUST be a reference
+ * to the same writer, otherwise the overall Certificate written will not be
+ * valid.
+ */
+static CHIP_ERROR DecodeConvertCert(TLVReader & reader, ASN1Writer & writer, ASN1Writer & tbsWriter, ChipCertificateData & certData)
 {
     CHIP_ERROR err;
     TLVType containerType;
@@ -568,7 +582,7 @@ static CHIP_ERROR DecodeConvertCert(TLVReader & reader, ASN1Writer & writer, Chi
     ASN1_START_SEQUENCE
     {
         // tbsCertificate TBSCertificate,
-        ReturnErrorOnFailure(DecodeConvertTBSCert(reader, writer, certData));
+        ReturnErrorOnFailure(DecodeConvertTBSCert(reader, tbsWriter, certData));
 
         // signatureAlgorithm   AlgorithmIdentifier
         // AlgorithmIdentifier ::= SEQUENCE
@@ -603,31 +617,56 @@ DLL_EXPORT CHIP_ERROR ConvertChipCertToX509Cert(const ByteSpan chipCert, Mutable
 
     certData.Clear();
 
-    ReturnErrorOnFailure(DecodeConvertCert(reader, writer, certData));
+    ReturnErrorOnFailure(DecodeConvertCert(reader, writer, writer, certData));
 
     x509Cert.reduce_size(writer.GetLengthWritten());
 
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR DecodeChipCert(const ByteSpan chipCert, ChipCertificateData & certData)
+CHIP_ERROR DecodeChipCert(const ByteSpan chipCert, ChipCertificateData & certData, BitFlags<CertDecodeFlags> decodeFlags)
 {
     TLVReader reader;
 
     reader.Init(chipCert);
-
-    return DecodeChipCert(reader, certData);
+    return DecodeChipCert(reader, certData, decodeFlags);
 }
 
-CHIP_ERROR DecodeChipCert(TLVReader & reader, ChipCertificateData & certData)
+CHIP_ERROR DecodeChipCert(TLVReader & reader, ChipCertificateData & certData, BitFlags<CertDecodeFlags> decodeFlags)
 {
-    ASN1Writer writer;
-
-    writer.InitNullWriter();
+    ASN1Writer nullWriter;
+    nullWriter.InitNullWriter();
 
     certData.Clear();
 
-    return DecodeConvertCert(reader, writer, certData);
+    if (decodeFlags.Has(CertDecodeFlags::kGenerateTBSHash))
+    {
+        // Create a buffer and writer to capture the TBS (to-be-signed) portion of the certificate
+        // when we decode (and convert) the certificate, so we can hash it to create the TBSHash.
+        chip::Platform::ScopedMemoryBuffer<uint8_t> asn1TBSBuf;
+        VerifyOrReturnError(asn1TBSBuf.Alloc(kMaxCHIPCertDecodeBufLength), CHIP_ERROR_NO_MEMORY);
+        ASN1Writer tbsWriter;
+        tbsWriter.Init(asn1TBSBuf.Get(), kMaxCHIPCertDecodeBufLength);
+
+        ReturnErrorOnFailure(DecodeConvertCert(reader, nullWriter, tbsWriter, certData));
+
+        // Hash the encoded TBS certificate. Only SHA256 is supported.
+        VerifyOrReturnError(certData.mSigAlgoOID == kOID_SigAlgo_ECDSAWithSHA256, CHIP_ERROR_UNSUPPORTED_SIGNATURE_TYPE);
+        chip::Crypto::Hash_SHA256(asn1TBSBuf.Get(), tbsWriter.GetLengthWritten(), certData.mTBSHash);
+        certData.mCertFlags.Set(CertFlags::kTBSHashPresent);
+    }
+    else
+    {
+        ReturnErrorOnFailure(DecodeConvertCert(reader, nullWriter, nullWriter, certData));
+    }
+
+    // If requested by the caller, mark the certificate as trusted.
+    if (decodeFlags.Has(CertDecodeFlags::kIsTrustAnchor))
+    {
+        certData.mCertFlags.Set(CertFlags::kIsTrustAnchor);
+    }
+
+    return CHIP_NO_ERROR;
 }
 
 CHIP_ERROR DecodeChipDN(TLVReader & reader, ChipDN & dn)

diff --git a/src/credentials/tests/TestChipCert.cpp b/src/credentials/tests/TestChipCert.cpp
@@ -1275,6 +1275,34 @@ static void TestChipCert_CertId(nlTestSuite * inSuite, void * inContext)
     }
 }
 
+static void TestChipCert_DecodingOptions(nlTestSuite * inSuite, void * inContext)
+{
+    CHIP_ERROR err;
+    ByteSpan cert;
+    ChipCertificateData certData;
+
+    err = GetTestCert(TestCert::kRoot01, sNullLoadFlag, cert);
+    NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
+
+    // Decode with default (null) options
+    err = DecodeChipCert(cert, certData);
+    NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
+    NL_TEST_ASSERT(inSuite, !certData.mCertFlags.Has(CertFlags::kIsTrustAnchor));
+
+    // Decode as trust anchor
+    err = DecodeChipCert(cert, certData, CertDecodeFlags::kIsTrustAnchor);
+    NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
+    NL_TEST_ASSERT(inSuite, certData.mCertFlags.Has(CertFlags::kIsTrustAnchor));
+
+    // Decode with TBS Hash calculation
+    err = DecodeChipCert(cert, certData, CertDecodeFlags::kGenerateTBSHash);
+    NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
+    NL_TEST_ASSERT(inSuite, certData.mCertFlags.Has(CertFlags::kTBSHashPresent));
+    // When the TBS hash is available signature verification should work
+    err = VerifyCertSignature(certData, certData); // test cert is a self-signed root
+    NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
+}
+
 static void TestChipCert_LoadDuplicateCerts(nlTestSuite * inSuite, void * inContext)
 {
     CHIP_ERROR err;
@@ -2147,6 +2175,7 @@ static const nlTest sTests[] = {
     NL_TEST_DEF("Test CHIP Certificate Usage", TestChipCert_CertUsage),
     NL_TEST_DEF("Test CHIP Certificate Type", TestChipCert_CertType),
     NL_TEST_DEF("Test CHIP Certificate ID", TestChipCert_CertId),
+    NL_TEST_DEF("Test CHIP Certificate Decoding Options", TestChipCert_DecodingOptions),
     NL_TEST_DEF("Test Loading Duplicate Certificates", TestChipCert_LoadDuplicateCerts),
     NL_TEST_DEF("Test CHIP Generate Root Certificate", TestChipCert_GenerateRootCert),
     NL_TEST_DEF("Test CHIP Generate Root Certificate with Fabric", TestChipCert_GenerateRootFabCert),

diff --git a/src/lib/asn1/ASN1.h b/src/lib/asn1/ASN1.h
@@ -210,6 +210,8 @@ class DLL_EXPORT ASN1Writer
     void InitNullWriter(void);
     size_t GetLengthWritten(void) const;
 
+    bool IsNullWriter() const { return mBuf == nullptr; }
+
     CHIP_ERROR PutInteger(int64_t val);
     CHIP_ERROR PutBoolean(bool val);
     CHIP_ERROR PutObjectId(const uint8_t * val, uint16_t valLen);