PSA Initial Attestation service #9668

moranpeker · 2019-02-11T21:40:14Z

Description

Attestation service can create a token on request, which contains a fix set of
device specific data.

Contains the implementation of ‘psa_initial_attest_get_token_size’ , ‘psa_initial_attest_get_token’ and ‘psa_attestation_inject_key’.

PSA Initial Attestation Service based on TF-M Attestation Service that taken from TF-M master.
TF-M latest attestation version is: c43181daf54f69f53de58593a50dd6a9c233eecd

Signing of token
‘psa_attestation_inject_key’ API - Device must contain an attestation key pair. The token is signed with the private part of attestation key pair. The public key is used to verify the token authenticity.

Inject attestation key by importing a given private key or generate a key pair.
Attestation key stored as a persistent key with a specific key-id.

Claims in the initial attestation token
The initial attestation token is formed of claims.
List of claims are included in the token:
https://confluence.arm.com/display/IoTBU/Claims+in+PSA+IAT+report
In the current implementation no bootloader exists in single & dual V7 .
In the current implementation temporary mandatory claims (ONLY) are given by a hard coded temp bootloader status data (attestation_bootloader_data.c).

Code structure
The PSA interface for the Initial Attestation Service is located in
components\TARGET_PSA\services\attestation.
The only headers to be included by applications that want to use functions from
the PSA API are psa_initial_attestation_api.h and psa_attest_inject_key.h (under components\TARGET_PSA\inc\psa folder).

Service source files

CBOR library: qcbor: This library is used to create a proper CBOR token. It is a fork of this external QCBOR library.
For now, TFM source files are part of this PR and located in components\TARGET_PSA\services\attestation\tfm_impl + header files ‘psa_initial_attestation_api.h’ and ‘attestation.h’.
An importer needs to be created to import TFM source files.

Code checked over K64F and PSOC6.
Pass compliance tests over K64F and PSOC6.

This PR includes and depends on the security lifecycle bug fix ( PR = #9745 )

Pull request type

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

Reviewers

Release Notes

A brief description of changes introduced.
Add PSA Initial Attestation Service implementation.
An analysis of effects: components affected, potential consequences for users and reasons for the addition or change.
this code is a new feature and doesn't effect existing code.
Migration guidance: actions for updating the current code. Please include code snippets to illustrate before and after the addition or change.
This breaks backwards compatibility with Nucleo F411RE entropy injection.

ciarmcom · 2019-02-11T22:00:30Z

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

cmonr · 2019-02-12T00:10:00Z

.astyleignore

@@ -23,3 +23,4 @@ targets
 components/802.15.4_RF
 components/wifi
 tools
+components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_IMPL/tfm_impl


👍 for the commit comment about this.

One note: please revert file permissions changes 100644 → 100755 ?

cmonr · 2019-02-12T00:11:28Z

Checked over K64F and PSOC6.

Should PSOC6 also have its/their targets.json entries updated?

cmonr

A nit whilse others get a better chance to review.
Please revert the permissions of target.json and .astyle.yml (755 -> 644)

cmonr · 2019-02-12T00:14:28Z

Also, there are still some astyle issues: https://travis-ci.org/ARMmbed/mbed-os/jobs/491854379

alekshex · 2019-02-12T12:46:02Z

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_EMUL/psa_attest_inject_key.c

+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ *
+ */


please change license for all to Apache.

Copyright (c) 2006-2013 ARM Limited

SPDX-License-Identifier: Apache-2.0

Licensed under the Apache License, Version 2.0 (the "License");

you may not use this file except in compliance with the License.

You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software

distributed under the License is distributed on an "AS IS" BASIS,

WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

See the License for the specific language governing permissions and

limitations under the License.
*/

Please leave all TF-M sources license as is. only our wrappers should be under Apache.
TF-M license is handled by legal currently

alekshex · 2019-02-12T13:00:06Z

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_EMUL/psa_initial_attestation_api.c

+        return err;
+    }
+
+    *token_size = out_vec[0].len;


do we need this line?
or this takes care of it psa_outvec out_vec[1] = { { token, *token_size } };

alekshex

first pass

