Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)

[Sean Christopherson]

On Thu, May 30, 2019 at 09:14:10AM -0700, Andy Lutomirski wrote:
> On Thu, May 30, 2019 at 8:04 AM Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
> >
> > On 5/30/19 10:31 AM, Andy Lutomirski wrote:
> > > Hi all-
> > >
> > > After an offline discussion with Sean yesterday, here are some updates
> > > to the user API parts of my proposal.
> > >
> > > Unfortunately, Sean convinced me that MAXPERM doesn't work the way I
> > > described it because, for SGX2, the enclave loader won't know at load
> > > time whether a given EAUG-ed page will ever be executed.  So here's an
> > > update.
> > >
> > > First, here are the requrements as I see them, where EXECUTE, EXECMOD,
> > > and EXECMEM could be substituted with other rules at the LSM's
> > > discretion:
> > >
> > >   - You can create a WX or RWX mapping if and only if you have EXECMEM.
> > >
> > >   - To create an X mapping of an enclave page that has ever been W, you
> > > need EXECMOD.
> >
> > EXECMOD to what file? The enclave file from which the page's content
> > originated, the sigstruct file, or /dev/sgx/enclave?
> 
> I leave that decision to you :)  The user should need permission to do
> an execmod thing on an enclave, however that wants to be encoded.

But that decision dictates how the SGX API handles sigstruct.  If LSMs
want to associate EXECMOD with sigstruct, then SGX needs to take sigstruct
early and hold a reference to the file for the lifetime of the enclave.
And if we're going to do that, the whole approach of inheriting
permissions from source VMAs becomes unnecessary complexity.

> >
> > >   - To create an X mapping of an enclave page that came from EADD, you
> > > need EXECUTE on the source file.  Optionally, we could also permit
> > > this if you have EXECMOD.
> >
> > What is the "source file" i.e. the target of the check?  Enclave file,
> > sigstruct file, or /dev/sgx/enclave?
> 
> Enclave file -- that is, the file backing the vma from which the data is loaded.

It wasn't explicitly called out in Andy's proposal(s), but the idea is
that the SGX driver would effectively inherit permissions from the source
VMA (EADD needs a source for the initial value of the encave page).

I have two gripes with that approach:

  - Requires enclave builder to mark enclave pages executable in the
    non-enclave VMAs, which may unnecessarily require EXECMOD on the
    source file, or even worse, EXECMEM, and potentially increases the
    attack surface since the file must be executable.

  - Is completely unnecessary if the enclave holds a reference to the
    sigstruct file, as LSMs can easily apply labels to the sigstruct,
    e.g. EXECUTE on the sigstruct instead of EXECUTE on the source file.


After the bajillion mails we've generated, AIUI we've come up with two
concepts that are viable: inheriting permissions from the source VMA
vs. using sigstruct as a proxy for the enclave.  Andy's proposals rely on
the inheritance concept.  The proposal below is based on the sigstruct
proxy concept.

For those not familiar with SGX details, sigstruct can be used as a proxy
because hardware enforces that the measurement stored in the sigstruct
exactly matches the measurement generated by the enclave build process,
e.g. adding a page as RX instead of R will change the measurement.

Core Concepts:
  - FILE_{READ,WRITE,EXEC} on /dev/sgx/enclave effectively gates access to
    EPC.  All real world enclaves will need all three permissions.
  - sigstruct is the proxy for enclave from an LSM perspective, e.g.
    SELinux can define ENCLAVE__EXECUTE and ENCLAVE__EXECMOD and apply
    them to the sigstruct file.
  - Take sigstruct at ECREATE so that ADD_REGION immediately followed by
    mprotect() works as expected (because SGX.mprotect() needs sigstruct
    to pass to security_enclave_mprotect(), see below).
  - SGX driver takes a reference to the backing sigstruct file if it
    exists so that the file can be provided to LSMs during mprotect().
  - Optional: SGX driver *requires* sigstruct to be backed by file, purely
    to enforce userspace infrastructure is in place for LSM support.

W^X handling:
  - mmap() to /dev/sgx/enclave only allowed with PROT_NONE, i.e. force
    userspace through mprotect() to simplify the kernel implementation.
  - Add vm_ops mprotect() ops hook (I'll refer to SGX's implementation
    as SGX.mprotect())
  - Take explicit ALLOW_WRITE at ADD_REGION, a.k.a. EADD
  - ADD_REGION also used to describe EAUG region (tentatively for SGX2).
  - Track "can be written at some point in time (past or future)" as
    ALLOW_WRITE (to avoid confusiong with MAY_WRITE).  A priori knowledge
    of writability avoids having to track/coordinate PROT_WRITE across
    VMAs and MMs.
  - SGX.mprotect() returns -EPERM if PROT_WRITE && !ALLOW_WRITE.
  - Add security_enclave_mprotect() LSM hook, called by SGX.mprotect(),
    e.g. int security_enclave_mprotect(struct file *sigstruct,
                                       unsigned long prot,
                                       bool allow_write)
  - Intention is that EXECMOD is required if PROT_EXEC and ALLOW_WRITE.

Enclave {white,black}listing:
  - Optional/Future: add security_enclave_create(), invoked during
    SGX ECREATE ioctl(), e.g.
       int security_enclave_create(struct vm_area_struct *sigstruct)

  - If this LSM hook is implemented, having sigstruct at ECREATE
    allows LSMs to determine whether or not the enclave is allowed to
    execute before allocating EPC for the enclave, e.g. unwanted enclaves
    can't DoS wanted enclaves.

LSM implementation possibilities:

  - Define ENCLAVE__EXECUTE and ENCLAVE__EXECMOD, require them on the
    process.  Does not require sigstruct to be backed by file, but cannot
    achieve per-enclave granularity.  

  - Define ENCLAVE__EXECUTE and ENCLAVE__EXECMOD, require them on the
    sigstruct, i.e. force sigstruct to reside in filesystem.  Allows
    per-enclave granularity.

  - Reuse FILE__EXECUTE and FILE__EXECMOD on sigstruct.  Likely has
    implications that may or may not be concerning, e.g. the sigstruct
    file itself is weirdly executable.

  - Adding ENCLAVE__EXECUTE and ENCLAVE__EXECMOD means the sigstruct,
    which may be emdedded in the same file as the enclave, does *not*
    require FILE__EXECUTE or FILE__EXECMOD, e.g. can be read-only.

  - LSMs can (will?) require ENCLAVE__EXECUTE and ENCLAVE__EXECMOD to
    effectively map an enclave, even if the process acquired the enclave
    via SCM_RIGHTS (enclaves are tracked by fds).  This is good or bad
    depending on your perspective.

Userspace changes:

  - EADD ioctl adds flags param to take ALLOW_WRITE

  - ECREATE ioctl takes sigstruct instead of EINIT

  - Initial mmap() must be PROT_NONE.

  - sigstruct likely needs to reside in a file (this may not affect
    some userspace implementations).