> 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 <jbo...@nvidia.com><mailto:jbo...@nvidia.com>
>>> Sent: Saturday, January 21, 2023 6:59 AM
>>> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
>>> Cc: Jan Bobek <jbo...@nvidia.com><mailto:jbo...@nvidia.com>; Laszlo Ersek
>>> <ler...@redhat.com><mailto:ler...@redhat.com>; Yao,
>>> Jiewen <jiewen....@intel.com><mailto:jiewen....@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(-)
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99201): https://edk2.groups.io/g/devel/message/99201
Mute This Topic: https://groups.io/mt/96412382/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
