Re: [edk2-devel] [PATCH V5 1/2] OvmfPkg: Introduce Tdx BFV/CFV PCDs and PcdOvmfImageSizeInKb

["Yao, Jiewen" <jiewen.yao@intel.com>]

Hi Min
I agree with Gerd and Ard in this case.

It is NOT so obvious that the FTW is produced then consumed in the code. What if the attacker prepares some special configuration to trigger the FTW process at the first boot, the code will do *read* before *write*? That is a potential attack surface.

In my view, the design should be so simple that it is obviously no bug.
I recommend to measure the whole CFV, to eliminate any potential attack surface, which is always the best way in security.

To ensure the developer does configure the PCD correctly, I also recommend to add "ASSERT(CfvSize == Fv->FvLength" MeasureConfigurationVolume().

Thank you
Yao Jiewen


> -----Original Message-----
> From: Xu, Min M <min.m.xu@intel.com>
> Sent: Wednesday, September 1, 2021 3:19 PM
> To: Ard Biesheuvel <ardb@kernel.org>; devel@edk2.groups.io;
> kraxel@redhat.com
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Brijesh Singh <brijesh.singh@amd.com>; Erdem
> Aktas <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>;
> Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky
> <thomas.lendacky@amd.com>
> Subject: RE: [edk2-devel] [PATCH V5 1/2] OvmfPkg: Introduce Tdx BFV/CFV PCDs
> and PcdOvmfImageSizeInKb
> 
> On September 1, 2021 2:57 PM, Ard Biesheuvel wrote:
> > On Wed, 1 Sept 2021 at 08:10, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > >
> > >   Hi,
> > >
> > > > > I didn't fully investigate what kind of attacks one can do.  I'm
> > > > > pretty sure simply making the variable store larger and the spare
> > > > > smaller works, so parts of the variable store are outside the area
> > > > > you are measuring.  Not fully sure whenever one can actually
> > > > > reorder the sections to move the varstore completely into the
> > > > > unmeasured area.  Or play out other attacks with the same effect, like
> > bloating some header struct.
> > > > >
> > > > > Simply measuring everything (including the spare) will stop all that.
> > > > > Changes wouldn't go unnoticed, period.  No ifs and buts.  So I'm
> > > > > wondering why you not doing that?  Performance?  Wouldn't be the
> > > > > first time a performance optimization pokes a hole into a security
> > concept ...
> > > > >
> > > > The measurement value of the CFV (provisioned configuration data) is
> > > > extended to RTMR registers (similar to TPM PCRs). At the same time
> > > > it is recorded in the TD Event log.
> > > > These information will be used by the Attestation server (This is the so-
> called
> > Attestation).
> > > > In other words there is a known *good* CFV measurement value. Any
> > > > changes to the CFV, for example the layout, the order of the
> > > > variables, the content of the variables will produce a *bad* CFV
> > measurement.
> > >
> > > Yes.  The attacker would need a varstore with a modified layout being
> > > approved by the attestation server as first step, then he would be
> > > able to modify variables unnoticed in a second step.
> > >
> > > So, assuming an attacker isn't able to carry out the first step it
> > > should be all fine in theory.  When it comes to security it never
> > > hurts to have another line of defense though, so I would still
> > > strongly recommend to measure the complete varstore (including spare).
> > >
> > > At the end of the day it is your call, I'm not going to veto the patch.
> > > But I'll reserve the right to pull a "told you so" in case someone
> > > manages to exploit that some day.
> > >
> >
> > Have to agree with Gerd here: if those contents are being interpreted by the
> > code, and may therefore affect its execution, I don't think it should be omitted
> > from the measurement unless there is a compelling reason for it. Omitting it
> > simply because you can doesn't seem sufficient justification to me.
> CFV (the variables part) is treated as external input. For example, the secure
> boot
> variables. From the security perspective external input maybe modified by some
> malicious users. That's why it is measured so that in the later Attestation can
> find
> the modification. This is the same reason why the data downloaded from QEMU
> (thru fw_cfg) should be measured.
> As to the spare part in varstore, it is not external input, is it? It's produced and
> consumed
> by code itself. From this perspective it should not be measured. If the spare part
> is included in the measurement, then the *good* measurement is not known
> anymore.
> Because no one knows about the content of spare part in advance.
> >
> > --
> > Ard.