| Message ID | 20190619222401.14942-1-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | security: x86/sgx: SGX vs. LSM | expand |
On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote:
> [SNAP]
The reason for delaying my proposal for encl->refcount (which I will
post as RFC and won't merge it before getting screened from you) was
that I really focused making the most well thought comments to this.
Generally I think that we need to get your SRCU changes in before
anything in the form that the patch does not anything else but only
that. Without doing that comparing changes dealing with the life-cycle
of an enclave is way too much mind bending.
/Jarkko
On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote: I still don't get why we need this whole mess and do not simply admit that there are two distinct roles: 1. Creator 2. User In the SELinux context Creator needs FILE__WRITE and FILE__EXECUTE but User does not. It just gets the fd from the Creator. I'm sure that all the SGX2 related functionality can be solved somehow in this role playing game. An example would be the usual case where enclave is actually a loader that loads the actual piece of software that one wants to run. Things simply need to be designed in a way the Creator runs the loader part. These are non-trivial problems but oddball security model is not going to make them disappear - on the contrary it will make designing user space only more complicated. I think this is classical example of when something overly complicated is invented in the kernel only to realize that it should be solved in the user space. It would not be like the only use case where some kind of privileged daemon is used for managing some a kernel provided resource. I think a really good conclusion from this discussion that has taken two months is to realize that nothing needs to be done in this area (except *maybe* noexec check). /Jarkko
On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote: > On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote: > > I still don't get why we need this whole mess and do not simply admit > that there are two distinct roles: > > 1. Creator > 2. User Because SELinux has existing concepts of EXECMEM and EXECMOD. > In the SELinux context Creator needs FILE__WRITE and FILE__EXECUTE but > User does not. It just gets the fd from the Creator. I'm sure that all > the SGX2 related functionality can be solved somehow in this role > playing game. > > An example would be the usual case where enclave is actually a loader > that loads the actual piece of software that one wants to run. Things > simply need to be designed in a way the Creator runs the loader part. > These are non-trivial problems but oddball security model is not going > to make them disappear - on the contrary it will make designing user > space only more complicated. > > I think this is classical example of when something overly complicated > is invented in the kernel only to realize that it should be solved in > the user space. > > It would not be like the only use case where some kind of privileged > daemon is used for managing some a kernel provided resource. > > I think a really good conclusion from this discussion that has taken two > months is to realize that nothing needs to be done in this area (except > *maybe* noexec check). Hmm, IMO we need to support at least equivalents to EXECMEM and EXECMOD. That being said, we can do so without functional changes to the SGX uapi, e.g. add reserved fields so that the initial uapi can be extended *if* we decide to go with the "userspace provides maximal protections" path, and use the EPCM permissions as the maximal protections for the initial upstreaming. That'd give us a minimal implemenation for initial upstreaming and would eliminate Cedric's blocking complaint. The "whole mess" of whitelisting, blacklisting and SGX2 support would be deferred until post-upstreaming.
On 7/8/2019 10:29 AM, Sean Christopherson wrote: > On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote: >> On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote: >> >> I still don't get why we need this whole mess and do not simply admit >> that there are two distinct roles: >> >> 1. Creator >> 2. User > > Because SELinux has existing concepts of EXECMEM and EXECMOD. > >> In the SELinux context Creator needs FILE__WRITE and FILE__EXECUTE but >> User does not. It just gets the fd from the Creator. I'm sure that all >> the SGX2 related functionality can be solved somehow in this role >> playing game. >> >> An example would be the usual case where enclave is actually a loader >> that loads the actual piece of software that one wants to run. Things >> simply need to be designed in a way the Creator runs the loader part. >> These are non-trivial problems but oddball security model is not going >> to make them disappear - on the contrary it will make designing user >> space only more complicated. >> >> I think this is classical example of when something overly complicated >> is invented in the kernel only to realize that it should be solved in >> the user space. Are you talking about changing enclave loaders in user mode? That'd break all existing code. I don't think we shall ever consider this approach. >> >> It would not be like the only use case where some kind of privileged >> daemon is used for managing some a kernel provided resource. >> >> I think a really good conclusion from this discussion that has taken two >> months is to realize that nothing needs to be done in this area (except >> *maybe* noexec check). > > Hmm, IMO we need to support at least equivalents to EXECMEM and EXECMOD. > > That being said, we can do so without functional changes to the SGX uapi, > e.g. add reserved fields so that the initial uapi can be extended *if* we > decide to go with the "userspace provides maximal protections" path, and > use the EPCM permissions as the maximal protections for the initial > upstreaming. > > That'd give us a minimal implemenation for initial upstreaming and would > eliminate Cedric's blocking complaint. The "whole mess" of whitelisting, > blacklisting and SGX2 support would be deferred until post-upstreaming. >
On Mon, Jul 08, 2019 at 10:29:30AM -0700, Sean Christopherson wrote: > On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote: > > On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote: > > > > I still don't get why we need this whole mess and do not simply admit > > that there are two distinct roles: > > > > 1. Creator > > 2. User > > Because SELinux has existing concepts of EXECMEM and EXECMOD. What is the official documentation for those? I've only found some explanations from discussions and some RHEL sysadmin guides. > That being said, we can do so without functional changes to the SGX uapi, > e.g. add reserved fields so that the initial uapi can be extended *if* we > decide to go with the "userspace provides maximal protections" path, and > use the EPCM permissions as the maximal protections for the initial > upstreaming. > > That'd give us a minimal implemenation for initial upstreaming and would > eliminate Cedric's blocking complaint. The "whole mess" of whitelisting, > blacklisting and SGX2 support would be deferred until post-upstreaming. I'd like that approach more too. /Jarkko
On Tue, Jul 09, 2019 at 07:22:03PM +0300, Jarkko Sakkinen wrote: > On Mon, Jul 08, 2019 at 10:29:30AM -0700, Sean Christopherson wrote: > > On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote: > > > On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote: > > > > > > I still don't get why we need this whole mess and do not simply admit > > > that there are two distinct roles: > > > > > > 1. Creator > > > 2. User > > > > Because SELinux has existing concepts of EXECMEM and EXECMOD. > > What is the official documentation for those? I've only found some > explanations from discussions and some RHEL sysadmin guides. No clue. My knowledge was gleaned from the code and from Stephen's feedback. The high level breakdown: - FILE__EXECUTE: required to gain X on a mapping to a regular file - FILE__EXECUTE + FILE__WRITE: required to gain WX or W->X on a shared mapping to a regular file - FILE__EXECMOD: required to gain W->X on a private mapping of a regular file - PROCESS__EXECMEM: required to gain WX on a private mapping to a regular file, OR to gain X on an anonymous mapping. Translating those to SGX, with a lot of input from Stephen, I ended up with the following: - FILE__ENCLAVE_EXECUTE: equivalent to FILE__EXECUTE, required to gain X on an enclave page loaded from a regular file - PROCESS2__ENCLAVE_EXECDIRTY: hybrid of EXECMOD and EXECUTE+WRITE, required to gain W->X on an enclave page - PROCESS2__ENCLAVE_EXECANON: subset of EXECMEM, required to gain X on an enclave page that is loaded from an anonymous mapping - PROCESS2__ENCLAVE_MAPWX: subset of EXECMEM, required to gain WX on an enclave page > > That being said, we can do so without functional changes to the SGX uapi, > > e.g. add reserved fields so that the initial uapi can be extended *if* we > > decide to go with the "userspace provides maximal protections" path, and > > use the EPCM permissions as the maximal protections for the initial > > upstreaming. > > > > That'd give us a minimal implemenation for initial upstreaming and would > > eliminate Cedric's blocking complaint. The "whole mess" of whitelisting, > > blacklisting and SGX2 support would be deferred until post-upstreaming. > > I'd like that approach more too. > > /Jarkko
On 7/9/2019 10:09 AM, Sean Christopherson wrote: > On Tue, Jul 09, 2019 at 07:22:03PM +0300, Jarkko Sakkinen wrote: >> On Mon, Jul 08, 2019 at 10:29:30AM -0700, Sean Christopherson wrote: >>> On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote: >>>> On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote: >>>> >>>> I still don't get why we need this whole mess and do not simply admit >>>> that there are two distinct roles: >>>> >>>> 1. Creator >>>> 2. User >>> >>> Because SELinux has existing concepts of EXECMEM and EXECMOD. >> >> What is the official documentation for those? I've only found some >> explanations from discussions and some RHEL sysadmin guides. > > No clue. My knowledge was gleaned from the code and from Stephen's > feedback. > > > The high level breakdown: > > - FILE__EXECUTE: required to gain X on a mapping to a regular file > > > - FILE__EXECUTE + FILE__WRITE: required to gain WX or W->X on a shared > mapping to a regular file > > - FILE__EXECMOD: required to gain W->X on a private mapping of a regular > file > > - PROCESS__EXECMEM: required to gain WX on a private mapping to a regular > file, OR to gain X on an anonymous mapping. > > > Translating those to SGX, with a lot of input from Stephen, I ended up > with the following: > > - FILE__ENCLAVE_EXECUTE: equivalent to FILE__EXECUTE, required to gain X > on an enclave page loaded from a regular file > > - PROCESS2__ENCLAVE_EXECDIRTY: hybrid of EXECMOD and EXECUTE+WRITE, > required to gain W->X on an enclave page EXECMOD basically indicates a file containing self-modifying code. Your ENCLAVE_EXECDIRTY is however a process permission, which is illogical. > - PROCESS2__ENCLAVE_EXECANON: subset of EXECMEM, required to gain X on > an enclave page that is loaded from an > anonymous mapping > > - PROCESS2__ENCLAVE_MAPWX: subset of EXECMEM, required to gain WX on an > enclave page > > > >>> That being said, we can do so without functional changes to the SGX uapi, >>> e.g. add reserved fields so that the initial uapi can be extended *if* we >>> decide to go with the "userspace provides maximal protections" path, and >>> use the EPCM permissions as the maximal protections for the initial >>> upstreaming. >>> >>> That'd give us a minimal implemenation for initial upstreaming and would >>> eliminate Cedric's blocking complaint. The "whole mess" of whitelisting, >>> blacklisting and SGX2 support would be deferred until post-upstreaming. >> >> I'd like that approach more too. >> >> /Jarkko
On Tue, Jul 09, 2019 at 01:41:28PM -0700, Xing, Cedric wrote: > On 7/9/2019 10:09 AM, Sean Christopherson wrote: > >Translating those to SGX, with a lot of input from Stephen, I ended up > >with the following: > > > > - FILE__ENCLAVE_EXECUTE: equivalent to FILE__EXECUTE, required to gain X > > on an enclave page loaded from a regular file > > > > - PROCESS2__ENCLAVE_EXECDIRTY: hybrid of EXECMOD and EXECUTE+WRITE, > > required to gain W->X on an enclave page > > EXECMOD basically indicates a file containing self-modifying code. Your > ENCLAVE_EXECDIRTY is however a process permission, which is illogical. How is it illogical? If a PROCESS wants to EXECute a DIRTY ENCLAVE page, then it needs PROCESS2__ENCLAVE_EXECDIRTY. FILE__EXECMOD on /dev/sgx/enclave is a process permission masquerading as a file permission, let's call it what it is.
On 7/9/2019 3:25 PM, Sean Christopherson wrote: > On Tue, Jul 09, 2019 at 01:41:28PM -0700, Xing, Cedric wrote: >> On 7/9/2019 10:09 AM, Sean Christopherson wrote: >>> Translating those to SGX, with a lot of input from Stephen, I ended up >>> with the following: >>> >>> - FILE__ENCLAVE_EXECUTE: equivalent to FILE__EXECUTE, required to gain X >>> on an enclave page loaded from a regular file >>> >>> - PROCESS2__ENCLAVE_EXECDIRTY: hybrid of EXECMOD and EXECUTE+WRITE, >>> required to gain W->X on an enclave page >> >> EXECMOD basically indicates a file containing self-modifying code. Your >> ENCLAVE_EXECDIRTY is however a process permission, which is illogical. > > How is it illogical? If a PROCESS wants to EXECute a DIRTY ENCLAVE page, > then it needs PROCESS2__ENCLAVE_EXECDIRTY Just think of the purpose of FILE__EXECMOD. It indicates to LSM the file has self-modifying code, hence W->X transition should be considered "normal" and allowed, regardless which process that file is loaded into. The same thing for enclaves here. Whether an enclave contains self-modifying code is specific to that enclave, regardless which process it is loaded into. But what are you doing is quite the opposite, and that's I mean by "illogical".
On Mon, Jul 08, 2019 at 10:29:30AM -0700, Sean Christopherson wrote: Good evening to everyone. > That being said, we can do so without functional changes to the SGX > uapi, e.g. add reserved fields so that the initial uapi can be > extended *if* we decide to go with the "userspace provides maximal > protections" path, and use the EPCM permissions as the maximal > protections for the initial upstreaming. > > That'd give us a minimal implemenation for initial upstreaming and > would eliminate Cedric's blocking complaint. The "whole mess" of > whitelisting, blacklisting and SGX2 support would be deferred until > post-upstreaming. Are we convinced the 'mess' will be any easier to clean up after the driver is upstreamed? The primary problem is that we haven't addressed the issue of what this technology is designed to do and its implications with respect to the kernel. As a result we are attempting to implement controls which we are comfortable with and understand rather then those that are relevant. Have a good evening. Dr. Greg As always, Dr. Greg Wettstein, Ph.D, Worker IDfusion, LLC Implementing SGX secured and modeled 4206 N. 19th Ave. intelligent network endpoints. Fargo, ND 58102 PH: 701-281-1686 EMAIL: greg@idfusion.net ------------------------------------------------------------------------------ "Courage is not the absence of fear, but rather the judgement that something else is more important than fear." -- Ambrose Redmoon
On 7/9/2019 6:28 PM, Dr. Greg wrote: > On Mon, Jul 08, 2019 at 10:29:30AM -0700, Sean Christopherson wrote: > > Good evening to everyone. > >> That being said, we can do so without functional changes to the SGX >> uapi, e.g. add reserved fields so that the initial uapi can be >> extended *if* we decide to go with the "userspace provides maximal >> protections" path, and use the EPCM permissions as the maximal >> protections for the initial upstreaming. >> >> That'd give us a minimal implemenation for initial upstreaming and >> would eliminate Cedric's blocking complaint. The "whole mess" of >> whitelisting, blacklisting and SGX2 support would be deferred until >> post-upstreaming. > > Are we convinced the 'mess' will be any easier to clean up after the > driver is upstreamed? > > The primary problem is that we haven't addressed the issue of what > this technology is designed to do and its implications with respect to > the kernel. As a result we are attempting to implement controls which > we are comfortable with and understand rather then those that are > relevant. I don't think it's about easier or harder to clean up the mess, but a divide-and-conquer strategy. After all, SGX and LSM are kind of orthogonal as long as SGX doesn't compromise the protection provided by LSM. Let's step back and look at what started this lengthy discussion. The primary problem of v20 was that the SGX module allows executable enclave pages to be created from non-executable regular pages, which could be exploited by adversaries to grant executable permissions to pages that would otherwise be denied without SGX. And that could be fixed simply by capping EPCM permissions to whatever allowed on the source page, without touching LSM. Of course the drawback is loss of functionality - i.e. self-modifying enclaves cannot be loaded unless the calling process has EXECMEM. But that should suffice, as most SGX1 applications don't contain self-modifying code anyway. Then we could switch our focus back to LSM and work out what's relevant, especially for SGX2 and beyond. > Have a good evening. > > Dr. Greg > > As always, > Dr. Greg Wettstein, Ph.D, Worker > IDfusion, LLC Implementing SGX secured and modeled > 4206 N. 19th Ave. intelligent network endpoints. > Fargo, ND 58102 > PH: 701-281-1686 EMAIL: greg@idfusion.net > ------------------------------------------------------------------------------ > "Courage is not the absence of fear, but rather the judgement that > something else is more important than fear." > -- Ambrose Redmoon >
On 2019-07-08 10:29, Sean Christopherson wrote: > On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote: >> On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote: >> >> I still don't get why we need this whole mess and do not simply admit >> that there are two distinct roles: >> >> 1. Creator >> 2. User > > Because SELinux has existing concepts of EXECMEM and EXECMOD. > >> In the SELinux context Creator needs FILE__WRITE and FILE__EXECUTE but >> User does not. It just gets the fd from the Creator. I'm sure that all >> the SGX2 related functionality can be solved somehow in this role >> playing game. >> >> An example would be the usual case where enclave is actually a loader >> that loads the actual piece of software that one wants to run. Things >> simply need to be designed in a way the Creator runs the loader part. >> These are non-trivial problems but oddball security model is not going >> to make them disappear - on the contrary it will make designing user >> space only more complicated. >> >> I think this is classical example of when something overly complicated >> is invented in the kernel only to realize that it should be solved in >> the user space. >> >> It would not be like the only use case where some kind of privileged >> daemon is used for managing some a kernel provided resource. >> >> I think a really good conclusion from this discussion that has taken two >> months is to realize that nothing needs to be done in this area (except >> *maybe* noexec check). > > Hmm, IMO we need to support at least equivalents to EXECMEM and EXECMOD. > > That being said, we can do so without functional changes to the SGX uapi, > e.g. add reserved fields so that the initial uapi can be extended *if* we > decide to go with the "userspace provides maximal protections" path, and > use the EPCM permissions as the maximal protections for the initial > upstreaming. Why do you need to add reserved fields now? Isn't this what incorporating the struct size in the ioctl number is for? -- Jethro Beekman | Fortanix
On Tue, Jul 09, 2019 at 04:11:08PM -0700, Xing, Cedric wrote: > On 7/9/2019 3:25 PM, Sean Christopherson wrote: > >On Tue, Jul 09, 2019 at 01:41:28PM -0700, Xing, Cedric wrote: > >>On 7/9/2019 10:09 AM, Sean Christopherson wrote: > >>>Translating those to SGX, with a lot of input from Stephen, I ended up > >>>with the following: > >>> > >>> - FILE__ENCLAVE_EXECUTE: equivalent to FILE__EXECUTE, required to gain X > >>> on an enclave page loaded from a regular file > >>> > >>> - PROCESS2__ENCLAVE_EXECDIRTY: hybrid of EXECMOD and EXECUTE+WRITE, > >>> required to gain W->X on an enclave page > >> > >>EXECMOD basically indicates a file containing self-modifying code. Your > >>ENCLAVE_EXECDIRTY is however a process permission, which is illogical. > > > >How is it illogical? If a PROCESS wants to EXECute a DIRTY ENCLAVE page, > >then it needs PROCESS2__ENCLAVE_EXECDIRTY > Just think of the purpose of FILE__EXECMOD. It indicates to LSM the file has > self-modifying code, hence W->X transition should be considered "normal" and > allowed, regardless which process that file is loaded into. > > The same thing for enclaves here. Whether an enclave contains self-modifying > code is specific to that enclave, regardless which process it is loaded > into. > > But what are you doing is quite the opposite, and that's I mean by > "illogical". Ah. My intent was to minimize the number of new labels, and because W->X scenarios are not guaranteed to be backed by a file, I went with a per process permission. Ditto for EXECANON. I'm not opposed to also having a per file permission that can be used when possible. Something like this? And maybe merge EXECANON and EXECDIRTY into a single permission? Depending on whether sigstruct is required to be in a pinned file, EAUG pages would need either EXECDIRTY or EXECMOD. static int selinux_enclave_load(struct vm_area_struct *vma, unsigned long prot, bool measured) { const struct cred *cred = current_cred(); u32 sid = cred_sid(cred); int ret; /* Currently supported only in noexec kernels. */ WARN_ON_ONCE(!default_noexec); /* Only executable enclave pages are restricted in any way. */ if (!(prot & PROT_EXEC)) return 0; if (!measured) { ret = enclave_has_perm(sid, PROCESS2__ENCLAVE_EXECUNMR); if (ret) goto out; } if (!vma->vm_file || IS_PRIVATE(file_inode(vma->vm_file))) { ret = enclave_has_perm(sid, PROCESS2__ENCLAVE_EXECANON); if (ret) goto out; /* Ability to do W->X within the enclave. */ if (prot & PROT_WRITE) ret = enclave_has_perm(sid, PROCESS2__ENCLAVE_EXECDIRTY); } else { ret = file_has_perm(cred, vma->vm_file, FILE__ENCLAVE_EXECUTE); if (ret) goto out; /* * Load code from a modified private mapping, or from any file * mapping with the ability to do W->X within the enclave. */ if (vma->anon_vma || (prot & PROT_WRITE)) ret = file_has_perm(cred, vma->vm_file, FILE__ENCLAVE_EXECMOD); } out: return ret; }
On Tue, Jul 09, 2019 at 10:09:17AM -0700, Sean Christopherson wrote: > On Tue, Jul 09, 2019 at 07:22:03PM +0300, Jarkko Sakkinen wrote: > > On Mon, Jul 08, 2019 at 10:29:30AM -0700, Sean Christopherson wrote: > > > On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote: > > > > On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote: > > > > > > > > I still don't get why we need this whole mess and do not simply admit > > > > that there are two distinct roles: > > > > > > > > 1. Creator > > > > 2. User > > > > > > Because SELinux has existing concepts of EXECMEM and EXECMOD. > > > > What is the official documentation for those? I've only found some > > explanations from discussions and some RHEL sysadmin guides. > > No clue. My knowledge was gleaned from the code and from Stephen's > feedback. OK, thanks for elaboration. Got nailed some details I was missing :-) Anyway, to accompany your code changes I'm eager to document this not least because it is a good peer test that this all make sense (you cannot "unit test" a security model so that is the next best thing). Still, we need a documentation reference to reflect the narrative for these changes, seriously. It cannot be that SELinux is widely deployed and it completely lacks documentation for its basic objects, can it? /Jarkko
On Wed, Jul 10, 2019 at 11:19:30PM +0300, Jarkko Sakkinen wrote: > On Tue, Jul 09, 2019 at 10:09:17AM -0700, Sean Christopherson wrote: > > On Tue, Jul 09, 2019 at 07:22:03PM +0300, Jarkko Sakkinen wrote: > > > On Mon, Jul 08, 2019 at 10:29:30AM -0700, Sean Christopherson wrote: > > > > On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote: > > > > > On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote: > > > > > > > > > > I still don't get why we need this whole mess and do not simply admit > > > > > that there are two distinct roles: > > > > > > > > > > 1. Creator > > > > > 2. User > > > > > > > > Because SELinux has existing concepts of EXECMEM and EXECMOD. > > > > > > What is the official documentation for those? I've only found some > > > explanations from discussions and some RHEL sysadmin guides. > > > > No clue. My knowledge was gleaned from the code and from Stephen's > > feedback. > > OK, thanks for elaboration. Got nailed some details I was missing :-) > > Anyway, to accompany your code changes I'm eager to document this not > least because it is a good peer test that this all make sense (you > cannot "unit test" a security model so that is the next best thing). > > Still, we need a documentation reference to reflect the narrative > for these changes, seriously. It cannot be that SELinux is widely > deployed and it completely lacks documentation for its basic > objects, can it? The vast majority of documentation I've found is on *using* SELinux, e.g. writing policies and whatnot. I haven't found anything on its internal details, although I admitedly haven't looked all that hard.
On Wed, Jul 10, 2019 at 11:19:30PM +0300, Jarkko Sakkinen wrote: > Still, we need a documentation reference to reflect the narrative > for these changes, seriously. It cannot be that SELinux is widely > deployed and it completely lacks documentation for its basic > objects, can it? I found one good reference: https://selinuxpTroject.org/page/ObjectClassesPerms It describes EXECMOD as: "Make executable a file mapping that has been modified by copy-on-write. (Text relocation)" This makes me wonder how EXECMOD even connects to this discussion? Enclave is never a COW mapping. Seems like there is a huge diff on how SELinux's official documentation describes it and how it is described here... /Jarkko
Just some questions on these. On Tue, Jul 09, 2019 at 10:09:17AM -0700, Sean Christopherson wrote: > - FILE__ENCLAVE_EXECUTE: equivalent to FILE__EXECUTE, required to gain X > on an enclave page loaded from a regular file One thing that I have hard time to perceive is that whether the process or the target object has them. So would this be in the files extended attribute or does process need to possess this or both? > - PROCESS2__ENCLAVE_EXECDIRTY: hybrid of EXECMOD and EXECUTE+WRITE, > required to gain W->X on an enclave page Still puzzling with EXECMOD given that how it is documented in https://selinuxproject.org/page/ObjectClassesPerms. If anything in that document is out of date, would be nice if it was updated. > - PROCESS2__ENCLAVE_EXECANON: subset of EXECMEM, required to gain X on > an enclave page that is loaded from an > anonymous mapping > > - PROCESS2__ENCLAVE_MAPWX: subset of EXECMEM, required to gain WX on an > enclave page I guess these three belong to the process and are not attached to file. How in SELinux anyway process in the first place acquires any SELinux permissions? I guess getty or whatever login process can set its perms before setuid() et al somehow (I don't know how) because they run as root? /Jarkko
On 7/10/2019 3:16 PM, Jarkko Sakkinen wrote: > Just some questions on these. > > On Tue, Jul 09, 2019 at 10:09:17AM -0700, Sean Christopherson wrote: >> - FILE__ENCLAVE_EXECUTE: equivalent to FILE__EXECUTE, required to gain X >> on an enclave page loaded from a regular file > > One thing that I have hard time to perceive is that whether the process > or the target object has them. So would this be in the files extended > attribute or does process need to possess this or both? The target object. >> - PROCESS2__ENCLAVE_EXECDIRTY: hybrid of EXECMOD and EXECUTE+WRITE, >> required to gain W->X on an enclave page > > Still puzzling with EXECMOD given that how it is documented in > https://selinuxproject.org/page/ObjectClassesPerms. If anything in that > document is out of date, would be nice if it was updated. If you search for "EXECMOD" in security/selinux/hooks.c in the latest (Linux-5.2) master, you'll find only one occurrence - at line 3702. The logic over there, if translated into English, basically says FILE__EXECMOD is required (on the backing file) if mprotect() is called to request X on a private file mapping that has been modified by the calling process. That's what Sean meant by "W->X". EXCLAVE_EXECDIRTY is similar to EXECMOD but because of his "maximal protection" model, LSM couldn't distinguish between "W->X" and "X->W", hence those two are collapsed into a single case - WX in "maximal protection". >> - PROCESS2__ENCLAVE_EXECANON: subset of EXECMEM, required to gain X on >> an enclave page that is loaded from an >> anonymous mapping >> >> - PROCESS2__ENCLAVE_MAPWX: subset of EXECMEM, required to gain WX on an >> enclave page > > I guess these three belong to the process and are not attached to file. Correct. ENCLAVE_EXECANON basically means the calling process doesn't care what permissions given to enclave pages as the SIGSTRUCT alone is considered sufficient validation. This has a security impact process wide so shall be a process permission. ENCLAVE_{EXECDIRTY|MAPWX} express enclave specific requirements/behaviors and IMO shall be enclave permissions, probably manifested as file permissions on the file containing SIGSTRUCT. Sean was taking a shortcut to make them process scope in order to avoid keeping the SIGSTRUCT file around, which was what I criticized as "illogical". > How in SELinux anyway process in the first place acquires any SELinux > permissions? I guess getty or whatever login process can set its perms > before setuid() et al somehow (I don't know how) because they run as > root? > > /Jarkko >
On Wed, Jul 10, 2019 at 01:31:04PM -0700, Sean Christopherson wrote: > On Wed, Jul 10, 2019 at 11:19:30PM +0300, Jarkko Sakkinen wrote: > > On Tue, Jul 09, 2019 at 10:09:17AM -0700, Sean Christopherson wrote: > > > On Tue, Jul 09, 2019 at 07:22:03PM +0300, Jarkko Sakkinen wrote: > > > > On Mon, Jul 08, 2019 at 10:29:30AM -0700, Sean Christopherson wrote: > > > > > On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote: > > > > > > On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote: > > > > > > > > > > > > I still don't get why we need this whole mess and do not simply admit > > > > > > that there are two distinct roles: > > > > > > > > > > > > 1. Creator > > > > > > 2. User > > > > > > > > > > Because SELinux has existing concepts of EXECMEM and EXECMOD. > > > > > > > > What is the official documentation for those? I've only found some > > > > explanations from discussions and some RHEL sysadmin guides. > > > > > > No clue. My knowledge was gleaned from the code and from Stephen's > > > feedback. > > > > OK, thanks for elaboration. Got nailed some details I was missing :-) > > > > Anyway, to accompany your code changes I'm eager to document this not > > least because it is a good peer test that this all make sense (you > > cannot "unit test" a security model so that is the next best thing). > > > > Still, we need a documentation reference to reflect the narrative > > for these changes, seriously. It cannot be that SELinux is widely > > deployed and it completely lacks documentation for its basic > > objects, can it? > > The vast majority of documentation I've found is on *using* SELinux, > e.g. writing policies and whatnot. I haven't found anything on its > internal details, although I admitedly haven't looked all that hard. I think these are the best references. Object classes: https://selinuxproject.org/page/ObjectClassesPerms Background/concepts: https://selinuxproject.org/page/Category:Notebook#Notebook_Sections /Jarkko
On Wed, Jul 10, 2019 at 04:16:42PM -0700, Xing, Cedric wrote: > > Still puzzling with EXECMOD given that how it is documented in > > https://selinuxproject.org/page/ObjectClassesPerms. If anything in that > > document is out of date, would be nice if it was updated. > > If you search for "EXECMOD" in security/selinux/hooks.c in the latest > (Linux-5.2) master, you'll find only one occurrence - at line 3702. > > The logic over there, if translated into English, basically says > FILE__EXECMOD is required (on the backing file) if mprotect() is called to > request X on a private file mapping that has been modified by the calling > process. That's what Sean meant by "W->X". Looking at that part of code, there is this comment: /* * We are making executable a file mapping that has * had some COW done. Since pages might have been * written, check ability to execute the possibly * modified content. This typically should only * occur for text relocations. */ There is no COW done with enclaves, never. Thus, EXECMOD does not connect in any possible way to SGX. OR, that comment is false. Which one is it? Also the official documentation for SELinux speaks only about COW mappings. Also the condition supports all this as a *private* file mapping ends up to the anon_vma list when it gets written. We have a *shared* file mapping Nothing that you say makes sense to me, sorry... /Jarkko
On 7/11/19 5:26 AM, Jarkko Sakkinen wrote: > On Wed, Jul 10, 2019 at 04:16:42PM -0700, Xing, Cedric wrote: >>> Still puzzling with EXECMOD given that how it is documented in >>> https://selinuxproject.org/page/ObjectClassesPerms. If anything in that >>> document is out of date, would be nice if it was updated. >> >> If you search for "EXECMOD" in security/selinux/hooks.c in the latest >> (Linux-5.2) master, you'll find only one occurrence - at line 3702. >> >> The logic over there, if translated into English, basically says >> FILE__EXECMOD is required (on the backing file) if mprotect() is called to >> request X on a private file mapping that has been modified by the calling >> process. That's what Sean meant by "W->X". > > Looking at that part of code, there is this comment: > > /* > * We are making executable a file mapping that has > * had some COW done. Since pages might have been > * written, check ability to execute the possibly > * modified content. This typically should only > * occur for text relocations. > */ > > There is no COW done with enclaves, never. Thus, EXECMOD does not > connect in any possible way to SGX. OR, that comment is false. > > Which one is it? > > Also the official documentation for SELinux speaks only about COW > mappings. > > Also the condition supports all this as a *private* file mapping ends up > to the anon_vma list when it gets written. We have a *shared* file > mapping > > Nothing that you say makes sense to me, sorry... The existing permissions don't map cleanly to SGX but I think Sean and Cedric were trying to make a best-effort approximation to the underlying concepts in a manner that permits control over the introduction of executable content. Sure, the existing EXECMOD check is only applied today when there is an attempt to make executable a previously modified (detected based on COW having occurred) private file mapping. But the general notion of controlling the ability to execute modified content is still meaningful. In the case of regular files, having both FILE__WRITE and FILE__EXECUTE to the file is sufficient because that implies the ability to execute modified content. And those FILE__* checks predated the introduction of EXECMOD and EXECMEM. The mapping of /dev/sgx/enclave doesn't really fit existing categories because it doesn't provide the same semantics as a shared mapping of a regular file. Userspace will always need both FILE__WRITE and FILE__EXECUTE to /dev/sgx/enclave.
On Thu, Jul 11, 2019 at 10:32:41AM -0400, Stephen Smalley wrote: > The existing permissions don't map cleanly to SGX but I think Sean and > Cedric were trying to make a best-effort approximation to the underlying > concepts in a manner that permits control over the introduction of > executable content. > > Sure, the existing EXECMOD check is only applied today when there is an > attempt to make executable a previously modified (detected based on COW > having occurred) private file mapping. But the general notion of > controlling the ability to execute modified content is still meaningful. OK to summarize EXECMOD does not connect with SGX in any possible way but SGX needs something that mimics EXECMOD behaviour? And the same probably goes for EXECMEM, which is also private mapping related concept. > In the case of regular files, having both FILE__WRITE and FILE__EXECUTE to > the file is sufficient because that implies the ability to execute modified > content. And those FILE__* checks predated the introduction of EXECMOD and > EXECMEM. Right. > The mapping of /dev/sgx/enclave doesn't really fit existing categories > because it doesn't provide the same semantics as a shared mapping of a > regular file. Userspace will always need both FILE__WRITE and FILE__EXECUTE > to /dev/sgx/enclave. Right. Yeah, that has been the confusing part for me that they've been shuffled in the discussion together with SGX concepts and hooks like there was any connection. Thank you for clarifying this! /Jarkko
On 7/11/2019 10:51 AM, Jarkko Sakkinen wrote: > On Thu, Jul 11, 2019 at 10:32:41AM -0400, Stephen Smalley wrote: >> The existing permissions don't map cleanly to SGX but I think Sean and >> Cedric were trying to make a best-effort approximation to the underlying >> concepts in a manner that permits control over the introduction of >> executable content. >> >> Sure, the existing EXECMOD check is only applied today when there is an >> attempt to make executable a previously modified (detected based on COW >> having occurred) private file mapping. But the general notion of >> controlling the ability to execute modified content is still meaningful. > > OK to summarize EXECMOD does not connect with SGX in any possible way > but SGX needs something that mimics EXECMOD behaviour? Stephen may correct me if I'm wrong. EXECMOD is granted to files, to indicate the bearer contains self-modifying code (or text relocation). So if it applies the enclaves, there are two aspects of it: (1) An enclave may be loaded from multiple image files, among which some may contain self-modifying code and hence would require EXECMOD on each of them. At runtime, W->X will be allowed/denied for pages loaded from image files having/not having EXECMOD. (2) But there are pages not loaded from any files - e.g. pages EAUG'ed at runtime. We are trying to use the file containing SIGSTRUCT as the "proxy file" - i.e. EXECMOD on the proxy file indicates the enclave may load code into EAUG'ed pages at runtime. (3) Well, this is not an aspect of EXECMOD. Yet there's another category of pages - pages EADD'ed from anonymous memory. They are different than EAUG'ed pages in that their contents are supplied by untrusted code. How to control their accesses is still being debated. My argument was that the source pages must be executable before they could be loaded executable in EPC. Andy argued that SIGSTRUCT alone could be considered sufficient validation on the contents in certain usages, so both Sean and I had proposed PROCESS2__ENCLAVE_EXECANON as a new permission. But for the 1st upstream I think we all agree to do just the minimum until real requirements come up. After all, adding is generally easier than taking away existing things.
For those of you whom I neglected to cc on v3, here's a quick recap: My original plan was for my next RFC to be an implementation of Andy's proposed "dynamic tracking" model, but I was completely flummoxed by the auditing[1]. Cedric's RFC has the same auditing complexities, so I I ended up back at the "make userspace state its intentions" approach. There are no significant LSM changes in v4, e.g. a bug fix and some renaming. I'm spinning v4 early to get the cc list correct, and also because I'm about to disappear on vacation for two weeks. Except for patch 12 (see below), the SGX changes have been fully tested, including updating the kernel's selftest as well as my own fork of (an old version of) Intel's SDK to use the new UAPI. The LSM changes have been smoke tested, but I haven't actually configured AppArmor or SELinux to verify the permissions work as intended. Patches 1-3 are not directly related to LSM support. They're included here as the actual LSM RFC patches are essentially untestable without them, and so that the patches apply to Jarkko's tree. Ignore patches 1-3 unless you actually want to run code. Patches 4-11 are the meat of the RFC. Patch 12 is purely to show how we might implement SGX2 support. It's not intended to be included in the initial upstreaming of SGX. The full code is available at https://github.com/sean-jc/linux.git in a few forms (tagged); sgx-lsm-v4 - Jarkko's full tree plus patches 1-11 sgx-lsm-v4-eaug - Everything above plus patch 12 <boilerplate> This series is a delta to Jarkko's ongoing SGX series and applies on Jarkko's current master at https://github.com/jsakkine-intel/linux-sgx.git: 91f3aa6d241d ("docs: x86/sgx: Document the enclave API") The basic gist of the approach is to track an enclave's page protections separately from any vmas that map the page, and separate from the hardware enforced protections. The SGX UAPI is modified to require userspace to explicitly define the protections for each enclave page, i.e. the ioctl to add pages to an enclave is extended to take PROT_{READ,WRITE,EXEC} flags. An enclave page's protections are the maximal protections that userspace can use to map the page, e.g. mprotect() and mmap() are rejected if the protections for the vma would be more permissible than those of the associated enclave page. Tracking protections for an enclave page (in additional to vmas) allows SGX to invoke LSM upcalls while the enclave is being built. This is critical to enabling LSMs to implement policies for enclave pages that are functionally equivalent to existing policies for normal pages. </boilerplate> [1] https://lkml.kernel.org/r/20190614003759.GE18385@linux.intel.com v4: - Rename SGX__EXECMEM and SGX__EXECMOD to SGX__MAPWX and SGX_EXECDIRTY respectively [Stephen]. - Fix an inverted check on IS_PRIVATE file check [Stephen]. - Take a '__u8 prot' in SGX_IOC_ENCLAVE_ADD_PAGE [Jarkko]. - Rebased to Jarkko's latest code base. - Replace patch 1 with a variant that does encl_mm tracking via mmu_notifier and SRCU. Not relevant for most people, but I wanted to show the end state if we get rid of the per-vma tracking. v3: https://patchwork.kernel.org/cover/11000601/ - Clear VM_MAY* flags instead of using .may_mprotect() to enforce maximal enclave page protections. - Update the SGX selftest to work with the new API. - Rewrite SELinux code to use SGX specific permissions, with the goal of addressing Andy's feedback regarding what people will actually care about when it comes to SGX, e.g. add permissions for restricing unmeasured code and stop trying to infer permissions from the source of each enclave page. - Add a (very minimal) AppArmor patch. - Show line of sight to SGX2 support. - Rebased to Jarkko's latest code base. v2: https://lkml.kernel.org/r/20190606021145.12604-1-sean.j.christopherson@intel.com - Dropped the patch(es) to extend the SGX UAPI to allow adding multiple enclave pages in a single syscall [Jarkko]. - Reject ioctl() immediately on LSM denial [Stephen]. - Rework SELinux code to avoid checking EXEMEM multiple times [Stephen]. - Adding missing equivalents to existing selinux_file_protect() checks [Stephen]. - Hold mmap_sem across copy_to_user() to prevent a TOCTOU race when checking the source vma [Stephen]. - Stubify security_enclave_load() if !CONFIG_SECURITY [Stephen]. - Make flags a 32-bit field [Andy]. - Don't validate the SECINFO protection flags against the enclave page's protection flags [Andy]. - Rename mprotect() hook to may_mprotect() [Andy]. - Test 'vma->vm_flags & VM_MAYEXEC' instead of manually checking for a noexec path [Jarkko]. - Drop the SGX defined flags (use PROT_*) [Jarkko]. - Improve comments and changelogs [Jarkko]. v1: https://lkml.kernel.org/r/20190531233159.30992-1-sean.j.christopherson@intel.com Sean Christopherson (12): x86/sgx: Use mmu_notifier.release() instead of per-vma refcounting x86/sgx: Do not naturally align MAP_FIXED address selftests: x86/sgx: Mark the enclave loader as not needing an exec stack x86/sgx: Require userspace to define enclave pages' protection bits x86/sgx: Enforce noexec filesystem restriction for enclaves mm: Introduce vm_ops->may_mprotect() LSM: x86/sgx: Introduce ->enclave_map() hook for Intel SGX security/selinux: Require SGX_MAPWX to map enclave page WX LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX security/selinux: Add enclave_load() implementation security/apparmor: Add enclave_load() implementation LSM: x86/sgx: Show line of sight to LSM support SGX2's EAUG arch/x86/Kconfig | 2 + arch/x86/include/uapi/asm/sgx.h | 6 +- arch/x86/kernel/cpu/sgx/driver/ioctl.c | 69 ++++-- arch/x86/kernel/cpu/sgx/driver/main.c | 106 ++++++++- arch/x86/kernel/cpu/sgx/encl.c | 277 ++++++++++++----------- arch/x86/kernel/cpu/sgx/encl.h | 22 +- arch/x86/kernel/cpu/sgx/reclaim.c | 71 ++---- include/linux/lsm_hooks.h | 20 ++ include/linux/mm.h | 2 + include/linux/security.h | 18 ++ mm/mprotect.c | 15 +- security/apparmor/include/audit.h | 2 + security/apparmor/lsm.c | 14 ++ security/security.c | 12 + security/selinux/hooks.c | 72 ++++++ security/selinux/include/classmap.h | 6 +- tools/testing/selftests/x86/sgx/Makefile | 2 +- tools/testing/selftests/x86/sgx/main.c | 32 ++- 18 files changed, 532 insertions(+), 216 deletions(-)