[Skiboot] [PATCH v2 6/6] secvar/edk2: enforce a PK update enrolled in setup mode to be signed by itself

[Eric Richter]

On 11/2/21 8:16 AM, Daniel Axtens wrote:
> Hi Eric,
> 
>> When the system is in setup mode, that is, no PK has been enrolled in the PK
>> variable, variable signature enforcement is disabled. Therefore, variable
>> update signatures do not need to be authorized by a variable already stored in
>> a variable.
>>
>> However, variable updates must still be in the AUTH format, which still
>> contains a pkcs7 signature. Bare ESL files are *not* accepted as an update to
>> a variable. So, a variable update must still be signed, albeit with any key.
>>
>> Not enforcing signature checks is intended to be a convenience feature, however
>> enrolling a variable into the PK carries extra weight. Once data is enrolled
>> into the PK variable, signatures are now required for all further variable
>> updates.
>>
>> Due to this extra importance when enrolling a PK, the PK should most definitely
>> contain valid data. Therefore, as a sanity check, when enrolling a PK for the
>> first time (i.e. in setup mode), the PK variable update must be signed by its
>> own private key, which ensures ownership of the private key and validity of the
>> public key to be stored in the PK variable.
> 
> Hmm. In my work for [REDACTED], I had explictly _not_ required this. I
> could do this, I'll just need to get things renamed from "Allow
> unauthenticated PK updates" to "Allow self-signed PK updates".
> 
> Although, as with your case, I had also done all my tests (such as they
> are at the moment) with a self-signed key.
> 

I'm okay with unifying this with [REDACTED] if it means we don't
pull another project-that-must-not-be-named and have a thousand
different quirks and different behaviors across implementations.

> Does it take us closer to EDK2 compat or further away? I am guessing
> EDK2 also requires that there is an AUTH structure but just throws it
> away after maybe doing some basic syntactical sanity checks?
> 

Slightly further, though again, so far every example I'm finding does
the same procedure. Apparently it's a bit of a mess, where certain
firmware may allow bare ESLs or even certs.

As a side soapbox moment: I'm not a fan of unauthenticated updates
period. I don't see why we shouldn't be signature checking all
updates, the convenience isn't really that much more when the updates
still have to be signed in the AUTH format. Either that or we support
bare ESLs when in setup mode, but that'd likely end up being more
work to introduce more fun error cases when users enroll a PK first.

> I'm not _really_ sold on the value of this but I'm open to being
> convinced.
> 

Neither am I tbh, but this is bringing it in line with our
documentation and examples. We don't have a ton of documentation
to update if we skip this patch, but I tend to favor the
documentation being what the code *should* be.

> Beyond that it looks OK, I'll have a closer look once I get through the
> refactorings.
> 
> Kind regards,
> Daniel
>