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

core: huk definition #5570

Closed
wants to merge 2 commits into from
Closed

core: huk definition #5570

wants to merge 2 commits into from

Conversation

ldts
Copy link
Contributor

@ldts ldts commented Oct 5, 2022

The RPMB key derivation algorithm consumes the HUK and therefore is impacted by changes in its value.

The platform specific function plat_rpmb_key_is_ready() should be allowed to query the HUK in order to allow/deny burning the RPMB key.

Signed-off-by: Jorge Ramirez-Ortiz jorge@foundries.io

The RPMB key derivation algorithm consumes the HUK and therefore is
impacted by changes in its value.

The platform specific function plat_rpmb_key_is_ready() should be
allowed to query the HUK in order to allow/deny burning the RPMB key.

Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
@ldts
Copy link
Contributor Author

ldts commented Oct 5, 2022

if this change is accepted, I will update the zynqmp and the stm32mp15_huk PR to use this value. Versal will follow - the versal huk is pending in my trees waiting for current stuff to be merged...2bc7d71

@@ -13,6 +13,8 @@

struct tee_hw_unique_key {
uint8_t data[HW_UNIQUE_KEY_LENGTH];
/* The hardware unique key is now inmmutable */
bool locked;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this relate to plat_rpmb_key_is_ready()?

Edit: reading the description again it seems that plat_rpmb_key_is_ready() should check this value. Are we just moving the logic to decide when the key can be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plat_rpmb_key_is_ready often depends on the platform being considered closed/secured - ie, the bootrom booting signed firmware. However it should also depend on the HUK key being immutable (so the RPMB derived key doesnt change on a subsequent boot causing the RPMB not to be accesible).

So IMO having some information with respect to the RPMB key derivation mutability is required. I propose to do it here since the only reason for variability in the RPMB key is the HUK changing, but really what plat_rpmb_key_is_ready needs to know is if the key that has been generated can change; then it can decide whether to allow the key to be written or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Jens. It's up to the platform to refuse or not to provide a HUK. There is no unique definition of what is a locked/closed device, possibly several levels of device closure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not saying anything different. I think I am not being understood. Let me summarize Wittegenstein style:

