search

gramineproject / graphene

Graphene / Graphene-SGX - a library OS for Linux multi-process applications, with Intel SGX support
https://grapheneproject.io
GNU Lesser General Public License v3.0
767 stars 261 forks source link

[Pal/Linux-SGX] Allow file-backed writable mmaps #1954

Open dimakuv opened 3 years ago

dimakuv commented 3 years ago

  1. We don't allow file-backed writable mmaps (mmap(0, size, PROT_WRITE, MAP_SHARED, file_fd, 0)) even for sgx.allowed_files. We simply return a DENIED error: https://github.com/oscarlab/graphene/blob/4e26fe7e3280a66130000769ea48477bc8da6ade/Pal/src/host/Linux-SGX/db_files.c#L430-L437

  2. We already support writable mmaps on sgx.protected_files: https://github.com/oscarlab/graphene/blob/4e26fe7e3280a66130000769ea48477bc8da6ade/Pal/src/host/Linux-SGX/db_files.c#L353

  3. And we don't want to support writable mmaps on sgx.trusted_files; this is already correctly enforced: https://github.com/oscarlab/graphene/blob/4e26fe7e3280a66130000769ea48477bc8da6ade/Pal/src/host/Linux-SGX/db_files.c#L257

We should allow item 1. There is no reason to have it disabled (allowed files are anyway purely for debugging and are insecure).

dimakuv commented 3 years ago

@vijaydhanraj Please look at this. This looks like a rather simple change.

vijaydhanraj commented 3 years ago

sure, will take a look at this issue.

dimakuv commented 3 years ago

So this issue turned out to be more complicated to do right than it seemed. The main problem is that if we keep allowed files' mappings in untrusted memory, there are not only zero security guarantees, but also tricky problems of say TOCTOU. So even reading and parsing the allowed file may lead to reading corrupted data.

The only way seems to do the same as currently in protected files: copy allowed files' content inside the enclave, let the app access the in-enclave copy, and copy back on explicit fsync and close. But since allowed files are frequently used for shared futexes (and other kinds of quick sync among processes), this "half-way emulation" is not enough.

So currently it's just not worth implementing this feature unless we have (and understand!) a specific use case that will benefit from it. Not closing the issue, but this is put on a back burner.