Device key implementation #6642

yossi2le · 2018-04-15T18:08:41Z

Description

This PR introducing the device key implementation.
The DeviceKey is a mechanism that implements key derivations from a Root Of Trust (ROT) without ever exposing the ROT itself. In DeviceKey there are two API's, one for key derivation of 128 or 256 bits keys and one for injecting the ROT. injection of ROT is needed only if True Random Generator (TRNG) is not supported.

More details and example can be found in the following repository:
https://github.com/ARMmbed/mbed-os-example-devicekey

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[X ] Feature
[ ] Breaking change

0xc0170 · 2018-04-16T07:04:46Z

drivers/DeviceKey.h

+        return instance;
+    }
+
+    virtual ~DeviceKey();


Why is dtor virtual? I do not see any virtual methods, not intended to be a base class ?

0xc0170 · 2018-04-16T07:06:30Z

drivers/DeviceKey.h

+     * @param output buffer for the CMAC result.
+     * @return 0 on success, negative error code on failure
+     */
+    int calc_cmac(const unsigned char *input, size_t isize, uint32_t *ikey_buff, int ikey_size, unsigned char *output);


why chosen calc rather calculate ?

0xc0170 · 2018-04-16T07:09:01Z

drivers/DeviceKey.h

+     * @param ikey_type type of the required key. Type must be 16 bytes or 32 bytes.
+     * @return 0 on success, negative error code on failure
+     */
+    int device_key_derived_key(const unsigned char *isalt, size_t isalt_size, unsigned char *output, uint16_t ikey_type);


device_key_derived_key - I have to read the description what it does, can this have more meaningful name? From the description - generate_derived_key() or similar? Device key is object's name.

0xc0170 · 2018-04-16T07:10:36Z

drivers/DeviceKey.h