  1. OP-TEE uses the HUK to derive secrets.
  2. The RPMB key must be a secret.
  3. OP-TEE does not enforce a secure HUK.
    It follows that OP-TEE secrets are not necessarily secrets
    It also follows that the RPMB key that OP-TEE writes is not a secret.

Please let me know if you disagree in anything so far so I can keep on building. Otherwise there is no point to spend more time on this and I can close the PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. isn't accurate. OP-TEE assumes a secure HUK. If the HUK is known outside OP-TEE it's assumed that it's treated as securely as OP-TEE itself can, that is, the security of the HUK is not supposed to be reduced.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, no, it is 100 accurate: you are saying exactly what I am saying. OP-TEE does not enforce a secure HUK. However it treats the HUK as if it was secure..hence all these discussions we have been having on the same subject.

This commit allows the HUK user to know - and not assume- if the HUK is secure or not. And as I was trying to explain (probably not very clearly), this is simpler than enforcing a secure HUK.

The HUK users need to know - depending on the product development phase- if the HUK can be trusted or not.

@jenswi-linaro
Copy link
Contributor

I would rather delegate the decision of when to write the RPMB key to the normal world like in #5338

@ldts
Copy link
Contributor Author

ldts commented Oct 6, 2022

I would rather delegate the decision of when to write the RPMB key to the normal world like in #5338

If OP-TEE calculates the key, OP-TEE will still need to let the normal world know if the key is to be trusted - which is the problem we are discussing.

Or if OP-TEE is not going to provide a key, then someone else will need to figure out how to derive (or where to store!) the RPMB key.

The good thing about OP-TEE deriving it is that the key doesn't need to be stored.

The problem of enforcing a secure HUK across all architectures and products is harder than the problem of letting the platform know if the HUK - and consequently the RPMB key- is a secret....I find this a more liberal/flexible approach to a problem with to many variants that is hard to attack head on.

Products and platforms can then make their own decisions.

@jenswi-linaro
Copy link
Contributor

I would rather delegate the decision of when to write the RPMB key to the normal world like in #5338

If OP-TEE calculates the key, OP-TEE will still need to let the normal world know if the key is to be trusted - which is the problem we are discussing.

Thanks for the clarification.

Or if OP-TEE is not going to provide a key, then someone else will need to figure out how to derive (or where to store!) the RPMB key.

The good thing about OP-TEE deriving it is that the key doesn't need to be stored.

Agreed

The problem of enforcing a secure HUK across all architectures and products is harder than the problem of letting the platform know if the HUK - and consequently the RPMB key- is a secret....I find this a more liberal/flexible approach to a problem with to many variants that is hard to attack head on.

Products and platforms can then make their own decisions.

I don't see the point of the notion of an untrusted HUK. Either you have it and it can be used or you don't have it. If it's not trusted then you don't have it.

We should try to avoid being flexible as that means more ifs and buts.

@ldts
Copy link
Contributor Author

ldts commented Oct 6, 2022

And for the end user I agree 100% with you: if as a user you are going to use a HUK you better make sure if it holds a secret that you can trust - and not all HUK drivers that OP-TEE provides comply.

Developing for security as is hard. And by not having these qualifiers on the quality of the keys used we are making it even harder.
Since OP-TEE does not enforce the security of the HUK - but it is aware of it being a secret or not- why cant this information be shared with the rest of the platform? what is wrong with doing it?

In fact, I dont understand why do we feel the need to have : WARNING: This OP-TEE configuration might be insecure! but we refuse to know and share if indeed a configuration is 100% insecure.

OP-TEE only offers guarantees if the user knows exactly what to do, what to change, where to look - which is great for contractors, I wont deny that. But having the chance to share with the user if the root of trust is broken and not doing it seems wrong to me.

@etienne-lms
Copy link
Contributor

Using an open device (a device where we can reprogram fuses or like) does not mean the HUK generated by OP-TEE is not reliable. It can be. One can burn a RPMB with a test key, not that secure, but still useful for test and development purpose.

IIUC the issue you want to address is to prevent one from bruning a RPMB key, or another external secure element device pairing key, with wrong data possibly bricking the device.

If you really want such guards, for example in the stm32mp15_huk.c from #5555, maybe the platform function could refuse to return the HUK when device is "closed" and target fuses not properly "locked" and maybe few other Soc information. That would remain a stm32mp15_huk.c specifc implementation, with a scope specifc to that platform.

@ldts
Copy link
Contributor Author

ldts commented Oct 6, 2022

Yes that is something that I was considering (returning an error on the huk as you are suggesting).

But I think it is simpler - and more generic- to have this change on the stm32mp15_huk.c

-   if (!ret && !stm32mp15_huk_initialized) {
-           stm32mp15_huk_initialized = true;
-           IMSG("HUK %slocked", lock ? "" : "un");
-           DHEXDUMP(hwkey->data, HW_UNIQUE_KEY_LENGTH);
+ if (!ret) {
+         if (!stm32mp15_huk_initialized) {
+                 stm32mp15_huk_initialized = true;
+                 stm32mp15_huk_locked = lock;
+
+                 IMSG("HUK %slocked", lock ? "" : "un");
+                 DHEXDUMP(hwkey->data, HW_UNIQUE_KEY_LENGTH);
+         }
+
+         /* tamper detection */
+         if (stm32mp15_huk_locked != lock)
+                 panic();
+
+         hwkey->locked = stm32mp15_huk_locked
        }
 

And then in main:

bool plat_rpmb_key_is_ready(void)
{
	struct tee_hw_unique_key hwkey = { };

	if (tee_otp_get_hw_unique_key(&hwkey) || !hwkey.locked)
		return false;

	return stm32mp_is_closed_device();
}

@ldts
Copy link
Contributor Author

ldts commented Oct 7, 2022

anyway, many ways of doing this but the key requirement is that HUK users need to know if the HUK is to be trusted.

So what do we do? Shall I just close this PR? do we feel it deserves a larger discussion? should we merge this proposal?

@jenswi-linaro
Copy link
Contributor

We should not merge this. As for the discussion, it seems we don't understand each other.

@ldts
Copy link
Contributor Author

ldts commented Oct 7, 2022

  1. Your position is that the HUK must be trusted and it should not be used without that guarantee (agree 100%)
  2. Your position (I think) is also that it is acceptable to merge HUK drivers that do not deliver that guarantee.
    if 2) is true, I do not understand that: maybe you could build on it?
    if 2) is not true then I need to fix plat-stm32mp15: generate HUK using an AES-GCM key from BSEC OTP #5555
    so need your input.

Example of usage that should be extended to all functions
in core/arch/arm/kernel/otp_stubs and drivers.

