Message-ID: <CAOASepN9t3jF4ajfKLG_odTZddOvXjY_DqLQhRPWhJ-imtjkgg@mail.gmail.com>
Date:   Wed, 20 Jun 2018 12:28:52 -0400
From:   Nathaniel McCallum <npmccallum@...hat.com>
To:     luto@...nel.org
Cc:     Neil Horman <nhorman@...hat.com>, jarkko.sakkinen@...ux.intel.com,
        x86@...nel.org, platform-driver-x86@...r.kernel.org,
        linux-kernel@...r.kernel.org, mingo@...hat.com,
        intel-sgx-kernel-dev@...ts.01.org, hpa@...or.com,
        dvhart@...radead.org, tglx@...utronix.de, andy@...radead.org,
        Peter Jones <pjones@...hat.com>
Subject: Re: [intel-sgx-kernel-dev] [PATCH v11 13/13] intel_sgx: in-kernel
 launch enclave

As I understand it, the current policy models under discussion look like this:

1. SGX w/o FLC (not being merged) looks like this:
  Intel CPU => (Intel signed) launch enclave => enclaves

2. SGX w/ FLC, looks like this:
  Intel CPU => kernel => launch enclave => enclaves

3. Andy is proposing this:
  Intel CPU => kernel => enclaves

This proposal is based on the fact that if the kernel can write to the
MSRs then a kernel compromise allows an attacker to run their own
launch enclave and therefore having an independent launch enclave adds
only complexity but not security.

Is it possible to restrict the ability of the kernel to change the
MSRs? For example, could a UEFI module manage the MSRs? Could the
launch enclave live entirely within that UEFI module?

4. I am suggesting this:
  Intel CPU => UEFI module => enclaves

