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

securechip: add initial Optiga implementation #1341

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

benma
Copy link
Collaborator

@benma benma commented Dec 12, 2024

This commit implements the securechip.h interface for the Optiga Trust M V3, with an interface and configuration roughly corresponding to how we use the ATECC608 secure chip:

  • A KDF key that is internally generated and cannot be read and written from the host MCU
  • A KDF key that is generated on the host
  • A monotonic counter attached to the first KDF key which limits the maxmium number of uses of the key over the lifetime of the device
  • Attestation key that is internally generated and used to sign attestation challenges

The factory setup configures the metadata of each object we use, setting the state to Operational. After this, metadata cannot be changed, and the access conditions apply as specified.

Shielded communication encrypts the communication with the chip and is active and enforced through the metadata access configs. It roughly corresponds to IO protection in the ATECC608.

In the ATECC608, we additionally authorize each command with the authorization_key, another pre-shared secret. The Optiga offers the same functionality, but we don't use it to authorize all commands, as it is redundant to using the shielded communication in terms of enabling the host MCU to execute commands.

@benma benma force-pushed the optiga branch 4 times, most recently from 5fb80cd to 8fcc0b9 Compare December 12, 2024 12:57
@benma benma requested a review from NickeZ December 12, 2024 13:32
@benma benma marked this pull request as ready for review December 12, 2024 13:32
@benma benma closed this Dec 12, 2024
@benma benma reopened this Dec 12, 2024
@benma benma mentioned this pull request Dec 12, 2024
@benma benma force-pushed the optiga branch 6 times, most recently from 18c858a to 100a9da Compare December 16, 2024 13:29
src/atecc/atecc.h Outdated Show resolved Hide resolved
src/optiga/pal/pal_os_lock.c Outdated Show resolved Hide resolved
@NickeZ
Copy link
Collaborator

NickeZ commented Dec 30, 2024

You have two spelling mistakes in the commit message: "An KDF" and "reundnant".

@benma benma force-pushed the optiga branch 2 times, most recently from ee352dc to 0017ef2 Compare December 30, 2024 09:23
@benma
Copy link
Collaborator Author

benma commented Dec 30, 2024

You have two spelling mistakes in the commit message: "An KDF" and "reundnant".

Thx, fixed.

Copy link
Collaborator

@NickeZ NickeZ left a comment

Choose a reason for hiding this comment

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

First round of review, I still need to go through the config and test the firmware.

src/optiga/optiga.c Show resolved Hide resolved
src/optiga/optiga.c Outdated Show resolved Hide resolved
src/optiga/optiga.c Outdated Show resolved Hide resolved
src/optiga/optiga.c Outdated Show resolved Hide resolved
src/optiga/optiga.c Outdated Show resolved Hide resolved
// not present.
static bool _read_lcso(const uint8_t* metadata, size_t metadata_len, uint8_t* lcso_out)
{
uint8_t tag_data[100] = {0};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this buffer 100 bytes long when LCSO is 1 byte?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My initial reaction was because we can't guarantee the chip or the library will not return more, so it would be UB. Then I realized the same is true for 100, so I now actually pass the input data length and do a size check before writing to the buffer. See the diff.

src/optiga/optiga.c Show resolved Hide resolved
src/optiga/optiga.c Show resolved Hide resolved
src/optiga/optiga.c Show resolved Hide resolved
src/optiga/optiga.c Show resolved Hide resolved
@benma benma force-pushed the optiga branch 3 times, most recently from e5bfb97 to 0d8a0e3 Compare January 7, 2025 11:38
benma and others added 2 commits January 21, 2025 13:38
This commit implements the securechip.h interface for the Optiga Trust
M V3, with an interface and configuration roughly corresponding to how
we use the ATECC608 secure chip:

- A KDF key that is internally generated and cannot be read and
written from the host MCU
- A KDF key that is generated on the host
- A monotonic counter attached to the first KDF key which limits the
  maxmium number of uses of the key over the lifetime of the device
- Attestation key that is internally generated and used to sign
  attestation challenges

The factory setup configures the metadata of each object we use,
setting the state to Operational. After this, metadata cannot be
changed, and the access conditions apply as specified.

Shielded communication encrypts the communication with the chip and is
active and enforced through the metadata access configs. It roughly
corresponds to IO protection in the ATECC608.

In the ATECC608, we additionally authorize each command with the
authorization_key, another pre-shared secret. The Optiga offers the
same functionality, but we don't use it to authorize all commands, as
it is redundant to using the shielded communication in terms of
enabling the host MCU to execute commands.

Co-Authored-By: Niklas <[email protected]>
To disable interrupts when processing Optiga commands.
@benma
Copy link
Collaborator Author

benma commented Jan 21, 2025

Rebased

return SC_OPTIGA_ERR_CREATE;
}

