Re: [RFC] VariablePolicy - Protocol, Libraries, and Implementation for VariableLock Alternative

Re: [RFC] VariablePolicy - Protocol, Libraries, and Implementation for VariableLock Alternative

[Bret Barkelew]

[-- Attachment #1: Type: text/plain, Size: 2938 bytes --]

Expanding the audience beyond the RFC list….
If no one has additional input, I’ll try to start formatting these as patches later this week. Thanks!

- Bret

From: Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>
Sent: Tuesday, January 28, 2020 5:36 PM
To: rfc@edk2.groups.io<mailto:rfc@edk2.groups.io>
Subject: [RFC] VariablePolicy - Protocol, Libraries, and Implementation for VariableLock Alternative

All,

VariablePolicy is our proposal for an expanded “VarLock-like” interface to constrain and govern platform variables.
I brought this up back in May to get initial comments on the interface and implications of the interface and the approach. We implemented it in Mu over the summer and it is not our defacto variable solution. It plugs in easily to the existing variable infrastructure, but does want to control some of the things that are currently managed by VarLock.

There are also some tweaks that would be needed if this were desired to be 100% optional code, but that’s no different than the current VarLock implementation which has implementation code directly tied to some of the common variable code.

I’ve structured this RFC in two pieces:

  *   The Core piece represents the minimum changes needed to implement Variable Policy and integrate it into Variable Services. It contains core driver code, central libraries and headers, and DXE driver for the protocol interface.
  *   The Extras piece contains recommended code for a full-feature implementation including a replacement for the VarLock protocol that enables existing code to continue functioning as-is. It also contains unit and integration tests. And as a bonus, it has a Rust implementation of the core business logic for Variable Policy.

The code can be found in the following two branches:
https://github.com/corthon/edk2/tree/personal/brbarkel/var_policy_rfc_core
https://github.com/corthon/edk2/tree/personal/brbarkel/var_policy_rfc_extra

A convenient way to see all the changes in one place is to look at a comparison:
https://github.com/corthon/edk2/compare/master...corthon:personal/brbarkel/var_policy_rfc_core
https://github.com/corthon/edk2/compare/personal/brbarkel/var_policy_rfc_core...corthon:personal/brbarkel/var_policy_rfc_extra

There’s additional documentation in the PPT and DOC files in the core branch:
https://github.com/corthon/edk2/blob/personal/brbarkel/var_policy_rfc_core/RFC%20VariablePolicy%20Proposal%20Presentation.pptx https://github.com/corthon/edk2/blob/personal/brbarkel/var_policy_rfc_core/RFC%20VariablePolicy%20Whitepaper.docx
(You’d need to download those to view.)

My ultimate intention for this is to submit it as a series of patches for acceptance into EDK2 as a replacement for VarLock. For now, I’m just looking for initial feedback on any broad changes that might be needed to get this into shape for more detailed code review on the devel list.

Thanks!

- Bret



[-- Attachment #2: Type: text/html, Size: 8793 bytes --]

Re: [edk2-devel] [RFC] VariablePolicy - Protocol, Libraries, and Implementation for VariableLock Alternative

[Kevin@Insyde]

[-- Attachment #1: Type: text/plain, Size: 3414 bytes --]

Bret,

We like the new functionality.

Our concern is our customers / we will need to modify all of the code that are consumers of EDKII_VARIABLE_LOCK_PROTOCOL to use the new protocols.  If you could review that issue we would be 100% happy.

Of course, that’s not always appropriate and we understand.

Kevin D Davis
Insyde Software

> On Feb 4, 2020, at 2:07 AM, Bret Barkelew via Groups.Io <bret.barkelew=microsoft.com@groups.io> wrote:
> 
> 
> Expanding the audience beyond the RFC list….
> If no one has additional input, I’ll try to start formatting these as patches later this week. Thanks!
>
> - Bret
>
> From: Bret Barkelew
> Sent: Tuesday, January 28, 2020 5:36 PM
> To: rfc@edk2.groups.io
> Subject: [RFC] VariablePolicy - Protocol, Libraries, and Implementation for VariableLock Alternative
>
> All,
>
> VariablePolicy is our proposal for an expanded “VarLock-like” interface to constrain and govern platform variables.
> I brought this up back in May to get initial comments on the interface and implications of the interface and the approach. We implemented it in Mu over the summer and it is not our defacto variable solution. It plugs in easily to the existing variable infrastructure, but does want to control some of the things that are currently managed by VarLock.
>
> There are also some tweaks that would be needed if this were desired to be 100% optional code, but that’s no different than the current VarLock implementation which has implementation code directly tied to some of the common variable code.
>
> I’ve structured this RFC in two pieces:
> The Core piece represents the minimum changes needed to implement Variable Policy and integrate it into Variable Services. It contains core driver code, central libraries and headers, and DXE driver for the protocol interface.
> The Extras piece contains recommended code for a full-feature implementation including a replacement for the VarLock protocol that enables existing code to continue functioning as-is. It also contains unit and integration tests. And as a bonus, it has a Rust implementation of the core business logic for Variable Policy.
>
> The code can be found in the following two branches:
> https://github.com/corthon/edk2/tree/personal/brbarkel/var_policy_rfc_core
> https://github.com/corthon/edk2/tree/personal/brbarkel/var_policy_rfc_extra
>
> A convenient way to see all the changes in one place is to look at a comparison:
> https://github.com/corthon/edk2/compare/master...corthon:personal/brbarkel/var_policy_rfc_core
> https://github.com/corthon/edk2/compare/personal/brbarkel/var_policy_rfc_core...corthon:personal/brbarkel/var_policy_rfc_extra
>
> There’s additional documentation in the PPT and DOC files in the core branch:
> https://github.com/corthon/edk2/blob/personal/brbarkel/var_policy_rfc_core/RFC%20VariablePolicy%20Proposal%20Presentation.pptx https://github.com/corthon/edk2/blob/personal/brbarkel/var_policy_rfc_core/RFC%20VariablePolicy%20Whitepaper.docx
> (You’d need to download those to view.)
>
> My ultimate intention for this is to submit it as a series of patches for acceptance into EDK2 as a replacement for VarLock. For now, I’m just looking for initial feedback on any broad changes that might be needed to get this into shape for more detailed code review on the devel list.
>
> Thanks!
>
> - Bret
>
>
> 

[-- Attachment #2: Type: text/html, Size: 10064 bytes --]

Re: [edk2-devel] [RFC] VariablePolicy - Protocol, Libraries, and Implementation for VariableLock Alternative

[Yao, Jiewen]

[-- Attachment #1: Type: text/plain, Size: 4551 bytes --]

HI Bret
Thanks for the work. The design doc is very good.

Some feedback/questions below:


  1.  We have 2 variable related protocol - EDKII_VARIABLE_LOCK_PROTOCOL and EDKII_VAR_CHECK_PROTOCOL. Do you want to deprecate both? Or only deprecate EDKII_VARIABLE_LOCK_PROTOCOL?
  2.  The Function - DumpVariablePolicy() - makes me confused in the beginning. In my thought, "Dump" means to show some debug message. But here, you want to return the policy. Can we change the name to GetVariablePolicy()?
  3.  The function - DisableVariablePolicy(). Does it disable current policy engine? Does it disable any future policy engine? Does it block RegisterVariablePolicy() call?
  4.  The function - LockVariablePolicy() - Can it lock the DisableVariablePolicy() call?
  5.  The use case "In MFG Mode Variable Policy Engine is disabled, thus these VPD variables can be created. These variables are locked with lock policy type LockNow, so that these variables can't be tampered with in Customer Mode."

This seems a perfect potential attack point. If the attacker wants to modify a read-only variable. He or she may trigger the MFG mode, then no variable is locked. Then the attacker can update the RO variable then reset the system to normal mode. Is that threat considered?

Thank you
Yao Jiewen


From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Bret Barkelew via Groups.Io
Sent: Tuesday, February 4, 2020 4:08 PM
To: devel@edk2.groups.io; rfc@edk2.groups.io
Subject: Re: [edk2-devel] [RFC] VariablePolicy - Protocol, Libraries, and Implementation for VariableLock Alternative

Expanding the audience beyond the RFC list....
If no one has additional input, I'll try to start formatting these as patches later this week. Thanks!

- Bret

From: Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>
Sent: Tuesday, January 28, 2020 5:36 PM
To: rfc@edk2.groups.io<mailto:rfc@edk2.groups.io>
Subject: [RFC] VariablePolicy - Protocol, Libraries, and Implementation for VariableLock Alternative

All,

VariablePolicy is our proposal for an expanded "VarLock-like" interface to constrain and govern platform variables.
I brought this up back in May to get initial comments on the interface and implications of the interface and the approach. We implemented it in Mu over the summer and it is not our defacto variable solution. It plugs in easily to the existing variable infrastructure, but does want to control some of the things that are currently managed by VarLock.

There are also some tweaks that would be needed if this were desired to be 100% optional code, but that's no different than the current VarLock implementation which has implementation code directly tied to some of the common variable code.

I've structured this RFC in two pieces:

  *   The Core piece represents the minimum changes needed to implement Variable Policy and integrate it into Variable Services. It contains core driver code, central libraries and headers, and DXE driver for the protocol interface.
  *   The Extras piece contains recommended code for a full-feature implementation including a replacement for the VarLock protocol that enables existing code to continue functioning as-is. It also contains unit and integration tests. And as a bonus, it has a Rust implementation of the core business logic for Variable Policy.

The code can be found in the following two branches:
https://github.com/corthon/edk2/tree/personal/brbarkel/var_policy_rfc_core
https://github.com/corthon/edk2/tree/personal/brbarkel/var_policy_rfc_extra

A convenient way to see all the changes in one place is to look at a comparison:
https://github.com/corthon/edk2/compare/master...corthon:personal/brbarkel/var_policy_rfc_core
https://github.com/corthon/edk2/compare/personal/brbarkel/var_policy_rfc_core...corthon:personal/brbarkel/var_policy_rfc_extra

There's additional documentation in the PPT and DOC files in the core branch:
https://github.com/corthon/edk2/blob/personal/brbarkel/var_policy_rfc_core/RFC%20VariablePolicy%20Proposal%20Presentation.pptx https://github.com/corthon/edk2/blob/personal/brbarkel/var_policy_rfc_core/RFC%20VariablePolicy%20Whitepaper.docx
(You'd need to download those to view.)

My ultimate intention for this is to submit it as a series of patches for acceptance into EDK2 as a replacement for VarLock. For now, I'm just looking for initial feedback on any broad changes that might be needed to get this into shape for more detailed code review on the devel list.

Thanks!

- Bret




[-- Attachment #2: Type: text/html, Size: 17223 bytes --]

Re: [EXTERNAL] Re: [edk2-devel] [RFC] VariablePolicy - Protocol, Libraries, and Implementation for VariableLock Alternative

[Bret Barkelew]

[-- Attachment #1.1: Type: text/plain, Size: 6895 bytes --]

Kevin,

Agreed and we were sensitive to that in our codebase as well. Surface and other consumers had drivers expecting VarLock and we didn’t want to have to rewrite them all (at least not immediately).

If you take a look at the MuVarPolicyFoundationDxe driver in the extras branch…
It contains a number of features that we considered fundamental for building complex constructions, but we’re (strictly speaking) a core part of the Variable Policy infrastructure.
One of those features is installing a VarLock interface that leverages Variable Policy.

https://github.com/corthon/edk2/tree/personal/brbarkel/var_policy_rfc_extra/MsCorePkg/MuVarPolicyFoundationDxe

Is that something you think you could work with (renamed, of course 😉).

- Bret

From: Kevin D Davis<mailto:kevin.davis@insyde.com>
Sent: Tuesday, February 4, 2020 3:31 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>
Cc: rfc@edk2.groups.io<mailto:rfc@edk2.groups.io>
Subject: [EXTERNAL] Re: [edk2-devel] [RFC] VariablePolicy - Protocol, Libraries, and Implementation for VariableLock Alternative

Bret,

We like the new functionality.

Our concern is our customers / we will need to modify all of the code that are consumers of EDKII_VARIABLE_LOCK_PROTOCOL to use the new protocols.  If you could review that issue we would be 100% happy.

Of course, that’s not always appropriate and we understand.

Kevin D Davis
Insyde Software


On Feb 4, 2020, at 2:07 AM, Bret Barkelew via Groups.Io <bret.barkelew=microsoft.com@groups.io> wrote:

Expanding the audience beyond the RFC list….
If no one has additional input, I’ll try to start formatting these as patches later this week. Thanks!

- Bret

From: Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>
Sent: Tuesday, January 28, 2020 5:36 PM
To: rfc@edk2.groups.io<mailto:rfc@edk2.groups.io>
Subject: [RFC] VariablePolicy - Protocol, Libraries, and Implementation for VariableLock Alternative

All,

VariablePolicy is our proposal for an expanded “VarLock-like” interface to constrain and govern platform variables.
I brought this up back in May to get initial comments on the interface and implications of the interface and the approach. We implemented it in Mu over the summer and it is not our defacto variable solution. It plugs in easily to the existing variable infrastructure, but does want to control some of the things that are currently managed by VarLock.

There are also some tweaks that would be needed if this were desired to be 100% optional code, but that’s no different than the current VarLock implementation which has implementation code directly tied to some of the common variable code.

I’ve structured this RFC in two pieces:

  *   The Core piece represents the minimum changes needed to implement Variable Policy and integrate it into Variable Services. It contains core driver code, central libraries and headers, and DXE driver for the protocol interface.
  *   The Extras piece contains recommended code for a full-feature implementation including a replacement for the VarLock protocol that enables existing code to continue functioning as-is. It also contains unit and integration tests. And as a bonus, it has a Rust implementation of the core business logic for Variable Policy.

The code can be found in the following two branches:
https://github.com/corthon/edk2/tree/personal/brbarkel/var_policy_rfc_core<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcorthon%2Fedk2%2Ftree%2Fpersonal%2Fbrbarkel%2Fvar_policy_rfc_core&data=02%7C01%7Cbret.barkelew%40microsoft.com%7Cdaa80648c1244dbfd32108d7a9ca13f4%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637164558782121558&sdata=0tKS5gx17R0dsBpEXjCEK6AIh1B5R6yVyQi55BTReHo%3D&reserved=0>
https://github.com/corthon/edk2/tree/personal/brbarkel/var_policy_rfc_extra<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcorthon%2Fedk2%2Ftree%2Fpersonal%2Fbrbarkel%2Fvar_policy_rfc_extra&data=02%7C01%7Cbret.barkelew%40microsoft.com%7Cdaa80648c1244dbfd32108d7a9ca13f4%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637164558782121558&sdata=1hlFTGKyzxbaA1BM48zRZEMt%2FyJU3%2Ft96YY0REUY5gs%3D&reserved=0>

A convenient way to see all the changes in one place is to look at a comparison:
https://github.com/corthon/edk2/compare/master...corthon:personal/brbarkel/var_policy_rfc_core<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcorthon%2Fedk2%2Fcompare%2Fmaster...corthon%3Apersonal%2Fbrbarkel%2Fvar_policy_rfc_core&data=02%7C01%7Cbret.barkelew%40microsoft.com%7Cdaa80648c1244dbfd32108d7a9ca13f4%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637164558782131553&sdata=0hIT%2FOYBtfpXg6XH97IMcNusYJxa6k8E5qAhCWZ6djA%3D&reserved=0>
https://github.com/corthon/edk2/compare/personal/brbarkel/var_policy_rfc_core...corthon:personal/brbarkel/var_policy_rfc_extra<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcorthon%2Fedk2%2Fcompare%2Fpersonal%2Fbrbarkel%2Fvar_policy_rfc_core...corthon%3Apersonal%2Fbrbarkel%2Fvar_policy_rfc_extra&data=02%7C01%7Cbret.barkelew%40microsoft.com%7Cdaa80648c1244dbfd32108d7a9ca13f4%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637164558782141545&sdata=wqv2WuSxZBuIB2LaMCh5P4YblQQEKGRXEdg%2FjlhCBlY%3D&reserved=0>

There’s additional documentation in the PPT and DOC files in the core branch:
https://github.com/corthon/edk2/blob/personal/brbarkel/var_policy_rfc_core/RFC%20VariablePolicy%20Proposal%20Presentation.pptx<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcorthon%2Fedk2%2Fblob%2Fpersonal%2Fbrbarkel%2Fvar_policy_rfc_core%2FRFC%2520VariablePolicy%2520Proposal%2520Presentation.pptx&data=02%7C01%7Cbret.barkelew%40microsoft.com%7Cdaa80648c1244dbfd32108d7a9ca13f4%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637164558782141545&sdata=37mYO3wTQS%2BUsdQW88mi%2B5aglkqDogR5msPZwIGZdLg%3D&reserved=0> https://github.com/corthon/edk2/blob/personal/brbarkel/var_policy_rfc_core/RFC%20VariablePolicy%20Whitepaper.docx<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcorthon%2Fedk2%2Fblob%2Fpersonal%2Fbrbarkel%2Fvar_policy_rfc_core%2FRFC%2520VariablePolicy%2520Whitepaper.docx&data=02%7C01%7Cbret.barkelew%40microsoft.com%7Cdaa80648c1244dbfd32108d7a9ca13f4%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637164558782151542&sdata=SNug18tr6iC1VMg72mEiIPykL5LQr2aI8yGq%2B7K9tTE%3D&reserved=0>
(You’d need to download those to view.)

My ultimate intention for this is to submit it as a series of patches for acceptance into EDK2 as a replacement for VarLock. For now, I’m just looking for initial feedback on any broad changes that might be needed to get this into shape for more detailed code review on the devel list.

Thanks!

- Bret





[-- Attachment #1.2: Type: text/html, Size: 14866 bytes --]

[-- Attachment #2: CDEBFAD3057149A8A7B1785DB33F2195.png --]
[-- Type: image/png, Size: 139 bytes --]

Re: [edk2-rfc] [edk2-devel] [RFC] VariablePolicy - Protocol, Libraries, and Implementation for VariableLock Alternative

[Yao, Jiewen]

Add devel@edk2.groups.io.

> -----Original Message-----
> From: Yao, Jiewen <jiewen.yao@intel.com>
> Sent: Friday, February 7, 2020 6:13 AM
> To: brbarkel@microsoft.com; rfc@edk2.groups.io
> Cc: edk2-devel@lists.01.org
> Subject: RE: [edk2-rfc] [edk2-devel] [RFC] VariablePolicy - Protocol, Libraries,
> and Implementation for VariableLock Alternative
> 
> Thanks, Bret.
> 
> Comments below.
> 
> > -----Original Message-----
> > From: brbarkel via [] <brbarkel=microsoft.com@[]>
> > Sent: Friday, February 7, 2020 5:49 AM
> > To: Yao; Yao, Jiewen <jiewen.yao@intel.com>; rfc@edk2.groups.io
> > Subject: Re: [edk2-rfc] [edk2-devel] [RFC] VariablePolicy - Protocol, Libraries,
> > and Implementation for VariableLock Alternative
> >
> > Sorry, Jiewen. I missed this email when it came in...
> >
> > > 1.  We have 2 variable related protocol - EDKII_VARIABLE_LOCK_PROTOCOL
> > and EDKII_VAR_CHECK_PROTOCOL. Do you want to deprecate both? Or only
> > deprecate EDKII_VARIABLE_LOCK_PROTOCOL?
> >
> > I think we would recommend deprecating both. Is there much consumption of
> > the VarCheck protocol? A platform could always opt to include both (or all 3!),
> > but they wouldn't be able to immediately tell which engine rejected the
> > SetVariable.
> 
> [Jiewen] Right. That is my understanding- deprecate both.
> I saw you only mention to deprecate VarLock, but not VarCheck. That is why I
> get confused.
> 
> I don’t recommend we have 3 engines. Too burden to maintain the code.
> 
> One possible option is to implement VarLockProtocol and VarCheckProtocol on
> top of VarPolicyProtocol. (like a thunk)
> As such, there is no impact on the existing private platform code, with the
> benefit that one central engine controls everything.
> Later, we can check platform one by one. After the last one is migrated, we can
> remove VarLock and VarCheck thunk.
> 
> 
> 
> 
> > > 2.  The Function - DumpVariablePolicy() - makes me confused in the
> beginning.
> > In my thought, "Dump" means to show some debug message. But here, you
> > want to return the policy. Can we change the name to GetVariablePolicy()?
> >
> > I see your point about possible confusion. I think we would try to clarify this in
> > the function documentation. Due to the code-first approach we've taken on
> this,
> > we already have partners who have implemented the policy with
> > DumpVariablePolicy as the name and it doesn't seem like a huge problem.
> >
> > > 3.  The function - DisableVariablePolicy(). Does it disable current policy
> engine?
> > Does it disable any future policy engine? Does it block RegisterVariablePolicy()
> > call?
> >
> > Disable turns off the enforcement. You should still be able to register new
> > policies for auditing purposes (they will still be returned by
> DumpVariablePolicy),
> > but no SetVariables will be rejected.
> >
> > > 4.  The function - LockVariablePolicy() - Can it lock the DisableVariablePolicy()
> > call?
> >
> > Correct. Once the interface is locked, it cannot be disabled. Locking should be
> > performed at EndOfDxe or ReadyToBoot or wherever the platform TCB is.
> >
> > > 5.  The use case "In MFG Mode Variable Policy Engine is disabled, thus these
> > VPD variables can be created. These variables are locked with lock policy type
> > LockNow, so that these variables can't be tampered with in Customer Mode."
> >
> > Correct. The MfgMode is just a suggestion and is entirely up to the platform.
> Our
> > MfgMode (which has not been open-sourced yet, but we're working on it) will
> > call DisableVariablePolicy because our threat model indicates that
> compromising
> > MfgMode is a total compromise (there are many other attack vectors once
> > MfgMode is compromised). As such, we protect it extensively. But there is
> > nothing in the core or extra code that would automatically disable the policy
> > engine for any platform that didn't want that. It is up to the platform, however,
> > to make sure they lock the policy engine (similar to locking SMM).
> 
> [Jiewen] OK, if my understanding is correct, the DisablePolicy() should only be
> called in some special environment, but NOT a normal feature involved in the
> normal boot.
> 
> Then I am feeling we should separate the DisablePolicy from this protocol,
> because this interface is very dangerous. I don’t want any driver call it by
> mistake.
> 
> Can we split the VARIABLE_POLICY to VARIABLE_POLICY (normal boot usage) +
> VARIABLE_POLICY_CONTROL (very special usage)
> 
> Like below:
> 
> typedef struct {
>   UINT64                     Revision;
>   REGISTER_VARIABLE_POLICY   RegisterVariablePolicy;
>   DUMP_VARIABLE_POLICY       DumpVariablePolicy;
>   LOCK_VARIABLE_POLICY       LockVariablePolicy;
> } _VARIABLE_POLICY_PROTOCOL;
> 
> typedef struct {
>   UINT64                     Revision;
>   DISABLE_VARIABLE_POLICY    DisableVariablePolicy;
>   IS_VARIABLE_POLICY_ENABLED IsVariablePolicyEnabled;
> } _VARIABLE_POLICY_CONTROL_PROTOCOL;
> 
> VARIABLE_POLICY_PROTOCOL is always installed.
> 
> In normal boot, the VARIABLE_POLICY_CONTROL_PROTOCOL is NOT installed.
> As such, the policy is always enforced.
> The VARIABLE_POLICY_CONTROL_PROTOCOL is only installed in some special
> mode, controlled by PCD - maybe.
> A platform may choose to:
> 1) Use static PCD to always disable VARIABLE_POLICY_CONTROL_PROTOCOL
> installation.
> 2) Use dynamic PCD to control VARIABLE_POLICY_CONTROL_PROTOCOL
> installation only in special mode.
> 
> 
> Thank you
> Yao Jiewen
> 

Re: [edk2-devel] [edk2-rfc] [edk2-devel] [RFC] VariablePolicy - Protocol, Libraries, and Implementation for VariableLock Alternative

[Bret Barkelew]

[-- Attachment #1: Type: text/plain, Size: 1618 bytes --]

I agree with you that this function is extremely sensitive and important, and I kinda like your idea of splitting the protocol into a "policy" and "policy control", but I don't think it make practical sense to split them this way. At the very least, you end up with a chicken and egg problem where we would need to read mfg state to decide whether we need to publish the other protocol, but mfg state would also determine whether we used the protocol to disable the protection.

Furthermore, there may be architectural (e.g. off-Soc) implementations of this that would still require a "lock" signal to disable their ability to receive the disable command. These solutions wouldn't have a concept of building with a "I want more security" PCD.

Our recommendation would be to make sure that "lock" is called prior to leaving TCB, similar to SMM or other hardware locks (which the platform already has to coordinate).

Two points to help manage this:
1) We would be willing to discuss adding a HSTI bit or DeviceState flag to indicate to an OS or attestation authority that the protections are disabled for the current boot.
2) We would recommend running the VarLockAudit test that we've provided with the release to make sure that not only are protections enabled, but the parameters of your policies are configured the way the platform wants them. It was very useful to us when trying to understand the overall system state we wanted.
https://github.com/corthon/edk2/tree/personal/brbarkel/var_policy_rfc_extra/UefiTestingPkg/AuditTests/UefiVarLockAudit

Looking forward to your thoughts.
Thanks!
- Bret

[-- Attachment #2: Type: text/html, Size: 1830 bytes --]

Re: [edk2-devel] [edk2-rfc] [edk2-devel] [RFC] VariablePolicy - Protocol, Libraries, and Implementation for VariableLock Alternative

[Yao, Jiewen]

[-- Attachment #1: Type: text/plain, Size: 5600 bytes --]

Maybe I jump to an implementation too soon. I should have shared more about my thought at first. I am sorry about that.

When I first saw the DisableVarPolicy() API, I felt it is a backdoor. This is a brand-new feature that does not exist today.
(I don’t treat VarPolicy as something new, it is an enhancement of VarCheck or VarLock. I have no concern at all.)
Also disabling UEFI secure boot requires either authenticated variable or physical user presence before.
With this feature, the UEFI secure boot can be disabled by a simple function call. (Maybe that can be considered as a feature)

It also reminds me that when UEFI secure boot was introduced. Almost all attacks are just to figure out a way to disable UEFI secure boot via different ways, such as other variable or jumper.
As such, my first reaction is that I want to have a way to eliminate the DisableVarPolicy() and always enforce policy. I believe it should be a *platform choice* to support DisableVarPolicy() or not.
Maybe in MSFT platform, this is required in MFG mode. Not a problem. As long as you have SDL process to review the MFG code and make you can do the right thing when the var policy and secure boot is disabled.
Maybe for my platform, I have no confidence on that. What I want to do is just to remove this DisableVarPolicy() feature at all. Instead of review all the MFG code to make sure they are not calling DisableVarPolicy, I can just do a simple check - This function does not exist. Done! That is the simplest way for me. Many EDKII feature uses the similar way – use a PCD to control a feature on/off.

Yes, I fully agree with you that “Our recommendation would be to make sure that "lock" is called prior to leaving TCB, similar to SMM or other hardware locks (which the platform already has to coordinate).”
That is good practice everyone should follow. However, if there is a call DisableVarPolicy() in the code, it must be before Lock. (Otherwise it does not work)
What if an attack can find a path to trigger DisableVarPolicy() before lock? This recommendation does not help.

If you don’t like splitting the protocol, that is fine.
I can offer another implementation choice. Defining a PCD – PcdSupportDisableVarPolicy. Pseudo code below:
EFI_STATUS
DisableVarPolicy()
{
  If (!FeaturePcdGet(PcdSupportDisableVarPolicy)) {
    Return UNSUPPORTED
  }
  // Follow existing way
}
With those 3 lines of code, you may configure TRUE for MSFT platform, with detail SDL review in MFG mode.
I may configure FALSE for my platform with confident that no one can violate the policy in any boot mode. Simple for me. ☺


Now let’s talk about Test.
I think the unit tests and system test MSFT did are very good. We learn a lot from that.
I do appreciate your great work.
On the other hand, the test only shows the *final system state*. The attacker may find another path to bypass the protection by triggering DisableVarPolicy() to unlock the system.
(I still remember the stupid mistake I made years ago that caused a RO variable not locked in S4 resume path. Also another mistake that a system state is unlocked in S3 resume path. ☹)
It is hard to design test to catch *all possible special path*. That is my headache. If there is such function existing in my firmware, I have to keep my eye on every possible path.
That is why I would like to have a way to eliminate this function in my firmware at all (keep it simple), but leave the flexibility to keep it enabled in other firmware.

Anyway, I like your idea to extend the security state to PCR. Just in case there is violation it can be recorded.

Thank you
Yao Jiewen

From: bret.barkelew via [] <bret.barkelew=microsoft.com@[]>
Sent: Friday, February 7, 2020 2:00 PM
To: Yao; Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [edk2-rfc] [edk2-devel] [RFC] VariablePolicy - Protocol, Libraries, and Implementation for VariableLock Alternative

I agree with you that this function is extremely sensitive and important, and I kinda like your idea of splitting the protocol into a "policy" and "policy control", but I don't think it make practical sense to split them this way. At the very least, you end up with a chicken and egg problem where we would need to read mfg state to decide whether we need to publish the other protocol, but mfg state would also determine whether we used the protocol to disable the protection.

Furthermore, there may be architectural (e.g. off-Soc) implementations of this that would still require a "lock" signal to disable their ability to receive the disable command. These solutions wouldn't have a concept of building with a "I want more security" PCD.

Our recommendation would be to make sure that "lock" is called prior to leaving TCB, similar to SMM or other hardware locks (which the platform already has to coordinate).

Two points to help manage this:
1) We would be willing to discuss adding a HSTI bit or DeviceState flag to indicate to an OS or attestation authority that the protections are disabled for the current boot.
2) We would recommend running the VarLockAudit test that we've provided with the release to make sure that not only are protections enabled, but the parameters of your policies are configured the way the platform wants them. It was very useful to us when trying to understand the overall system state we wanted.
https://github.com/corthon/edk2/tree/personal/brbarkel/var_policy_rfc_extra/UefiTestingPkg/AuditTests/UefiVarLockAudit

Looking forward to your thoughts.
Thanks!
- Bret

[-- Attachment #2: Type: text/html, Size: 10067 bytes --]

Re: [RFC] VariablePolicy - Protocol, Libraries, and Implementation for VariableLock Alternative

[Wang, Sunny (HPS SW)]

Hi Bret, 

Your proposal looks good to me, and most of my questions/concerns were already answered/solved by the GOOD discussion between you and Jiewen.

For now, I just have one remaining question. Can we also make DumpVariablePolicy as a platform choice? 
DumpVariablePolicy would also expose the information about how to pass the check to the attacker. If my understanding is correct, the attacker can even use the information to unlock the variable locked by LOCK_ON_VAR_STATE type policy. Therefore, If the DumpVariablePolicy is just to allow platform tests to audit the entire policy list, can we add a PCD or a build flag check to disable it in release build? Of course, I also agree with Jiewen's point about making DisableVarPolicy as a platform choice.

By the way, I had the other question that I already got the answer from the code and your document. Since it hasn't been discussed, I think it would be good to share the answer with others. The question is how VariablePolicy deals with the multiple RegisterVariablePolicy calls with the same variable name and GUID. The answer is that the first registered policy will be the winner. The later calls (from attackers) with the same name and GUID will just get EFI_ALREADY_STARTED and fail to register the compromised policy. Therefore, RegisterVariablePolicy looks good to me as well. 


Regards,
Sunny Wang

-----Original Message-----
From: rfc@edk2.groups.io [mailto:rfc@edk2.groups.io] On Behalf Of Bret Barkelew via Groups.Io
Sent: Wednesday, January 29, 2020 9:36 AM
To: rfc@edk2.groups.io
Subject: [edk2-rfc] [RFC] VariablePolicy - Protocol, Libraries, and Implementation for VariableLock Alternative

All,

VariablePolicy is our proposal for an expanded "VarLock-like" interface to constrain and govern platform variables.
I brought this up back in May to get initial comments on the interface and implications of the interface and the approach. We implemented it in Mu over the summer and it is not our defacto variable solution. It plugs in easily to the existing variable infrastructure, but does want to control some of the things that are currently managed by VarLock.

There are also some tweaks that would be needed if this were desired to be 100% optional code, but that's no different than the current VarLock implementation which has implementation code directly tied to some of the common variable code.

I've structured this RFC in two pieces:

  *   The Core piece represents the minimum changes needed to implement Variable Policy and integrate it into Variable Services. It contains core driver code, central libraries and headers, and DXE driver for the protocol interface.
  *   The Extras piece contains recommended code for a full-feature implementation including a replacement for the VarLock protocol that enables existing code to continue functioning as-is. It also contains unit and integration tests. And as a bonus, it has a Rust implementation of the core business logic for Variable Policy.

The code can be found in the following two branches:
https://github.com/corthon/edk2/tree/personal/brbarkel/var_policy_rfc_core
https://github.com/corthon/edk2/tree/personal/brbarkel/var_policy_rfc_extra

A convenient way to see all the changes in one place is to look at a comparison:
https://github.com/corthon/edk2/compare/master...corthon:personal/brbarkel/var_policy_rfc_core
https://github.com/corthon/edk2/compare/personal/brbarkel/var_policy_rfc_core...corthon:personal/brbarkel/var_policy_rfc_extra

There's additional documentation in the PPT and DOC files in the core branch:
https://github.com/corthon/edk2/blob/personal/brbarkel/var_policy_rfc_core/RFC%20VariablePolicy%20Proposal%20Presentation.pptx https://github.com/corthon/edk2/blob/personal/brbarkel/var_policy_rfc_core/RFC%20VariablePolicy%20Whitepaper.docx
(You'd need to download those to view.)

My ultimate intention for this is to submit it as a series of patches for acceptance into EDK2 as a replacement for VarLock. For now, I'm just looking for initial feedback on any broad changes that might be needed to get this into shape for more detailed code review on the devel list.

Thanks!

- Bret