Date:   Wed, 12 Jul 2023 18:57:00 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Luca Boccassi <bluca@...ian.org>
Cc:     Daniel P. Berrangé <berrange@...hat.com>,
        Borislav Petkov <bp@...en8.de>,
        Emanuele Giuseppe Esposito <eesposit@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        Thomas Gleixner <tglx@...utronix.de>, 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 05:23:18PM +0100, Luca Boccassi wrote:
> On Wed, 12 Jul 2023 at 16:43, Greg KH <gregkh@...uxfoundation.org> wrote:
> >
> > On Wed, Jul 12, 2023 at 03:06:46PM +0100, Daniel P. Berrangé wrote:
> > > 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.
> >
> > Have you looked at the past as proof of this?
> 
> I can't quite think of relevant bugs, in the recent past. Are you
> aware of past instances of kernel module signature verification being
> broken? Or userspace being allowed to do arbitrary kernel memory
> manipulation before ExitBootServices?

Yes.

And no, I will not provide examples for obvious reasons.

> > > 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.
> >
> > Really?  I know a lot of people who would disagree...
> 
> They'd better have some convincing reasons
> 
> > > > > 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.
> >
> > And how are you going to determine this?
> 
> Same as it's done for the bootloaders - does it enable a secure boot
> bypass -> yes/no

And how are you going to determine this?  Seriously, please explain the
auditing you are going to do here and who is going to maintain it and
fund the effort?

> > > > > 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.
> >
> > "SecureBoot" is a distro-specific thing, they handle the keys, it's not
> > anything the normal kernel deals with.
> 
> Nah, you are thinking of key management, but this is not about that.
> This is about vulnerabilities that allow secure boot bypass, and the
> normal kernel very much deals with that. Kernel module signature
> enforcement, Lockdown LSM, ensuring ExitBootServices is called at the
> right time, etc.

Yes, that's all fun things but they have nothing to do with a vague idea
of "SecureBoot" that you wish to apply for your specific threat model.
They make up the potential solution for your model, but individually
don't.

> > > > > 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.
> >
> > Why are you somehow trying to differenciate between "upstream" and
> > Fedora's kernels here?
> 
> Because they _are_ different, and a secure boot bypass can be added
> via a downstream patch or an upstream change, and it is important to
> differentiate as we don't want to needlessly revoke. Having to rollout
> a new Debian kernel because there is a vulnerability in a Fedora patch
> would be a silly waste of time.

Agreed, why would any of us care about Fedora-specific patches?

> > Why does "upstream" need any of this?
> 
> Because 'upstream' is involved in the boot chain

How exactly?

> > And how can you tell if upstream "makes a mistake" that compromises
> > secure boot?  Who is going to keep track of this?  Who is auditing all
> > of our fixes/changes to verify this?  Have you looked at the past year
> > or so and actually determined if you could properly determine this?  If
> > not, why not?
> 
> The same people who are already doing this for bootloaders, I would guess.

If you don't know who is doing this work, that means the work isn't
actually happening, so none of this will ever work at all.

sorry,

greg k-h

Powered by blists - more mailing lists