Re: [PATCH v38 10/24] mm: Add vm_ops->mprotect()

[Dr. Greg]

On Tue, Oct 20, 2020 at 09:40:00AM -0700, Sean Christopherson wrote:

Good morning, I hope the week has gone well for everyone.

> On Tue, Oct 20, 2020 at 05:01:18AM -0500, Dr. Greg wrote:
> >
> > With respect to the security issue at hand, the only relevant issue
> > would seem to be if a page had write permissions at one time in its
> > trajectory to having execute permisions, isn't this correct?

> No.  RW->RX has different properties than RWX.  E.g. an enclave that
> dynamically loads code is not the same thing as an enclave that
> allows simultaneously writing and executing a page.

Yes, it is certainly correct that a platform administrator may want to
restrict RWX, given that it makes an enclave susceptible to potential
arbitrary code execution if there is a programming error in the
enclave.

However, I think it is important for everyone interested in these
issues, to reflect back on what started all of this and that was
Andy's concern that the initial incantations of the driver allowed
execution of arbitrary memory without the ability of the LSM to get a
'look' at the code/memory.

My point in all of this is that a permissions trajectory for an
enclave that allows for write permissions on a path that terminates in
X permissions opens the door for arbitrary memory execution that the
platform security architect has no insight into or that the LSM will
have any control over.

There is no guarantee that dynamically loaded code has to come into
the enclave via anything that the operating system has visibility
into.  If the enclave can toggle RW->RX it is free to dynamically load
code, in encrypted form over the network and then execute it.

In fact, I would posit that this model will be a primary use for
dynamic code loading.  The SGX user community views 'confidential
computing' as much about protecting visibility into algorithms and
code as it is about data that is being operated on.  In certain
unnamed venues where I have consulted it is the primary concern.

So in the broadest sense, we have spent a year worrying about if and
how the LSM will have visibility into enclave based code and in the
end the only really relevant security mechanism available is limiting
page permission transitions that prevent dynamic code loading.  Modulo
of course the issue with RWX, where a platform owner may elect to try
and prevent an enclave writer from shooting themselves in the foot.

The issue at hand is that the primary security threat of the
technology is the same as what the user community wants to use it for.
Joanna Rutkowska called that out a half decade ago when she first
reviewed the technology.

> > The next paragraph of my reply wasn't included in your reply, but I
> > did state that the mprotect hook would be relevant if its purpose was
> > to disallow this permission trajectory and in the process disable
> > enclave dynamic code loading and execution.
> > 
> > So to assist everyone in understanding this issue and the security
> > implications involved, is the ultimate purpose of the mprotect hook to
> > disable dynamic code loading?

> No, it's to provide line of sight to enforcing granular LSM checks
> on enclave pages.  Jumping back to the RWX thing, as a defense in
> depth measure, a policy owner could set an SELinux policy to never
> allow RWX, even for enclaves that dynamically load code.
>
> Whether or not having per-page granluarity on enclave permission
> checks is valuable/interesting is debatable, e.g. it's why LSM
> integration is notably absent from the this series.  But, adding the
> ->mprotect() hook is relatively cheap and can always be removed if
> it's deemed unnecessary in the long run.  The reverse is not true;
> omitting ->mprotect() commits the kernel to an ABI that is either
> ugly and potentially painful (require all enclaves to declare full
> RWX permissions), or flat out prevents adding granular LSM support
> in the future (do nothing).

I believe your analysis with respect to the ability to remove
->mprotect is flawed.  The long standing consensus has been that
functionality never gets broken.  Once ->mprotect is a component of
the ABI it would have to be left intact since it could potentially
break things that elected to use it.  On the other hand there is a
long standing history of adding functionality.

I can't bring myself to believe that LSM's are going to be written
that will be making enclave security decisions on a page by page
basis.  Given what I have written above, I think all of this comes
down to giving platform administrators one of three decisions, in
order of most to least secure:

1.) Block dynamic code loading and execution.

2.) Block access to RWX pages.

3.) The wild west - no restrictions on enclave page protection manipulation.

>From a security perspective I would argue for the wisdom of making
option 1 unconditional via a kernel command-line parameter.

It may be that ->mprotect is the right mechanism to implement this.
If that is the case, frame the discussion and documentation so that it
reflects the actual security threat and the consideration and means
for dealing with it.

Hopefully all of this is useful to the stakeholders in this
technology.

Have a good weekend.

Dr. Greg

As always,
Dr. Greg Wettstein, Ph.D, Worker      Autonomously self-defensive
Enjellic Systems Development, LLC     IOT platforms and edge devices.
4206 19th Ave. N.
Fargo, ND  58102
PH: 701-281-1686                      EMAIL: greg@xxxxxxxxxxxx
------------------------------------------------------------------------------
"Politics is the business of getting power and privilege without possessing
 merit."
                                -- P.J. O'Rourke