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

[Eric Richter]

On 11/2/21 9:45 AM, Daniel Axtens wrote:
> 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!
> 

Thank Nick, he's the one that pointed it out to me when testing the first
version :)

>> 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)
> 
Outside the threat model, however still a reasonable concern.

>     * 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.
> 

A more aggressive shoving of TS into TPM only saves us from that window
of firmware update -> variable update, which granted, might be a long time
unless the user is really active in updating their keys for some reason.

The biggest Weird Bad Thing Happening In The Field™ I'm afraid of is
having a duplicate TS in PNOR and TPM. That *shouldn't* happen, but there
are no duplicate variable protections currently in secvar code. Migrating
on a variable bank write ensures that the old PNOR TS gets wiped and
written to TPM instead.

Side note: an attacker can't slip a TS into PNOR without breaking the
hash check, so exploiting this duplicate variable "weakness" shouldn't
be possible.

That said, it can be done with a forced write to the variable bank in
.process() if !(TS->flags & PROTECTED), even if there is nothing in
the queue.

> 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
> 

I'm open to either.

> 
>>
>> 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;

This is the bit that forces the flag setting on a variable update.
This function is be called on TS if any variable is updated, as
the timestamp for that particular variable will need to be adjusted.

>>  
>>  	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.
> 

This is the part that defines the TS variable for the first time, so now
new TS variable definitions (i.e. after key-clear or on first boot) are
stored in protected storage, i.e. TPM NV. I marked the other space where
this flag is set above.

secvar->flags are suggestions to the storage driver on how to handle
certain variables. Given that we have exactly one(1) storage driver at
the moment, this is directly instructing it to store it in TPM NV.

Question preempt: It is a suggestion, as if we had a system with enough
actually lockable storage, this flag would be ignored as all variables
are "protected" by default.

> Kind regards,
> Daniel
>