Code review

nrfconnect · Jan 2, 2024 · 73682ad · 73682ad
1 parent 134c1d6
commit 73682ad
Show file tree

Hide file tree

Showing 7 changed files with 40 additions and 156 deletions.
diff --git a/config/nrfconnect/chip-module/Kconfig b/config/nrfconnect/chip-module/Kconfig
@@ -242,6 +242,11 @@ config CHIP_DEVICE_GENERATE_ROTATING_DEVICE_UID
 
 endif # CHIP_FACTORY_DATA_BUILD
 
+# See config/zephyr/Kconfig for full definition
+config CHIP_FACTORY_RESET_ERASE_NVS
+	bool
+	default y if !CHIP_CRYPTO_PSA # For now erasing whole NVS sector is not supported for PSA Crypto
+
 config CHIP_LOG_SIZE_OPTIMIZATION
 	bool "Disable some detailed logs to decrease flash usage"
 	help
@@ -275,12 +280,4 @@ config CHIP_ENABLE_READ_CLIENT
 	  This config can be disabled for device types that do not require Read Client functionality.
 	  Disabling this config can save flash and RAM space.
 
-config CHIP_CRYPTO_PSA_ITS_NVM_OFFSET
-	hex "Specify the offset for Matter crypto keys in the ITS NVM space"
-	default 0x30000
-	depends on CHIP_CRYPTO_PSA
-	help
-	  Declared offset while using PSA key references. An example can override this definition based on implementation.
-	  The default value was selected not to interfere with OpenThread's default base which is 0x20000.
-
 endif # CHIP
