Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue with enclave creation and host_satp #19

Closed
lukenels opened this issue Mar 1, 2019 · 6 comments

Comments

@lukenels
Copy link

commented Mar 1, 2019

My understanding of the code is that there are no mechanisms for
preventing an enclave from creating additional enclaves itself.
Specifically, from machine/mtrap.c:mcall_trap to
sm/enclave.c:create_enclave there does not seem to be any check
on who issued the mcall. Is this the design intention for Keystone?

If so, it would allow for an enclave to gain control over the OS
by creating a new sub-enclave whose region overlaps with memory
being used by the OS. Additionally, it seems like the value of
satp used at enclave creation serves as a way of keeping track
of the creator of each enclave (in host_satp). This fails if the
OS ever changes its own satp, and might allow for an OS or enclave
to manipulate an enclave it doesn't own through manipulation of its
satp value.

If it is not the design intention, then a simple fix is to keep
track of whether each CPU is currently running an enclave (e.g., a
per-cpu enclave_mode boolean). Then, operations such as enclave
creation or destruction can fail if they come from an enclave.

Please let me know if I've missed something or if my understanding
of the code is incorrect. Thanks!

@lukenels lukenels changed the title Issue with enclave creation and `host_satp` Issue with enclave creation and host_satp Mar 1, 2019

@dkohlbre

This comment has been minimized.

Copy link
Collaborator

commented Mar 1, 2019

This is an interesting issue!
We're actually currently debating if/how to allow enclaves to launch other enclaves.

The current design won't actually let anything... THAT terrible happen from an enclave trying to create another enclave inside of the OS memory, since the parent enclave won't have any access rights to the child. We should either be explicitly allowing enclaves to create enclaves (and making sure they can't do anything bad with that), or we should be blocking them. I think for now we'll want to just disable enclaves creating child enclaves. I think the worst that happens here is the OS crashes (which is bad! just not that bad)

As for the satp tracking, @dayeol do you have any thoughts there?

@dayeol

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

Hi, thank you for reporting these 👍

(1) tracking CPU state.
We definitely need per-cpu state data as well so that the SM can know which context is an SBI from.
I was aware that there will be some problems if we don't check the caller of each SBI, but somehow I forgot to handle that. There're a lot more assumptions that we need to assert,
for example, run_enclave is supposed to be called only by the OS and so on.

(2) OS manipulating satp
The OS manipulating satp is not in the enclave's threat model.
The OS should handle all of the enclaves anyway, and it should be able to create/execute/destroy enclaves in the system.
The only threat in our scope is a user program tries to manage an enclave owned by "another" user program, which can be protected by satp mechanism.
Managing enclaves using satp is just a design choice for now though, because we assume that only the creator user program can command the enclave.

@dayeol

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

We need to fix (1) by letting SM know where SM SBIs are coming from.

@dayeol dayeol added the required label Mar 1, 2019

@dkohlbre

This comment has been minimized.

Copy link
Collaborator

commented Mar 1, 2019

Indeed, to clarify the satp thing further, the only potential issue is one enclave affecting the lifecycle of another enclave.
Once an enclave is created, it is 'owned' by no one in particular, the OS handles any ownership decisions for host processes.

@dkohlbre

This comment has been minimized.

Copy link
Collaborator

commented Mar 11, 2019

This is now part of issue #25 , I'll leave this open for now until we rework things to close that.

@dayeol dayeol closed this in b30b15e Apr 1, 2019

@dayeol

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

This issue has been resolved, but we still have to deal with #25.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.