alekshex · 2019-02-12T13:55:52Z

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_IMPL/attest_boot_status_loader.c

+ * \brief Indicates whether shared data area was already initialized.
+ *
+ */
+static uint32_t shared_data_init_done;


please assign initial uninitialized value

alekshex · 2019-02-12T13:56:49Z

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_EMUL/psa_initial_attestation_api.c

+    psa_outvec out_vec[1] = { { token_size, sizeof(*token_size) } };
+
+    err = initial_attest_get_token_size(in_vec, 1, out_vec, 1);
+    if (err != PSA_ATTEST_ERR_SUCCESS)


no need, err returned anyway, unless you want additional handling

alekshex · 2019-02-12T14:03:52Z

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_IMPL/attest_boot_status_loader.c

+    offset  = (uintptr_t)BOOT_TFM_SHARED_DATA_BASE + (uintptr_t)SHARED_DATA_HEADER_SIZE;
+
+    /* Add header to output buffer as well */
+    if (len < SHARED_DATA_HEADER_SIZE)


what if len > header size?
is the idea that len must be equal?

len its the buffer lenght. it must be bigger/equal to all boot status data.
this check in line 78:
if (len < ptr_tlv_header->tlv_tot_len + tlv_entry->tlv_len)

alekshex · 2019-02-12T14:04:14Z

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_IMPL/attest_boot_status_loader.c

+    if (len < SHARED_DATA_HEADER_SIZE)
+    {
+        return PSA_ATTEST_ERR_INIT_FAILED;
+    } else


why do you need else if you have return before?

alekshex · 2019-02-12T14:04:56Z

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_IMPL/attest_crypto.c

+#include "tfm_plat_crypto_keys.h"
+#include <string.h>
+
+static psa_hash_operation_t hash_handle;


please init to NULL initially

alekshex · 2019-02-12T14:21:40Z

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_IMPL/attest_crypto_keys.c

