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

[Andy Lutomirski <luto@kernel.org>]

On Thu, May 30, 2019 at 8:04 AM Stephen Smalley <sds@tycho.nsa.gov> 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.

>
> >   - 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.

>
> >
> > And I have two design proposals.  One is static and one is dynamic.
> > To implement either one, we will probably need a new .may_mprotect vm
> > operation, and that operation can call an LSM hook.  Or we can give
> > LSMs a way to detect that a given vm_area_struct is an enclave.  As I
> > see it, this is an implementation detail that is certainly solveable.
> >
> >
> > Static proposal:
> >
> >
> > EADD takes an execute_intent flag.  It calls a new hook:
> >
> >    int security_enclave_load(struct vm_area_struct *source, bool execute_intent);
> >
> > This hook will fail if execute_intent==true and the caller has neither
> > EXECUTE, EXECMOD, nor EXECMEM.
>
> EADD execute_intent flag is originally provided by whom (userspace or
> driver) on what basis? Which file is referenced by source->vm_file? Why
> trigger all three checks up front versus only checking if needed?  Won't
> this trigger a lot of unnecessary EXECMOD and EXECMEM denials that will
> need to be dontaudit'd? What if there is a mismatch between
> execute_intent and the initial permissions?

It's provided by userspace based on whether it thinks the data in
question is enclave code.  source->vm_file is the file from which the
code is being loaded.  I'm assuming that the user code will only set
excute_intent ==true if it actually wants to execute the code, so, if
there's a denial, it will be fatal.  The normal case will be that the
request will be granted on the basis of EXECUTE.

>
> >
> > EAUG sets execute_intent = false.
> >
> > EINIT takes a sigstruct pointer.  SGX can (when initially upstreamed
> > or later on once there's demand) call a new hook:
> >
> >    security_enclave_init(struct sigstruct *sigstruct, struct
> > vm_area_struct *source);
>
> Is struct sigstruct the same as struct sgx_sigstruct in the current
> patches (i.e. just the sigstruct data, no file)?  What file is
> referenced by source->vm_file (the sigstruct or the enclave or
> /dev/sgx/enclave)?  Is this hook only for enforcing a whitelist on what
> enclaves can be loaded?  What is the target of the check?

sigstruct is just the data.  source->vm_file is the file from which
the sigstruct came, which could be a .sigstruct file or could be the
main executable or a DSO that contains an embedded enclave.  The
sigstruct data is there so that an LSM (not necessarily SELinux) could
check MRENCLAVE or MRSIGNER, and the source is there so that the
file's label can be checked.

>
> > mmap() and mprotect() will require EXECMEM to create WX or RWX
> > mappings.  They will require EXECMOD to create RX or X mappings of an
> > execute_intent==false page.  They require no permissions in the other
> > cases.
>
> Does this occur for both setting initial permissions and runtime
> permissions or just runtime? Both userspace- and driver-initiated
> mmap/mprotect operations or just userspace-initiated ones?  Does the
> driver use interfaces that call the mmap/mprotect hooks or lower level
> functions?

These would occur for any mmap(), mprotect(), or ioctl() that changes
VMA permissions.  Actually arranging for the hooks to be called is an
implementation detail that might require a new .mprotect vm_operation.
As an alternative, security_enclave_init() or similar could supply
may_execmod and may_execmem flags to the driver, and the driver could
do these checks on its own when mmap() and mprotect() happen without a
new LSM callback.

>
> >
> >
> > Dynamic proposal:
> >
> >
> > EADD does not take any special flags.  It does something like this internally:
> >
> >    bool execute_intent = true;
> >    int security_enclave_load(struct vm_area_struct *source, bool
> > *execute_intent);
> >
> > The implementation of security_enclave_load() may set *execute_intent to false.
> > The driver records execute_intent after the LSM is done.
>
> On what basis does LSM decide whether to set *execute_intent?  If the
> process lacks all three permissions? What if there is a mismatch with
> the initial permissions?
>

I think it would set *execute_intent=false if the process lacks
EXECUTE on source->vm_file.  I'm not sure any more complexity is
required.  If the enclave has EXECMOD, then it will still work on the
basis of the mmap/mprotect rules.

> >
> > mmap() and mprotect() will require EXECMEM to create WX or RWX
> > mappings.  They will require EXECMOD to create RX or X mappings of an
> > execute_intent==false page.  They require no permissions in the other
> > cases.
> >
> >
> >
> > A benefit of the static proposal is that audit failures due to a lack
> > of EXECUTE permission are easy to implement and to understand in the
> > lods.  With the dynamic model, we can only really audit the lack of
> > EXECMOD or EXECMEM.  A benefit of the dynamic model is that we hide
> > what is arguably a decently large wart from the API.
> >
>