| Message ID | 20190619222401.14942-11-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | security: x86/sgx: SGX vs. LSM | expand |
> From: Christopherson, Sean J > Sent: Wednesday, June 19, 2019 3:24 PM > > Intended use of each permission: > > - SGX_EXECDIRTY: dynamically load code within the enclave itself > - SGX_EXECUNMR: load unmeasured code into the enclave, e.g. Graphene Why does it matter whether a code page is measured or not? > - SGX_EXECANON: load code from anonymous memory (likely Graphene) Graphene doesn't load code from anonymous memory. It loads code dynamically though, as in SGX_EXECDIRTY case. > - SGX_EXECUTE: load an enclave from a file, i.e. normal behavior Why is SGX_EXECUTE needed from security perspective? Or why isn't FILE__EXECUTE sufficient?
On 6/19/19 6:23 PM, Sean Christopherson wrote: > The goal of selinux_enclave_load() is to provide a facsimile of the > existing selinux_file_mprotect() and file_map_prot_check() policies, > but tailored to the unique properties of SGX. > > For example, an enclave page is technically backed by a MAP_SHARED file, > but the "file" is essentially shared memory that is never persisted > anywhere and also requires execute permissions (for some pages). > > Enclaves are also less priveleged than normal user code, e.g. SYSCALL > instructions #UD if attempted in an enclave. For this reason, add SGX > specific permissions instead of reusing existing permissions such as > FILE__EXECUTE so that policies can allow running code in an enclave, or > allow dynamically loading code in an enclave without having to grant the > same capability to normal user code outside of the enclave. > > Intended use of each permission: > > - SGX_EXECDIRTY: dynamically load code within the enclave itself > - SGX_EXECUNMR: load unmeasured code into the enclave, e.g. Graphene > - SGX_EXECANON: load code from anonymous memory (likely Graphene) > - SGX_EXECUTE: load an enclave from a file, i.e. normal behavior > > Note, equivalents to FILE__READ and FILE__WRITE are intentionally never > required. Writes to the enclave page are contained to the EPC, i.e. > never hit the original file, and read permissions have already been > vetted (or the VMA doesn't have PROT_READ, in which case loading the > page into the enclave will fail). > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > security/selinux/hooks.c | 55 +++++++++++++++++++++++++++-- > security/selinux/include/classmap.h | 5 +-- > 2 files changed, 55 insertions(+), 5 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index fc239e541b62..8a431168e454 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -6727,6 +6727,12 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux) > #endif > > #ifdef CONFIG_INTEL_SGX > +static inline int sgx_has_perm(u32 sid, u32 requested) > +{ > + return avc_has_perm(&selinux_state, sid, sid, > + SECCLASS_PROCESS2, requested, NULL); > +} > + > static int selinux_enclave_map(unsigned long prot) > { > const struct cred *cred = current_cred(); > @@ -6736,11 +6742,53 @@ static int selinux_enclave_map(unsigned long prot) > WARN_ON_ONCE(!default_noexec); > > if ((prot & PROT_EXEC) && (prot & PROT_WRITE)) > - return avc_has_perm(&selinux_state, sid, sid, > - SECCLASS_PROCESS2, PROCESS2__SGX_MAPWX, > - NULL); > + return sgx_has_perm(sid, PROCESS2__SGX_MAPWX); > + > return 0; > } > + > +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; > + > + /* SGX is supported only in 64-bit kernels. */ > + WARN_ON_ONCE(!default_noexec); > + > + /* Only executable enclave pages are restricted in any way. */ > + if (!(prot & PROT_EXEC)) > + return 0; > + > + /* > + * WX at load time only requires EXECDIRTY, e.g. to allow W->X. Actual > + * WX mappings require MAPWX (see selinux_enclave_map()). > + */ > + if (prot & PROT_WRITE) { > + ret = sgx_has_perm(sid, PROCESS2__SGX_EXECDIRTY); > + if (ret) > + goto out; > + } > + if (!measured) { > + ret = sgx_has_perm(sid, PROCESS2__SGX_EXECUNMR); > + if (ret) > + goto out; > + } > + > + if (!vma->vm_file || IS_PRIVATE(file_inode(vma->vm_file)) || > + vma->anon_vma) > + /* > + * Loading enclave code from an anonymous mapping or from a > + * modified private file mapping. > + */ > + ret = sgx_has_perm(sid, PROCESS2__SGX_EXECANON); > + else > + /* Loading from a shared or unmodified private file mapping. */ > + ret = file_has_perm(cred, vma->vm_file, FILE__SGX_EXECUTE); > +out: > + return ret; > +} Same comment on this patch: we might want to generalize the permission names in hopes of being able to reuse them in the future for similar constructs on other architectures. SGX -> ENCLAVE throughout? > #endif > > struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = { > @@ -6988,6 +7036,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { > > #ifdef CONFIG_INTEL_SGX > LSM_HOOK_INIT(enclave_map, selinux_enclave_map), > + LSM_HOOK_INIT(enclave_load, selinux_enclave_load), > #endif > }; > > diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h > index cfd91e879bdf..baa1757be46a 100644 > --- a/security/selinux/include/classmap.h > +++ b/security/selinux/include/classmap.h > @@ -7,7 +7,7 @@ > > #define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \ > "rename", "execute", "quotaon", "mounton", "audit_access", \ > - "open", "execmod" > + "open", "execmod", "sgx_execute" > > #define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \ > "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom", \ > @@ -52,7 +52,8 @@ struct security_class_mapping secclass_map[] = { > "setsockcreate", "getrlimit", NULL } }, > { "process2", > { "nnp_transition", "nosuid_transition", > - "sgx_mapwx", NULL } }, > + "sgx_mapwx", "sgx_execdirty", "sgx_execanon", "sgx_execunmr", > + NULL } }, > { "system", > { "ipc_info", "syslog_read", "syslog_mod", > "syslog_console", "module_request", "module_load", NULL } }, >
On 6/21/19 5:22 PM, Xing, Cedric wrote: >> From: Christopherson, Sean J >> Sent: Wednesday, June 19, 2019 3:24 PM >> >> Intended use of each permission: >> >> - SGX_EXECDIRTY: dynamically load code within the enclave itself >> - SGX_EXECUNMR: load unmeasured code into the enclave, e.g. Graphene > > Why does it matter whether a code page is measured or not? It won't be incorporated into an attestation? > >> - SGX_EXECANON: load code from anonymous memory (likely Graphene) > > Graphene doesn't load code from anonymous memory. It loads code dynamically though, as in SGX_EXECDIRTY case. So do we expect EXECANON to never be triggered at all? > >> - SGX_EXECUTE: load an enclave from a file, i.e. normal behavior > > Why is SGX_EXECUTE needed from security perspective? Or why isn't FILE__EXECUTE sufficient? Splitting the SGX permissions from the regular ones allows distinctions to be made between what can be executed in the host process and what can be executed in the enclave. The host process may be allowed FILE__EXECUTE to numerous files that do not contain any code ever intended to be executed within the enclave.
> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx- > owner@vger.kernel.org] On Behalf Of Stephen Smalley > Sent: Tuesday, June 25, 2019 2:10 PM > > On 6/21/19 5:22 PM, Xing, Cedric wrote: > >> From: Christopherson, Sean J > >> Sent: Wednesday, June 19, 2019 3:24 PM > >> > >> Intended use of each permission: > >> > >> - SGX_EXECDIRTY: dynamically load code within the enclave itself > >> - SGX_EXECUNMR: load unmeasured code into the enclave, e.g. > >> Graphene > > > > Why does it matter whether a code page is measured or not? > > It won't be incorporated into an attestation? Yes, it will. And because of that, I don't think LSM should care. > > > > >> - SGX_EXECANON: load code from anonymous memory (likely Graphene) > > > > Graphene doesn't load code from anonymous memory. It loads code > dynamically though, as in SGX_EXECDIRTY case. > > So do we expect EXECANON to never be triggered at all? I don't think so. And from security perspective, the decision I think shall base on whether the source pages are (allowed to be made) executable. > > > > >> - SGX_EXECUTE: load an enclave from a file, i.e. normal behavior > > > > Why is SGX_EXECUTE needed from security perspective? Or why isn't > FILE__EXECUTE sufficient? > > Splitting the SGX permissions from the regular ones allows distinctions > to be made between what can be executed in the host process and what can > be executed in the enclave. The host process may be allowed > FILE__EXECUTE to numerous files that do not contain any code ever > intended to be executed within the enclave. Given an enclave and its host process, any executable contents could be allowed in 1) Neither the enclave nor the host 2) Enclave only 3) Host only 4) Both the enclave and the host Given the fact that enclave can access host's memory, if a piece of code is NOT allowed in the host, then it shouldn't be allowed in enclave either. So #2 shall never happen. An enclave dictates/enforces its own contents cryptographically, so it's unnecessary to enforce #3 by LSM IMO. Then #1 and #4 are the only 2 cases to be supported - a single FILE__EXECUTE is sufficient. I'm not objecting to new permissions to make things more explicit, but that'd require updates to user mode tools. I think it just easier to reuse existing permissions.
On 6/27/19 4:19 PM, Xing, Cedric wrote: >> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx- >> owner@vger.kernel.org] On Behalf Of Stephen Smalley >> Sent: Tuesday, June 25, 2019 2:10 PM >> >> On 6/21/19 5:22 PM, Xing, Cedric wrote: >>>> From: Christopherson, Sean J >>>> Sent: Wednesday, June 19, 2019 3:24 PM >>>> >>>> Intended use of each permission: >>>> >>>> - SGX_EXECDIRTY: dynamically load code within the enclave itself >>>> - SGX_EXECUNMR: load unmeasured code into the enclave, e.g. >>>> Graphene >>> >>> Why does it matter whether a code page is measured or not? >> >> It won't be incorporated into an attestation? > > Yes, it will. And because of that, I don't think LSM should care. > >> >>> >>>> - SGX_EXECANON: load code from anonymous memory (likely Graphene) >>> >>> Graphene doesn't load code from anonymous memory. It loads code >> dynamically though, as in SGX_EXECDIRTY case. >> >> So do we expect EXECANON to never be triggered at all? > > I don't think so. And from security perspective, the decision I think shall base on whether the source pages are (allowed to be made) executable. > >> >>> >>>> - SGX_EXECUTE: load an enclave from a file, i.e. normal behavior >>> >>> Why is SGX_EXECUTE needed from security perspective? Or why isn't >> FILE__EXECUTE sufficient? >> >> Splitting the SGX permissions from the regular ones allows distinctions >> to be made between what can be executed in the host process and what can >> be executed in the enclave. The host process may be allowed >> FILE__EXECUTE to numerous files that do not contain any code ever >> intended to be executed within the enclave. > > Given an enclave and its host process, any executable contents could be allowed in > 1) Neither the enclave nor the host > 2) Enclave only > 3) Host only > 4) Both the enclave and the host > > Given the fact that enclave can access host's memory, if a piece of code is NOT allowed in the host, then it shouldn't be allowed in enclave either. So #2 shall never happen. > > An enclave dictates/enforces its own contents cryptographically, so it's unnecessary to enforce #3 by LSM IMO. > > Then #1 and #4 are the only 2 cases to be supported - a single FILE__EXECUTE is sufficient. > > I'm not objecting to new permissions to make things more explicit, but that'd require updates to user mode tools. I think it just easier to reuse existing permissions. FWIW, adding new permissions only requires updating policy configuration, not userspace code/tools. But in any event, we can reuse the execute-related permissions if it makes sense but still consider introducing additional, new permissions, possibly in a separate "enclave" security class, if we want explicit control over enclave loading, e.g. ENCLAVE__LOAD, ENCLAVE__INIT, etc. One residual concern I have with the reuse of FILE__EXECUTE is using it for the sigstruct file as the fallback case. If the sigstruct is always part of the same file as the code, then it probably doesn't matter. But otherwise, it is somewhat odd to have to allow the host process to execute from the sigstruct file if it is only data (the signature).
> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx- > owner@vger.kernel.org] On Behalf Of Stephen Smalley > Sent: Friday, June 28, 2019 9:17 AM > > FWIW, adding new permissions only requires updating policy configuration, > not userspace code/tools. But in any event, we can reuse the execute- > related permissions if it makes sense but still consider introducing > additional, new permissions, possibly in a separate "enclave" security > class, if we want explicit control over enclave loading, e.g. > ENCLAVE__LOAD, ENCLAVE__INIT, etc. I'm not so familiar with SELinux tools so my apology in advance if I end up mixing up things. I'm not only talking about the new permissions, but also how to apply them to enclave files. Intel SGX SDK packages enclaves as .so files, and I guess that's the most straight forward way that most others would do. So if different permissions are defined, then user mode tools would have to distinguish enclaves from regular .so files in order to grant them different permissions. Would that be something extra to existing tools? > > One residual concern I have with the reuse of FILE__EXECUTE is using it > for the sigstruct file as the fallback case. If the sigstruct is always > part of the same file as the code, then it probably doesn't matter. But > otherwise, it is somewhat odd to have to allow the host process to > execute from the sigstruct file if it is only data (the signature). I agree with you. But do you think it a practical problem today? As far as I know, no one is deploying sigstructs in dedicated files. I'm just trying to touch as few things as possible until there's definitely a need to do so.
On Fri, Jun 28, 2019 at 5:20 PM Xing, Cedric <cedric.xing@intel.com> wrote: > > > From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx- > > owner@vger.kernel.org] On Behalf Of Stephen Smalley > > Sent: Friday, June 28, 2019 9:17 AM > > > > FWIW, adding new permissions only requires updating policy configuration, > > not userspace code/tools. But in any event, we can reuse the execute- > > related permissions if it makes sense but still consider introducing > > additional, new permissions, possibly in a separate "enclave" security > > class, if we want explicit control over enclave loading, e.g. > > ENCLAVE__LOAD, ENCLAVE__INIT, etc. > > I'm not so familiar with SELinux tools so my apology in advance if I end up mixing up things. > > I'm not only talking about the new permissions, but also how to apply them to enclave files. Intel SGX SDK packages enclaves as .so files, and I guess that's the most straight forward way that most others would do. So if different permissions are defined, then user mode tools would have to distinguish enclaves from regular .so files in order to grant them different permissions. Would that be something extra to existing tools? It doesn't require any userspace code changes. It is just a matter of defining some configuration data in the policy for the new permissions, one or more security labels (tags) for the SGX .so files, and rules allowing access where desired, and then setting those security labels on the SGX .so files (via the security.selinux extended attribute on the files). Even the last part is generally handled by updating a configuration specifying how files should be labeled and then rpm automatically labels the files when created, or you can manually restorecon them. If the new permissions are defined in their own security class rather than reusing existing ones, then they can even be defined entirely via a local or third party policy module separate from the distro policy if desired/needed. > > > > > One residual concern I have with the reuse of FILE__EXECUTE is using it > > for the sigstruct file as the fallback case. If the sigstruct is always > > part of the same file as the code, then it probably doesn't matter. But > > otherwise, it is somewhat odd to have to allow the host process to > > execute from the sigstruct file if it is only data (the signature). > > I agree with you. But do you think it a practical problem today? As far as I know, no one is deploying sigstructs in dedicated files. I'm just trying to touch as few things as possible until there's definitely a need to do so. I don't know, and it wasn't clear to me from the earlier discussions. If not and if it is acceptable to require them to be in files in the first place, then perhaps it isn't necessary.
On Tue, Jun 25, 2019 at 2:09 PM Stephen Smalley <sds@tycho.nsa.gov> wrote: > > On 6/21/19 5:22 PM, Xing, Cedric wrote: > >> From: Christopherson, Sean J > >> Sent: Wednesday, June 19, 2019 3:24 PM > >> > >> Intended use of each permission: > >> > >> - SGX_EXECDIRTY: dynamically load code within the enclave itself > >> - SGX_EXECUNMR: load unmeasured code into the enclave, e.g. Graphene > > > > Why does it matter whether a code page is measured or not? > > It won't be incorporated into an attestation? > Also, if there is, in parallel, a policy that limits the set of enclave SIGSTRUCTs that are accepted, requiring all code be measured makes it harder to subvert by writing incompetent or maliciously incompetent enclaves.
> From: Andy Lutomirski [mailto:luto@kernel.org] > Sent: Saturday, June 29, 2019 4:42 PM > > On Tue, Jun 25, 2019 at 2:09 PM Stephen Smalley <sds@tycho.nsa.gov> > wrote: > > > > On 6/21/19 5:22 PM, Xing, Cedric wrote: > > >> From: Christopherson, Sean J > > >> Sent: Wednesday, June 19, 2019 3:24 PM > > >> > > >> Intended use of each permission: > > >> > > >> - SGX_EXECDIRTY: dynamically load code within the enclave itself > > >> - SGX_EXECUNMR: load unmeasured code into the enclave, e.g. > > >> Graphene > > > > > > Why does it matter whether a code page is measured or not? > > > > It won't be incorporated into an attestation? > > > > Also, if there is, in parallel, a policy that limits the set of enclave > SIGSTRUCTs that are accepted, requiring all code be measured makes it > harder to subvert by writing incompetent or maliciously incompetent > enclaves. As analyzed in my reply to one of Stephen's comments, no executable page shall be "enclave only" as enclaves have access to host's memory, so if an executable page in regular memory is considered posting a threat to the process, it should be considered posting the same threat inside an enclave as well. That said, every executable enclave page should have an executable source page (doesn’t have to executable, as long as mprotect(X) would succeed on it, as shown in my patch), hence any exploits mountable on the enclave page shall also be mountable using the source page. Given only the weakest link matters in security, I argue that SGX_EXECUNMR is unnecessary from the process's perspective. SGX_EXECUNMR does impact security from the enclave's perspective, thus it is reflected in enclave's measurement, which is part of SGX ISA. It's the enclave vendor's responsibility to ensure code pages are properly measured and that's largely automated by tools. It's highly unlikely an ISV would "forget" to measure a page so I don't think SGX_EXECUNMR has much value for ISVs. So the only case left is the enclave author left a page unmeasured with a malicious intent. As that's part of the enclave measurement, it would get caught at EINIT because of an untrusted/blacklisted signing key, or it doesn't because of the lack of whitelisting/blacklisting mechanism. But in the latter case, the adversary could just measure the malicious page as the final measurement or signing key doesn't matter anyway. Sean's series doesn't have an enclave_init() hook so it will always be the latter case, where the final measurement doesn't matter. Therefore, SGX_EXECUNMR doesn't have any value as adversaries could always measure all code pages to satisfy the policy without worrying about final measurements.
On Mon, Jul 1, 2019 at 10:46 AM Xing, Cedric <cedric.xing@intel.com> wrote: > > > From: Andy Lutomirski [mailto:luto@kernel.org] > > Sent: Saturday, June 29, 2019 4:42 PM > > > > On Tue, Jun 25, 2019 at 2:09 PM Stephen Smalley <sds@tycho.nsa.gov> > > wrote: > > > > > > On 6/21/19 5:22 PM, Xing, Cedric wrote: > > > >> From: Christopherson, Sean J > > > >> Sent: Wednesday, June 19, 2019 3:24 PM > > > >> > > > >> Intended use of each permission: > > > >> > > > >> - SGX_EXECDIRTY: dynamically load code within the enclave itself > > > >> - SGX_EXECUNMR: load unmeasured code into the enclave, e.g. > > > >> Graphene > > > > > > > > Why does it matter whether a code page is measured or not? > > > > > > It won't be incorporated into an attestation? > > > > > > > Also, if there is, in parallel, a policy that limits the set of enclave > > SIGSTRUCTs that are accepted, requiring all code be measured makes it > > harder to subvert by writing incompetent or maliciously incompetent > > enclaves. > > As analyzed in my reply to one of Stephen's comments, no executable page shall be "enclave only" as enclaves have access to host's memory, so if an executable page in regular memory is considered posting a threat to the process, it should be considered posting the same threat inside an enclave as well. Huh? The SDM (37.3 in whateve version I'm reading) says "Code fetches from inside an enclave to a linear address outside that enclave result in a #GP(0) exception." Enclaves execute enclave code only. In any event, I believe we're discussing taking readable memory from outside the enclave and copying it to an executable code inside the enclave. > > That said, every executable enclave page should have an executable source page (doesn’t have to executable, as long as mprotect(X) would succeed on it, as shown in my patch) Does Sean's series require this? I think that, if we can get away with it, it's a lot nicer to *not* require user code to map the source pages PROT_EXEC. Some policy may check that it's VM_MAYEXEC or check some other attribute of the VMA, but actually requiring PROT_EXEC seems like we're weakening existing hardening measures to enforce a policy, which is a mistake.
> From: Stephen Smalley [mailto:stephen.smalley@gmail.com] > Sent: Friday, June 28, 2019 6:16 PM > > On Fri, Jun 28, 2019 at 5:20 PM Xing, Cedric <cedric.xing@intel.com> > wrote: > > > > > From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx- > > > owner@vger.kernel.org] On Behalf Of Stephen Smalley > > > Sent: Friday, June 28, 2019 9:17 AM > > > > > > FWIW, adding new permissions only requires updating policy > > > configuration, not userspace code/tools. But in any event, we can > > > reuse the execute- related permissions if it makes sense but still > > > consider introducing additional, new permissions, possibly in a > > > separate "enclave" security class, if we want explicit control over > enclave loading, e.g. > > > ENCLAVE__LOAD, ENCLAVE__INIT, etc. > > > > I'm not so familiar with SELinux tools so my apology in advance if I > end up mixing up things. > > > > I'm not only talking about the new permissions, but also how to apply > them to enclave files. Intel SGX SDK packages enclaves as .so files, and > I guess that's the most straight forward way that most others would do. > So if different permissions are defined, then user mode tools would have > to distinguish enclaves from regular .so files in order to grant them > different permissions. Would that be something extra to existing tools? > > It doesn't require any userspace code changes. It is just a matter of > defining some configuration data in the policy for the new permissions, > one or more security labels (tags) for the SGX .so files, and rules > allowing access where desired, and then setting those security labels on > the SGX .so files (via the security.selinux extended attribute on the > files). Even the last part is generally handled by updating a > configuration specifying how files should be labeled and then rpm > automatically labels the files when created, or you can manually > restorecon them. If the new permissions are defined in their own > security class rather than reusing existing ones, then they can even be > defined entirely via a local or third party policy module separate from > the distro policy if desired/needed. I'm not objecting to what you proposed but just trying to understand more. SGX enclaves don't look any different than regular shared objects except the meta data section, which is implementation dependent (all enclaves built by Intel's SDK have .note.sgxmeta sections but others could do something completely different and may not even use ELF sections). Then how does rpm tell whether a .so file is a regular shared object or an SGX enclave? My understanding is, rpm has to be able to distinguish those two in order to label them correctly (differently). Am I correct? > > > > > > > > > One residual concern I have with the reuse of FILE__EXECUTE is using > > > it for the sigstruct file as the fallback case. If the sigstruct is > > > always part of the same file as the code, then it probably doesn't > > > matter. But otherwise, it is somewhat odd to have to allow the host > > > process to execute from the sigstruct file if it is only data (the > signature). > > > > I agree with you. But do you think it a practical problem today? As > far as I know, no one is deploying sigstructs in dedicated files. I'm > just trying to touch as few things as possible until there's definitely > a need to do so. > > I don't know, and it wasn't clear to me from the earlier discussions. > If not and if it is acceptable to require them to be in files in the > first place, then perhaps it isn't necessary.
> From: Andy Lutomirski [mailto:luto@kernel.org] > Sent: Monday, July 01, 2019 10:54 AM > > On Mon, Jul 1, 2019 at 10:46 AM Xing, Cedric <cedric.xing@intel.com> > wrote: > > > > > From: Andy Lutomirski [mailto:luto@kernel.org] > > > Sent: Saturday, June 29, 2019 4:42 PM > > > > > > On Tue, Jun 25, 2019 at 2:09 PM Stephen Smalley <sds@tycho.nsa.gov> > > > wrote: > > > > > > > > On 6/21/19 5:22 PM, Xing, Cedric wrote: > > > > >> From: Christopherson, Sean J > > > > >> Sent: Wednesday, June 19, 2019 3:24 PM > > > > >> > > > > >> Intended use of each permission: > > > > >> > > > > >> - SGX_EXECDIRTY: dynamically load code within the enclave > itself > > > > >> - SGX_EXECUNMR: load unmeasured code into the enclave, e.g. > > > > >> Graphene > > > > > > > > > > Why does it matter whether a code page is measured or not? > > > > > > > > It won't be incorporated into an attestation? > > > > > > > > > > Also, if there is, in parallel, a policy that limits the set of > > > enclave SIGSTRUCTs that are accepted, requiring all code be measured > > > makes it harder to subvert by writing incompetent or maliciously > > > incompetent enclaves. > > > > As analyzed in my reply to one of Stephen's comments, no executable > page shall be "enclave only" as enclaves have access to host's memory, > so if an executable page in regular memory is considered posting a > threat to the process, it should be considered posting the same threat > inside an enclave as well. What I was trying to say was, an executable page, if considered a threat to the enclosing process, should always be considered a threat no matter it is in that process's memory or inside an enclave enclosed in that same process's address space. Therefore, for a page in regular memory, if it is denied executable, it is because it is considered a potential security threat to the enclosing process, so it shall not be used as the source for an executable enclave page, as the same threat exists regardless it is in regular memory or EPC. Does that make more sense? > > Huh? The SDM (37.3 in whateve version I'm reading) says "Code fetches > from inside an enclave to a linear address outside that enclave result > in a #GP(0) exception." Enclaves execute enclave code only. > > In any event, I believe we're discussing taking readable memory from > outside the enclave and copying it to an executable code inside the > enclave. You are correct. SGX ISA doesn't care the source page as it only takes care of the security the enclave itself. But LSM on the other hand also takes care of the enclosing process. That said, a page, if denied executable because it is considered a potential threat to the process by LSM, should also be denied (by LSM) as the source for an executable enclave page because the same threat would exist even if it resides inside an enclave, for enclaves have access to all of the enclosing process's memory. > > > > > That said, every executable enclave page should have an executable > > source page (doesn’t have to executable, as long as mprotect(X) would > > succeed on it, as shown in my patch) > > Does Sean's series require this? I think that, if we can get away with > it, it's a lot nicer to *not* require user code to map the source pages > PROT_EXEC. Some policy may check that it's VM_MAYEXEC or check some > other attribute of the VMA, but actually requiring PROT_EXEC seems like > we're weakening existing hardening measures to enforce a policy, which > is a mistake. My patch doesn't require X on source pages either. I said "would", meaning X *would* be granted but doesn't have to be granted. You can see this in selinux_enclave_load() calling selinux_file_mprotect() in my code. The purpose is to determine if X *would* be granted to the source pages without actually granting X.
Hi Andy, > From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx- > owner@vger.kernel.org] On Behalf Of Xing, Cedric > Sent: Monday, July 01, 2019 11:54 AM > > > > > > That said, every executable enclave page should have an executable > > > source page (doesn’t have to executable, as long as mprotect(X) > would > > > succeed on it, as shown in my patch) > > > > Does Sean's series require this? I think that, if we can get away > with > > it, it's a lot nicer to *not* require user code to map the source > pages > > PROT_EXEC. Some policy may check that it's VM_MAYEXEC or check some > > other attribute of the VMA, but actually requiring PROT_EXEC seems > like > > we're weakening existing hardening measures to enforce a policy, which > > is a mistake. > > My patch doesn't require X on source pages either. I said "would", > meaning X *would* be granted but doesn't have to be granted. You can see > this in selinux_enclave_load() calling selinux_file_mprotect() in my > code. The purpose is to determine if X *would* be granted to the source > pages without actually granting X. Forgot to conclude that we are on the same page for the requirement on the source pages. And given that requirement (enclave page cannot be X unless source would also be allowed X), measuring enclave code pages or not doesn't make any difference from the enclosing process's perspective in terms of security. So it only makes a difference for the enclave, which however has been covered cryptographically by its measurement already. So SGX_EXECUNMR doesn't have any practical use, thus I don't think it should be added as a new permission.
On Mon, Jul 1, 2019 at 11:54 AM Xing, Cedric <cedric.xing@intel.com> wrote: > > > From: Andy Lutomirski [mailto:luto@kernel.org] > > Sent: Monday, July 01, 2019 10:54 AM > > > > On Mon, Jul 1, 2019 at 10:46 AM Xing, Cedric <cedric.xing@intel.com> > > wrote: > > > > > > > From: Andy Lutomirski [mailto:luto@kernel.org] > > > > Sent: Saturday, June 29, 2019 4:42 PM > > > > > > > > On Tue, Jun 25, 2019 at 2:09 PM Stephen Smalley <sds@tycho.nsa.gov> > > > > wrote: > > > > > > > > > > On 6/21/19 5:22 PM, Xing, Cedric wrote: > > > > > >> From: Christopherson, Sean J > > > > > >> Sent: Wednesday, June 19, 2019 3:24 PM > > > > > >> > > > > > >> Intended use of each permission: > > > > > >> > > > > > >> - SGX_EXECDIRTY: dynamically load code within the enclave > > itself > > > > > >> - SGX_EXECUNMR: load unmeasured code into the enclave, e.g. > > > > > >> Graphene > > > > > > > > > > > > Why does it matter whether a code page is measured or not? > > > > > > > > > > It won't be incorporated into an attestation? > > > > > > > > > > > > > Also, if there is, in parallel, a policy that limits the set of > > > > enclave SIGSTRUCTs that are accepted, requiring all code be measured > > > > makes it harder to subvert by writing incompetent or maliciously > > > > incompetent enclaves. > > > > > > As analyzed in my reply to one of Stephen's comments, no executable > > page shall be "enclave only" as enclaves have access to host's memory, > > so if an executable page in regular memory is considered posting a > > threat to the process, it should be considered posting the same threat > > inside an enclave as well. > > What I was trying to say was, an executable page, if considered a threat to the enclosing process, should always be considered a threat no matter it is in that process's memory or inside an enclave enclosed in that same process's address space. > > Therefore, for a page in regular memory, if it is denied executable, it is because it is considered a potential security threat to the enclosing process, so it shall not be used as the source for an executable enclave page, as the same threat exists regardless it is in regular memory or EPC. Does that make more sense? It does make sense, but I'm not sure it's correct to assume that any LSM policy will always allow execution on enclave source pages if it would allow execution inside the enclave. As an example, here is a policy that seems reasonable: Task A cannot execute dynamic non-enclave code (no execmod, no execmem, etc -- only approved unmodified file pages can be executed). But task A can execute an enclave with MRENCLAVE == such-and-such, and that enclave may be loaded from regular anonymous memory -- the MRENCLAVE is considered enough verification. > > My patch doesn't require X on source pages either. I said "would", meaning X *would* be granted but doesn't have to be granted. You can see this in selinux_enclave_load() calling selinux_file_mprotect() in my code. The purpose is to determine if X *would* be granted to the source pages without actually granting X. As above, I'm not convinced this assumption is valid.
> From: Andy Lutomirski [mailto:luto@kernel.org] > Sent: Monday, July 01, 2019 12:33 PM > > It does make sense, but I'm not sure it's correct to assume that any LSM > policy will always allow execution on enclave source pages if it would > allow execution inside the enclave. As an example, here is a policy > that seems reasonable: > > Task A cannot execute dynamic non-enclave code (no execmod, no execmem, > etc -- only approved unmodified file pages can be executed). > But task A can execute an enclave with MRENCLAVE == such-and-such, and > that enclave may be loaded from regular anonymous memory -- the > MRENCLAVE is considered enough verification. You are right. That's a reasonable policy. But I still can't see the need for SGX_EXECUNMR, as MRENCLAVE is considered enough verification.
On Mon, Jul 01, 2019 at 01:03:51PM -0700, Xing, Cedric wrote: > > From: Andy Lutomirski [mailto:luto@kernel.org] > > Sent: Monday, July 01, 2019 12:33 PM > > > > It does make sense, but I'm not sure it's correct to assume that any LSM > > policy will always allow execution on enclave source pages if it would > > allow execution inside the enclave. As an example, here is a policy > > that seems reasonable: > > > > Task A cannot execute dynamic non-enclave code (no execmod, no execmem, > > etc -- only approved unmodified file pages can be executed). > > But task A can execute an enclave with MRENCLAVE == such-and-such, and > > that enclave may be loaded from regular anonymous memory -- the > > MRENCLAVE is considered enough verification. > > You are right. That's a reasonable policy. But I still can't see the need for > SGX_EXECUNMR, as MRENCLAVE is considered enough verification. That assumes the enclave/loader developer will never make a mistake, and that policy owners are going to do a deep dive on the EEXTEND values for an enclave (and will never make a mistake). User errors aside, EXECUNMR would also be useful in conjunction with MRSIGNER, e.g. allow all enclaves signed by X, but disallow unmeasured code.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index fc239e541b62..8a431168e454 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -6727,6 +6727,12 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux) #endif #ifdef CONFIG_INTEL_SGX +static inline int sgx_has_perm(u32 sid, u32 requested) +{ + return avc_has_perm(&selinux_state, sid, sid, + SECCLASS_PROCESS2, requested, NULL); +} + static int selinux_enclave_map(unsigned long prot) { const struct cred *cred = current_cred(); @@ -6736,11 +6742,53 @@ static int selinux_enclave_map(unsigned long prot) WARN_ON_ONCE(!default_noexec); if ((prot & PROT_EXEC) && (prot & PROT_WRITE)) - return avc_has_perm(&selinux_state, sid, sid, - SECCLASS_PROCESS2, PROCESS2__SGX_MAPWX, - NULL); + return sgx_has_perm(sid, PROCESS2__SGX_MAPWX); + return 0; } + +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; + + /* SGX is supported only in 64-bit kernels. */ + WARN_ON_ONCE(!default_noexec); + + /* Only executable enclave pages are restricted in any way. */ + if (!(prot & PROT_EXEC)) + return 0; + + /* + * WX at load time only requires EXECDIRTY, e.g. to allow W->X. Actual + * WX mappings require MAPWX (see selinux_enclave_map()). + */ + if (prot & PROT_WRITE) { + ret = sgx_has_perm(sid, PROCESS2__SGX_EXECDIRTY); + if (ret) + goto out; + } + if (!measured) { + ret = sgx_has_perm(sid, PROCESS2__SGX_EXECUNMR); + if (ret) + goto out; + } + + if (!vma->vm_file || IS_PRIVATE(file_inode(vma->vm_file)) || + vma->anon_vma) + /* + * Loading enclave code from an anonymous mapping or from a + * modified private file mapping. + */ + ret = sgx_has_perm(sid, PROCESS2__SGX_EXECANON); + else + /* Loading from a shared or unmodified private file mapping. */ + ret = file_has_perm(cred, vma->vm_file, FILE__SGX_EXECUTE); +out: + return ret; +} #endif struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = { @@ -6988,6 +7036,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { #ifdef CONFIG_INTEL_SGX LSM_HOOK_INIT(enclave_map, selinux_enclave_map), + LSM_HOOK_INIT(enclave_load, selinux_enclave_load), #endif }; diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h index cfd91e879bdf..baa1757be46a 100644 --- a/security/selinux/include/classmap.h +++ b/security/selinux/include/classmap.h @@ -7,7 +7,7 @@ #define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \ "rename", "execute", "quotaon", "mounton", "audit_access", \ - "open", "execmod" + "open", "execmod", "sgx_execute" #define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \ "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom", \ @@ -52,7 +52,8 @@ struct security_class_mapping secclass_map[] = { "setsockcreate", "getrlimit", NULL } }, { "process2", { "nnp_transition", "nosuid_transition", - "sgx_mapwx", NULL } }, + "sgx_mapwx", "sgx_execdirty", "sgx_execanon", "sgx_execunmr", + NULL } }, { "system", { "ipc_info", "syslog_read", "syslog_mod", "syslog_console", "module_request", "module_load", NULL } },
The goal of selinux_enclave_load() is to provide a facsimile of the existing selinux_file_mprotect() and file_map_prot_check() policies, but tailored to the unique properties of SGX. For example, an enclave page is technically backed by a MAP_SHARED file, but the "file" is essentially shared memory that is never persisted anywhere and also requires execute permissions (for some pages). Enclaves are also less priveleged than normal user code, e.g. SYSCALL instructions #UD if attempted in an enclave. For this reason, add SGX specific permissions instead of reusing existing permissions such as FILE__EXECUTE so that policies can allow running code in an enclave, or allow dynamically loading code in an enclave without having to grant the same capability to normal user code outside of the enclave. Intended use of each permission: - SGX_EXECDIRTY: dynamically load code within the enclave itself - SGX_EXECUNMR: load unmeasured code into the enclave, e.g. Graphene - SGX_EXECANON: load code from anonymous memory (likely Graphene) - SGX_EXECUTE: load an enclave from a file, i.e. normal behavior Note, equivalents to FILE__READ and FILE__WRITE are intentionally never required. Writes to the enclave page are contained to the EPC, i.e. never hit the original file, and read permissions have already been vetted (or the VMA doesn't have PROT_READ, in which case loading the page into the enclave will fail). Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- security/selinux/hooks.c | 55 +++++++++++++++++++++++++++-- security/selinux/include/classmap.h | 5 +-- 2 files changed, 55 insertions(+), 5 deletions(-)