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

undefined shift & buffer overflow in `get_host_satp()` #20

Closed
xiw opened this issue Mar 3, 2019 · 6 comments

Comments

@xiw
Copy link

commented Mar 3, 2019

There are two bugs in get_host_satp():

 if(!TEST_BIT(encl_bitmap, eid))
    return ENCLAVE_NOT_ACCESSIBLE;

  *satp = enclaves[eid].host_satp;

First, TEST_BIT(encl_bitmap, eid) introduces undefined behavior, as eid can be any value and oversized shifting is undefined in C. We should add a check before the TEST_BIT check, something like (eid < ENCL_MAX), assuming ENCL_MAX is always smaller than the shifting amount and we don't have that many enclaves/PMP regions anyway.

Second, this can cause a buffer overflow in enclaves[eid]. On RISC-V only the lower 5 bits are considered for a 32-bit shifting amount. For instance, 1 << 32 produces the same value as 1 << 0 (i.e., the result is 1, rather than 0). One may pass in a large eid whose lower 5 bits correspond to a valid enclave ID; this will bypass the TEST_BIT check and cause a buffer overflow. Adding the check on eid for the first bug should fix this buffer overflow.

There are similar checks on eid in destroy_enclave, run_enclave, resume_enclave. We don't necessarily have to fix them right now, as long as they are called from mcall_sm_destroy_enclave, mcall_sm_run_enclave, and mcall_sm_resume_enclave, all of which invoke get_host_satp. Though it's probably a good idea to clean up the checks at some point (e.g., to put them in one place).

@ekiwi

This comment has been minimized.

Copy link

commented Mar 3, 2019

This seems to be a duplicate of issue #3 (with a much better explanation though!)

@xiw

This comment has been minimized.

Copy link
Author

commented Mar 4, 2019

@ekiwi Aha - thanks for the pointer! We were more concerned about the buffer overflow issue, which can be fixed together with the undefined shift.

@dkohlbre

This comment has been minimized.

Copy link
Collaborator

commented Mar 4, 2019

Yeah, we'll keep this as a non-dupe and figure out how we want to go after it. I think we may be changing some of the code around there more than just fixing this specific set of bugs.

@xiw

This comment has been minimized.

Copy link
Author

commented Mar 4, 2019

@dkohlbre Thanks for the reply! Adding a check on eid would be a simple fix. Another possible way is to remove encl_bitmap and simply use the state field of struct enclave_t for this check.

One could even further push the enclave ID allocation out of the security monitor, letting the OS specify an eid in create_enclave (like in SGX/Komodo).

@dkohlbre

This comment has been minimized.

Copy link
Collaborator

commented Mar 4, 2019

I agree entirely, we'd been planning to remove the use of the old encl_bitmap entirely, thus why #3 was still open.
I'll be doing it in stages, wrap the macros with checks, and then update our usage of the state field to properly encapsulate this functionality.

dkohlbre added a commit to dkohlbre/riscv-pk that referenced this issue Mar 4, 2019

Updated all usage of encl_bitmap to be wrapped for easy removal later…
…. As part of this added safety checks for EID usage for issues keystone-enclave#3 and keystone-enclave#20

dkohlbre added a commit to dkohlbre/riscv-pk that referenced this issue Mar 4, 2019

Updated all usage of encl_bitmap to be wrapped for easy removal later…
…. As part of this added safety checks for EID usage for issues keystone-enclave#3 and keystone-enclave#20
@dkohlbre

This comment has been minimized.

Copy link
Collaborator

commented Mar 11, 2019

This code no longer exists since the cleanup landed.

@dkohlbre dkohlbre closed this Mar 11, 2019

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.