Re: [edk2-devel] [PATCH v1 0/4] Don't require self-signed PK in setup mode

["Sean" <spbrogan@outlook.com>]

Did i mention how much i hate reviewing code over email?  :)

Even after looking at it a few times I missed that change.

I think this change looks fine.  Personally, i think this code has 
terrible readability and high complexity and with high impact.  It would 
be great to at least get unit tests if not a full refactor of the 
library.  I also think the edk2 idea of "custom mode" should be 
removed.  But that feedback isn't for this patch and I don't think this 
patch should be held up just because the surrounding code is less than 
ideal.

For patch 1 and 4.

Reviewed-by: Sean Brogan <sean.brogan@microsoft.com>

Thanks

Sean




On 1/27/2023 2:05 PM, Jan Bobek wrote:
>> I read your replacement a little different.
>>
>> -  if ((InCustomMode () && UserPhysicalPresent ()) || ((mPlatformMode == SETUP_MODE) && !IsPk)) {
>>
>> with
>>
>> +  if (  (InCustomMode () && UserPhysicalPresent ())
>> +     || (  (mPlatformMode == SETUP_MODE)
>> +        && !(FeaturePcdGet (PcdRequireSelfSignedPk) && IsPk)))
>> +  {
>>
>>
>> In the upper part you replaced it says !IsPk.  What am i missing?
>>
>>
>> If a payload was in this function targeting a KEK change with no PK
>> installed it would go in the original IF condition.  In the new code
>> it would because device is not in custom mode and the payload is not
>> targeted PK.
> Is it possible that you're missing the negation (i.e. exclamation mark)
> in front of the parethesis? The old code was
>
>    !IsPk
>
> The new code is
>
>    !(FeaturePcdGet (PcdRequireSelfSignedPk) && IsPk)
>
> If IsPk is FALSE, both of these evaluate to TRUE no matter what the PCD
> is.
>
> -Jan
>
>> On 1/25/2023 1:38 PM, Jan Bobek wrote:
>>> Hi Sean,
>>>
>>>>   From looking over the patch 1/4 email i have a concern.
>>>>
>>>> In AuthService.c on the conditional change you made. Aren't we losing
>>>> a case where we are evaluating a nonPK payload signed by the PK?
>>>> Given the system is in SetupMode that means there is no PK but is this
>>>> the conditional path that is used when installing Secure boot keys in
>>>> reverse (DBX,DX,KEK,PK) order?
>>> Thanks for sharing your concern! They way I think about the change is
>>> that I've replaced the expression
>>>
>>>       IsPk
>>>
>>> with
>>>
>>>       FeaturePcdGet (PcdRequireSelfSignedPk) && IsPk
>>>
>>> and nothing else. When you look at it this way, it's fairly easy to
>>> break down how it affects the logic:
>>>
>>> 1. Assume PcdRequireSelfSignedPk is TRUE. In this case, the two
>>>      expressions are exactly the same and no change in behavior occurs,
>>>      just as we want.
>>>
>>> 2. Assume IsPk is FALSE. In this case, both expressions evaluate to
>>>      FALSE, no matter what the PCD is configured to. That's also good,
>>>      because we don't want to change how non-PK payloads are handled.
>>>
>>> 3. In fact, the only change in behavior that occurs is when
>>>      PcdRequireSelfSignedPk is FALSE and IsPk is TRUE; here the former
>>>      expression would be TRUE, whereas the latter is FALSE. That's exactly
>>>      what we want: we wish to enter the first block of the if-else branch
>>>      (which changes the variable similarly to when we're in custom mode)
>>>      rather than falling through to the third block (which checks the
>>>      self-signature).
>>>
>>> To directly answer your question, I don't think the behavior changes at
>>> all when processing non-PK payloads, by virtue of IsPk being FALSE and
>>> what I said in point (2.) above.
>>>
>>> Additionally, yes, the first block of the if-else branch is exactly the
>>> path that's taken when enrolling KEK/DB/DBX in Setup mode, and one that
>>> has always been available even without Custom mode. It used to be that
>>> you couldn't use this path to enroll PK without Custom mode (precisely
>>> because of !IsPk in the condition), but I'm hoping to enable this path
>>> with my patch.
>>>
>>> Let me know if I haven't answered or misunderstood your question.
>>>
>>>> Is there testing you have done?  This code should be pretty easy to do
>>>> host based unit testing on. Any chance you have authored that to
>>>> confirm use cases are not unexpectedly impacted?  Example of host
>>>> based unit test of library is here:
>>>> edk2/SecurityPkg/Library/SecureBootVariableLib/UnitTest at master ·
>>>> tianocore/edk2
>>>> (github.com)<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Fedk2%2Ftree%2Fmaster%2FSecurityPkg%2FLibrary%2FSecureBootVariableLib%2FUnitTest&data=05%7C01%7Cjbobek%40nvidia.com%7Cf2eff15ce44945cc0b4e08db00ad6625%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638104516992386171%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=djptmpLUwESbPeqFwFUz5sVbS1MBnD%2FPua9H2y1nxFE%3D&reserved=0>
>>> I've done an ad-hoc test by commenting out the switch to/from Custom
>>> mode in EnrollFromDefaultKeysApp from SecurityPkg, booting in Setup mode
>>> and using the modified app to enroll the keys. You could do a similar
>>> test from the OS, but in my case this was more straightforward.
>>>
>>> I am aware of the host-based unit testing library in edk2, and I agree
>>> that it would be great to have the code in AuthVariableLib tested for
>>> all the different cases. However, I don't have any such tests right now,
>>> and while I'm willing to potentially look into writing some, I'd have to
>>> do it more or less on the side, meaning it could take a while.
>>>
>>> Best,
>>> -Jan
>>>
>>>> On 1/22/2023 10:13 PM, Yao, Jiewen wrote:
>>>>
>>>> Hi Sean
>>>> I would like to hear your feedback, since it is a little different from the original MSFT patch.
>>>>
>>>> Would you please take a look?
>>>>
>>>> Thank you
>>>> Yao, Jiewen
>>>>
>>>>
>>>>
>>>> -----Original Message-----
>>>> From: Jan Bobek <jbobek@nvidia.com><mailto:jbobek@nvidia.com>
>>>> Sent: Saturday, January 21, 2023 6:59 AM
>>>> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
>>>> Cc: Jan Bobek <jbobek@nvidia.com><mailto:jbobek@nvidia.com>; Laszlo Ersek <lersek@redhat.com><mailto:lersek@redhat.com>; Yao,
>>>> Jiewen <jiewen.yao@intel.com><mailto:jiewen.yao@intel.com>
>>>> Subject: [PATCH v1 0/4] Don't require self-signed PK in setup mode
>>>>
>>>> Hi all,
>>>>
>>>> I'm sending out v1 of my patch series that addresses a UEFI spec
>>>> non-compliance when enrolling PK in setup mode. Additional info can be
>>>> found in bugzilla [1]; the changes are split into 4 patches as
>>>> suggested by Laszlo Ersek in comment #4.
>>>>
>>>> I've based my work on the patch by Matthew Carlson; I've credited him
>>>> with co-authorship of the first patch even though in the end I decided
>>>> to do the implementation a bit differently.
>>>>
>>>> Comments & reviews welcome!
>>>>
>>>> Cheers,
>>>> -Jan
>>>>
>>>> References:
>>>> 1. https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2506&data=05%7C01%7Cjbobek%40nvidia.com%7Cf2eff15ce44945cc0b4e08db00ad6625%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638104516992386171%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=PXam5BVxMmUwgRdGsq1RcnwC8yb8nmOzcLz3oKica7s%3D&reserved=0
>>>>
>>>> Jan Bobek (4):
>>>>    SecurityPkg: limit verification of enrolled PK in setup mode
>>>>    OvmfPkg: require self-signed PK when secure boot is enabled
>>>>    ArmVirtPkg: require self-signed PK when secure boot is enabled
>>>>    SecurityPkg: don't require PK to be self-signed by default
>>>>
>>>> SecurityPkg/SecurityPkg.dec                             | 7 +++++++
>>>> ArmVirtPkg/ArmVirtCloudHv.dsc                           | 4 ++++
>>>> ArmVirtPkg/ArmVirtQemu.dsc                              | 4 ++++
>>>> ArmVirtPkg/ArmVirtQemuKernel.dsc                        | 4 ++++
>>>> OvmfPkg/Bhyve/BhyveX64.dsc                              | 3 +++
>>>> OvmfPkg/CloudHv/CloudHvX64.dsc                          | 3 +++
>>>> OvmfPkg/IntelTdx/IntelTdxX64.dsc                        | 3 +++
>>>> OvmfPkg/Microvm/MicrovmX64.dsc                          | 3 +++
>>>> OvmfPkg/OvmfPkgIa32.dsc                                 | 3 +++
>>>> OvmfPkg/OvmfPkgIa32X64.dsc                              | 3 +++
>>>> OvmfPkg/OvmfPkgX64.dsc                                  | 3 +++
>>>> SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf | 3 +++
>>>> SecurityPkg/Library/AuthVariableLib/AuthService.c       | 9 +++++++--
>>>> 13 files changed, 50 insertions(+), 2 deletions(-)