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

[Stephen Smalley <sds@tycho.nsa.gov>]

On 5/16/19 6:23 PM, Xing, Cedric wrote:
> Hi Andy,
> 
>>> 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.
> 
> I understand your concern here. But I guess we are making too much assumption on how enclaves are structured/packaged. My concern is, what if a SIGSTRUCT really has to be from memory? For example, an enclave (along with its SIGSTRUCT) could be embedded inside a shared object (or even the "main" executable) so it shows up in memory to begin with. Of course it could be copied to a memfd but whatever "attributes" (e.g. path, or SELinux class/type) associated with the original file would be lost, so I'm not sure if that would work.
> 
> I'm also with you that applications tend to use read() instead of mmap() for accessing files. But in our case that'd be necessary only if .sigstruct is a separate file (hence needs to be read separately). What if (and I guess most implementations would) the SIGSTRUCT is embedded in the same file as the enclave? mmap() is the more common practice when dealing with executable images, and in that case SIGSTRUCT will have already been mmap()'d.
> 
> I'm with you again that it's kind of unprecedented to look at the backing inode. But I believe we should strive to allow as large variety of applications/usages as possible and I don't see any alternatives without losing flexibility.
> 
>>>
>>>>
>>>> /* 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.
> 
> My apology for the confusion here.
> 
> I thought EXECMOD applied to files (and memory mappings backed by them) but I was probably wrong. It sounds like EXECMOD applies to the whole process so would allow all pages within a process's address space to be modified then executed, regardless the backing files. Am I correct this time?

No, you were correct the first time I think; EXECMOD is used to control 
whether a process can make executable a private file mapping that has 
previously been modified (e.g. text relocation); it is a special case to 
support text relocations without having to allow full EXECMEM (i.e. 
execute arbitrary memory).

SELinux checks relevant to W^X include:

- EXECMEM: mmap/mprotect PROT_EXEC an anonymous mapping (regardless of 
PROT_WRITE, since we know the content has to have been written at some 
point) or a private file mapping that is also PROT_WRITE.
- EXECMOD: mprotect PROT_EXEC a private file mapping that has been 
previously modified, typically for text relocations,
- FILE__WRITE: mmap/mprotect PROT_WRITE a shared file mapping,
- FILE__EXECUTE: mmap/mprotect PROT_EXEC a file mapping.

(ignoring EXECSTACK and EXECHEAP here since they aren't really relevant 
to this discussion)

So if you want to ensure W^X, then you wouldn't allow EXECMEM for the 
process, EXECMOD by the process to any file, and the combination of both 
FILE__WRITE and FILE__EXECUTE by the process to any file.

If the /dev/sgx/enclave mappings are MAP_SHARED and you aren't using an 
anonymous inode, then I would expect that only the FILE__WRITE and 
FILE__EXECUTE checks are relevant.

> 
> I was not saying enclaves were exempt to good security practices. What I was trying to say was, EPC pages are *not* subject to the same attacks as regular pages so I suspect there will be a desire to enforce different policies on them, especially after new SGX2 features/applications become available. So I think it beneficial to distinguish between regular vs. enclave virtual ranges. And to do that, a new VM_SGX flag in VMA is probably a very simple/easy way. And with that VM_SGX flag, we could add a new security_sgx_mprot() hook so that LSM modules/policies could act differently.
> 
> And if you are with me on that bigger picture, the next question is: what should be the default behavior of security_sgx_mprot() for existing/non-SGX-aware LSM modules/policies? I'd say a reasonable default is to allow R, RW and RX, but not anything else. It'd suffice to get rid of EXECMEM/EXECMOD requirements on enclave applications. For SGX1, EPCM permissions are immutable so it really doesn't matter what security_sgx_mprot() does. For SGX2 and beyond, there's still time and new SGX-aware LSM modules/policies will probably have emerged by then.
> 
> -Cedric
>