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

[Andy Lutomirski <luto@kernel.org>]

> On May 15, 2019, at 8:03 PM, Xing, Cedric <cedric.xing@intel.com> wrote:
>
> Hi Andy,
>
>> From: Andy Lutomirski [mailto:luto@kernel.org]
>>
>>> On Wed, May 15, 2019 at 3:46 PM James Morris <jmorris@namei.org> wrote:
>>>
>>> On Wed, 15 May 2019, Andy Lutomirski wrote:
>>>
>>>>> Why not just use an xattr, like security.sgx ?
>>>>
>>>> Wouldn't this make it so that only someone with CAP_MAC_ADMIN could
>>>> install an enclave?  I think that this decision should be left up the
>>>> administrator, and it should be easy to set up a loose policy where
>>>> anyone can load whatever enclave they want.  That's what would happen
>>>> in my proposal if there was no LSM loaded or of the LSM policy didn't
>>>> restrict what .sigstruct files were acceptable.
>>>>
>>>
>>> You could try user.sigstruct, which does not require any privs.
>>>
>>
>> I don't think I understand your proposal.  What file would this
>> attribute be on?  What would consume it?
>>
>> I'm imagining that there's some enclave in a file
>> crypto_thingy.enclave.  There's also a file crypto_thingy.sigstruct.
>> crypto_thingy.enclave has type lib_t or similar so that it's
>> executable.  crypto_thingy.sigstruct has type sgx_sigstruct_t.  The
>> enclave loader does, in effect:
>>
>> void *source_data = mmap(crypto_thingy.enclave, PROT_READ | PROT_EXEC, ...);
>> int sigstruct_fd = open("crypto_thingy.sigstruct", O_RDONLY);
>> int enclave_fd = open("/dev/sgx/enclave", O_RDWR);
>>
>> ioctl(enclave_fd, SGX_IOC_ADD_SOME_DATA, source_data + source_offset,
>> enclave_offset, len, ...);
>> ioctl(enclave_fd, SGX_IOC_ADD_SOME_DATA, source_data + source_offset2,
>> enclave_offset2, len, ...);
>> etc.
>>
>> /* Here's where LSMs get to check that the sigstruct is acceptable.
>> The CPU will check that the sigstruct matches the enclave. */
>> ioctl(enclave_fd, SGX_INIT_THE_ENCLAVE, sigstruct_fd);
>
> SIGSTRUCT isn't necessarily stored on disk so may not always have a fd. How about the following?
> void *ss_pointer = mmap(sigstruct_fd, PROT_READ,...);
> ioctl(enclave_fd, SGX_INIT_THE_ENCLAVE, ss_pointer);
>
> The idea here is SIGSTRUCT will still be passed in memory so it works the same way when no LSM modules are loaded or basing its decision on the .sigstruct file. Otherwise, an LSM module can figure out the backing file (and offset within that file) by looking into the VMA covering ss_pointer.

I don’t love this approach.  Application authors seem likely to use
read() instead of mmap(), and it’ll still work in many cares. It would
also complicate the kernel implementation, and looking at the inode
backing the vma that backs a pointer is at least rather unusual.
Instead, if the sigstruct isn’t on disk because it’s dynamic or came
from a network, the application can put it in a memfd.

>
>>
>> /* Actually map the thing */
>> mmap(enclave_fd RO section, PROT_READ, ...);
>> mmap(enclave_fd RW section, PROT_READ | PROT_WRITE, ...);
>> mmap(enclave_fd RX section, PROT_READ | PROT_EXEC, ...);
>>
>> /* This should fail unless EXECMOD is available, I think */
>> mmap(enclave_fd RWX section, PROT_READ | PROT_WRITE | PROT_EXEC);
>>
>> And the idea here is that, if the .enclave file isn't mapped
>> PROT_EXEC, then mmapping the RX section will also require EXECMEM or
>> EXECMOD.
>
> From security perspective, I think it reasonable to give EXECMEM and EXECMOD to /dev/sgx/enclave because the actual permissions are guarded by EPCM permissions, which are "inherited" from the source pages, whose permissions have passed LSM checks.

I disagree.  If you deny a program EXECMOD, it’s not because you
distrust the program. It’s because you want to enforce good security
practices.  (Or you’re Apple and want to disallow third-party JITs.)
A policy that accepts any sigstruct but requires that enclaves come
from disk and respect W^X seems entirely reasonable.

I think that blocking EXECMOD has likely served two very real security
purposes. It helps force application and library developers to write
and compile their code in a way that doesn’t rely on dangerous tricks
like putting executable trampolines on the stack.  It also makes it
essentially impossible for an exploit to run actual downloaded machine
code — if there is no way to run code that isn’t appropriately
labeled, then attackers are more limited in what they can do.

I don’t think that SGX should become an exception to either of these.
Code should not have an excuse to use WX memory just because it’s in
an enclave. Similarly, an exploit should not be able to run an
attacker-supplied enclave as a way around a policy that would
otherwise prevent downloaded code from running.


—Andy