Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clear ITS keys on factory reset #436

Closed
wants to merge 2 commits into from
Closed

Clear ITS keys on factory reset #436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c6d0842
[nrf fromtree] [Zephyr] Clear ITS keys on factory reset
ArekBalysNordic May 6, 2024
02a3488
[nrf noup] [Zephyr] Prevent from removing DAC Private Key
ArekBalysNordic May 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions config/zephyr/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,14 @@ config CHIP_FACTORY_RESET_ERASE_NVS
configuration is supposed to be cleared on a factory reset, including
device-specific entries.

config CHIP_FACTORY_RESET_ERASE_PSA_ITS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know the full background of this change, but do we want to make it configurable? It seems like we may have problem if these data will not be removed, so maybe we should just do it always? Also shouldn't we enable it for our platform or set default to y for everyone?

bool "Erase all Matter-related crypto material from PSA ITS storage"
depends on CHIP_CRYPTO_PSA
help
Erases all crypto materials from PSA ITS (Internal Trusted Storage)
dedicated to Matter purposes. Crypto materials can be defined for example
as Persistent Session keys.

config CHIP_MALLOC_SYS_HEAP
bool "Memory allocator based on Zephyr sys_heap"
imply SYS_HEAP_RUNTIME_STATS
Expand Down
22 changes: 21 additions & 1 deletion src/platform/Zephyr/ConfigurationManagerImpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,11 @@

#ifdef CONFIG_NET_L2_OPENTHREAD
#include <platform/ThreadStackManager.h>
#endif
#endif

#ifdef CONFIG_CHIP_FACTORY_RESET_ERASE_PSA_ITS
#include <crypto/CHIPCryptoPALPSA.h>
#endif

namespace chip {
namespace DeviceLayer {
Expand Down Expand Up @@ -210,6 +214,22 @@ void ConfigurationManagerImpl::DoFactoryReset(intptr_t arg)
ConnectivityMgr().ErasePersistentInfo();
#endif

#ifdef CONFIG_CHIP_FACTORY_RESET_ERASE_PSA_ITS
// Ensure that all persistent PSA crypto materials are removed.
for (uint32_t keyID = static_cast<uint32_t>(chip::Crypto::KeyIdBase::Minimum);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (uint32_t keyID = static_cast<uint32_t>(chip::Crypto::KeyIdBase::Minimum);
for (psa_key_id_t keyID = chip::to_underlying(Crypto::KeyIdBase::Minimum);

etc. to not cast via uint32_t.

Though now that I think about it, this will invoke psa_destroy_key for 255 possible fabrics even though we have only 5 configured. Maybe it's better to run this loop only for the [minimum, minimum + MAX_FABRICS) range? Or it doesn't generate any latency?

keyID <= static_cast<uint32_t>(chip::Crypto::KeyIdBase::Maximum); keyID++)
{
#ifdef CONFIG_CHIP_CRYPTO_PSA_MIGRATE_DAC_PRIV_KEY
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would run this if unconditionally.

// Prevent from removing DAC Private Key
if (keyID == static_cast<uint32_t>(chip::Crypto::KeyIdBase::DACPrivKey))
{
continue;
}
#endif
psa_destroy_key(static_cast<psa_key_id_t>(keyID));
}
#endif

PlatformMgr().Shutdown();
}

Expand Down
Loading