-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[RFC] POC Remote Attestation #4011
base: master
Are you sure you want to change the base?
Conversation
a97f2ef
to
4d2d616
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HI @elias-vd ,
Thanks for this RFC code. Remote attestation is certainly an interesting use case for a TEE.
Please see some initial comments below. Also if we merge this new feature will want at least some basic sanity testing in xtest
too. optee_examples
is fine for examples, but it is xtest
that we run in CI and during release testing. But that can be addressed later obviously since this is just an RFC.
@@ -280,6 +281,11 @@ static TEE_Result system_open_ta_binary(struct system_ctx *ctx, | |||
goto err_oom; | |||
params[0].value.a = h; | |||
|
|||
#ifdef CFG_ATTESTATION_PTA | |||
utc = to_user_ta_ctx(tee_ta_get_calling_session()->ctx); | |||
memcpy(utc->ta_image_sha256, tag, tag_len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assumes the TA does not use shared libraries. Otherwise system_open_ta_binary()
is called several times with the same context. First to load the TA main executable, then to load additional libraries. So ta_image_sha256
would be overwritten by the hash of the last library.
An upcall into ldelf
might be required to address this issue properly. @jenswi-linaro?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for taking the time to look at my POC
I am completely aware of the fact that my POC only works if the TA do not load any libraries. But in our use case it is not intended to load any libraries. Nevertheless, if OPTEE want to offer remote attestation it has to offer a mechanism to measure a TA.
To throw in my thoughts about this topic:
- From the point of view of remote attestation, it does not matter what is loaded, as long it is measured. This goes further to the point, that we would like to make our key in optee_os/keys/default_ta.pem publicly available. So that anyone can load TAs, but also everyone (remote) can attest the TA if he knows the corresponding hashes.
- To my (limited) understanding of the OPTEE OS, I think remote attestation is best implemented as a pseudo TA. Then anyone can implement the protocol he wants. But the OS has to offer three things to the PTA.
- Endorsement Key: Access to this key is hardware dependent and not part of the OPTEE project
- System measurement: The PTA needs access to the measurements of Secure Boot, this is also platform specific
- TA measurement: The TA measurement must be deterministic. I think this is problematic with the current situation. The main problem that I see, is the order in which the libraries getting loaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could instead of copying the tag, hash it together with previous value. No TA code will be executed until all libraries are loaded so there's no risk of executing with an incomplete digest. But this is only the easy part, the hard part comes with dlopen()
and friends. See #3189 (comment)
However, if the TA doesn't use dlopen()
at all it shouldn't be a problem.
core/pta/attestation.c
Outdated
|
||
TEE_Result res = TEE_ERROR_GENERIC; | ||
|
||
typedef struct attestation_certificate { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No typedef please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix here
core/pta/attestation.c
Outdated
size_t rn_len = 32; | ||
uint8_t rn[rn_len]; | ||
|
||
res = get_random_number(rn); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use crypto_rng_read()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix here
core/pta/attestation.c
Outdated
{ | ||
|
||
uint32_t size; | ||
uint32_t offset = 7; // Beginn at the start of the modulus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We normally don't use C++-style comment in OP-TEE (except for the SPDX tag in .c
files.
What do others think? Should we accept them now? (they are allowed in the Linux kernel apparently).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We normally don't use C++-style comment in OP-TEE (except for the SPDX tag in
.c
files.
What do others think? Should we accept them now? (they are allowed in the Linux kernel apparently).
Do you have a link?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://lkml.org/lkml/2019/6/27/117
And there this older (2016) rant from Linus https://lkml.org/lkml/2016/7/8/625 where he says he is favorable to C++-style comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Speaking for myself here:
I guess I could "tolerate" them in .c files, but I still think they should be avoided.
In my mind those are lazy comments that you add as reminders for yourself to be cleaned up before submitting the patch.
So sure you can use them, but each time I see one I'll be thinking "so you don't have time to write a proper comment?". ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jenswi-linaro +1, the power of habits I suppose :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I admit it's bit due to habit. But there's at least one technical reason to avoid them. They cannot be used in .h files included by assembly files, so there are places where they are inappropriate. Why not use comments that are acceptable everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix here
core/pta/attestation.c
Outdated
uint8_t **d, size_t *d_size) | ||
{ | ||
|
||
uint32_t size; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
= 0;
(coding rule, all local variables must be initialized)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix here
return TEE_ERROR_SECURITY; | ||
|
||
if (priv[offset + 1] != 0x82) | ||
return TEE_ERROR_SECURITY; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (priv[0] != 0x30 || priv[1] != 0x82 || priv[offset] != 0x02 || priv[offset + 1] != 0x82)
return TEE_ERROR_SECURITY;
What are the guarantees that accessing priv[x]
is valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix here
att_data.sig_size = p[3].memref.size; | ||
|
||
// Allocating the memory for the Attestation data | ||
att_data.iv = calloc(att_data.iv_size, sizeof(uint8_t)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Memory leak if set_data()
is called several times?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I will address that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix here
core/pta/attestation.c
Outdated
return TEE_SUCCESS; | ||
} | ||
|
||
static TEE_Result decrypt_priv_key(uint8_t *plain, size_t *plain_size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some platforms may have endorsement keys embedded in the SoC itself. In those scenarios it may be useful to provide this function as a weak function which platforms can implement at their end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for reviewing my PR.
This function decrypts the attestation identifier key (AIK), which is stored in the attestation data struct (Image here). So this function does not decrypt the endorsement key this is done by the get_endorsement_key
function. And there jfossier already requested the __weak
symbol. Therefore I do not think the weak symbol is necessary here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I thought about it again and after some research I understand your suggestion. You are absolutely right, I think it is the preferred solution to replace the decrypt_priv_key()
function and ditch the get_endorsementkey()
function. This will also keep the endorsement key in hardware.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues, I think you understand now what I was trying to say. There are hardwares where endorsement key never leaves the SoC. In such cases, SoC vendors may want to override the complete decrypt_priv_key function as per their requirements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix here
core/pta/attestation.c
Outdated
|
||
TEE_Result __weak get_endorsement_key(uint8_t *key) | ||
{ | ||
// TODO: Dummy function get_endorsement_key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could huk_subkey_derive()
with a new enum huk_subkey_usage
be used for this?
It may still make sense to keep this function __weak
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it could. But the preferred way would be to keep the endorsement key in hardware, as discussed with ruchi393. In case, there is no appropriate hardware for keeping the key in hardware this is a viable option. But I do not like the idea to include it here, because the stuff with the endorsement key is always hardware dependent and you must think about where to get your endorsement key from.
core/pta/attestation.c
Outdated
TEE_Result __weak get_sys_measurement(uint8_t *ptr) | ||
{ | ||
// TODO: Dummy function | ||
// get_sys_measurement -> do something hardware specific |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Measurement as in measured boot in case you have a TPM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly. But we do not have a TPM in mind. We would prefer to provision the Chips (Processor or FPGA) and then boot them with secure boot/HAB into a secure state.
core/pta/attestation.h
Outdated
* Dummy functions makred as __weak, | ||
* some hardware specific hooks needed. | ||
*/ | ||
TEE_Result get_sys_measurement(uint8_t *ptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These functions need a prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix here.
{ 0xa2b0b139, 0x82dc, 0x4ffc, \ | ||
{ 0xa8, 0xa8, 0x7d, 0x7c, 0x63, 0x66, 0xe9, 0x84 } } | ||
|
||
#define TA_NAME "attestation.pta" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't needed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix here.
crypto_cipher_final(ctx); | ||
*plain_size = enc_size - plain[enc_size - 1]; | ||
|
||
return res; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Memory leakage, ctx
isn't freed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix here
core/pta/attestation.c
Outdated
|
||
res = crypto_hash_init(ctx); | ||
if (res) | ||
return res; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Memory leakage, ctx
isn't freed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think, I free it with crypto_hash_free_ctx(ctx);
on line 239.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not when returning here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I got it. That is the reason for this goto err:
structure you are using in the core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix here
core/pta/attestation.c
Outdated
|
||
res = pub_asn1_decode(&n_ptr, &e_ptr, &n_size, &e_size); | ||
if (res != TEE_SUCCESS) | ||
return res; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Memory leakage, key_pair
isn't freed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please correct me if I am wrong, but there is no function like crypto_acipher_free_rsa_keypair
. There is only one for the public key. So I have to free each bignum seperatly?
return res; | |
crypto_bignum_free(key_pair->e); | |
crypto_bignum_free(key_pair->d); | |
crypto_bignum_free(key_pair->n); | |
crypto_bignum_free(key_pair->p); | |
crypto_bignum_free(key_pair->q); | |
crypto_bignum_free(key_pair->qp); | |
crypto_bignum_free(key_pair->dp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because that function hasn't been needed yet. Feel free to add one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix here, but the crypto_acipher_free_rsa_keypair
problem, I will adress it tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crypto_acipher_free_rsa_keypair
#4028
core/pta/attestation.c
Outdated
return res; | ||
|
||
// decode the pubkey (AIK) | ||
struct rsa_public_key key; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please declare local variables at the beginning of a code block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix here.
core/pta/attestation.c
Outdated
|
||
res = decrypt_priv_key(plain_priv, &plain_priv_len); | ||
if (res) | ||
return res; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Memory leakage, plain_priv
isn't freed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix here
Greetings, It is nearly a month without comments. So I suggest to close the PR and the ones associated with it. I have to direct my energy to different projects. The repos, I will keep them on Githup for the future. Thank you all for your comments and if anyone in the future wants to ask questions or just talk about it, feel free to send me an email. |
@elias-vd, the lack of comments is could very well be due to no one noticing that you made updates. Sometime notifications aren't sent out when new patches are added. So adding some short comment each time after you've updated helps getting attention. |
In order to make progress here, please squash in all the fixes to their parent commit and rebase on master. |
before we close this PR, I'd consider taking this over from @elias-vd (who was on my team when he did this work). |
Please do, I personally think remote attestation is a very useful feature and building block for greater security solutions. I'd like to be part of this discussion, but I've too many other things going on right now. Which is a challenge for everyone here I suppose. |
awaiting permission from @elias-vd for his branch |
This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time. |
I intend to rebase, don't close |
This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time. |
I have resolved the first few conflicts. Will observe CI |
This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time. |
@jenswi-linaro I'll need some help with interpreting CI trouble. A had resolved conflicts a while ago but CI checks fail.... |
Please squash and rebase on master. That should hopefully take care of the CI errors. |
32e9870
to
08e2b5c
Compare
I don't get it:
the squashed commit contains that line |
That means that you need to add your Signed-off-by tag too. |
08e2b5c
to
e0f11dc
Compare
As you can see in the first two lines, the text after the |
Elias is no longer at SCS, that's the reason for the authorship email change. @elias-vd may I ask you to reset the commit authorship to your bluewin address as suggested above? |
This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time. |
I'll reach out to @elias-vd again.... |
ebae9fb
to
e86173a
Compare
3b50b06
to
586e144
Compare
Signed-off-by: E.von Däniken <elias.vondaeniken@bluewin.ch> Signed-off-by: Alain Brenzikofer <alain.brenzikofer@scs.ch>
586e144
to
af4063b
Compare
What is the current for this PR? |
We need to do a bit of work to keep up with changes after the latest rebase breaking our build. I currently can't look into it but I'll reach out to @elias-vd. |
OK, please let us know with an explicit comment in this PR when it should be reviewed again. |
After discussing with @elias-vd we think it would be easy to make the build pass again, but we wonder how big the chances are that this PR will ever be merged and the feature maintained. We are currently not able to maintain this or provide a CI test suite. So, if the OP-TEE core team is motivated to merge this PR and if there is someone to maintain remote attestation, we're happy to do the finishing on this PR. But without such commitment we have to let this go. |
I apology for not having spent more time on this. Why is the PTA such complex? I like the idea of having a fingerprint of the executed TA. @jenswi-linaro may say a work on this. It could be part of the instance context. Format of attestation data and certif is a concern. If this contribution really on a substratee format, we should name this substratee_ta_attestion or find a generic way for various client frameworks. It would ease to clearly describe the APIs of the TA (optee_examples) and PTA (optee_os). This really need to go through a review process to land, unless what we can't maintain unread untested materials. Sorry for the delay, we all have things to do :) |
I agree with @etienne-lms, the idea is sound. But it's only worth doing if we can do it properly. |
@jenswi-linaro understood. Then I'd like to make it clear that we will currently only be able to do minor changes. Your review would be highly welcome, but I fear we (our company scs) won't invest more into this than we already did until we have a paying customer project requiring it. (We are actually in the middle of founding a spinoff which will certainly be interested in RA for TrustZone! But it may take a moment) |
I see, perhaps it's best to let this rest until you can commit to see this through. |
This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time. |
I see a potential to dedicate resources to this 1-2 months from now. posting to keep it alive |
Added enhancement label to keep it alive. |
Contains the PTA for the example of remote attestation.
Conceptual overview: https://www.substratee.com/hw_ra_by_distributor.html
Linked PRs:
Signed-off-by: Elias von Daeniken elias.vondaeniken@bluewin.ch