+#define DEVICE_KEY_16BYTE 16
+#define DEVICE_KEY_32BYTE 32
+
+enum DeviceKeyStatus {


These are return types:

why then methods have int return type ?

not part of the class?

Using the enum can cause type problems in C++. Especially when errors need to be propogated upwards. Just using int simplifies a lot of things for users.

Either way @SenRamakri has bigger plans with error codes.

@0xc0170 @geky
There is a process now to add Error Codes to mbed-os and when this happen, we will change the implementation to use the new error codes mechanism.
Until then We thought it is better for our code to use an internal error code mechanism and return int in order to excuse the caller from the need of using our enum.

Sounds good to me. Still does not answer my second question why is this not part of the class itself.

@0xc0170
Actually only because it mimics the idea of the future solution of _MbedErrorStatus enum. I don't mind to make it part of the class but it’s not the way it would be implemented in the future.

Personally in these situations I still use the enum but then use a typdef in any location where data might be used by another processor.
Then again that's what I do for C not C++.

Worst case I'd rather a typedef be used than a bare int; to give better context for anyone using it.

0xc0170 · 2018-04-16T07:13:46Z

drivers/DeviceKey.h

+ * @ingroup drivers
+ */
+
+class DeviceKey : private mbed::NonCopyable<DeviceKey> {


What about interrupt safety/thread safety? There should be a note for a driver

0xc0170 · 2018-04-16T07:14:37Z

@yossi2le Who should review this addition? I would assume nvstore and mbedtls teams. Let me know, I'll assign reviewers

0xc0170 · 2018-04-16T07:15:46Z

Please review Ci failures (travis docs and events)

geky · 2018-04-16T08:03:00Z

NVStore is only available if the device has programmable flash (DEVICE_FLASH macro).
https://github.com/ARMmbed/mbed-os/blob/master/features/nvstore/source/nvstore.h#L21-L26

Does Device Key need the same? (Cause of build failures)

yossi2le · 2018-04-16T08:39:33Z

@0xc0170 regarding the review of nvstore and mbedtls. mbedtls indeed should be assigned to this review. nvstore from the other hand is under my team (storage) responsibility.
thanks.

yossi2le · 2018-04-16T08:41:10Z

@geky. Thanks for that, you absolutely right about the DEVICE_FLASH macro.

ciarmcom · 2018-04-16T11:50:24Z

ARM Internal Ref: IOTSSL-2222

…and fixing doxygen issues

hanno-becker · 2018-04-18T08:26:34Z

drivers/DeviceKey.cpp

+        goto finish;
+    }
+
+    return DEVICEKEY_SUCCESS;


The cipher context must be freed on case of success, too. Suggestion: just set ret = DEVICEKEY_SUCCESS here.

@hanno-arm
10x for that. it will be fix.

hanno-becker · 2018-04-18T08:27:06Z

drivers/DeviceKey.cpp

+
+finish:
+    mbedtls_cipher_free( &ctx );
+    return ret;


You are returning a mixture of own error codes and Mbed TLS specific ones here -- is this intended?

hanno-becker · 2018-04-18T08:42:02Z

drivers/DeviceKey.cpp

+    trng_init(&trng_obj);
+
+    int ret = trng_get_bytes(&trng_obj, (unsigned char *)output, in_size, &size);
+    if (DEVICEKEY_SUCCESS != ret || in_size != size) {


This is not valid use of the TRNG API -- trng_get_bytes is allowed to return less than what was requested, and in this case it's the responsibility of the caller to repeat the call until enough entropy has been gathered.

@hanno-arm
10x for that. Actually the documentation is not clear enough and I think missing an example. I have fixed it. Please review and let me know if this answer the API requirement.

hanno-becker · 2018-04-18T08:50:10Z

drivers/DeviceKey.cpp

+
+finish:
+    if (double_size_salt != NULL) {
+        delete[] double_size_salt;


Please reliably zeroize double_size_salt to avoid leakage in memory. Consider e.g. mbedtls_zeroize.

@hanno-arm
mbedtls_zeroize is static function duplicate inside every mbedtls module. For now I have copied it as static function to my code. I think we should discuss on where this function should sit in order to have only one copy for everyone.

@yossi2le Moving the zeroize function to a separate compilation unit is being considered at the moment. For now, I suggest we live with the additional copy here.

hanno-becker · 2018-04-18T10:28:56Z

drivers/DeviceKey.cpp

+        }
+
+        //Double the salt size cause cmac always return just 16 bytes
+        double_size_salt = new unsigned char[isalt_size * 2];


What it is the reason for the use of this doubled salt, as opposed to a different modification of the salt to generate the second half of the key?

Are you following some standard here?

@hanno-arm We are open to proposals. We do not have any preferences here. If you see any problem with the current method please let us know.

hanno-becker · 2018-04-18T10:31:27Z

drivers/DeviceKey.cpp

+    }
+
+    //First we read if key exist. If it is exists, we return DEVICEKEY_ALREADY_EXIST error
+    uint32_t read_key[DEVICE_KEY_32BYTE / sizeof(uint32_t)] = {0};


Why is read_key not defined as a uint8_t array?

@hanno-arm This is because NVStore had API of uint32_t. This is internal allocation therefore has no impact externally. Do you have any concerns?

@dannybenor No, I don't have concerns, I only found it surprising when I read it. Thanks for your answer!

hanno-becker

The TRNG API usage is not used correctly, and the salt needs to be reliably zeroized in the key derivation function.

hanno-becker

I would like to request more information on why CMAC was used here for the key generation. A natural alternative would be to use Mbed TLS' implementation of PKCS5-PBKDF2 instead (HKDF is not yet implemented in Mbed TLS, unfortunately). This function takes password and salt of variable lengths and generates a key of variable length from it using HMAC as the underlying primitive. Using this seems straightforward and would also allow to make the DeviceKey interface more flexible by supporting an arbitrary output key length, which might be useful in the future.

mbed-ci · 2018-05-22T20:01:14Z

Build : FAILURE

Build number : 2107
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6642/

cmonr · 2018-05-22T20:08:06Z

@yossi2le @dannybenor Fyi, it looks like we're still facing some CI node issues. Will restart once things are stable again.

cmonr · 2018-05-22T21:22:47Z

/morph build

mbed-ci · 2018-05-22T22:25:13Z

Build : SUCCESS

Build number : 2108
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6642/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-05-22T23:51:32Z

Test : FAILURE

Build number : 1909
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6642/1909

mbed-ci · 2018-05-23T04:49:57Z

Exporter Build : SUCCESS

Build number : 1741
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6642/

0xc0170 · 2018-05-23T08:36:15Z

Starting tests now. We identified that some boards require bigger timeout (test takes at least 40 seconds for some nucleo and GCC ARM, tested locally).

/morph build

mbed-ci · 2018-05-23T12:40:27Z

Build : SUCCESS

Build number : 2123
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6642/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

cmonr · 2018-05-23T19:39:34Z

Shuffling around some jobs.

/morph export-build

cmonr · 2018-05-23T19:42:20Z

/morph test

mbed-ci · 2018-05-23T22:28:37Z

Test : SUCCESS

Build number : 1921
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6642/1921

mbed-ci · 2018-05-24T02:03:56Z

Exporter Build : SUCCESS

Build number : 1754
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6642/

cmonr · 2018-05-24T02:12:16Z

/morph uvisor-test

0xc0170 · 2018-05-24T09:28:41Z

/morph uvisor-test

0xc0170 · 2018-05-24T09:54:16Z

/morph uvisor-test

dannybenor · 2018-05-24T10:14:53Z

Approved from my side

hanno-becker · 2018-05-24T10:24:15Z

features/device_key/source/DeviceKey.cpp

+
+    do {
+
+    	mbedtls_cipher_init(&ctx);


@yossi2le What is the reason you moved init+free inside this loop?

@hanno-arm
Two devices was failed during CI tests in the second round of the loop when we did CMC update.
We have suspected that maybe free and init will clear up some resources and move it into the loop what actually solved the problem.

@yossi2le That shouldn't happen and should be investigated. In any case, free + init in every iteration is valid usage, too, for the new version is certainly fine.

Device key implementation

a8febd5

0xc0170 added the needs: review label Apr 16, 2018

0xc0170 requested changes Apr 16, 2018

View reviewed changes

0xc0170 requested a review from SenRamakri April 16, 2018 07:14

0xc0170 added needs: work and removed needs: review labels Apr 16, 2018

0xc0170 requested review from mazimkhan and hanno-becker April 16, 2018 09:17

0xc0170 added tracking component: security labels Apr 16, 2018

ciarmcom added the mirrored label Apr 16, 2018

Changing methods names, adding NVSTORE_ENABLED check for compilation …

206ca6c

…and fixing doxygen issues

cmonr added needs: review and removed needs: work labels Apr 16, 2018

hanno-becker reviewed Apr 18, 2018

View reviewed changes

hanno-becker suggested changes Apr 18, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Apr 18, 2018

hanno-becker suggested changes Apr 19, 2018

View reviewed changes

Increase tests timeout to 45 second

f33f4da

yossi2le dismissed dannybenor’s stale review via f33f4da May 23, 2018 08:30

0xc0170 added needs: CI and removed needs: work labels May 23, 2018

0xc0170 approved these changes May 23, 2018

View reviewed changes

0xc0170 added ready for merge and removed needs: CI labels May 24, 2018

hanno-becker reviewed May 24, 2018

View reviewed changes

hanno-becker approved these changes May 24, 2018

View reviewed changes

dannybenor approved these changes May 24, 2018

View reviewed changes

cmonr merged commit 06180e8 into ARMmbed:master May 24, 2018

cmonr removed the ready for merge label May 24, 2018

ciarmcom added the closed_in_jira label May 24, 2018

simonbutcher mentioned this pull request Jun 6, 2018

DeviceKey made Mbed TLS Configuration changes that haven't been upstreamed #7142

Closed

Device key implementation #6642

Device key implementation #6642

Uh oh!

Conversation

yossi2le commented Apr 15, 2018

Description

Pull request type

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0Grit Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Apr 16, 2018

Uh oh!

0xc0170 commented Apr 16, 2018

Uh oh!

geky commented Apr 16, 2018

Uh oh!

yossi2le commented Apr 16, 2018

Uh oh!

yossi2le commented Apr 16, 2018

Uh oh!

ciarmcom commented Apr 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yossi2le Apr 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hanno-becker Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hanno-becker left a comment

Choose a reason for hiding this comment

Uh oh!

hanno-becker left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

0xc0170 Apr 16, 2018 •

edited

Loading

0Grit Apr 16, 2018 •

edited

Loading

yossi2le Apr 22, 2018 •

edited

Loading

hanno-becker Apr 23, 2018 •

edited

Loading

hanno-becker left a comment •

edited

Loading