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


Jan Bobek
 

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://bugzilla.tianocore.org/show_bug.cgi?id=2506

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

--
2.30.2


Yao, Jiewen
 

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@...>
Sent: Saturday, January 21, 2023 6:59 AM
To: devel@edk2.groups.io
Cc: Jan Bobek <jbobek@...>; Laszlo Ersek <lersek@...>; Yao,
Jiewen <jiewen.yao@...>
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://bugzilla.tianocore.org/show_bug.cgi?id=2506

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

--
2.30.2


Sean
 

Jan,

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?

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)


Thanks

Sean




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@...>
Sent: Saturday, January 21, 2023 6:59 AM
To: devel@edk2.groups.io
Cc: Jan Bobek <jbobek@...>; Laszlo Ersek <lersek@...>; Yao,
Jiewen <jiewen.yao@...>
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://bugzilla.tianocore.org/show_bug.cgi?id=2506

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

--
2.30.2






Jan Bobek
 

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://github.com/tianocore/edk2/tree/master/SecurityPkg/Library/SecureBootVariableLib/UnitTest>
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@...><mailto:jbobek@...>
Sent: Saturday, January 21, 2023 6:59 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Jan Bobek <jbobek@...><mailto:jbobek@...>; Laszlo Ersek <lersek@...><mailto:lersek@...>; Yao,
Jiewen <jiewen.yao@...><mailto:jiewen.yao@...>
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://bugzilla.tianocore.org/show_bug.cgi?id=2506

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


Sean
 

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.

Thanks
Sean

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://github.com/tianocore/edk2/tree/master/SecurityPkg/Library/SecureBootVariableLib/UnitTest>
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@...><mailto:jbobek@...>
Sent: Saturday, January 21, 2023 6:59 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Jan Bobek <jbobek@...><mailto:jbobek@...>; Laszlo Ersek <lersek@...><mailto:lersek@...>; Yao,
Jiewen <jiewen.yao@...><mailto:jiewen.yao@...>
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://bugzilla.tianocore.org/show_bug.cgi?id=2506

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


Jan Bobek
 

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@...><mailto:jbobek@...>
Sent: Saturday, January 21, 2023 6:59 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Jan Bobek <jbobek@...><mailto:jbobek@...>; Laszlo Ersek <lersek@...><mailto:lersek@...>; Yao,
Jiewen <jiewen.yao@...><mailto:jiewen.yao@...>
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(-)


Sean
 

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

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@...><mailto:jbobek@...>
Sent: Saturday, January 21, 2023 6:59 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Jan Bobek <jbobek@...><mailto:jbobek@...>; Laszlo Ersek <lersek@...><mailto:lersek@...>; Yao,
Jiewen <jiewen.yao@...><mailto:jiewen.yao@...>
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(-)


Yao, Jiewen
 

Thanks Sean.

Acked-by: Jiewen Yao <Jiewen.yao@...>

-----Original Message-----
From: Sean Brogan <spbrogan@...>
Sent: Saturday, January 28, 2023 10:37 AM
To: Jan Bobek <jbobek@...>
Cc: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@...>; Sean Brogan
<sean.brogan@...>; Laszlo Ersek <lersek@...>
Subject: Re: [edk2-devel] [PATCH v1 0/4] Don't require self-signed PK in setup
mode

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

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%2F
Library%2FSecureBootVariableLib%2FUnitTest&data=05%7C01%7Cjbobek%40n
vidia.com%7Cf2eff15ce44945cc0b4e08db00ad6625%7C43083d15727340c1b7d
b39efd9ccc17a%7C0%7C0%7C638104516992386171%7CUnknown%7CTWFpbG
Zsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
0%3D%7C3000%7C%7C%7C&sdata=djptmpLUwESbPeqFwFUz5sVbS1MBnD%2FP
ua9H2y1nxFE%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@...><mailto:jbobek@...>
Sent: Saturday, January 21, 2023 6:59 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Jan Bobek <jbobek@...><mailto:jbobek@...>; Laszlo
Ersek <lersek@...><mailto:lersek@...>; Yao,
Jiewen <jiewen.yao@...><mailto:jiewen.yao@...>
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%40n
vidia.com%7Cf2eff15ce44945cc0b4e08db00ad6625%7C43083d15727340c1b7d
b39efd9ccc17a%7C0%7C0%7C638104516992386171%7CUnknown%7CTWFpbG
Zsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
0%3D%7C3000%7C%7C%7C&sdata=PXam5BVxMmUwgRdGsq1RcnwC8yb8nmO
zcLz3oKica7s%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(-)