+    crypto_ret = psa_open_key(PSA_KEY_LIFETIME_PERSISTENT, key_id, &handle);
+    if (crypto_ret != PSA_SUCCESS)
+    {
+        return TFM_PLAT_ERR_SYSTEM_ERR;


no crypto deinit?

I removed all calling to psa_crypto_init - we assume init called before crypto operations, if not an error will be return.

alekshex · 2019-02-12T14:23:20Z

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_IMPL/attest_iat_claims_loader.c

+#include "attestation.h"
+#include "crypto.h"
+
+extern int32_t g_caller_id;


must be initialized

no need in extern

alekshex · 2019-02-12T14:24:49Z

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_IMPL/attest_iat_claims_loader.c

+/* Hash of attestation public key */
+static enum tfm_plat_err_t attest_public_key_sha256(uint32_t *size, uint8_t *buf)
+{
+    const psa_key_id_t key_id = 17;


alekshex · 2019-02-12T14:25:07Z

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_IMPL/attest_iat_claims_loader.c

+
+    crypto_ret = psa_open_key(PSA_KEY_LIFETIME_PERSISTENT, key_id, &handle);
+    if (crypto_ret != PSA_SUCCESS) {
+        return TFM_PLAT_ERR_SYSTEM_ERR;


alekshex · 2019-02-12T14:30:06Z

...nts/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_IMPL/psa_inject_attestation_key_impl.c

+
+    status = psa_create_key(lifetime, key_id, &handle);
+    if (status != PSA_SUCCESS) {
+        return (status);


alekshex · 2019-02-12T14:47:53Z

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_IMPL/attest_crypto.c

+{
+    if (data_to_hash.ptr != NULL) {
+        psa_hash_update(&hash_handle, data_to_hash.ptr, data_to_hash.len);
+    } else {


why else needed?

This function caled by get attestation token and get attestation token size.
for the second purpose - data_to_hash(the payload) its null and no hash needed

alekshex · 2019-02-12T14:49:54Z

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_IPC/psa_attest_inject_key.c

+        public_key_data, public_key_data_size
+    };
+    out_vec[1] = (psa_outvec) {
+        public_key_data_length, sizeof(*public_key_data_length)


why sizeof?

is the idea here to have sizeof member pointed to?

alekshex · 2019-02-12T14:51:07Z

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_IPC/psa_initial_attestation_api.c

+    psa_outvec out_vec[1] = { { token, *token_size } };
+
+    handle = psa_connect(PSA_ATTEST_GET_TOKEN_ID, MINOR_VER);
+    if (handle <= 0)


0 is also illegal?

0xc0170

[ ] Functionality change

Please set PR type

If functionality change, I believe this one yes, will require release notes addition , see https://os.mbed.com/docs/mbed-os/v5.11/contributing/workflow.html#functionality-change

0xc0170 · 2019-02-15T13:13:41Z

.astyleignore

@@ -23,3 +23,4 @@ targets
 components/802.15.4_RF
 components/wifi
 tools
+components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_IMPL/tfm_impl


One note: please revert file permissions changes 100644 → 100755 ?

0xc0170 · 2019-02-15T13:13:55Z

TESTS/psa/attestation/main.cpp

@@ -0,0 +1,148 @@
+/*
+* Copyright (c) 2018 ARM Limited. All rights reserved.


0xc0170 · 2019-02-15T13:16:50Z

TESTS/psa/attestation/main.cpp

+    psa_key_handle_t handle = 0;
+    psa_open_key(PSA_KEY_LIFETIME_PERSISTENT, key_id, &handle);
+    psa_destroy_key(handle);
+    // mbedtls_psa_cr/ypto_free();


don't leave dead code behind

0xc0170 · 2019-02-15T13:17:18Z

components/TARGET_PSA/inc/psa/psa_attest_inject_key.h

+/*   These APIs are still evolving and are meant as a prototype for review.*/
+/*   The APIs will change depending on feedback and will be firmed up      */
+/*   to a stable set of APIs once all the feedback has been considered.    */
+/***************************************************************************/


Is this ready fro the release or kept here only during this review?

0xc0170 · 2019-02-15T13:17:50Z

targets/targets.json

@@ -1456,7 +1456,8 @@
            "KPSDK_CODE",
            "MCU_K64F",
            "Freescale_EMAC",
-            "PSA"
+            "PSA",


Same for this file, file permissions are being changed

0xc0170 · 2019-02-15T13:18:08Z

components/TARGET_PSA/services/attestation/COMPONENT_SPE/psa_attestation_partition.c

@@ -0,0 +1,230 @@
+// ---------------------------------- Includes ---------------------------------


missing license header

0xc0170 · 2019-02-20T14:42:39Z

@moranpeker Please let us know what has been update (PR status)

moranpeker · 2019-02-20T15:42:30Z

Hi @0xc0170 , the PR is rebased over master and ready for review.
@avolinski will review it too.

Can you please change the label to 'need: review" ? ( I can't change it )

alekshex

please see comments

alekshex · 2019-02-20T17:23:21Z

TESTS/psa/attestation/main.cpp

+{
+#if (defined(COMPONENT_PSA_SRV_IPC) || defined(MBEDTLS_ENTROPY_NV_SEED))
+    uint8_t seed[MBEDTLS_PSA_INJECT_ENTROPY_MIN_SIZE] = {0};
+    /* inject some a seed for test*/


/* inject some seed for test*/

alekshex · 2019-02-20T17:23:41Z

TESTS/psa/attestation/main.cpp

+        seed[i] = i;
+    }
+
+    /* don't really care if this succeed this is just to make crypto init pass*/


/* don't really care if this succeeds this is just to make crypto init pass*/

alekshex · 2019-02-20T17:24:41Z

TESTS/psa/spm_client/COMPONENT_SPE/psa_setup.c

 extern const uint32_t crypto_srv_external_sids[4];
 extern const uint32_t platform_external_sids[1];

-spm_partition_t g_partitions[4] = {
+spm_partition_t g_partitions[5] = {


why not in define?

psa_setup is generated file by calling 'tools/psa/generate_tfm_partition_code.py' script

That script could just omit the size, if only to avoid such comments.

alekshex · 2019-02-20T17:24:55Z

TESTS/psa/spm_server/COMPONENT_SPE/psa_setup.c

 extern const uint32_t crypto_srv_external_sids[4];
 extern const uint32_t platform_external_sids[1];

-spm_partition_t g_partitions[5] = {
+spm_partition_t g_partitions[6] = {


psa_setup is generated file by calling 'tools/psa/generate_tfm_partition_code.py' script

alekshex · 2019-02-20T17:25:08Z

TESTS/psa/spm_smoke/COMPONENT_SPE/psa_setup.c

 extern const uint32_t crypto_srv_external_sids[4];
 extern const uint32_t platform_external_sids[1];

-spm_partition_t g_partitions[4] = {
+spm_partition_t g_partitions[5] = {


psa_setup is generated file by calling 'tools/psa/generate_tfm_partition_code.py' script

alekshex · 2019-02-20T19:23:13Z

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_IMPL/attest_iat_claims_loader.c

+                                 *size,
+                                 (size_t *) size);
+    if (crypto_ret != PSA_SUCCESS) {
+        free(public_key);


too many same kind of operations: free, close
please consider goto with appropriate error code

alekshex · 2019-02-20T19:25:34Z

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_IMPL/attest_iat_claims_loader.c

+ * \param[in]   p_src  Pointer to the ID
+ * \param[in]   size   Length of the ID
+ */
+static inline void copy_id(uint8_t *p_dst, uint8_t *p_src, size_t size)


code duplication, copy_key can be used here also.
just name it copy_src_to_dst

And if the ptr is uint8*, why aren't you using memcpy??

alekshex · 2019-02-20T19:26:20Z

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_IMPL/attestation_bootloader_data.c

+
+#include "attestation_bootloader_data.h"
+
+/* Temporary Boodloader data - conatians temp mandatory claims */


alekshex · 2019-02-20T19:27:13Z

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_IMPL/psa_attestation_stubs.c

+        return PSA_ATTEST_ERR_INVALID_INPUT;
+    }
+    return PSA_ATTEST_ERR_SUCCESS;
+}


alekshex · 2019-02-20T19:28:12Z

...nts/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_IMPL/psa_inject_attestation_key_impl.c

+
+    status = psa_create_key(lifetime, key_id, &handle);
+    if (status != PSA_SUCCESS) {
+        return (status);


no crypto deinit?

-initialized psa_hash_operation_t to zeros -fix TF-M attestation code - increase t_cose_crypto_hash bytes to handle max psa_hash_operation_t size

from Apache-2.0 to BSD-3-Clause

0xc0170 · 2019-02-28T14:37:07Z

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_IMPL/attest_crypto.c

+ *
+ * Copyright (c) 2018-2019, Laurence Lundblade. All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause


just checking, this attestation is already in Readme covered as non apache file and license file present in the attestation folder?

Answer: yes, I've just checked

0xc0170 · 2019-02-28T14:39:12Z

@moranpeker Can you review build failures? Latest commits are fixing these or still to do?

moranpeker · 2019-02-28T14:41:58Z

@moranpeker Can you review build failures? Latest commits are fixing these or still to do?

last CI run failed because I asked from @NirSonnenschein to stop the running for new commits I pushed.

mbed-ci · 2019-02-28T18:33:52Z

Test run: FAILED

Summary: 1 of 9 test jobs failed
Build number : 6
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-IAR8

mikisch81 · 2019-02-28T20:28:01Z

@cmonr was the last CI run aborted? If so can it restart?

cmonr · 2019-02-28T20:52:25Z

@mikisch81 It's because a new commit was added to the PR, thus changing the HEAD's sha.
Will restart.

last CI run failed because I asked from @NirSonnenschein to stop the running for new commits I pushed.

cmonr · 2019-02-28T20:54:36Z

CI started

mbed-ci · 2019-02-28T23:05:52Z

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 7
Build artifacts

cmonr · 2019-03-01T00:58:24Z

@moranpeker @orenc17 I want to confirm something.

https://travis-ci.org/ARMmbed/mbed-os/jobs/500094537

Does this PR modify what is generated in this test?

cmonr · 2019-03-01T01:00:28Z

CC @ARMmbed/mbed-os-maintainers

mikisch81 · 2019-03-01T06:50:11Z

@NirSonnenschein @cmonr @0xc0170
There was apperently a race between this PR and another PR which re-generated code.
Therefore for each PR the psa_autogen job passed, but after they were merged the next PRs failed.
We need to call python tools/psa/generate_mbed_spm_partition_code.py on master in order to regenerate again.

mikisch81 · 2019-03-01T06:51:23Z

I can do it when I will be near a PC, but if anyone can do it earlier it's to the best

mikisch81 · 2019-03-01T07:04:20Z

Also python tools/psa/generate_tfm_partition_code.py needs to be called.

0xc0170 · 2019-03-01T07:17:22Z

Thanks @mikisch81 !

mikisch81 · 2019-03-01T09:00:32Z

#9899 will fix the Travis issue

adbridge · 2019-03-08T13:59:03Z

@moranpeker According to @teetak01 " this breaks backwards compatibility with Nucleo F411RE entropy injection.". Can you please add this to the release notes section and change the type to breaking change rather than functionality change.

moranpeker · 2019-03-10T15:16:25Z

@moranpeker According to @teetak01 " this breaks backwards compatibility with Nucleo F411RE entropy injection.". Can you please add this to the release notes section and change the type to breaking change rather than functionality change.

Done

ciarmcom requested a review from a team February 11, 2019 22:00

ciarmcom added the needs: review label Feb 11, 2019

cmonr reviewed Feb 12, 2019

View reviewed changes

cmonr requested a review from a team February 12, 2019 00:10

cmonr previously requested changes Feb 12, 2019

View reviewed changes

alekshex reviewed Feb 12, 2019

View reviewed changes

0xc0170 previously requested changes Feb 15, 2019

View reviewed changes

0xc0170 reviewed Feb 15, 2019

View reviewed changes

cmonr added needs: work and removed needs: review labels Feb 15, 2019

moranpeker force-pushed the psa-init-attestation branch from c84ecb3 to b1bd662 Compare February 17, 2019 14:32

NirSonnenschein added the release-version: 5.12.0-rc1 label Feb 20, 2019

moranpeker force-pushed the psa-init-attestation branch from 7fb66a9 to 1808bf2 Compare February 20, 2019 14:14

cmonr added needs: review and removed needs: work labels Feb 20, 2019

alekshex reviewed Feb 20, 2019

View reviewed changes

0xc0170 added the needs: work label Feb 21, 2019

Moran Peker added 2 commits February 28, 2019 15:54

initialized psa_hash_operation_t to zeros before calling hash_setup

2b8621a

-initialized psa_hash_operation_t to zeros -fix TF-M attestation code - increase t_cose_crypto_hash bytes to handle max psa_hash_operation_t size

change license to file that mostly based on TF-M implementation

116e14a

from Apache-2.0 to BSD-3-Clause

0xc0170 reviewed Feb 28, 2019

View reviewed changes

0xc0170 added needs: work and removed needs: CI labels Feb 28, 2019

mikisch81 mentioned this pull request Feb 28, 2019

mbed-SPM updates #9823

Merged

add casting to increased void pointer

83d084a

cmonr added needs: CI risk: A and removed needs: work risk: R labels Feb 28, 2019

cmonr merged commit befed11 into ARMmbed:master Mar 1, 2019

cmonr removed the needs: CI label Mar 1, 2019

adbridge added the BREAKING-CHANGE label Mar 8, 2019

		@@ -0,0 +1,148 @@
		/*
		* Copyright (c) 2018 ARM Limited. All rights reserved.

		@@ -0,0 +1,230 @@
		// ---------------------------------- Includes ---------------------------------


		#include "attestation_bootloader_data.h"

		/* Temporary Boodloader data - conatians temp mandatory claims */

PSA Initial Attestation service #9668

PSA Initial Attestation service #9668

Uh oh!

Conversation

moranpeker commented Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Reviewers

Release Notes

Uh oh!

ciarmcom commented Feb 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmonr commented Feb 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmonr 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

Uh oh!

cmonr commented Feb 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

alekshex left a comment

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!

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!

moranpeker commented Feb 11, 2019 •

edited

Loading

cmonr commented Feb 12, 2019 •

edited

Loading

cmonr left a comment •

edited

Loading

cmonr commented Feb 12, 2019 •

edited

Loading