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

Incorrect use of trust-all flag in secret_prepare_fw #4649

Open
matejcik opened this issue Feb 19, 2025 · 0 comments
Open

Incorrect use of trust-all flag in secret_prepare_fw #4649

matejcik opened this issue Feb 19, 2025 · 0 comments
Labels
bug Something isn't working as expected

Comments

@matejcik
Copy link
Contributor

matejcik commented Feb 19, 2025

There are multiple problems with trust_all in secret_prepare_fw for STM32U5:

  1. Per line 343, an "install restricted" screen is shown even if firmware the allow_run_with_secret flag. This is an unintentional side-effect of a sequence of refactors, and the flag used should be allow_run_with_secret instead of trust_all
    if (sectrue != trust_all && sectrue == optiga_secret_present) {
    // Untrusted firmware, locked bootloader. Show the restricted screen.
    show_install_restricted_screen();
    }
  2. left-over code block disables secret access if trust_all is not set -- even though the secret is already always disabled on line 342 or 348
    if (sectrue != trust_all) {
    secret_disable_access();
    }
    }

The remaining use of trust_all is in the block that check whether the U5 chip is fresh in provisioning:

if (sectrue == trust_all && sectrue == allow_run_with_secret &&
sectrue == optiga_secret_writable && secfalse == optiga_secret_present) {
// Secret is not present and the secret sector is writable.
// This means the U5 chip is unprovisioned.
// Allow trusted firmware (prodtest presumably) to access the secret sector,
// early return here.
return;
}

If not, there's an early return and the secret sector access stays unlocked so that prodtest can write into it.

Semantically, this function should not make decisions based on "full trust" status of the firmware.
I propose renaming the flag to allow_provisioning_access, and modifying the check like so:

  if (sectrue == allow_provisioning_access &&
      sectrue == optiga_secret_writable && secfalse == optiga_secret_present) {
    // Secret is not present and the secret sector is writable.
    // This means the U5 chip is unprovisioned.
    // Allow trusted firmware (prodtest presumably) to access the secret sector,
    // early return here.
    return;
  }

This makes the intention of the code clearer, and leaves responsibility on the caller to specify which firmwares have "provisioning access". It could be any full-trust firmware (which is what we'll implement now), or we can implement a more specific check for prodtest in the future.

The provisioning access no longer depends on "allow_run_with_secret", which also seems more semantically correct: (a) there is no "secret" to be protected in this condition, and (b) the caller can specify whether "allow_run_with_secret" is a precondition or not. In theory we could disallow prodtest from running on an already provisioned device this way. (won't be possible in practice, but...)

@matejcik matejcik added the bug Something isn't working as expected label Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as expected
Projects
Status: No status
Development

No branches or pull requests

1 participant