Message-ID: <ZK6zdsIbnQFUmK69@redhat.com>
Date:   Wed, 12 Jul 2023 15:06:46 +0100
From:   Daniel P. Berrangé <berrange@...hat.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     Emanuele Giuseppe Esposito <eesposit@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        Thomas Gleixner <tglx@...utronix.de>, bluca@...ian.org,
        lennart@...ttering.net, Ingo Molnar <mingo@...hat.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Masahiro Yamada <masahiroy@...nel.org>,
        Alexander Potapenko <glider@...gle.com>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v2] x86/boot: add .sbat section to the bzImage

On Wed, Jul 12, 2023 at 03:28:40PM +0200, Borislav Petkov wrote:
> On Wed, Jul 12, 2023 at 01:48:45PM +0100, Daniel P. Berrangé wrote:
> > That doesn't make it useless, as the 3rd/4th/5th fields in the SBAT
> > file are just human targetted metadata. The validation process just
> > works off the 1st/2nd field.
> 
> It's a good thing I asked - feels like I'm just scratching the surface
> on what this thing actually is and the commit message is not explaining
> any of that.
> 
> First, second field, that's what, "linux,1"?

Each sbat CSV file line has following fields:

  component_name: the name we're comparing
  component_generation: the generation number for the comparison
  vendor_name: human readable vendor name
  vendor_package_name: human readable package name
  vendor_version: human readable package version (maybe machine parseable too, not specified here)
  vendor_url: url to look stuff up, contact, whatever.

So 'linux' is 'component_name' and '1' is component_generation

> > From a functional POV, it doesn't have to be unique identified,
> > as it is just a human targetted metadata field. A friendly git
> > version as from 'git describe' is more appropriate than a build
> > ID sha.
> 
> So can you explain what exactly that version is supposed to describe?
> Exact kernel sources the kernel was built from? Or a random, increasing
> number which tools can use to mark as bad?

AFAICT beyond being "human readable package version", it is a fairly
arbitrary decision. A release version number for formal releases, or
a 'git describe' version string for git snapshots both satisfy the
versioning requirement IMHO.

> How do you prevent people from binary-editing that section? Secure boot
> does that because that changes the signed kernel image?

The PE files are signed by the vendor who builds them, using their
SecureBoot signing key. The data covered by the signature includes
the '.sbat' section.

IOW, if you binary edit the section, the SecureBoot signature
verification fails and the kernel won't be booted.

> > > And then why does it have to be a separate section? All those
> > > requirements need to be written down.
> 
> You missed this question.

That's simply what the spec defines as the approach.

The PE file format used by EFI applications has multiple
sections and the spec has declare that the '.sbat' section
is where this data shall live.

> > The first line just identifies the file format and should
> > never change:
> > 
> >   sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
> 
> Why do you even need it then?

First it identifies the data format, and second if a
problem is ever discovered with the  SBAT concept,
a fixed approach can be indicated by changing to
'sbat,2,.....' and thus have the effect of revoking
use of any binaries which declare the 'sbat,1,....'
version. Pretty unlikely this will happen, but a useful
backup plan/safety net.

> > The second line identifies the kernel generation
> > 
> >   linux,1,The Linux Developers,linux,6.5.0-rc1,https://linux.org
> > 
> > The first field 'linux' should never change once decided upon, as it is
> > the name of the upstream project's EFI component - in this case the
> > linux kernel.
> > 
> > The second field '1' is the most important one, as it is the mechanism
> > through which revokation takes places, and the only one a human upstream
> > maintainer should manually change.
> 
> Hold on, how often are those things going to change? And who's going to
> change them? I sure hope we won't start getting patches constantly
> updating those numbers?

It is hard to predict the future, but my gut feeling is very infrequently.

I can't say I recall any specific Linux bugs that would warrant it, but
those involved in Linux/Bootloade/SecureBoot world can probably answer
this better than me. IIUC, the scope of bugs relevent to this is quite
narrow.

> > If there is discovered a flaw in Linux that allows the Secure Boot chain
> > to be broken (eg some flaw allowed linux to be exploited as a mechanism
> > to load an unsigned binary), then this 'generation' number would need
> > to be incremented when a fix is provided in upstream Linux trees.
> 
> Oh boy, there it is. And then when those fixes need to be backported to
> stable, then those patches updating that number would need to be
> backported too. I can already see the mess on the horizon.

If applicable, yes.

> > The SBAT config for shim would be updated to say 'linux,2' was the new
> > baseline, at which point it would refuse to load any binaries that still
> > had 'linux,1' in their sbat PE section.
> 
> Ok, I fetch the latest upstream kernel, it has "linux,1", shim refuses
> to load. I go, edit the sources, increment that to "linux,234" and secure
> boot works. No fixes applied.

Everything involved in the boot path is covered by signatures, so if
SecureBoot is enabled you can't simply build custom binaries. They
won't run unless you modify your EFI firmware config to trust your
own signing keys. If a user wants to compromise their own machine in
that way, that's fine.

> > When a downstream vendor builds the kernel they would actually add a
> > third record, where they append a vendor identifier to the 'linux'
> > component name, so the .sbat PE section might say.
> > 
> >  $ cat linux.sbat
> >  sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
> >  linux,1,The Linux Developers,linux,6.5.0-rc1,https://linux.org
> >  linux.fedora,1,The Fedora Project,linux,6.5.0-rc1,https://fedoraproject.org
> > 
> > this allows Fedora to deal with revokation if they make a downstream
> > only mistake that compromises SecureBoot.
> 
> What does that mean, "allows Fedora to deal with revokation"?

Lets say Fedora applied a non-upstream kernel patch that compromised
SecureBoot. Upstream Linux has no reason to change their SBAT component
generation number. Instead Fedora would change their 'linux.fedora'
component generation number to reflect their own mistake.

> In any case, I think this does not belong in the upstream kernel as this
> will turn us into CVE trackers. Distros sure, ofc, that's more along the
> lines of what they do but not the upstream kernel.

My understanding is that CVEs in general would not apply in this
scenario, the scope of bugs that are impactful is reasonably
narrow. I'll defer to people more experienced in SecureBoot though
if they have a better answer.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Powered by blists - more mailing lists