Under this architecture, the kernel isn't involved in policy at all
and users get exactly the same freedoms they already have with Secure
Boot. Further, the launch enclave can be independently updated and
could be distributed in linux-firmware. The UEFI module can also be
shared across operating systems. If I want to have my own enclave
policy, then I can build the UEFI module myself, with my
modifications, and I can disable Secure Boot. Alternatively,
distributions that want to set their own policies can build their own
UEFI module and sign it with their vendor key.
On Mon, Jun 18, 2018 at 5:59 PM Andy Lutomirski <luto@...nel.org> wrote:
>
> On Tue, Jun 12, 2018 at 10:45 AM Neil Horman <nhorman@...hat.com> wrote:
> >
> > On Mon, Jun 11, 2018 at 09:55:29PM -0700, Andy Lutomirski wrote:
> > > On Mon, Jun 11, 2018 at 4:52 AM Neil Horman <nhorman@...hat.com> wrote:
> > > >
> > > > On Sun, Jun 10, 2018 at 10:17:13PM -0700, Andy Lutomirski wrote:
> > > > > > On Jun 9, 2018, at 10:39 PM, Andy Lutomirski <luto@...nel.org> wrote:
> > > > > >
> > > > > > On Fri, Jun 8, 2018 at 10:32 AM Jarkko Sakkinen
> > > > > > <jarkko.sakkinen@...ux.intel.com> wrote:
> > > > > >>
> > > > > >> The Launch Enclave (LE) generates cryptographic launch tokens for user
> > > > > >> enclaves. A launch token is used by EINIT to check whether the enclave
> > > > > >> is authorized to launch or not. By having its own launch enclave, Linux
> > > > > >> has full control of the enclave launch process.
> > > > > >>
> > > > > >> LE is wrapped into a user space proxy program that reads enclave
> > > > > >> signatures outputs launch tokens. The kernel-side glue code is
> > > > > >> implemented by using the user space helper framework.  The IPC between
> > > > > >> the LE proxy program and kernel is handled with an anonymous inode.
> > > > > >>
> > > > > >> The commit also adds enclave signing tool that is used by kbuild to
> > > > > >> measure and sign the launch enclave. CONFIG_INTEL_SGX_SIGNING_KEY points
> > > > > >> to a PEM-file for the 3072-bit RSA key that is used as the LE public key
> > > > > >> pair. The default location is:
> > > > > >>
> > > > > >>  drivers/platform/x86/intel_sgx/sgx_signing_key.pem
> > > > > >>
> > > > > >> If the default key does not exist kbuild will generate a random key and
> > > > > >> place it to this location. KBUILD_SGX_SIGN_PIN can be used to specify
> > > > > >> the passphrase for the LE public key.
> > > > > >
> > > > > > It seems to me that it might be more useful to just commit a key pair
> > > > > > into the kernel.  As far as I know, there is no security whatsoever
> > > > > > gained by keeping the private key private, so why not make
> > > > > > reproducible builds easier by simply fixing the key?
> > > > >
> > > > > Having thought about this some more, I think that you should
> > > > > completely remove support for specifying a key. Provide a fixed key
> > > > > pair, hard code the cache, and call it a day. If you make the key
> > > > > configurable, every vendor that has any vendor keys (Debian, Ubuntu,
> > > > > Fedora, Red Hat, SuSE, Clear Linux, etc) will see that config option
> > > > > and set up their own key pair for no gain whatsoever.  Instead, it'll
> > > > > give some illusion of security and it'll slow down operations in a VM
> > > > > guest due to swapping out the values of the MSRs.  And, if the code to
> > > > > support a locked MSR that just happens to have the right value stays
> > > > > in the kernel, then we'll risk having vendors actually ship one
> > > > > distro's public key hash, and that will seriously suck.
> > > > >
> > > > If you hard code the key pair however, doesn't that imply that anyone can sign a
> > > > user space binary as a launch enclave, and potentially gain control of the token
> > > > granting process?
> > >
> > > Yes and no.
> > >
> > > First of all, the kernel driver shouldn't be allowing user code to
> > > launch a launch enclave regardless of signature.  I haven't gotten far
> > > enough in reviewing the code to see whether that's the case, but if
> > > it's not, it should be fixed before it's merged.
> > >
> > Ok, I agree with you here.
> >
> > > But keep in mind that control of the token granting process is not the
> > > same thing as control over the right to launch an enclave.  On systems
> > > without the LE hash MSRs, Intel controls the token granting process
> > > and, barring some attack, an enclave that isn't blessed by Intel can't
> > > be launched.  Support for that model will not be merged into upstream
> > > Linux.  But on systems that have the LE hash MSRs and leave them
> > > unlocked, there is effectively no hardware-enforced launch control.
> > > Instead we have good old kernel policy.  If a user wants to launch an
> > > enclave, they need to get the kernel to launch the enclave, and the
> > > kernel needs to apply its policy.  The patch here (the in-kernel
> > > launch enclave) has a wide-open policy.
> > >
> >
> > Right, also agree here.  Systems without Flexible Launch Control are a
> > non-starter, we're only considering FLC systems here
> >
> > > So, as a practical matter, if every distro has their own LE key and
> > > keeps it totally safe, then a system that locks the MSRs to one
> > > distro's key makes it quite annoying to run another distro's intel_sgx
> > > driver, but there is no effect on the actual security of the system.
> > >
> > I agree that for systems that firmware-lock the msrs are annoying, but I would
> > think that IHV's would want to keep those msrs unlocked specifically to allow a
> > wide range of distributions to use this feature.
> >
> > As for benefits to security, I think there are some.  Namely, by leaving the
> > MSRS unlocked, A distribution can, rather than providing their own distirbution
> > key, pass the root of trust on to the end user.  I can easily envision a
> > downstream customer that wants to use SGX, and do so in such a way that they are
> > assured that their OS vendor doesn't have the ability to run an LE on their
> > system (at least not without the visual cue of specifying a different key hash
> > at the OS boot).
>
> Which achieves what, exactly?  The launch public key hash isn't the
> root of trust of anything except for a really awkward mechanism to
> limit the enclaves that get run.  If there is actual demand to limit
> enclaves that get run, let's do it correctly: add some code in the
> kernel that enforces a policy before launching an enclave.
>
> If the MSRs are unlocked, there is no stronger guarantee available
> even if you supply your own custom LE.  If the kernel is owned, the
> attacker can just change the MSRs.
>
> --Andy

Powered by blists - more mailing lists