DeviceKey: [Security Fix] Generated ROT-key is still used when TRNG fails

[boomer41]

Description

Fix security bug in DeviceKey's implementation.
With the current implementation, a possible failure of the TRNG cannot be noticed, as the return value in line 269 is immediately overriden in line 274.
When the TRNG somehow fails (e. g. not (yet) ready), the key may stay zeroed out. This is falsely reported back to the generation logic as a success, thus making the RootOfTrust possibly predictable.

This fix ensures that a failed TRNG is properly propagated back to the caller, and that the output is not used as ROT.

@Securityteam (if any): I'd suggest a small security notice to the users that the DeviceKey may be insecure, as the TRNG may have unknowingly failed.

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

[ciarmcom]

@boomer41, thank you for your changes.
@ARMmbed/mbed-os-storage @ARMmbed/mbed-os-maintainers please review.

[yossi2le]

LGTM. Thanks for that catch.

[0xc0170]

@Securityteam (if any): I'd suggest a small security notice to the users that the DeviceKey may be insecure, as the TRNG may have unknowingly failed.

@yossi2le can you review this one as well?

[yossi2le]

@Securityteam (if any): I'd suggest a small security notice to the users that the DeviceKey may be insecure, as the TRNG may have unknowingly failed.

@boomer41 can you please explain why we need the security notice after this fix will be merged? Do you think there is a real reason to suspect that TRNG may fail and mbedtls_entropy_func will return a success status?

[boomer41]

@yossi2le I don't know the characteristics of every HAL implementation of the TRNG for every supported CPU. I don't know how the entropy function actually works internally either.

I also can't imagine that the TRNG actually has failed in a single case. But some users may have strict security policies that enforces key rotation when where is only a slight possibility for insecure keys.

This is only suggestion, as I don't know ARM's or mbed's security policy. This is up to you to decide.

[0xc0170]

CI started

[boomer41]

@yossi2le I just found this:

mbed-os/features/mbedtls/platform/src/mbed_trng.c

Line 23 in ab1c2be

trng_init(&trng_obj);

When mbedtls polls for entropy, it initializes a trng_t structure via trng_init every time, uses it, and then destroys it.

As an example, on the STM-family of CPUs, only one trng_t structure can be created.

mbed-os/targets/TARGET_STM/trng_api.c

Line 37 in ab1c2be

error("Only 1 RNG instance supported\r\n");

When the ROT is generated in a separate task, whilst another TRNG operation in another Task is also running, the TRNG could actually fail.

Or did I miss something?

[yossi2le]

@boomer41
Thanks for the info. I don't think adding the security message is the correct way to go. Actually, we can add one more check for the generate_key_by_random to see that the buffer is indeed changed after the call to mbedtls_entropy_func but it's not a bulletproof either.
In my opinion, after fixing this bug the security issue should be resolved.

In regards to the STM example you provided, the system will halt if this situation will arise. the
error("Only 1 RNG instance supported\r\n")
is actually causing a runtime error and the operation is stopped.

[boomer41]

@yossi2le
Thank you for that information regarding error().

But I do think we don't understand each other. With "security message" i do mean some kind of email broadcasted to the users, informing them that already generated DeviceKeys may/could be insecure.

If you do already mean this, I apologize.
Thanks.

[yossi2le]

@boomer41 Thanks for the clarification. I apologize but I indeed misunderstood your request. I will pass your remark forward and I agree we should distribute this message out.
Thanks for everything.

[mbed-ci]

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

• jenkins-ci/mbed-os-ci_greentea-test

[cmonr]

jenkins-ci/greentea-test failure unrelated to PR.

Restarted.