There is no reason I can think of for not reporting a truly
unsecure platform executing a Trusted-OS.

Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
@jenswi-linaro
Copy link
Contributor

No, I disagree on 1 and 2.

@ldts
Copy link
Contributor Author

ldts commented Oct 10, 2022

This is kind of surprising since you wrote: "I don't see the point of the notion of an untrusted HUK. Either you have it and it can be used or you don't have it. If it's not trusted then you don't have it."

what did you mean then? your position is totally cryptic to me.

@ldts
Copy link
Contributor Author

ldts commented Oct 23, 2022

This discussion is important IMO. please could we resume with it.

To provide with some background -aside of RPMB- take for example the SCP03 support we have in the SE05x crypto driver controlling a whole family of HSMs

SCP03 defines a set of symmetric keys that must be rotated once the platform is deemed secured. The rotation scheme that we (you and I during the review process) agreed was, rather than storing these keys in memory, we would derive their values from a secure key. In OP-TEE this is the HUK.
As a consequence, if the SCP03 keys are rotated and the HUK then changes, the secure element will no longer be reachable (and it can not be reset, so it is lost forever...all keys and certificates and blobs no longer accessible).

So the SE05x key rotation scheme - just as the RPMB key generation scheme - would benefit from having an indication if the HUK is stable before going ahead.

Incidentally, the current __weak definition of HUK is silly for most purposes as it doesnt generate a per-device unique key- something that causes issues when dealing with fleets of devices. So perhaps 1) that __weak should use just a CID if available (so at least it is unique...at the moment is not unique nor secure so it would be a step forward) and 2) OP-TEE requires HUK drivers to generate immutable secret value and raises a security warning when that is not the case.

what do you think?

@jenswi-linaro
Copy link
Contributor

Why not just let the platform return an error if it detects that the HUK can't be used yet?

On QEMU and Hikey it's from a testing point of good enough to use zeroes.

@ldts
Copy link
Contributor Author

ldts commented Oct 24, 2022

Right and that would be an option if we had a __weak implementation that provides a unique id.

What NXP does in their hardware is that while the platform is not booting secured firmware (the SoC hasnt been fused/closed), the hardware provides a real unique id. But then, once the board is secured (fused) it generates a different one which is final and therefore secured.

This is the ideal situation we have been trying to mimic on the other HUK drivers.. And the reason why plat_rpmb_is_ready usually checks for the closed state on those platforms. But it really is all about the HUK.

@jenswi-linaro
Copy link
Contributor

Right and that would be an option if we had a __weak implementation that provides a unique id.

I suppose we could remove the __weak implementation and make it truly platform specific.

A HUK is supposed to be unique and secure. I don't see a point in using a non-secure HUK as NXP does, but perhaps I'm missing a use case.

For testing purposes, you can of course use any non-changing HUK, but OP-TEE still treats it as if it's unique and secure.

@ldts
Copy link
Contributor Author

ldts commented Oct 25, 2022

I think the root of the problem is that a HUK is needed during development that is truly unique - not just a string of zeroes common to all OP-TEE devices.

if the __weak implementation returned a unique ID when executing on real hardware we could then enforce/recommend that any platform specific HUK driver must be unique and immutable and whenever possible a secret (ie, only readable when the BOOTROM is booting a signed firmware)

having a generic HUK returning a unique id when executing on various hardware should be doable (ie just use the CID value use for booting).

Then it would be clear at build time if the HUK derived keys are secured.

@jenswi-linaro
Copy link
Contributor

I think the root of the problem is that a HUK is needed during development that is truly unique - not just a string of zeroes common to all OP-TEE devices.

Why can't zeroes be used?

if the __weak implementation returned a unique ID when executing on real hardware we could then enforce/recommend that any platform specific HUK driver must be unique and immutable and whenever possible a secret (ie, only readable when the BOOTROM is booting a signed firmware)

The __weak implementation is generic, I don't see how it can be unique. To me, it seems complicated to have a HUK that changes depending on the security state of the platform. NXP has chosen that approach, but as I said they may have a use case I'm not seeing. What's your use case with a changing HUK?

having a generic HUK returning a unique id when executing on various hardware should be doable (ie just use the CID value use for booting).

What CID? The one from the eMMC? How can that be secure or even non-changing?

It seems I'm missing a key point in your use case.

@github-actions
Copy link

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.

@github-actions github-actions bot added the Stale label Nov 25, 2022
@github-actions github-actions bot closed this Dec 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants