[RFC] Add framework to support REE-FS encrypted TAs #3340

b49020 · 2019-10-23T10:10:56Z

Add framework to support loading of encrypted TAs from REE-FS using symmetric authenticated encryption scheme supported by OP-TEE.

This framework only support buffered REE-FS TAs as encrypted TA needs to be read at once and decrypted into temporary secure memory buffer.

The default encryption key is derived from hardware unique key which can be overridden via platform specific encryption key.

Also it adds support in TA dev kit to encrypt as well as sign TA in case corresponding TA build time configuration option CFG_ENCRYPT_TA=y is enabled.

Looking forward to your feedback/comments.

-Sumit

jforissier

Hi Sumit,

Thanks for proposing this. TA encryption has been requested on several occasions already, and this proposal is a good start I think. Please see my comments below.

ta/mk/ta_dev_kit.mk

core/arch/arm/kernel/ree_fs_ta.c

ta/arch/arm/link.mk

jforissier · 2019-10-23T15:35:53Z

Assuming all TAs need to be encrypted with the same key might be a bit too restrictive? I know we have the same limitation for the signature already so maybe it's OK.

core/arch/arm/kernel/ree_fs_ta.c

core/include/signed_hdr.h

ta/mk/ta_dev_kit.mk

core/arch/arm/kernel/otp_stubs.c

b49020 · 2019-10-24T08:05:36Z

Assuming all TAs need to be encrypted with the same key might be a bit too restrictive? I know we have the same limitation for the signature already so maybe it's OK.

What use-cases do you foresee to use a unique encryption key per TA?

jforissier · 2019-10-24T09:26:57Z

Assuming all TAs need to be encrypted with the same key might be a bit too restrictive? I know we have the same limitation for the signature already so maybe it's OK.

What use-cases do you foresee to use a unique encryption key per TA?

For example. TA developers are usually not the ones who develop the TEE, they are third-parties. Maybe developer A does not want developer B to be able to decrypt his TA?

b49020 · 2019-10-24T10:23:57Z

Assuming all TAs need to be encrypted with the same key might be a bit too restrictive? I know we have the same limitation for the signature already so maybe it's OK.

What use-cases do you foresee to use a unique encryption key per TA?

For example. TA developers are usually not the ones who develop the TEE, they are third-parties. Maybe developer A does not want developer B to be able to decrypt his TA?

I think to address this case, first step would be to authenticate a TA to a particular third party then only based on that identity encryption key should be provided by platform. IOW, we need some framework for multiple signing keys. Then we should be able to extend the abstraction layer to pass a TA identity while requesting for encryption key.

b49020 · 2019-10-25T09:42:18Z

Updated PR. The reason for IBART and Shippable seems to be due to missing python package required for authenticated encryption. So can we update ci via installing following package:

sudo pip3 install pycryptodomex

scripts/sign_encrypt.py

b49020 · 2019-10-25T10:07:29Z

The reason for IBART and Shippable seems to be due to missing python package required for authenticated encryption.

BTW, I have changed to include Cryptodome package if we have enabled TA to be encrypted. So that we don't add a dependency for existing sign only use-cases.

jenswi-linaro · 2019-10-25T10:45:04Z

Using a fixed symmetric key for TA decryption and validation is a bit inflexible. I would have expected some public key scheme, like https://en.wikipedia.org/wiki/Pretty_Good_Privacy, to handle the symmetric key used to encrypt the TA.

I realize that this could be a step in that direction. However, there's one problem. The format of a TA is to be considered part of the ABI, which we always try to avoid breaking. So once we introduce a new TA format we're stuck with it. If we introduce various encrypted TA formats with baby steps we'll accumulate a bit of a baggage before we have something truly useful.

b49020 · 2019-10-25T12:50:07Z

Using a fixed symmetric key for TA decryption and validation is a bit inflexible. I would have expected some public key scheme, like https://en.wikipedia.org/wiki/Pretty_Good_Privacy, to handle the symmetric key used to encrypt the TA.

I would start by asking you a question. What value do you see for a random symmetric key per TA if the receiver is common i.e. the device with a single asymmetric key? I would rather see public key scheme adding inflexible layer as follows:

Performance overhead of asymmetric decryption during loading of TAs. Some may not want to use it.
How would this handle per device unique encryption key case?
How can we use encryption key provisioned in the hardware crypto accelerator?

In this PR, I have intentionally left handling of encryption keys for the platform to handle which rather I think would provide OEMs the flexibility to use customised interfaces as per their security requirements.

And for the validation part of TA, if you look carefully it rather uses asymmetric signing key. The reason to use authenticated encryption is just to provide additional integrity of encrypted blob.

I realize that this could be a step in that direction. However, there's one problem. The format of a TA is to be considered part of the ABI, which we always try to avoid breaking. So once we introduce a new TA format we're stuck with it. If we introduce various encrypted TA formats with baby steps we'll accumulate a bit of a baggage before we have something truly useful.

I think we need to start with one at-least :) which should provide enough abstraction to cater to many real world use-cases so that the ABI is the least affected.

core/arch/arm/kernel/ree_fs_ta.c

jenswi-linaro · 2019-11-04T07:57:38Z

What value do you see for a random symmetric key per TA if the receiver is common i.e. the device with a single asymmetric key?

It doesn't have any value, in fact I think that for OP-TEE symmetric encryption would probably always be preferable.

However, this scheme is still a bit inflexible. How about extending it a bit? I propose to add a hash field and a flags field in struct shdr_encrypted_ta. The TA encryption key will be derived from the added hash field and the hash of the public key which was used to verify the TA. This way we'll have per SP symmetric encryption keys when needed. The flags field tells if it's a device unique or class wide TA encryption key which should be used.

b49020 · 2019-11-04T10:21:21Z

The TA encryption key will be derived from the added hash field and the hash of the public key which was used to verify the TA. This way we'll have per SP symmetric encryption keys when needed.

Wouldn't this add an additional overhead to derive the encryption key while loading every TA?

I think SP specific encryption key should be derived at once only either during boot time (SPs supported by device are fixed like in the current case where we support only single SP) or dynamically at run-time whenever a new security domain is created to support a particular SP.

And during TA load, we only need to provide the identity of SP to which TA belongs to fetch corresponding encryption key which can be added once multiple SPs are supported in OP-TEE.

BTW, if you agree, in current implementation I can add hash of the public key which is used to verify TA as a salt during derivation of default TA encryption key at boot-time.

The flags field tells if it's a device unique or class wide TA encryption key which should be used.

Wouldn't this be decided at the time of provisioning if the device needs to be provisioned with device unique or class wide TA encryption key? Which scenarios do you think where a device would be provisioned with both type of keys?

jenswi-linaro · 2019-11-04T11:06:07Z

The TA encryption key will be derived from the added hash field and the hash of the public key which was used to verify the TA. This way we'll have per SP symmetric encryption keys when needed.

Wouldn't this add an additional overhead to derive the encryption key while loading every TA?

It's just a few hashes which are calculated, I don't think this is a performance issue. It's also more safe to avoid keeping the TA encryption key in clear text in memory more than necessary.

The flags field tells if it's a device unique or class wide TA encryption key which should be used.

Wouldn't this be decided at the time of provisioning if the device needs to be provisioned with device unique or class wide TA encryption key? Which scenarios do you think where a device would be provisioned with both type of keys?

I can't see any strong use cases at the moment, but let's turn that around. Can we be 100% sure that no such use cases can be valid? What are the drawbacks with selecting it in the TA header? It improves security a bit since it's easy to tell if a TA is encrypted with a device specific key or not.

b49020 · 2019-11-04T11:40:42Z

The TA encryption key will be derived from the added hash field and the hash of the public key which was used to verify the TA. This way we'll have per SP symmetric encryption keys when needed.

Wouldn't this add an additional overhead to derive the encryption key while loading every TA?

It's just a few hashes which are calculated, I don't think this is a performance issue. It's also more safe to avoid keeping the TA encryption key in clear text in memory more than necessary.

Sounds reasonable, will add derivation at TA load time. BTW, can you elaborate regarding purpose of hash field in struct shdr_encrypted_ta, since we already have public key embedded in tee.bin whose hash can be calculated at run-time? Is it just a flag to tell whether to include hash as salt or not?

