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

PSA Initial Attestation service #9668

Merged
merged 56 commits into from Mar 1, 2019

Conversation

@moranpeker
Copy link
Contributor

commented Feb 11, 2019

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 ciarmcom requested a review from ARMmbed/mbed-os-maintainers Feb 11, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

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

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

This comment has been minimized.

Copy link
@cmonr

cmonr Feb 12, 2019

Contributor

👍 for the commit comment about this.

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Feb 15, 2019

Member

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

@cmonr cmonr requested a review from ARMmbed/mbed-os-psa Feb 12, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

Checked over K64F and PSOC6.

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

@cmonr
Copy link
Contributor

left a comment

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

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2019

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

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_EMUL/psa_attest_inject_key.c Outdated
*
* SPDX-License-Identifier: BSD-3-Clause
*
*/

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 12, 2019

Contributor

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.
    */

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 12, 2019

Contributor

2018-2019

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 12, 2019

Contributor

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

return err;
}

*token_size = out_vec[0].len;

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 12, 2019

Contributor

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

@avolinski
Copy link
Contributor

left a comment

first pass

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_IMPL/attest_boot_status_loader.c Outdated
* \brief Indicates whether shared data area was already initialized.
*
*/
static uint32_t shared_data_init_done;

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 12, 2019

Contributor

please assign initial uninitialized value

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_EMUL/psa_initial_attestation_api.c Outdated
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)

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 12, 2019

Contributor

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

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)

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 12, 2019

Contributor

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

This comment has been minimized.

Copy link
@moranpeker

moranpeker Feb 25, 2019

Author Contributor

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)

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_IMPL/attest_boot_status_loader.c Outdated
if (len < SHARED_DATA_HEADER_SIZE)
{
return PSA_ATTEST_ERR_INIT_FAILED;
} else

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 12, 2019

Contributor

why do you need else if you have return before?

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_IMPL/attest_crypto.c Outdated
#include "tfm_plat_crypto_keys.h"
#include <string.h>

static psa_hash_operation_t hash_handle;

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 12, 2019

Contributor

please init to NULL initially

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

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 12, 2019

Contributor

no crypto deinit?

This comment has been minimized.

Copy link
@moranpeker

moranpeker Feb 25, 2019

Author Contributor

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

#include "attestation.h"
#include "crypto.h"

extern int32_t g_caller_id;

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 12, 2019

Contributor

must be initialized

This comment has been minimized.

Copy link
@moranpeker

moranpeker Feb 13, 2019

Author Contributor

no need in extern

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_IMPL/attest_iat_claims_loader.c Outdated
/* 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;

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 12, 2019

Contributor

...


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

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 12, 2019

Contributor

..


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

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 12, 2019

Contributor

deinit

This comment has been minimized.

Copy link
@moranpeker

moranpeker Feb 25, 2019

Author Contributor

same.

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_IMPL/attest_crypto.c Outdated
{
if (data_to_hash.ptr != NULL) {
psa_hash_update(&hash_handle, data_to_hash.ptr, data_to_hash.len);
} else {

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 12, 2019

Contributor

why else needed?

This comment has been minimized.

Copy link
@moranpeker

moranpeker Feb 13, 2019

Author Contributor

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

components/TARGET_PSA/services/attestation/COMPONENT_PSA_SRV_IPC/psa_attest_inject_key.c Outdated
public_key_data, public_key_data_size
};
out_vec[1] = (psa_outvec) {
public_key_data_length, sizeof(*public_key_data_length)

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 12, 2019

Contributor

why sizeof?

This comment has been minimized.

Copy link
@moranpeker

moranpeker Feb 13, 2019

Author Contributor

why not ?

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 21, 2019

Contributor

is the idea here to have sizeof member pointed to?

This comment has been minimized.

Copy link
@moranpeker

moranpeker Feb 25, 2019

Author Contributor

yes

psa_outvec out_vec[1] = { { token, *token_size } };

handle = psa_connect(PSA_ATTEST_GET_TOKEN_ID, MINOR_VER);
if (handle <= 0)

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 12, 2019

Contributor

0 is also illegal?

This comment has been minimized.

Copy link
@moranpeker

moranpeker Feb 13, 2019

Author Contributor

right

@0xc0170
Copy link
Member

left a comment

[ ] 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

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

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Feb 15, 2019

Member

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

TESTS/psa/attestation/main.cpp Outdated
@@ -0,0 +1,148 @@
/*
* Copyright (c) 2018 ARM Limited. All rights reserved.

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Feb 15, 2019

Member

2019 now?

TESTS/psa/attestation/main.cpp Outdated
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();

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Feb 15, 2019

Member

don't leave dead code behind

components/TARGET_PSA/inc/psa/psa_attest_inject_key.h Outdated
/* 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. */
/***************************************************************************/

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Feb 15, 2019

Member

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

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

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Feb 15, 2019

Member

Same for this file, file permissions are being changed

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

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Feb 15, 2019

Member

missing license header

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

@moranpeker moranpeker force-pushed the moranpeker:psa-init-attestation branch Feb 17, 2019

@moranpeker moranpeker force-pushed the moranpeker:psa-init-attestation branch to 1808bf2 Feb 20, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

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

@moranpeker

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

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 )

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

@avolinski
Copy link
Contributor

left a comment

please see comments

{
#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*/

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 20, 2019

Contributor

/* inject some seed for test*/

seed[i] = i;
}

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

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 20, 2019

Contributor

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

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] = {

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 20, 2019

Contributor

why not in define?

This comment has been minimized.

Copy link
@moranpeker

moranpeker Feb 25, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Feb 25, 2019

Contributor

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

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] = {

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 20, 2019

Contributor

def

This comment has been minimized.

Copy link
@moranpeker

moranpeker Feb 25, 2019

Author Contributor

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

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] = {

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 20, 2019

Contributor

def

This comment has been minimized.

Copy link
@moranpeker

moranpeker Feb 25, 2019

Author Contributor

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

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

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 20, 2019

Contributor

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

* \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)

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 20, 2019

Contributor

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??


#include "attestation_bootloader_data.h"

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

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 20, 2019

Contributor

contains

return PSA_ATTEST_ERR_INVALID_INPUT;
}
return PSA_ATTEST_ERR_SUCCESS;
}

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 20, 2019

Contributor

legal eof?


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

This comment has been minimized.

Copy link
@avolinski

avolinski Feb 20, 2019

Contributor

no crypto deinit?

This comment has been minimized.

Copy link
@moranpeker

moranpeker Feb 25, 2019

Author Contributor

same

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

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

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Feb 28, 2019

Member

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

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Feb 28, 2019

Member

Answer: yes, I've just checked

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

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

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

@moranpeker

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

@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.

@mikisch81 mikisch81 referenced this pull request Feb 28, 2019

Merged

mbed-SPM updates #9823

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 28, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

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

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 28, 2019

Test run: SUCCESS

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

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

28 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARMC5 Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARMC5 Success
Details
jenkins-ci/mbed2-build-ARMC6 Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR8 Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 10413 cycles (+1213 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the needs: CI label Mar 1, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

@mikisch81

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

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

@mikisch81

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

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

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

Thanks @mikisch81 !

@mikisch81

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

#9899 will fix the Travis issue

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2019

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.