return _verify_config();
Copy link
Collaborator Author

@benma benma Jan 21, 2025

Choose a reason for hiding this comment

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

This causes a noticable delay after boot, before the orientation arrows animation starts. Not sure what to do about it. Maybe check the config on first use of a securechip function instead of at every boot? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

perhaps we could increase the frequency of the timer that controls the SDK?

@NickeZ
Copy link
Collaborator

NickeZ commented Jan 29, 2025

In the ATECC608, we additionally authorize each command with the authorization_key, another pre-shared secret. The Optiga offers the same functionality, but we don't use it to authorize all commands, as it is redundant to using the shielded communication in terms of enabling the host MCU to execute commands.

I agree with the conclusion but not the reason :). I think the issue is that we need to store both keys in the same flash memory. Therefore there is a single point of failure anyway. However, in the future, if we would have external flash, or some other place to store one of the keys, it would make an attack where you read out the keys "twice" as complex. As you would have to read out memory from two chips.

So for now, we shouldn't use it.

@benma
Copy link
Collaborator Author

benma commented Jan 29, 2025

In the ATECC608, we additionally authorize each command with the authorization_key, another pre-shared secret. The Optiga offers the same functionality, but we don't use it to authorize all commands, as it is redundant to using the shielded communication in terms of enabling the host MCU to execute commands.

I agree with the conclusion but not the reason :). I think the issue is that we need to store both keys in the same flash memory. Therefore there is a single point of failure anyway. However, in the future, if we would have external flash, or some other place to store one of the keys, it would make an attack where you read out the keys "twice" as complex. As you would have to read out memory from two chips.

So for now, we shouldn't use it.

In this case you could still use one key for shielded connection, but have the key be a derivation/combination of multiple keys (that's already the case, but both parts of the key are stored in flash, but "far" apart - but they might as well be stored on different chips). Authorization seems to be for a different purpose, e.g. tying it to a user password like done in #1344, not primarily to authorize the MCU in itself (which the shielded comms also does).

@NickeZ
Copy link
Collaborator

NickeZ commented Jan 29, 2025

In this case you could still use one key for shielded connection, but have the key be a derivation/combination of multiple keys (that's already the case, but both parts of the key are stored in flash, but "far" apart - but they might as well be stored on different chips). Authorization seems to be for a different purpose, e.g. tying it to a user password like done in #1344, not primarily to authorize the MCU in itself (which the shielded comms also does).

True, you could just derive the enc key from multiple places.

if ((return_status) != OPTIGA_UTIL_SUCCESS) { \
return (return_status); \
} \
while (OPTIGA_LIB_BUSY == (optiga_lib_status)) { \
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably have some kind of timeout here, in case the secure chip never responds. So perhaps delay_ms(10) for 50 times or something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, actually, since the SDK is called from a timer and the SDK probably has a timout we should be fine. I would have to double check.


// In a metadata object (0x20 <len> <tag> <tag len> <tag data> ...),
// extract tag data for a specific tag.
// Returns false if the metadata is invalid or the tag is not present.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also returns false in case data doesn't fit in data_out based on data_len_out

@NickeZ
Copy link
Collaborator

NickeZ commented Jan 29, 2025

I've tested it successfully with a couple of transactions. I still need to review the metadata of the objects.

@NickeZ
Copy link
Collaborator

NickeZ commented Jan 30, 2025

I've looked at the configuration and can't find anything more to comment on

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants