[edk2-devel] [PATCH 4/4] OvmfPkg: add TPM2_SHA1_ENABLE build option

Gerd Hoffmann posted 4 patches 4 years, 3 months ago
There is a newer version of this series
[edk2-devel] [PATCH 4/4] OvmfPkg: add TPM2_SHA1_ENABLE build option
Posted by Gerd Hoffmann 4 years, 3 months ago
Allows to compile OVMF without HashInstanceLibSha1,
i.e. no SHA1 hash support in TPM/TCG modules.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/OvmfTpmComponentsDxe.dsc.inc | 2 ++
 OvmfPkg/OvmfTpmComponentsPei.dsc.inc | 2 ++
 OvmfPkg/OvmfTpmDefines.dsc.inc       | 1 +
 3 files changed, 5 insertions(+)

diff --git a/OvmfPkg/OvmfTpmComponentsDxe.dsc.inc b/OvmfPkg/OvmfTpmComponentsDxe.dsc.inc
index 6806eb245e2b..1952a848b17c 100644
--- a/OvmfPkg/OvmfTpmComponentsDxe.dsc.inc
+++ b/OvmfPkg/OvmfTpmComponentsDxe.dsc.inc
@@ -8,7 +8,9 @@
       Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
       NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
       HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
+!if $(TPM2_SHA1_ENABLE) == TRUE
       NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
+!endif
       NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
       NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
       NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
diff --git a/OvmfPkg/OvmfTpmComponentsPei.dsc.inc b/OvmfPkg/OvmfTpmComponentsPei.dsc.inc
index 94bc124f9b78..fbe905603312 100644
--- a/OvmfPkg/OvmfTpmComponentsPei.dsc.inc
+++ b/OvmfPkg/OvmfTpmComponentsPei.dsc.inc
@@ -13,7 +13,9 @@
   SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
     <LibraryClasses>
       HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
+!if $(TPM2_SHA1_ENABLE) == TRUE
       NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
+!endif
       NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
       NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
       NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
diff --git a/OvmfPkg/OvmfTpmDefines.dsc.inc b/OvmfPkg/OvmfTpmDefines.dsc.inc
index de55cbdcf852..7db7ad7e7934 100644
--- a/OvmfPkg/OvmfTpmDefines.dsc.inc
+++ b/OvmfPkg/OvmfTpmDefines.dsc.inc
@@ -7,3 +7,4 @@
 
   # has no effect unless TPM2_ENABLE == TRUE
   DEFINE TPM1_ENABLE             = TRUE
+  DEFINE TPM2_SHA1_ENABLE        = TRUE
-- 
2.31.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82456): https://edk2.groups.io/g/devel/message/82456
Mute This Topic: https://groups.io/mt/86487987/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH 4/4] OvmfPkg: add TPM2_SHA1_ENABLE build option
Posted by Stefan Berger 4 years, 3 months ago
On 10/21/21 8:20 AM, Gerd Hoffmann wrote:
> Allows to compile OVMF without HashInstanceLibSha1,
> i.e. no SHA1 hash support in TPM/TCG modules.


Does that then mean that the SHA1 bank in a TPM 2 stays untouched, 
meaning the PCRs there won't get extended even though the bank is there 
and active?


    Stefan

>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   OvmfPkg/OvmfTpmComponentsDxe.dsc.inc | 2 ++
>   OvmfPkg/OvmfTpmComponentsPei.dsc.inc | 2 ++
>   OvmfPkg/OvmfTpmDefines.dsc.inc       | 1 +
>   3 files changed, 5 insertions(+)
>
> diff --git a/OvmfPkg/OvmfTpmComponentsDxe.dsc.inc b/OvmfPkg/OvmfTpmComponentsDxe.dsc.inc
> index 6806eb245e2b..1952a848b17c 100644
> --- a/OvmfPkg/OvmfTpmComponentsDxe.dsc.inc
> +++ b/OvmfPkg/OvmfTpmComponentsDxe.dsc.inc
> @@ -8,7 +8,9 @@
>         Tpm2DeviceLib|SecurityPkg/Library/Tpm2DeviceLibRouter/Tpm2DeviceLibRouterDxe.inf
>         NULL|SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
>         HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.inf
> +!if $(TPM2_SHA1_ENABLE) == TRUE
>         NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> +!endif
>         NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
>         NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
>         NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> diff --git a/OvmfPkg/OvmfTpmComponentsPei.dsc.inc b/OvmfPkg/OvmfTpmComponentsPei.dsc.inc
> index 94bc124f9b78..fbe905603312 100644
> --- a/OvmfPkg/OvmfTpmComponentsPei.dsc.inc
> +++ b/OvmfPkg/OvmfTpmComponentsPei.dsc.inc
> @@ -13,7 +13,9 @@
>     SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf {
>       <LibraryClasses>
>         HashLib|SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.inf
> +!if $(TPM2_SHA1_ENABLE) == TRUE
>         NULL|SecurityPkg/Library/HashInstanceLibSha1/HashInstanceLibSha1.inf
> +!endif
>         NULL|SecurityPkg/Library/HashInstanceLibSha256/HashInstanceLibSha256.inf
>         NULL|SecurityPkg/Library/HashInstanceLibSha384/HashInstanceLibSha384.inf
>         NULL|SecurityPkg/Library/HashInstanceLibSha512/HashInstanceLibSha512.inf
> diff --git a/OvmfPkg/OvmfTpmDefines.dsc.inc b/OvmfPkg/OvmfTpmDefines.dsc.inc
> index de55cbdcf852..7db7ad7e7934 100644
> --- a/OvmfPkg/OvmfTpmDefines.dsc.inc
> +++ b/OvmfPkg/OvmfTpmDefines.dsc.inc
> @@ -7,3 +7,4 @@
>   
>     # has no effect unless TPM2_ENABLE == TRUE
>     DEFINE TPM1_ENABLE             = TRUE
> +  DEFINE TPM2_SHA1_ENABLE        = TRUE


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82457): https://edk2.groups.io/g/devel/message/82457
Mute This Topic: https://groups.io/mt/86487987/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH 4/4] OvmfPkg: add TPM2_SHA1_ENABLE build option
Posted by Gerd Hoffmann 4 years, 3 months ago
On Thu, Oct 21, 2021 at 09:24:55AM -0400, Stefan Berger wrote:
> 
> On 10/21/21 8:20 AM, Gerd Hoffmann wrote:
> > Allows to compile OVMF without HashInstanceLibSha1,
> > i.e. no SHA1 hash support in TPM/TCG modules.
> 
> Does that then mean that the SHA1 bank in a TPM 2 stays untouched, meaning
> the PCRs there won't get extended even though the bank is there and active?

Not fully sure.  The tcg2 config menu looks like this:

[ ... ]
   TPM2 Active PCR Hash       SHA1, SHA256
   Algorithm
   TPM2 Hardware Supported    SHA1, SHA256, SHA384,
   Hash Algorithm             SHA512
   BIOS Supported Hash        SHA256, SHA384, SHA512
   Algorithm
[ ... ]
   TCG2 Protocol Configuration
   Supported Event Log Format TCG_2
   Hash Algorithm Bitmap      SHA256, SHA384, SHA512
   Number of PCR Banks        3
   Active PCR Banks           SHA256

     PCR Bank: SHA1           [ ]
     PCR Bank: SHA256         [X]
     PCR Bank: SHA384         [ ]
     PCR Bank: SHA512         [ ]
[ ... ]

Which looks correct to me (SHA1 bank present but not active).

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82509): https://edk2.groups.io/g/devel/message/82509
Mute This Topic: https://groups.io/mt/86487987/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH 4/4] OvmfPkg: add TPM2_SHA1_ENABLE build option
Posted by Stefan Berger 4 years, 3 months ago
On 10/22/21 2:39 AM, Gerd Hoffmann wrote:
> On Thu, Oct 21, 2021 at 09:24:55AM -0400, Stefan Berger wrote:
>> On 10/21/21 8:20 AM, Gerd Hoffmann wrote:
>>> Allows to compile OVMF without HashInstanceLibSha1,
>>> i.e. no SHA1 hash support in TPM/TCG modules.
>> Does that then mean that the SHA1 bank in a TPM 2 stays untouched, meaning
>> the PCRs there won't get extended even though the bank is there and active?
> Not fully sure.  The tcg2 config menu looks like this:
>
> [ ... ]
>     TPM2 Active PCR Hash       SHA1, SHA256
>     Algorithm
>     TPM2 Hardware Supported    SHA1, SHA256, SHA384,
>     Hash Algorithm             SHA512
>     BIOS Supported Hash        SHA256, SHA384, SHA512
>     Algorithm
> [ ... ]
>     TCG2 Protocol Configuration
>     Supported Event Log Format TCG_2
>     Hash Algorithm Bitmap      SHA256, SHA384, SHA512
>     Number of PCR Banks        3
>     Active PCR Banks           SHA256
>
>       PCR Bank: SHA1           [ ]
>       PCR Bank: SHA256         [X]
>       PCR Bank: SHA384         [ ]
>       PCR Bank: SHA512         [ ]
> [ ... ]
>
> Which looks correct to me (SHA1 bank present but not active).

I see this also but when I get into Linux and run tpm2_pcrread I see the 
SHA1 bank active but not having received any PCR extensions from the 
firmware, which is not supposed to happen. So I think you should drop 
this patch and I'll change the set of active PCR banks on the 
swtpm_setup level.

    Stefan


>
> take care,
>    Gerd
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82515): https://edk2.groups.io/g/devel/message/82515
Mute This Topic: https://groups.io/mt/86487987/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH 4/4] OvmfPkg: add TPM2_SHA1_ENABLE build option
Posted by Gerd Hoffmann 4 years, 3 months ago
  Hi,

> >     TPM2 Active PCR Hash       SHA1, SHA256
> >     Algorithm

> >     Active PCR Banks           SHA256

> I see this also but when I get into Linux and run tpm2_pcrread I see the
> SHA1 bank active but not having received any PCR extensions from the
> firmware, which is not supposed to happen.

Because of the discrepancy above I guess.

> So I think you should drop this
> patch and I'll change the set of active PCR banks on the swtpm_setup level.

Yes.  I think the code base is not ready for this.

I can disable sha1 in the tpm2 config menu, with the effect that SHA1 is
removed from the "TPM2 Active PCR Hash Algorithm" list.  But that works
only in case ovmf is built with sha1 *enabled*.

OVMF with SHA1 support disabled neither disabling the bank automatically
nor allowing me to do this manually is clearly a non-starter.  This
needs fixing before we can consider to disable SHA1 support.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82517): https://edk2.groups.io/g/devel/message/82517
Mute This Topic: https://groups.io/mt/86487987/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH 4/4] OvmfPkg: add TPM2_SHA1_ENABLE build option
Posted by James Bottomley 4 years, 3 months ago
On Fri, 2021-10-22 at 06:50 -0400, Stefan Berger wrote:
[...]
> I see this also but when I get into Linux and run tpm2_pcrread I see
> the SHA1 bank active but not having received any PCR extensions from
> the firmware, which is not supposed to happen.

That's not entirely correct: the TCG firmware profile just requires us
to log through at least one bank; it doesn't require that all active
banks be logged.  I've got several physical systems with three active
banks but only one or two measured through.

The knock on problem the
linux kernel is going to have is that we do tend to expect the sha1
bank to be extended into if any others are, so someone is going to have
to update expectations ... we should have this in hand already as sha1
is deprecated.

>  So I think you should drop this patch and I'll change the set of
> active PCR banks on the swtpm_setup level.

Even if the firmware deactivated the sha1 bank, the kernel expectation
problem is still going to exist.

James





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82518): https://edk2.groups.io/g/devel/message/82518
Mute This Topic: https://groups.io/mt/86487987/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH 4/4] OvmfPkg: add TPM2_SHA1_ENABLE build option
Posted by Stefan Berger 4 years, 3 months ago
On 10/22/21 7:49 AM, James Bottomley wrote:
> On Fri, 2021-10-22 at 06:50 -0400, Stefan Berger wrote:
> [...]
>> I see this also but when I get into Linux and run tpm2_pcrread I see
>> the SHA1 bank active but not having received any PCR extensions from
>> the firmware, which is not supposed to happen.
> That's not entirely correct: the TCG firmware profile just requires us
> to log through at least one bank; it doesn't require that all active
> banks be logged.  I've got several physical systems with three active
> banks but only one or two measured through.
The problem with this is that you can then fake measured boot on that 
system using it's unused SHA1 bank and extend into it whatever you want 
and create a fake log along with it and the quote is going to look alright.
>
> The knock on problem the
> linux kernel is going to have is that we do tend to expect the sha1
> bank to be extended into if any others are, so someone is going to have
> to update expectations ... we should have this in hand already as sha1
> is deprecated.
>
>>   So I think you should drop this patch and I'll change the set of
>> active PCR banks on the swtpm_setup level.
> Even if the firmware deactivated the sha1 bank, the kernel expectation
> problem is still going to exist.

Is that older Linux kernels or which part still requires sha1? A pointer 
would be good. I would have to revert the change to not activat ethe 
SHA1 bank from swtpm_setup if that's going to create headaches. I 
thought some hardware TPM 2's today are only providing a SHA256 bank and 
so it shouldn't be a problem.


     Stefan


>
> James
>
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82519): https://edk2.groups.io/g/devel/message/82519
Mute This Topic: https://groups.io/mt/86487987/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH 4/4] OvmfPkg: add TPM2_SHA1_ENABLE build option
Posted by James Bottomley 4 years, 3 months ago
On Fri, 2021-10-22 at 07:57 -0400, Stefan Berger wrote:
> On 10/22/21 7:49 AM, James Bottomley wrote:
> > On Fri, 2021-10-22 at 06:50 -0400, Stefan Berger wrote:
> > [...]
> > > I see this also but when I get into Linux and run tpm2_pcrread I
> > > see the SHA1 bank active but not having received any PCR
> > > extensions from the firmware, which is not supposed to happen.
> > That's not entirely correct: the TCG firmware profile just requires
> > us to log through at least one bank; it doesn't require that all
> > active banks be logged.  I've got several physical systems with
> > three active banks but only one or two measured through.
>  
> The problem with this is that you can then fake measured boot on
> that system using it's unused SHA1 bank and extend into it whatever
> you want and create a fake log along with it and the quote is going
> to look alright.

I don't think you can.  The measured boot PCRs in unused banks should
always be their default values and the measurement software should
check for this.  So on a system that only uses the sha256 bank, the
sha1 bank PCR0-7 should be all zeros ... if they aren't this should be
a measurement failure.

That means that if you try to replace the sha256 agile log with one
containing fake sha1 entries, the attestation still fails because the
sha256 bank doesn't have default entries.

> > The knock on problem the linux kernel is going to have is that we
> > do tend to expect the sha1 bank to be extended into if any others
> > are, so someone is going to have to update expectations ... we
> > should have this in hand already as sha1 is deprecated.
> > 
> > >   So I think you should drop this patch and I'll change the set
> > > of active PCR banks on the swtpm_setup level.
> >  
> > Even if the firmware deactivated the sha1 bank, the kernel
> > expectation problem is still going to exist.
> 
> Is that older Linux kernels or which part still requires sha1? A
> pointer would be good. I would have to revert the change to not
> activat ethe SHA1 bank from swtpm_setup if that's going to create
> headaches. I thought some hardware TPM 2's today are only providing a
> SHA256 bank and so it shouldn't be a problem.

The problem is IMA: it's hash is a kernel config parameter which
defaults to sha1.  It then tries to calculate the boot aggregate over
the configured hash bank and doesn't check if it's unused.

What IMA should probably be doing is working out which bank the bios is
logging through and using that as the hash instead of having it as a
Kconfig parameter.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82520): https://edk2.groups.io/g/devel/message/82520
Mute This Topic: https://groups.io/mt/86487987/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH 4/4] OvmfPkg: add TPM2_SHA1_ENABLE build option
Posted by Stefan Berger 4 years, 3 months ago
On 10/22/21 8:40 AM, James Bottomley wrote:

> On Fri, 2021-10-22 at 07:57 -0400, Stefan Berger wrote:
>> On 10/22/21 7:49 AM, James Bottomley wrote:
>>> On Fri, 2021-10-22 at 06:50 -0400, Stefan Berger wrote:
>>> [...]
>>>> I see this also but when I get into Linux and run tpm2_pcrread I
>>>> see the SHA1 bank active but not having received any PCR
>>>> extensions from the firmware, which is not supposed to happen.
>>> That's not entirely correct: the TCG firmware profile just requires
>>> us to log through at least one bank; it doesn't require that all
>>> active banks be logged.  I've got several physical systems with
>>> three active banks but only one or two measured through.
>>   
>> The problem with this is that you can then fake measured boot on
>> that system using it's unused SHA1 bank and extend into it whatever
>> you want and create a fake log along with it and the quote is going
>> to look alright.
> I don't think you can.  The measured boot PCRs in unused banks should
> always be their default values and the measurement software should
> check for this.  So on a system that only uses the sha256 bank, the
> sha1 bank PCR0-7 should be all zeros ... if they aren't this should be
> a measurement failure.
>
> That means that if you try to replace the sha256 agile log with one
> containing fake sha1 entries, the attestation still fails because the
> sha256 bank doesn't have default entries.

You can still pretend that your system only has an active SHA1 bank and 
serve the fake log. Which part would raise suspicion about that on the 
side that looks at that trusted boot log, SHA1 PCR 0-7 state, and quote 
then?


>>>>    So I think you should drop this patch and I'll change the set
>>>> of active PCR banks on the swtpm_setup level.
>>>   
>>> Even if the firmware deactivated the sha1 bank, the kernel
>>> expectation problem is still going to exist.
>> Is that older Linux kernels or which part still requires sha1? A
>> pointer would be good. I would have to revert the change to not
>> activat ethe SHA1 bank from swtpm_setup if that's going to create
>> headaches. I thought some hardware TPM 2's today are only providing a
>> SHA256 bank and so it shouldn't be a problem.
> The problem is IMA: it's hash is a kernel config parameter which
> defaults to sha1.  It then tries to calculate the boot aggregate over
> the configured hash bank and doesn't check if it's unused.
>
> What IMA should probably be doing is working out which bank the bios is
> logging through and using that as the hash instead of having it as a
> Kconfig parameter.

I think IMA is doing the right thing and extending into SHA1 and SHA256 
PCRs if the banks are active and with the boot aggregate puts a lid on 
top of the PCRs 0-7(,8-9). IMA may help raise the suspicion about abuse 
of an unused PCR bank by the firmware but looking at the measured boot 
log etc. alone I think is not enough.

At least a test with a recent kernel seems to work out alright when only 
the SHA256 bank is active.

    Stefan




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82521): https://edk2.groups.io/g/devel/message/82521
Mute This Topic: https://groups.io/mt/86487987/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH 4/4] OvmfPkg: add TPM2_SHA1_ENABLE build option
Posted by James Bottomley 4 years, 3 months ago
On Fri, 2021-10-22 at 09:13 -0400, Stefan Berger wrote:
> On 10/22/21 8:40 AM, James Bottomley wrote:
> 
> > On Fri, 2021-10-22 at 07:57 -0400, Stefan Berger wrote:
> > > On 10/22/21 7:49 AM, James Bottomley wrote:
> > > > On Fri, 2021-10-22 at 06:50 -0400, Stefan Berger wrote:
> > > > [...]
> > > > > I see this also but when I get into Linux and run
> > > > > tpm2_pcrread I see the SHA1 bank active but not having
> > > > > received any PCR extensions from the firmware, which is not
> > > > > supposed to happen.
> > > > 
> > > > That's not entirely correct: the TCG firmware profile just
> > > > requires us to log through at least one bank; it doesn't
> > > > require that all active banks be logged.  I've got several
> > > > physical systems with three active banks but only one or two
> > > > measured through.
> > >   
> > > The problem with this is that you can then fake measured boot on
> > > that system using it's unused SHA1 bank and extend into it
> > > whatever you want and create a fake log along with it and the
> > > quote is going to look alright.
> > 
> > I don't think you can.  The measured boot PCRs in unused banks
> > should always be their default values and the measurement software
> > should check for this.  So on a system that only uses the sha256
> > bank, the sha1 bank PCR0-7 should be all zeros ... if they aren't
> > this should be a measurement failure.
> > 
> > That means that if you try to replace the sha256 agile log with one
> > containing fake sha1 entries, the attestation still fails because
> > the sha256 bank doesn't have default entries.
> 
> You can still pretend that your system only has an active SHA1 bank
> and serve the fake log.

Which "You" can fake a TPM quote?  The whole design of the TPM system
is supposed to be that what goes into the TPM can't be erased, only
updated and we can get definitive proof of the values using a quote. 
You can fake the log to be sha1 only but you can't make it match the
quote that includes the sha256 banks.

> at that trusted boot log, SHA1 PCR 0-7 state, and quote then?

You don't just quote the bank you think is being logged ... you should
quote all banks of the TPM; that way you can't be duped in this
fashion.

> > > > >    So I think you should drop this patch and I'll change the
> > > > > set of active PCR banks on the swtpm_setup level.
> > > >   
> > > > Even if the firmware deactivated the sha1 bank, the kernel
> > > > expectation problem is still going to exist.
> > >  
> > > Is that older Linux kernels or which part still requires sha1? A
> > > pointer would be good. I would have to revert the change to not
> > > activat ethe SHA1 bank from swtpm_setup if that's going to create
> > > headaches. I thought some hardware TPM 2's today are only
> > > providing a SHA256 bank and so it shouldn't be a problem.
> >  
> > The problem is IMA: it's hash is a kernel config parameter which
> > defaults to sha1.  It then tries to calculate the boot aggregate
> > over the configured hash bank and doesn't check if it's unused.
> > 
> > What IMA should probably be doing is working out which bank the
> > bios is logging through and using that as the hash instead of
> > having it as a Kconfig parameter.
> 
> I think IMA is doing the right thing and extending into SHA1 and
> SHA256 PCRs if the banks are active and with the boot aggregate puts
> a lid on top of the PCRs 0-7(,8-9). IMA may help raise the suspicion
> about abuse of an unused PCR bank by the firmware but looking at the
> measured boot log etc. alone I think is not enough.

The problem is not where IMA extends, it's where it gets the boot
aggregate from.  If the IMA hash is sha1 and a sha1 bank exists, it
will use it alone for the boot aggregate.

> At least a test with a recent kernel seems to work out alright when
> only the SHA256 bank is active.

Well, yes, if IMA is configured as sha1 and no sha1 bank exists, it
will fall back to sha256, but that doesn't cover the boot aggregate
problem above.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82523): https://edk2.groups.io/g/devel/message/82523
Mute This Topic: https://groups.io/mt/86487987/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH 4/4] OvmfPkg: add TPM2_SHA1_ENABLE build option
Posted by Stefan Berger 4 years, 3 months ago
On 10/22/21 10:17 AM, James Bottomley wrote:
> On Fri, 2021-10-22 at 09:13 -0400, Stefan Berger wrote:
>> On 10/22/21 8:40 AM, James Bottomley wrote:
>>
>>> On Fri, 2021-10-22 at 07:57 -0400, Stefan Berger wrote:
>>>> On 10/22/21 7:49 AM, James Bottomley wrote:
>>>>> On Fri, 2021-10-22 at 06:50 -0400, Stefan Berger wrote:
>>>>> [...]
>>>>>> I see this also but when I get into Linux and run
>>>>>> tpm2_pcrread I see the SHA1 bank active but not having
>>>>>> received any PCR extensions from the firmware, which is not
>>>>>> supposed to happen.
>>>>> That's not entirely correct: the TCG firmware profile just
>>>>> requires us to log through at least one bank; it doesn't
>>>>> require that all active banks be logged.  I've got several
>>>>> physical systems with three active banks but only one or two
>>>>> measured through.
>>>>    
>>>> The problem with this is that you can then fake measured boot on
>>>> that system using it's unused SHA1 bank and extend into it
>>>> whatever you want and create a fake log along with it and the
>>>> quote is going to look alright.
>>> I don't think you can.  The measured boot PCRs in unused banks
>>> should always be their default values and the measurement software
>>> should check for this.  So on a system that only uses the sha256
>>> bank, the sha1 bank PCR0-7 should be all zeros ... if they aren't
>>> this should be a measurement failure.
>>>
>>> That means that if you try to replace the sha256 agile log with one
>>> containing fake sha1 entries, the attestation still fails because
>>> the sha256 bank doesn't have default entries.
>> You can still pretend that your system only has an active SHA1 bank
>> and serve the fake log.
> Which "You" can fake a TPM quote?  The whole design of the TPM system
> is supposed to be that what goes into the TPM can't be erased, only
> updated and we can get definitive proof of the values using a quote.
What I meant is the admin runs TPM2_PCR_Extend on PCRs 0-7 of the unused 
sha1 bank and extends it with known good values and has a log that goes 
with it and presents these to a validator along with the quote on the 
sha1 bank.
> You can fake the log to be sha1 only but you can't make it match the
> quote that includes the sha256 banks.

Yes, that's right. The client must insist that the sha256 bank, and any 
other possible bank, is quoted so that the system cannot just pretend 
that it only has a XYZ [sha1] bank (unlikely for TPM 2), and ABC banks 
[sha256] doesn't exist there, even though the SHA256 matches the true 
log. A quote by itself doesn't quote all the banks. You have to select 
which banks to quote and the client needs to have some control over that 
it seems to for sure see what the true firmware did.

   Stefan


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82527): https://edk2.groups.io/g/devel/message/82527
Mute This Topic: https://groups.io/mt/86487987/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH 4/4] OvmfPkg: add TPM2_SHA1_ENABLE build option
Posted by James Bottomley 4 years, 3 months ago
On Fri, 2021-10-22 at 10:52 -0400, Stefan Berger wrote:
> On 10/22/21 10:17 AM, James Bottomley wrote:
> > On Fri, 2021-10-22 at 09:13 -0400, Stefan Berger wrote:
> > > On 10/22/21 8:40 AM, James Bottomley wrote:
> > > 
> > > > On Fri, 2021-10-22 at 07:57 -0400, Stefan Berger wrote:
> > > > > On 10/22/21 7:49 AM, James Bottomley wrote:
> > > > > > On Fri, 2021-10-22 at 06:50 -0400, Stefan Berger wrote:
> > > > > > [...]
> > > > > > > I see this also but when I get into Linux and run
> > > > > > > tpm2_pcrread I see the SHA1 bank active but not having
> > > > > > > received any PCR extensions from the firmware, which is
> > > > > > > not supposed to happen.
> > > > > >  
> > > > > > That's not entirely correct: the TCG firmware profile just
> > > > > > requires us to log through at least one bank; it doesn't
> > > > > > require that all active banks be logged.  I've got several
> > > > > > physical systems with three active banks but only one or
> > > > > > two measured through.
> > > > >    
> > > > > The problem with this is that you can then fake measured boot
> > > > > on that system using it's unused SHA1 bank and extend into it
> > > > > whatever you want and create a fake log along with it and the
> > > > > quote is going to look alright.
> > > > 
> > > > I don't think you can.  The measured boot PCRs in unused banks
> > > > should always be their default values and the measurement
> > > > software should check for this.  So on a system that only uses
> > > > the sha256 bank, the sha1 bank PCR0-7 should be all zeros ...
> > > > if they aren't this should be a measurement failure.
> > > > 
> > > > That means that if you try to replace the sha256 agile log with
> > > > one containing fake sha1 entries, the attestation still fails
> > > > because the sha256 bank doesn't have default entries.
> > >  
> > > You can still pretend that your system only has an active SHA1
> > > bank and serve the fake log.
> >  
> > Which "You" can fake a TPM quote?  The whole design of the TPM
> > system is supposed to be that what goes into the TPM can't be
> > erased, only updated and we can get definitive proof of the values
> > using a quote.
>  
> What I meant is the admin runs TPM2_PCR_Extend on PCRs 0-7 of the
> unused sha1 bank and extends it with known good values and has a log
> that goes with it and presents these to a validator

Yes, I got all that.

>  along with the quote on the  sha1 bank.

The validator shouldn't accept that quote ... it should require a quote
covering all banks.  This is the point: you can't fake the quote and
the quote should cover all banks to assure you that unextended banks
really are.

> > You can fake the log to be sha1 only but you can't make it match
> > the quote that includes the sha256 banks.
> 
> Yes, that's right. The client must insist that the sha256 bank, and
> any other possible bank, is quoted so that the system cannot just
> pretend that it only has a XYZ [sha1] bank (unlikely for TPM 2),

Impossible per the TPM spec.

>  and ABC banks [sha256] doesn't exist there, even though the SHA256
> matches the true log. A quote by itself doesn't quote all the banks.
> You have to select which banks to quote and the client needs to have
> some control over that it seems to for sure see what the true
> firmware did.

Hey, I'm not going to disagree that the TPM system leaves many ways for
people to shoot themselves in the foot.  The only point I'm making is
that if you use it correctly (which I fully accept is somewhat complex)
you quote all banks and thus can't be tricked into accepting a fake log
through a bank unextended by firmware.

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82528): https://edk2.groups.io/g/devel/message/82528
Mute This Topic: https://groups.io/mt/86487987/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH 4/4] OvmfPkg: add TPM2_SHA1_ENABLE build option
Posted by Stefan Berger 4 years, 3 months ago
On 10/22/21 11:01 AM, James Bottomley wrote:
> On Fri, 2021-10-22 at 10:52 -0400, Stefan Berger wrote:
>
>>   along with the quote on the  sha1 bank.
> The validator shouldn't accept that quote ... it should require a quote
> covering all banks.  This is the point: you can't fake the quote and
> the quote should cover all banks to assure you that unextended banks
> really are.

Unfortunately this seems to be flawed on the TPM2_Quote level...

    Stefan


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82540): https://edk2.groups.io/g/devel/message/82540
Mute This Topic: https://groups.io/mt/86487987/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH 4/4] OvmfPkg: add TPM2_SHA1_ENABLE build option
Posted by James Bottomley 4 years, 3 months ago
On Fri, 2021-10-22 at 11:48 -0400, Stefan Berger wrote:
> On 10/22/21 11:01 AM, James Bottomley wrote:
> > On Fri, 2021-10-22 at 10:52 -0400, Stefan Berger wrote:
> > 
> > >   along with the quote on the  sha1 bank.
> > The validator shouldn't accept that quote ... it should require a
> > quote covering all banks.  This is the point: you can't fake the
> > quote and the quote should cover all banks to assure you that
> > unextended banks really are.
> 
> Unfortunately this seems to be flawed on the TPM2_Quote level...

In what way?

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82541): https://edk2.groups.io/g/devel/message/82541
Mute This Topic: https://groups.io/mt/86487987/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-