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.

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

[crypto] allow platform to define its own CSPRNG #3

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Conversation

LuDuda
Copy link
Owner

@LuDuda LuDuda commented Jan 23, 2022

This PR provides the following changes:

  • Introduce a new otPlatCryptoRandom API
  • Move usage of mbedtls_entropy_* and mbedtls_ctr_drbg_* out of ARM PSA enabled platforms
  • Stop exposing internal mbedtls contextes for entropy and CTR DRBG in OpenThread API

Background: With TF-M enabled, access to secure random number generation should be done only via psa_random_get function. Non-secure environment should not use crypto HW directly (which might be the case by using legacy mbedtls entropy and ctr_drbg functions.

Signed-off-by: Lukasz Duda [email protected]

@LuDuda
Copy link
Owner Author

LuDuda commented Jan 23, 2022

FYI @Damian-Nordic

@codecov-commenter
Copy link

codecov-commenter commented Jan 23, 2022

Codecov Report

Merging #3 (4fd25e3) into main (215c459) will increase coverage by 5.63%.
The diff coverage is 91.40%.

@@            Coverage Diff             @@
##             main       #3      +/-   ##
==========================================
+ Coverage   81.89%   87.53%   +5.63%     
==========================================
  Files         415      486      +71     
  Lines       49557    60289   +10732     
==========================================
+ Hits        40584    52773   +12189     
+ Misses       8973     7516    -1457     
Impacted Files Coverage Δ
include/openthread/coap.h 100.00% <ø> (ø)
src/cli/cli.hpp 100.00% <ø> (ø)
src/core/api/coap_api.cpp 65.17% <0.00%> (-1.19%) ⬇️
src/core/backbone_router/backbone_tmf.hpp 100.00% <ø> (ø)
src/core/backbone_router/bbr_local.hpp 88.88% <ø> (ø)
src/core/backbone_router/bbr_manager.hpp 100.00% <ø> (ø)
src/core/border_router/routing_manager.cpp 93.40% <ø> (ø)
src/core/common/random.hpp 100.00% <ø> (ø)
src/core/mac/mac.hpp 100.00% <ø> (+7.31%) ⬆️
src/core/mac/sub_mac.hpp 92.00% <ø> (+20.00%) ⬆️
... and 196 more


OT_TOOL_WEAK otError otPlatCryptoRandomDeinit(void)
{
mbedtls_ctr_drbg_free(&sCtrDrbgContext);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about sEntropyContext?

#endif

#if !OPENTHREAD_RADIO
static mbedtls_ctr_drbg_context sCtrDrbgContext;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't it be more future-proof if the context was passed via otCryptoContext like in other crypto functions? For example, what if a single application runs multiple OT instances?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or you did so, because Random::Crypto::MbedTlsContextGet() is global anyway?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, currently there are no multi-instances of RNGs algorithms in OT. Let me maybe push this change and explicitly ask OT community whether they envision a use-case for the future, and if so, i will add otCryptoContext param to otPlatCryptoRandom* functions.

zhanglongxia and others added 18 commits January 24, 2022 09:47
- THCI changes for bbr reset procedure to make a better effort to
  reset and to also make sure to restart the otbr-agent prior to
  attempting a factoryreset to cover the case where the agent cannot
  currently communicate to the rcp.

- THCI change to move from sudo service restart otbr-agent to sudo
  systemctl restart otbr-agent, which is more comprehensive.

- THCI changes for multicast to use ipmaddr in both soc and bbr cases.
`bbr skipseqnuminc` should complete without error.
This commit contain smaller style changes in `Mac` and `SubMac`
This commit also adds `MacFilterAddressModeToString()`.
…read#7343)

This will prevent unexpected multicast packets on the Thread network.
…thread#7350)

This commit adds `AddressResolver::LookUp()` method which resolves
an EID to an RLOC17 by checking if an existing entry already exists
in the address cache table.
"BBR Sequence Number Updating" section in Thread Specification
requires special wraps when increasing BBR Dataset Sequence Number.
This commit adds `Commissioner::Dataset` class which mirrors the
public `otCommissioningDataset`. It also adds `ResignMode` enum
which is used as input to `Stop()` method to indicate whether or
not to resign from being the commissioner.
This commit enables BR to withdraw its local NAT64 prefix if a smaller
one is found in network data.

It might happen when two BRs join the Thread network at merely the
same time and thus both advertise its local NAT64 prefix. The BR with
numerically greater prefix will withdraw its NAT64 prefix.
Provide a setter for the message code to change it after
initialization of the message.

This complements the existing getter function `otCoapMessageGetCode`.
This commit removes the `otLogResult{Bbr/Platform}()` macros from
`logging.hpp` and instead have each module implement `LogError()`
method. The main goal behind this change to simplify the logging
module and reduce number of macros/definitions in it (to prepare for
new logging model to be introduced).
This commit avoid setting aError which is not read.
This commit provides the following changes:

 * Introduce a new otPlatCryptoRandom API
 * Move usage of mbedtls_entropy_* and mbedtls_ctr_drbg_* out of
   ARM PSA enabled platforms
 * Stop exposing internal mbedtls contextes for entropy and CTR DRBG
   in OpenThread API

Signed-off-by: Lukasz Duda <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.