[Skiboot] [PATCH v2 3/6] secvar/edk2: store timestamp variable in protected storage

[Daniel Axtens]

Eric Richter <erichte at linux.ibm.com> writes:

> Each signed variable update contains a timestamp -- this timestamp is checked
> against the previous timestamp seen for that particular variable (if any), and
> the update is rejected if the timestamp is not a later time than the previous.
>
> This timestamp check is intended to prevent re-use of signed update files.
> Currently, the code stores the timestamps in the TS variable, which is then
> stored in regular variable storage (typically PNOR). This patch promotes the
> variable to "protected storage" (typically TPM NV), so avoid this variable
> being accidentally cleared.
>

Or maliciously cleared...

Anyway, good catch, I can't believe it never occurred to me to check this!

> This change should only come into effect when either:
>  - initializing secvar for the first time (i.e. first boot, or
>     after a key-clear-request)
>  - processing any variable update
>
> Systems that already have a TS variable in PNOR will not be affected until
> either of the above actions are taken.

Hmm. So you could obviously be more agressive here - for example moving
it unconditionally. I am trying to construct an attack scenario where it
matters.

What would you need?

 - something that would be blocked by TS. This would be a variable
   update.

 - an ability to reset PNOR TS. This is easy, just corrupt the PNOR.

 - A variable update you could perform based solely on the contents of
   TPM protected storage. That's not too hard, let's say you have a db
   update that's signed by PK.

The obvious attack doesn't work:

 - PK-holder distributes a signed db update ("old update"). Machine
   owner applies this update. Attacker saves the update.

 - Later, the db key is compromised.

   * Attacker obtainst the compromised key and signs a malicious kernel
     with the compromised key.

   * PK-holder distributes a new signed db update ("new update") which
     overwrites the compromised key.

   * Machine owner applies the new update.

 - The machine owner applies this new firmware and reboots.

 - Attacker:
    * compromises the box
    * corrupts the SECBOOT partition of the PNOR
    * puts the old update in the PNOR.
    * installs their malicious kernel
    * reboots the machine

This fails because skiboot will reset the update queue along with the
rest of the PNOR when verification fails.

So if an attacker were to corrupt the secboot partition, they'd instead
render the OS unbootable: you'd get to petitboot and then not be able to
verify anything because you have no db.

An attacker would have to do the following more complex dance:

 - Attacker:

    * compromises the control plane such that they get console/petitboot
      access (so BMC/FSP serial or IPMI)

    * corrupts the SECBOOT partition of the PNOR
    * installs their malicious kernel
    * reboots the machine to petitboot

    * drops to the petitboot shell
    * installs the old update
    * reboots to malicious kernel

(assuming I haven't missed anything!)

Obviously the flip side of being more agressive is that you end up
running a greater risk of Weird Bad Things Happening In The Field. Given
the general... for lack of a better term "fiddlyness" of TPMs, that's a
pretty live concern.

idk what the right answer is, I guess it's a risk tolerance question.

I guess there's possibly an argument to be made for doing what is fast
becoming my favourite dumb trick for resolving things I don't like about
skiboot, which is to say do the agressive and more correct thing
upstream and another more conservative thing in stable :P


>
> Signed-off-by: Eric Richter <erichte at linux.ibm.com>
> ---
>  libstb/secvar/backend/edk2-compat-process.c | 4 +++-
>  libstb/secvar/backend/edk2-compat.c         | 1 +
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c
> index 770c3706..d69e066f 100644
> --- a/libstb/secvar/backend/edk2-compat-process.c
> +++ b/libstb/secvar/backend/edk2-compat-process.c
> @@ -45,7 +45,9 @@ int update_variable_in_bank(struct secvar *update_var, const char *data,
>  	else
>  		var->flags |= SECVAR_FLAG_VOLATILE;
>  
> -	if (key_equals(update_var->key, "PK") || key_equals(update_var->key, "HWKH"))
> +	if (key_equals(update_var->key, "PK")
> +	    || key_equals(update_var->key, "HWKH")
> +	    || key_equals(update_var->key, "TS"))
>  		var->flags |= SECVAR_FLAG_PROTECTED;
>  
>  	return 0;
> diff --git a/libstb/secvar/backend/edk2-compat.c b/libstb/secvar/backend/edk2-compat.c
> index 9e61fbc6..d7975fa2 100644
> --- a/libstb/secvar/backend/edk2-compat.c
> +++ b/libstb/secvar/backend/edk2-compat.c
> @@ -89,6 +89,7 @@ static int edk2_compat_pre_process(struct list_head *variable_bank,
>  		memcpy(tsvar->key, "TS", 3);
>  		tsvar->key_len = 3;
>  		tsvar->data_size = sizeof(struct efi_time) * 4;
> +		tsvar->flags = SECVAR_FLAG_PROTECTED;
>  		memset(tsvar->data, 0, tsvar->data_size);
>  		list_add_tail(variable_bank, &tsvar->link);
>  	}

I don't really understand quite enough of how the variable storage works
to know if this is adequate. I can have another crack at it another day.

Kind regards,
Daniel