The flags field tells if it's a device unique or class wide TA encryption key which should be used.

Wouldn't this be decided at the time of provisioning if the device needs to be provisioned with device unique or class wide TA encryption key? Which scenarios do you think where a device would be provisioned with both type of keys?

I can't see any strong use cases at the moment, but let's turn that around. Can we be 100% sure that no such use cases can be valid? What are the drawbacks with selecting it in the TA header? It improves security a bit since it's easy to tell if a TA is encrypted with a device specific key or not.

I don't see any drawbacks, will add this flag.

jforissier · 2019-11-04T12:03:37Z

I propose to add a hash field and a flags field in struct shdr_encrypted_ta. The TA encryption key will be derived from the added hash field and the hash of the public key which was used to verify the TA.

Would it be derived only from the hash field and the public key used for signing? If so, the public key has to be kept secret to attackers, so not really public in the broadest sense.

Perhaps a different public key should be used? OP-TEE could have its own private key used for decryption of the TA encryption keys. In other words, struct shdr_encrypted_ta would contain an encrypted TA decryption key.

jenswi-linaro · 2019-11-04T12:09:22Z

Sounds reasonable, will add derivation at TA load time. BTW, can you elaborate regarding purpose of hash field in struct shdr_encrypted_ta, since we already have public key embedded in tee.bin whose hash can be calculated at run-time? Is it just a flag to tell whether to include hash as salt or not?

I was thinking that this could be a way of allowing multiple different encryption keys, but using the same public key to verify the TA. It could be useful if it's not the same entity that encrypts the TA that also signs it. If this seems very unlikely, let's skip it. It's probably better to have multiple public keys instead if multiple SPs or sub-SPs are needed.

jenswi-linaro · 2019-11-04T12:20:13Z

I propose to add a hash field and a flags field in struct shdr_encrypted_ta. The TA encryption key will be derived from the added hash field and the hash of the public key which was used to verify the TA.

Would it be derived only from the hash field and the public key used for signing?

No, some secret data would be used too, either class wide or device specific.

Perhaps a different public key should be used? OP-TEE could have its own private key used for decryption of the TA encryption keys. In other words, struct shdr_encrypted_ta would contain an encrypted TA decryption key.

I was thinking about this too, but then there's the problem with hiding the secret key. That's why I changed my mind regarding the encryption key. So my idea is to derive the encryption key from TA headers and the public key (together with some secret data stored in OTP or such).

b49020 · 2019-11-04T12:31:18Z

Sounds reasonable, will add derivation at TA load time. BTW, can you elaborate regarding purpose of hash field in struct shdr_encrypted_ta, since we already have public key embedded in tee.bin whose hash can be calculated at run-time? Is it just a flag to tell whether to include hash as salt or not?

I was thinking that this could be a way of allowing multiple different encryption keys, but using the same public key to verify the TA. It could be useful if it's not the same entity that encrypts the TA that also signs it. If this seems very unlikely, let's skip it. It's probably better to have multiple public keys instead if multiple SPs or sub-SPs are needed.

Yeah it seems very unlikely, so will skip it.

jforissier · 2019-11-04T12:35:10Z

I was thinking about this too, but then there's the problem with hiding the secret key. That's why I changed my mind regarding the encryption key. So my idea is to derive the encryption key from TA headers and the public key (together with some secret data stored in OTP or such).

We have the same problem with the RPMB key already, that can be handled in a platform-specific way. At least we would not be using a public key as a private one (more or less). Another advantage is, the encrypted TA would not necessarily depend on the device which could be an annoying limitation for deployment I suppose.

b49020 · 2019-11-04T13:16:18Z

Perhaps a different public key should be used? OP-TEE could have its own private key used for decryption of the TA encryption keys. In other words, struct shdr_encrypted_ta would contain an encrypted TA decryption key.

I have already highlighted some of shortcomings of this approach here.

jforissier · 2019-11-04T13:48:31Z

@b49020 you mean the three points below?

Performance overhead of asymmetric decryption during loading of TAs. Some may not want to use it.

The TA binary would be encrypted with a symmetric key, therefore decryption would be fast. Only the symmetric key would be encrypted using public key crypto.

How would this handle per device unique encryption key case?

That's just one specific case: encrypt the TA decryption key with the device's public key.

How can we use encryption key provisioned in the hardware crypto accelerator?

What would prevent doing so? You provision a key pair key in the hardware and use the public part to encrypt the TA key. The TEE can find out which hardware key to use since it has the public key in the TA header.

b49020 · 2019-11-05T05:32:23Z

@jforissier

you mean the three points below?

Yes.

Performance overhead of asymmetric decryption during loading of TAs. Some may not want to use it.

The TA binary would be encrypted with a symmetric key, therefore decryption would be fast. Only the symmetric key would be encrypted using public key crypto.

Yes that's only the additional asymmetric decryption I mean here.

How would this handle per device unique encryption key case?

That's just one specific case: encrypt the TA decryption key with the device's public key.

Its not just one specific case, there are use-cases where one would not like to rely on a common key for every device. How do you foresee to derive a device's asymmetric key? One of methods (not tested) that I explored earlier was to seed a PRNG with device specific secret (OTP) and derive an RSA key.

BTW, ECC asymmetric key might be good candidate here performance wise, but haven't looked at its derivation part.

How can we use encryption key provisioned in the hardware crypto accelerator?

What would prevent doing so? You provision a key pair key in the hardware and use the public part to encrypt the TA key.

My point here was if hardware crypto accelerator could better protect symmetric TA encryption key and also provide fast decryption than why would one like to put it in encrypted form along with TA binary?

The TEE can find out which hardware key to use since it has the public key in the TA header.

b49020 · 2019-11-21T10:02:48Z

Update PR to add header description regarding security properties of REE-FS TAs.

b49020 · 2019-11-21T10:06:50Z

The travis error is a false positive in checkpatch.

jenswi-linaro

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

b49020 · 2019-11-21T10:42:45Z

Thanks @jenswi-linaro, tag applied.

jforissier · 2019-11-21T13:46:09Z

The travis error is a false positive in checkpatch.

IMO the warning has some merits, and the fix is simple enough, why not change the code to this? (untested!)

#define SHDR_ENC_GET_SIZE(x)	({ typeof(x) _x = (x); \
				   (sizeof(struct shdr_encrypted_ta) + \
				   _x->iv_size + _x->tag_size) })

jforissier · 2019-11-21T13:57:06Z

Could CFG_ENCRYPT_TA be enabled by default for one TA (at least) in optee_test so that this feature can be tested by CI?

Add framework to support loading of encrypted TAs from REE-FS using symmetric authenticated encryption scheme supported by OP-TEE. The default encryption key is derived from hardware unique key which can be overridden via platform specific encryption key. Signed-off-by: Sumit Garg <sumit.garg@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Add CFG_ENCRYPT_TA as TA build time configuration option to enable encryption of TA using encryption key provided via TA_ENC_KEY build time option. The default value of TA_ENC_KEY is derived from 16 zero bytes default hardware unique key. Also rename scripts/sign.py to scripts/sign_encrypt.py to reflect optional encryption support along with signing of TAs. Signed-off-by: Sumit Garg <sumit.garg@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

b49020 · 2019-11-22T06:51:25Z

@jforissier

IMO the warning has some merits, and the fix is simple enough, why not change the code to this? (untested!)

Thanks for your suggestion, PR updated with the fix.

Could CFG_ENCRYPT_TA be enabled by default for one TA (at least) in optee_test so that this feature can be tested by CI?

Enabling it by default for one of TA in optee_test might break optee_test on some platforms which have already overridden tee_otp_get_hw_unique_key() which makes HUK as non-zero which in-turn leads to TA_ENC_KEY to be different. So those platforms also need to provide TA_ENC_KEY=<key-in-hex-string-format> in addition to CFG_ENCRYPT_TA=y as build time flags for corresponding TA.

But I think in case of OP-TEE CI which uses hikey tee_otp_get_hw_unique_key() isn't overridden, so enabling CFG_ENCRYPT_TA=y would be fine for optee_test.

@jbech-linaro Can we enable CFG_ENCRYPT_TA=y build flag while compiling optee_test in our CI?

jforissier · 2019-11-22T07:38:12Z

@b49020

But I think in case of OP-TEE CI which uses hikey tee_otp_get_hw_unique_key() isn't overridden, so enabling CFG_ENCRYPT_TA=y would be fine for optee_test.

Sounds good to me, I think it can be done by changing this line:
https://github.com/jbech-linaro/ibart/blob/master/jobdefs/examples/hikey6220.yaml#L38

jbech-linaro · 2019-11-22T08:44:13Z

@jbech-linaro Can we enable CFG_ENCRYPT_TA=y build flag while compiling optee_test in our CI?

Sure, right now I'm running both QEMU and HiKey. Sounds like enable it for one of them and keep it disabled for the other gives best coverage. Which one shall we enable?

b49020 · 2019-11-22T08:53:50Z

Which one shall we enable?

Either one would work for me. I think based on CI runtime we could pick one. We can add CFG_ENCRYPT_TA=y for platform which runs faster.

b49020 · 2019-11-22T08:57:12Z

@jbech-linaro Also, can you please run IBART again on this PR once you add CFG_ENCRYPT_TA=y?

jbech-linaro · 2019-11-22T09:01:53Z

@jbech-linaro Also, can you please run IBART again on this PR once you add CFG_ENCRYPT_TA=y?

Sure, the only thing that we need to be aware of is that the flag is live the second I change it. I.e., all other PR's will have that flag enabled. But, that should be a "nop", since this is a brand new thing.

