RE: [RFC PATCH v4 10/12] security/selinux: Add enclave_load() implementation

From: "Xing, Cedric" <cedric.xing@intel.com>
To: Andy Lutomirski <luto@kernel.org>, Stephen Smalley <sds@tycho.nsa.gov>
Cc: "Christopherson, Sean J" <sean.j.christopherson@intel.com>,
	"Jarkko Sakkinen" <jarkko.sakkinen@linux.intel.com>,
	"linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
	"linux-security-module@vger.kernel.org" 
	<linux-security-module@vger.kernel.org>,
	"selinux@vger.kernel.org" <selinux@vger.kernel.org>,
	"Roberts, William C" <william.c.roberts@intel.com>,
	"Schaufler, Casey" <casey.schaufler@intel.com>,
	James Morris <jmorris@namei.org>,
	"Hansen, Dave" <dave.hansen@intel.com>,
	Jethro Beekman <jethro@fortanix.com>,
	"Dr . Greg Wettstein" <greg@enjellic.com>
Subject: RE: [RFC PATCH v4 10/12] security/selinux: Add enclave_load() implementation
Date: Mon, 1 Jul 2019 17:46:04 +0000	[thread overview]
Message-ID: <960B34DE67B9E140824F1DCDEC400C0F6551D558@ORSMSX116.amr.corp.intel.com> (raw)
In-Reply-To: <CALCETrWPzSaFUWi4q4Vq_0RrtNMFZAKkwKkya=p6cfB50x2tMQ@mail.gmail.com>

> 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.