diff --git a/src/crypto/CHIPCryptoPALPSA.h b/src/crypto/CHIPCryptoPALPSA.h
@@ -62,7 +62,8 @@ enum class KeyIdBase : psa_key_id_t
 {
     Minimum     = CHIP_CONFIG_CRYPTO_PSA_KEY_ID_BASE,
     Operational = Minimum, ///< Base of the PSA key ID range for Node Operational Certificate private keys
-    Maximum     = Operational + kMaxValidFabricIndex,
+    DACPrivKey  = Operational + kMaxValidFabricIndex + 1,
+    Maximum     = DACPrivKey,
 };
 
 static_assert(to_underlying(KeyIdBase::Minimum) >= PSA_KEY_ID_USER_MIN && to_underlying(KeyIdBase::Maximum) <= PSA_KEY_ID_USER_MAX,

diff --git a/src/platform/nrfconnect/CHIPPlatformConfig.h b/src/platform/nrfconnect/CHIPPlatformConfig.h
@@ -56,7 +56,7 @@
 
 #ifdef CONFIG_CHIP_CRYPTO_PSA
 #ifndef CHIP_CONFIG_CRYPTO_PSA_KEY_ID_BASE
-#define CHIP_CONFIG_CRYPTO_PSA_KEY_ID_BASE CONFIG_CHIP_CRYPTO_PSA_ITS_NVM_OFFSET
+#define CHIP_CONFIG_CRYPTO_PSA_KEY_ID_BASE 0x30000
 #endif // CHIP_CONFIG_CRYPTO_PSA_KEY_ID_BASE
 #endif // CONFIG_CHIP_CRYPTO_PSA
 

diff --git a/src/platform/nrfconnect/FactoryDataParser.c b/src/platform/nrfconnect/FactoryDataParser.c
@@ -104,105 +104,6 @@ static bool DecodeEntry(zcbor_state_t * states, void * buffer, size_t bufferSize
     return res;
 }
 
-bool SerializeFactoryData(uint8_t * factoryDataBuff, size_t factoryDataSize, const struct FactoryData * factoryData)
-{
-    zcbor_state_t cbor_state[MAX_FACTORY_DATA_NESTING_LEVEL];
-    zcbor_new_encode_state(cbor_state, MAX_FACTORY_DATA_NESTING_LEVEL, factoryDataBuff, factoryDataSize, 0);
-
-    if (!(zcbor_map_start_encode(cbor_state, 1) && zcbor_tstr_put_lit_cast(cbor_state, "version") &&
-          zcbor_uint32_put(cbor_state, (uint32_t) (factoryData->version)) && zcbor_tstr_put_lit_cast(cbor_state, "passcode") &&
-          zcbor_uint32_put(cbor_state, factoryData->passcode) && zcbor_tstr_put_lit_cast(cbor_state, "sn") &&
-          zcbor_bstr_encode_ptr(cbor_state, factoryData->sn.data, factoryData->sn.len) &&
-          zcbor_tstr_put_lit_cast(cbor_state, "date") &&
-          zcbor_bstr_encode_ptr(cbor_state, factoryData->date.data, factoryData->date.len) &&
-          zcbor_tstr_put_lit_cast(cbor_state, "hw_ver_str") &&
-          zcbor_bstr_encode_ptr(cbor_state, factoryData->hw_ver_str.data, factoryData->hw_ver_str.len) &&
-          zcbor_tstr_put_lit_cast(cbor_state, "rd_uid") &&
-          zcbor_bstr_encode_ptr(cbor_state, factoryData->rd_uid.data, factoryData->rd_uid.len) &&
-          zcbor_tstr_put_lit_cast(cbor_state, "dac_cert") &&
-          zcbor_bstr_encode_ptr(cbor_state, factoryData->dac_cert.data, factoryData->dac_cert.len) &&
-          zcbor_tstr_put_lit_cast(cbor_state, "pai_cert") &&
-          zcbor_bstr_encode_ptr(cbor_state, factoryData->pai_cert.data, factoryData->pai_cert.len) &&
-          zcbor_tstr_put_lit_cast(cbor_state, "spake2_it") && zcbor_uint32_put(cbor_state, (uint32_t) (factoryData->spake2_it)) &&
-          zcbor_tstr_put_lit_cast(cbor_state, "spake2_salt") &&
-          zcbor_bstr_encode_ptr(cbor_state, factoryData->spake2_salt.data, factoryData->spake2_salt.len) &&
-          zcbor_tstr_put_lit_cast(cbor_state, "spake2_verifier") &&
-          zcbor_bstr_encode_ptr(cbor_state, factoryData->spake2_verifier.data, factoryData->spake2_verifier.len) &&
-          zcbor_tstr_put_lit_cast(cbor_state, "vendor_name") &&
-          zcbor_bstr_encode_ptr(cbor_state, factoryData->vendor_name.data, factoryData->vendor_name.len) &&
-          zcbor_tstr_put_lit_cast(cbor_state, "product_name") &&
-          zcbor_bstr_encode_ptr(cbor_state, factoryData->product_name.data, factoryData->product_name.len) &&
-          zcbor_tstr_put_lit_cast(cbor_state, "part_number") &&
-          zcbor_bstr_encode_ptr(cbor_state, factoryData->part_number.data, factoryData->part_number.len) &&
-          zcbor_tstr_put_lit_cast(cbor_state, "product_url") &&
-          zcbor_bstr_encode_ptr(cbor_state, factoryData->product_url.data, factoryData->product_url.len) &&
-          zcbor_tstr_put_lit_cast(cbor_state, "product_label") &&
-          zcbor_bstr_encode_ptr(cbor_state, factoryData->product_label.data, factoryData->product_label.len) &&
-          zcbor_tstr_put_lit_cast(cbor_state, "enable_key") &&
-          zcbor_bstr_encode_ptr(cbor_state, factoryData->enable_key.data, factoryData->enable_key.len)))
-    {
-        return false;
-    }
-    if (factoryData->hwVerPresent)
-    {
-        if (!(zcbor_tstr_put_lit_cast(cbor_state, "hw_ver") && zcbor_uint32_put(cbor_state, (uint32_t) (factoryData->hw_ver))))
-        {
-            return false;
-        }
-    }
-    if (factoryData->vendorIdPresent)
-    {
-        if (!(zcbor_tstr_put_lit_cast(cbor_state, "vendor_id") &&
-              zcbor_uint32_put(cbor_state, (uint32_t) (factoryData->vendor_id))))
-        {
-            return false;
-        }
-    }
-    if (factoryData->productIdPresent)
-    {
-        if (!(zcbor_tstr_put_lit_cast(cbor_state, "product_id") &&
-              zcbor_uint32_put(cbor_state, (uint32_t) (factoryData->product_id))))
-        {
-            return false;
-        }
-    }
-    if (factoryData->discriminatorPresent)
-    {
-        if (!(zcbor_tstr_put_lit_cast(cbor_state, "discriminator") &&
-              zcbor_uint32_put(cbor_state, (uint32_t) (factoryData->discriminator))))
-        {
-            return false;
-        }
-    }
-    if (factoryData->productFinishPresent)
-    {
-        if (!(zcbor_tstr_put_lit_cast(cbor_state, "product_finish") &&
-              zcbor_uint32_put(cbor_state, (uint32_t) (factoryData->product_finish))))
-        {
-            return false;
-        }
-    }
-    if (factoryData->primaryColorPresent)
-    {
-        if (!(zcbor_tstr_put_lit_cast(cbor_state, "primary_color") &&
-              zcbor_uint32_put(cbor_state, (uint32_t) (factoryData->primary_color))))
-        {
-            return false;
-        }
-    }
-    if (factoryData->userDataPresent)
-    {
-        if (!(zcbor_tstr_put_lit_cast(cbor_state, "user") &&
-              zcbor_bstr_encode_ptr(cbor_state, factoryData->user.data, factoryData->user.len)))
-        {
-            return false;
-        }
-    }
-
-    return zcbor_map_end_encode(cbor_state, 1);
-    ;
-}
-
 bool FindUserDataEntry(struct FactoryData * factoryData, const char * entry, void * buffer, size_t bufferSize, size_t * outlen)
 {
     if ((!factoryData) || (!factoryData->user.data) || (!buffer) || (!outlen))
@@ -314,9 +215,7 @@ bool ParseFactoryData(uint8_t * buffer, uint16_t bufferSize, struct FactoryData
             // Date format is YYYY-MM-DD, so format needs to be validated and string parse to integer parts.
             struct zcbor_string date;
             res = res && zcbor_bstr_decode(states, &date);
-            updateSize(factoryData, "date", factoryData->date.len);
-            factoryData->date.data = (void *) date.value;
-            factoryData->date.len  = date.len;
+            updateSize(factoryData, "date", date.len);
             if (date.len == 10 && isdigit(date.value[0]) && isdigit(date.value[1]) && isdigit(date.value[2]) &&
                 isdigit(date.value[3]) && date.value[4] == '-' && isdigit(date.value[5]) && isdigit(date.value[6]) &&
                 date.value[7] == '-' && isdigit(date.value[8]) && isdigit(date.value[9]))
@@ -351,7 +250,7 @@ bool ParseFactoryData(uint8_t * buffer, uint16_t bufferSize, struct FactoryData
         {
             res = res && zcbor_bstr_decode(states, (struct zcbor_string *) &factoryData->dac_priv_key);
             updateSize(factoryData, "dac_key", factoryData->dac_priv_key.len);
-            factoryData->dacPrivateKeyPresent = true;
+            factoryData->dac_priv_key_offset = (size_t) ((void *) states->payload - (void *) buffer) - factoryData->dac_priv_key.len;
         }
         else if (strncmp("pai_cert", (const char *) currentString.value, currentString.len) == 0)
         {

diff --git a/src/platform/nrfconnect/FactoryDataParser.h b/src/platform/nrfconnect/FactoryDataParser.h
@@ -35,7 +35,6 @@ struct FactoryData
 {
     uint16_t version;
     struct FactoryDataString sn;
-    struct FactoryDataString date;
     uint16_t date_year;
     uint8_t date_month;
     uint8_t date_day;
@@ -68,23 +67,10 @@ struct FactoryData
     bool productFinishPresent;
     bool primaryColorPresent;
     bool userDataPresent;
-    bool dacPrivateKeyPresent;
     size_t actualSize;
+    size_t dac_priv_key_offset;
 };
 
-/**
- * @brief Serialize the new factory data set to binary CBOR format.
- * 
- * Serialized data will not contain DAC private key
- * 
- * @param factoryDataBuff buffer to store serialized factory data set
- * @param factoryDataSize size of the output buffer
- * @param factoryData reference pointer to the existing factory data set
- * @return true if the factory data set was serialized successfully
- * @return false if the error occurred, for example the output buffer size was to small.
- */
-bool SerializeFactoryData(uint8_t * factoryDataBuff, size_t factoryDataSize, const struct FactoryData * factoryData);
-
 /**
  * @brief Parses raw factory data into the factory data structure.
  *

diff --git a/src/platform/nrfconnect/FactoryDataProvider.cpp b/src/platform/nrfconnect/FactoryDataProvider.cpp
@@ -27,7 +27,7 @@
 #include <lib/support/logging/CHIPLogging.h>
 
 #ifdef CONFIG_CHIP_CRYPTO_PSA
-#include <platform/Zephyr/SysHeapMalloc.h>
+#include <lib/support/ScopedBuffer.h>
 #include <psa/crypto.h>
 #include <zephyr/drivers/flash.h>
 
@@ -116,24 +116,25 @@ CHIP_ERROR FactoryDataProvider<FlashFactoryData>::MoveDACPrivateKeyToSecureStora
                                                                                    size_t factoryDataSize)
 {
 
-    CHIP_ERROR err         = CHIP_NO_ERROR;
-    void * factoryDataBuff = nullptr;
+    CHIP_ERROR err = CHIP_NO_ERROR;
+    uint8_t ClearedDACPrivKey[kDACPrivateKeyLength];
+    memset(ClearedDACPrivKey, 0xFF, sizeof(ClearedDACPrivKey));
 
     if (!factoryDataPartition || factoryDataSize == 0)
     {
         return CHIP_ERROR_INVALID_ARGUMENT;
     }
 
     // Check if factory data contains DAC private key
-    if (mFactoryData.dacPrivateKeyPresent)
+    if (memcmp(mFactoryData.dac_priv_key.data, ClearedDACPrivKey, kDACPrivateKeyLength) != 0)
     {
         psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT;
         // If there is the new DAC private key present in the factory data set and also there is
         // the existing one in ITS NVM storage, then it is a security violation
         // TODO: Consider other reactions to this violation: blocking the application, factory reset, etc.
         if (psa_get_key_attributes(mDACPrivKeyId, &attributes) != PSA_SUCCESS)
         {
-            ChipLogDetail(DeviceLayer, "Found DAC Private Key in factory data set. Copying to ITS...");
+            ChipLogProgress(DeviceLayer, "Found DAC Private Key in factory data set. Copying to ITS...");
 
             psa_reset_key_attributes(&attributes);
             psa_set_key_type(&attributes, PSA_KEY_TYPE_ECC_KEY_PAIR(PSA_ECC_FAMILY_SECP_R1));
@@ -143,38 +144,38 @@ CHIP_ERROR FactoryDataProvider<FlashFactoryData>::MoveDACPrivateKeyToSecureStora
             psa_set_key_id(&attributes, mDACPrivKeyId);
             psa_set_key_usage_flags(&attributes, PSA_KEY_USAGE_EXPORT | PSA_KEY_USAGE_SIGN_MESSAGE);
 
-            VerifyOrExit(psa_import_key(&attributes, reinterpret_cast<uint8_t *>(mFactoryData.dac_priv_key.data),
-                                        kDACPrivateKeyLength, &mDACPrivKeyId) == PSA_SUCCESS,
-                         err = CHIP_ERROR_INTERNAL);
+            VerifyOrReturnError(psa_import_key(&attributes, reinterpret_cast<uint8_t *>(mFactoryData.dac_priv_key.data),
+                                               kDACPrivateKeyLength, &mDACPrivKeyId) == PSA_SUCCESS,
+                                CHIP_ERROR_INTERNAL);
         }
 
-        memset(mFactoryData.dac_priv_key.data, 0xff, mFactoryData.dac_priv_key.len);
-        mFactoryData.dacPrivateKeyPresent = false;
-
         // Allocate needed memory space to perform removal and moving DAC private key
         size_t alignedSize = mFactoryData.actualSize + (sizeof(uint32_t) - (mFactoryData.actualSize % sizeof(uint32_t)));
-        factoryDataBuff    = chip::DeviceLayer::Malloc::Calloc(alignedSize, sizeof(uint8_t));
+        chip::Platform::ScopedMemoryBuffer<uint8_t> factoryDataBuff;
+        VerifyOrReturnError(factoryDataBuff.Calloc(alignedSize), CHIP_ERROR_NO_MEMORY);
 
-        if (!factoryDataBuff)
-        {
-            ChipLogError(DeviceLayer, "Could not allocate memory for removing DAC private key!");
-            return CHIP_ERROR_NO_MEMORY;
-        }
+        // Copy existing factoryDataSet
+        memcpy(factoryDataBuff.Get(), factoryDataPartition, alignedSize);
 
-        VerifyOrExit(SerializeFactoryData(reinterpret_cast<uint8_t *>(factoryDataBuff), mFactoryData.actualSize, &mFactoryData),
-                     err = CHIP_ERROR_INTERNAL);
+        // Overwrite the DAC private key
+        memcpy(factoryDataBuff.Get() + mFactoryData.dac_priv_key_offset, ClearedDACPrivKey, kDACPrivateKeyLength);
 
         // Replace the old factory data set with the new one.
-        VerifyOrExit(0 == flash_erase(kFlashDev, kFactoryDataPartitionAddress, kFactoryDataPartitionSize), err = CHIP_ERROR_NO_MEMORY);
-        VerifyOrExit(0 == flash_write(kFlashDev, kFactoryDataPartitionAddress, factoryDataBuff, alignedSize), err = CHIP_ERROR_INTERNAL);
-
-        VerifyOrExit(ParseFactoryData(factoryDataPartition, static_cast<uint16_t>(factoryDataSize), &mFactoryData),
-                     err = CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND);
+        VerifyOrReturnError(0 == flash_erase(kFlashDev, kFactoryDataPartitionAddress, kFactoryDataPartitionSize),
+                            CHIP_ERROR_NO_MEMORY);
+        VerifyOrReturnError(0 == flash_write(kFlashDev, kFactoryDataPartitionAddress, factoryDataBuff.Get(), alignedSize),
+                            CHIP_ERROR_INTERNAL);
+
+        // Parse the factory Data again and verify if the procedure finished successfully
+        VerifyOrReturnError(ParseFactoryData(factoryDataPartition, static_cast<uint16_t>(factoryDataSize), &mFactoryData),
+                            CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND);
+
+        // Verify if the factory data does not contain the DAC private key anymore.
+        VerifyOrReturnError(memcmp(mFactoryData.dac_priv_key.data, ClearedDACPrivKey, kDACPrivateKeyLength) == 0,
+                            CHIP_ERROR_INTERNAL);
     }
 
-exit:
-    chip::DeviceLayer::Malloc::Free(factoryDataBuff);
-    return err;
+    return CHIP_NO_ERROR;
 }
 #endif
 

diff --git a/src/platform/nrfconnect/FactoryDataProvider.h b/src/platform/nrfconnect/FactoryDataProvider.h
@@ -184,7 +184,7 @@ class FactoryDataProvider : public chip::Credentials::DeviceAttestationCredentia
     struct FactoryData mFactoryData;
     FlashFactoryData mFlashFactoryData;
 #ifdef CONFIG_CHIP_CRYPTO_PSA
-    psa_key_id_t mDACPrivKeyId = static_cast<psa_key_id_t>(static_cast<uint32_t>(chip::Crypto::KeyIdBase::Maximum) + 1);
+    psa_key_id_t mDACPrivKeyId = to_underlying(chip::Crypto::KeyIdBase::DACPrivKey);
 #endif
 };