I know ... remote yml-support would fix this (jbech-linaro/ibart#8) and I do have a fix for it since a long time ago, it's just that I haven't been brave enough to submit the patch :).

I'll enabled the flag and re-start the job in a moment, I'll let you know once done.

jbech-linaro · 2019-11-22T09:04:42Z

@b49020 flag added to HiKey, job restarted (pending waiting for OP-TEE/build#398 to finish).

b49020 · 2019-11-22T09:06:32Z

Thanks @jbech-linaro

b49020 · 2019-11-22T09:29:26Z

@jbech-linaro This IBART failure is caused by missing Cryptodome package. Could you install it on your build machine?

sudo pip3 install pycryptodomex

jbech-linaro · 2019-11-22T09:36:42Z

Could you install it on your build machine?

Done and job has been restarted.

jforissier · 2019-11-22T12:59:35Z

OK, so it looks like this can be safely merged now. Thanks @b49020 -- now we can tick the box "encrypted TA support" 👍

I suggest updating the doc about TA formats once OP-TEE/optee_docs#51 is merged (ping @jbech-linaro!).

jbech-linaro · 2019-11-22T13:04:42Z

I suggest updating the doc about TA formats once OP-TEE/optee_docs#51 is merged

👍

(ping @jbech-linaro!).

I've seen the PR, I'm a bit busy on my end :)

b49020 · 2019-11-22T13:18:44Z

I suggest updating the doc about TA formats

Sure, will update optee_docs as well.

jforissier · 2020-04-16T07:26:26Z

Hi,

Sorry for posting to a closed PR, but I was recently asked about TA encryption.

I suggest updating the doc about TA formats

Sure, will update optee_docs as well.

Has the doc been updated? I don't remember and I can't find a description of this mechanism in https://optee.readthedocs.io/en/latest/architecture/trusted_applications.html.

How to use it as a TA developer? This commit tells a bit.
How to implement the platform-specific code? This commit has some information.

Thanks.

b49020 · 2020-04-16T07:31:48Z

Has the doc been updated?

Apologies as it slipped off my to-do list due to context switch. Thanks for reminding me. Will try to create a PR by tomorrow eod.

jforissier · 2020-04-16T07:40:55Z

@b49020 no worries 😉

b49020 · 2020-04-17T07:55:18Z

@jforissier OP-TEE/optee_docs#69

jforissier reviewed Oct 23, 2019

View reviewed changes

ta/mk/ta_dev_kit.mk Outdated Show resolved Hide resolved

core/arch/arm/kernel/ree_fs_ta.c Outdated Show resolved Hide resolved

ta/arch/arm/link.mk Outdated Show resolved Hide resolved

ta/arch/arm/link.mk Show resolved Hide resolved

jenswi-linaro reviewed Oct 23, 2019

View reviewed changes

core/arch/arm/kernel/ree_fs_ta.c Outdated Show resolved Hide resolved

core/include/signed_hdr.h Show resolved Hide resolved

ta/mk/ta_dev_kit.mk Outdated Show resolved Hide resolved

core/arch/arm/kernel/otp_stubs.c Outdated Show resolved Hide resolved

b49020 force-pushed the encypted_ta branch from 076c71a to 811f2d0 Compare October 25, 2019 09:34

jenswi-linaro reviewed Oct 25, 2019

View reviewed changes

scripts/sign_encrypt.py Outdated Show resolved Hide resolved

b49020 force-pushed the encypted_ta branch from 811f2d0 to 7af5a8b Compare October 25, 2019 10:03

b49020 force-pushed the encypted_ta branch 2 times, most recently from d88dc45 to fa44c85 Compare October 25, 2019 10:16

jenswi-linaro reviewed Nov 4, 2019

View reviewed changes

core/arch/arm/kernel/ree_fs_ta.c Outdated Show resolved Hide resolved

jenswi-linaro mentioned this pull request Nov 4, 2019

[RFC] Add option to restrict loading of shared libraries by TAs #3347

Closed

b49020 mentioned this pull request Nov 6, 2019

Secure provisioning of keys and TAs into OP TEE secure storage #3346

Closed

jenswi-linaro approved these changes Nov 21, 2019

View reviewed changes

b49020 force-pushed the encypted_ta branch from 1b3cc65 to 4095635 Compare November 21, 2019 10:41

b49020 force-pushed the encypted_ta branch from 4095635 to 15ffdce Compare November 22, 2019 06:31

b49020 force-pushed the encypted_ta branch from 15ffdce to b6c704d Compare November 22, 2019 06:34

jforissier merged commit 2de17fd into OP-TEE:master Nov 22, 2019

lxxstone mentioned this pull request Sep 30, 2021

Question about Encrypted TAs #4893

Closed

[RFC] Add framework to support REE-FS encrypted TAs #3340

[RFC] Add framework to support REE-FS encrypted TAs #3340

Conversation

b49020 commented Oct 23, 2019

jforissier left a comment

Choose a reason for hiding this comment

jforissier commented Oct 23, 2019

b49020 commented Oct 24, 2019

jforissier commented Oct 24, 2019

b49020 commented Oct 24, 2019

b49020 commented Oct 25, 2019

b49020 commented Oct 25, 2019

jenswi-linaro commented Oct 25, 2019

b49020 commented Oct 25, 2019

jenswi-linaro commented Nov 4, 2019

b49020 commented Nov 4, 2019

jenswi-linaro commented Nov 4, 2019

b49020 commented Nov 4, 2019

jforissier commented Nov 4, 2019

jenswi-linaro commented Nov 4, 2019

jenswi-linaro commented Nov 4, 2019

b49020 commented Nov 4, 2019

jforissier commented Nov 4, 2019

b49020 commented Nov 4, 2019

jforissier commented Nov 4, 2019 • edited Loading

b49020 commented Nov 5, 2019

b49020 commented Nov 21, 2019

b49020 commented Nov 21, 2019

jenswi-linaro left a comment

Choose a reason for hiding this comment

b49020 commented Nov 21, 2019

jforissier commented Nov 21, 2019

jforissier commented Nov 21, 2019

b49020 commented Nov 22, 2019

jforissier commented Nov 22, 2019

jbech-linaro commented Nov 22, 2019

b49020 commented Nov 22, 2019

b49020 commented Nov 22, 2019

jbech-linaro commented Nov 22, 2019

jbech-linaro commented Nov 22, 2019

b49020 commented Nov 22, 2019

b49020 commented Nov 22, 2019

jbech-linaro commented Nov 22, 2019

jforissier commented Nov 22, 2019

jbech-linaro commented Nov 22, 2019

b49020 commented Nov 22, 2019

jforissier commented Apr 16, 2020

b49020 commented Apr 16, 2020

jforissier commented Apr 16, 2020

b49020 commented Apr 17, 2020

jforissier commented Nov 4, 2019 •

edited

Loading