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

[Andy Lutomirski]

> On May 24, 2019, at 3:41 PM, Sean Christopherson <sean.j.christopherson@xxxxxxxxx> wrote:
> 
>> On Fri, May 24, 2019 at 02:27:34PM -0700, Andy Lutomirski wrote:
>> On Fri, May 24, 2019 at 1:03 PM Sean Christopherson
>> <sean.j.christopherson@xxxxxxxxx> wrote:
>>> 
>>>> On Fri, May 24, 2019 at 12:37:44PM -0700, Andy Lutomirski wrote:
>>>>> On Fri, May 24, 2019 at 11:34 AM Xing, Cedric <cedric.xing@xxxxxxxxx> wrote:
>>>>> 
>>>>> If "initial permissions" for enclaves are less restrictive than shared
>>>>> objects, then it'd become a backdoor for circumventing LSM when enclave
>>>>> whitelisting is *not* in place. For example, an adversary may load a page,
>>>>> which would otherwise never be executable, as an executable page in EPC.
>>>>> 
>>>>> In the case a RWX page is needed, the calling process has to have a RWX
>>>>> page serving as the source for EADD so PROCESS__EXECMEM will have been
>>>>> checked. For SGX2, changing an EPC page to RWX is subject to FILE__EXECMEM
>>>>> on /dev/sgx/enclave, which I see as a security benefit because it only
>>>>> affects the enclave but not the whole process hosting it.
>>>> 
>>>> So the permission would be like FILE__EXECMOD on the source enclave
>>>> page, because it would be mapped MAP_ANONYMOUS, PROT_WRITE?
>>>> MAP_SHARED, PROT_WRITE isn't going to work because that means you can
>>>> modify the file.
>>> 
>>> Was this in response to Cedric's comment, or to my comment?
>> 
>> Yours.  I think that requiring source pages to be actually mapped W is
>> not such a great idea.
> 
> I wasn't requiring source pages to be mapped W.  At least I didn't intend
> to require W.  What I was trying to say is that SGX could trigger an
> EXECMEM check if userspace attempted to EADD or EAUG an enclave page with
> RWX permissions, e.g.:
> 
>  if ((SECINFO.PERMS & RWX) == RWX) {
>      ret = security_mmap_file(NULL, RWX, ???);
>      if (ret)
>          return ret;
>  }
> 
> But that's a moot point if we add security_enclave_load() or whatever.
> 
>> 
>>> 
>>>> I'm starting to think that looking at the source VMA permission bits
>>>> or source PTE permission bits is putting a bit too much policy into
>>>> the driver as opposed to the LSM.  How about delegating the whole
>>>> thing to an LSM hook?  The EADD operation would invoke a new hook,
>>>> something like:
>>>> 
>>>> int security_enclave_load_bytes(void *source_addr, struct
>>>> vm_area_struct *source_vma, loff_t source_offset, unsigned int
>>>> maxperm);
>>>> 
>>>> Then you don't have to muck with mapping anything PROT_EXEC.  Instead
>>>> you load from a mapping of a file and the LSM applies whatever policy
>>>> it feels appropriate.  If the first pass gets something wrong, the
>>>> application or library authors can take it up with the SELinux folks
>>>> without breaking the whole ABI :)
>>>> 
>>>> (I'm proposing passing in the source_vma because this hook would be
>>>> called with mmap_sem held for read to avoid a TOCTOU race.)
>>>> 
>>>> If we go this route, the only substantial change to the existing
>>>> driver that's needed for an initial upstream merge is the maxperm
>>>> mechanism and whatever hopefully minimal API changes are needed to
>>>> allow users to conveniently set up the mappings.  And we don't need to
>>>> worry about how to hack around mprotect() calling into the LSM,
>>>> because the LSM will actually be aware of SGX and can just do the
>>>> right thing.
>>> 
>>> This doesn't address restricting which processes can run which enclaves,
>>> it only allows restricting the build flow.  Or are you suggesting this
>>> be done in addition to whitelisting sigstructs?
>> 
>> In addition.
>> 
>> But I named the function badly and gave it a bad signature, which
>> confused you.  Let's try again:
>> 
>> int security_enclave_load_from_memory(const struct vm_area_struct
>> *source, unsigned int maxperm);
> 
> I prefer security_enclave_load(), "from_memory" seems redundant at best.

Fine with me.

> 
>> Maybe some really fancy future LSM would also want loff_t
>> source_offset, but it's probably not terribly useful.  This same
>> callback would be used for EAUG.
>> 
>> Following up on your discussion with Cedric about sigstruct, the other
>> callback would be something like:
>> 
>> int security_enclave_init(struct file *sigstruct_file);
>> 
>> The main issue I see is that we also want to control the enclave's
>> ability to have RWX pages or to change a W page to X.  We might also
>> want:
>> 
>> int security_enclave_load_zeros(unsigned int maxperm);
> 
> What's the use case for this?  @maxperm will always be at least RW in
> this case, otherwise the page is useless to the enclave, and if the
> enclave can write the page, the fact that it started as zeros is
> irrelevant.

This is how EAUG could ask if RWX is okay. If an enclave is internally doing dynamic loading, the it will need a heap page with maxperm = RWX.  (If itâs well designed, it will make it RW and then RX, either by changing SECINFO or by asking the host to mprotect() it, but it still needs the overall RWX mask.).

Also, do real SGX1 enclave formats have BSS? If so, then either we need an ioctl or load zeros or user code is going to load from /dev/zero or just from the heap, but the LSM is going to play better with an ioctl, I suspect :)

> 
>> An enclave that's going to modify its own code will need memory with
>> maxperm = RWX or WX.
>> 
>> But this is a bit awkward if the LSM's decision depends on the
>> sigstruct.  We could get fancy and require that the sigstruct be
>> supplied before any EADD operations so that the maxperm decisions can
>> depend on the sigstruct.
>> 
>> Am I making more sense now?
> 
> Yep.  Requiring .sigstruct at ECREATE would be trivial.  If we wanted
> flexibility we could do:
> 
>   int security_enclave_load(struct file *file, struct vm_area_struct *vma,
>                             unsigned long prot);
> 
> And for ultimate flexibility we could pass both .sigstruct and the file
> pointer for /dev/sgx/enclave, but that seems a bit ridiculous.

I agree.

> 
> Passing both would allow tying EXECMOD to /dev/sgx/enclave as Cedric
> wanted (without having to play games and pass /dev/sgx/enclave to
> security_enclave_load()), but I don't think there's anything fundamentally
> broken with using .sigstruct for EXECMOD.  It requires more verbose
> labeling, but that's not a bad thing.

The benefit of putting it on .sigstruct is that it can be per-enclave.

As I understand it from Fedora packaging, the way this works on distros is generally that a package will include some files and their associated labels, and, if the package needs EXECMOD, then the files are labeled with EXECMOD and the author of the relevant code might get a dirty look.

This could translate to the author of an exclave that needs RWX regions getting a dirty look without leaking this permission into other enclaves.

(In my opinion, the dirty looks are actually the best security benefit of the entire concept of LSMs making RWX difficult.  A sufficiently creative attacker can almost always bypass W^X restrictions once theyâve pwned you, but W^X makes it harder to pwn you in the first place, and SELinux makes it really obvious when packaging a program that doesnât respect W^X.  The upshot is that a lot of programs got fixed.)