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

[Daniel Axtens]

Hi,

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

Nick, finding all our embarassing bugs _again_. Good work Nick!

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

Ok, I see how my preferred ultimate resolution to this (move TS to TPM
on any boot if it's in the PNOR) could get complex and probably needs
some more preceeding patches before we put it in firmware. On the theory
that "While there may be things that could be improved with this
submission, I believe that it is, at this time, (1) a worthwhile
modification to [skiboot], and (2) free of known issues which would
argue against its inclusion:"

Reviewed-by: Daniel Axtens <dja at axtens.net>

Kind regards,
Daniel

[1]: https://www.kernel.org/doc/html/v5.15/process/submitting-patches.html#reviewer-s-statement-of-oversight