[Linux-SGX] Add protected files implementation (SGX SDK file format) #1325

omeg · 2020-02-12T18:34:25Z

Affected components

Description of the changes

Protected files (PF) are a new type of file that can be specified in
the manifest (SGX only). They are encrypted on disk and transparently
decrypted when accessed by the Graphene payload.

Other features:

data is integrity protected (tamper resistance)
file swap protection (a PF can only be accessed when in a specific path)
transparency (Graphene payload sees PFs as regular files, no need to modify
the payload)

The following new manifest elements are added:

sgx.protected_files_key = <16-byte hex value>
sgx.protected_files.<name> = file:<host path>

sgx.protected_files_key specifies the encryption key and is only a temporary
implementation. This key should be provisioned with local/remote attestation
in the future.

Paths specifying PF entries can be files or directories. If a directory is
specified, all files/directories within are registered as protected
recursively (and are expected to be encrypted in the PF format).

Linux-SGX/tools directory contains the pf_crypt utility that converts files
to/from the protected format.

Internal protected file format in this version was ported from the SGX SDK:
https://github.com/intel/linux-sgx/tree/master/sdk/protected_fs

TODO:

Truncating files is not yet implemented.
The recovery file feature is disabled, this needs to be discussed if it's needed in Graphene.
Tests for invalid/malformed/corrupted files need to be ported to the new format.

Fixes #1610 (as a side effect).

How to test this PR?

Tests in LibOS/shim/test/fs now contain PF tests too (target is still test). Target to run just non-PF tests is fs-test.

This change is

sudharkrish · 2020-02-14T21:39:50Z

@omeg file's content has confidentiality, and integrity protected.

sudharkrish · 2020-02-14T23:06:51Z

Pal/src/host/Linux-SGX/protected_files.c, line 50 at r1 (raw file):

 */

#include <linux/fs.h>

This file protected_files.c is 2000+ lines, has all the code from SGX SDK library, that was spread across several files, based on functionality, like file_read_write.c, file_flush.c, file_crypto.c and so on. Can we have a sub-directory under host/Linux-SGX, like pf or protected-fs and split this feature into a bunch of files, its hard to review, such a huge file.

sudharkrish · 2020-02-14T23:09:15Z

Pal/src/host/Linux-SGX/pal_linux.h, line 163 at r1 (raw file):

/* Used to track map buffers for protected files */
DEFINE_LIST(pf_map);

Can you clarify or add some comments on how these pf_map buffers are used while flushing to the PF and whether it has any dependency with flushing from lru_cache to PF on disk.

sudharkrish · 2020-02-14T23:33:37Z

LibOS/shim/test/fs/manifest.template, line 37 at r1 (raw file):

sgx.protected_files_key = ffeeddccbbaa99887766554433221100
sgx.protected_files.input = file:tmp/pf_input
sgx.protected_files.output = file:tmp/pf_output

This part is not clear. In this example manifest, tmp/pf_input, tmp/pf_output, are these input and output files, (OR) are they directory-paths to input files and output files.
For example, if pf_input is a file(unencrypted) under tmp directory, and new protected file will be generated under tmp as pf_output(encrypted).

omeg

Reviewable status: 0 of 32 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @sudharkrish)

LibOS/shim/test/fs/manifest.template, line 37 at r1 (raw file):
As per note in the PR description:

Paths specifying PF entries can be files or directories. If a directory is
specified, all files/directories within are registered as protected
recursively (and are expected to be encrypted in the PF format).
Paths in the test manifest are directories. The test script populates pf_input with a number of encrypted files. pf_output is usually empty (depends on a particular test) and is used for writable protected files.

Pal/src/host/Linux-SGX/pal_linux.h, line 163 at r1 (raw file):

Previously, sudharkrish (Sudha Krishnakumar) wrote…

Can you clarify or add some comments on how these pf_map buffers are used while flushing to the PF and whether it has any dependency with flushing from lru_cache to PF on disk.

These buffers are allocated by pf_file_map() in db_files.c. They are flushed either when the address is unmapped or the file's fd is closed (_DkStreamUnmap() in db_streams.c and pf_file_close() in db_files.c respectively, both calling flush_pf_maps() in enclave_pf.c in the end).

Pal/src/host/Linux-SGX/protected_files.c, line 50 at r1 (raw file):

Previously, sudharkrish (Sudha Krishnakumar) wrote…

This file protected_files.c is 2000+ lines, has all the code from SGX SDK library, that was spread across several files, based on functionality, like file_read_write.c, file_flush.c, file_crypto.c and so on. Can we have a sub-directory under host/Linux-SGX, like pf or protected-fs and split this feature into a bunch of files, its hard to review, such a huge file.

Agreed, I'll split the implementation into smaller files.

sudharkrish · 2020-02-19T01:30:22Z

LibOS/shim/test/fs/test_pf.py, line 20 at r1 (raw file):

@unittest.skipUnless(HAS_SGX, 'Protected files require SGX support')
class TC_50_ProtectedFiles(TC_00_FileSystem):

Just wondering, if it would be possible to add some tests that shows performance of protected file access, like file read, file write and other protected file operations, when compared to regular non-protected files, when running within graphene. It will be helpful to have this peformance data.

sudharkrish · 2020-02-19T02:09:29Z

Pal/src/host/Linux-SGX/db_files.c, line 131 at r1 (raw file):

            }

            pf->refcount++;

Just wondering, if we would need a spinlock to protect increment or decrement of this reference count.

sudharkrish

Reviewable status: 0 of 32 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @omeg and @sudharkrish)

Pal/src/host/Linux/pal_linux_error.h, line 12 at r1 (raw file):

    switch (unix_errno) {
        case 0:
            return 0;

In general 0 is returned for success. Do you need this check for 0 in this error-related function.

sudharkrish · 2020-02-20T00:57:22Z

a discussion (no related file):
@omeg, sgx-protect fs has the option where user can provide custom-key(like how you are providing through manifest file) or it can be automatically generated using platform's sealing key. Using automatically generated keys(using platform key) will be a good feature, for applications that do not have usages to move protected files from one platform to another. Wondering what was the reason, for removing that functionality.

sudharkrish · 2020-02-20T01:04:00Z

LibOS/shim/test/fs/manifest.template, line 35 at r1 (raw file):

sgx.allowed_files.tmp_dir = file:tmp/

sgx.protected_files_key = ffeeddccbbaa99887766554433221100

Trying to think option, to avoid having key in the manifest. There is plan to have a generic mechanism for secret provisioning in graphene after remote attestation. For example, after remote attestation, graphene library can output a sealed(using SGX sealing) secret to filesystem. And then the application that uses, protected-files feature, can unseal the blob, to retrieve the user-provided custom key. For the short-term, if we can add support for provisioning sealed custom key, that will be better than having the key in the manifest file. This would also mean that we add support for SGX seal and unseal in graphene.

sudharkrish · 2020-02-20T01:30:41Z

LibOS/shim/test/fs/manifest.template

@@ -31,3 +31,7 @@ sgx.trusted_files.libpthread = file:../../../../Runtime/libpthread.so.0
 sgx.trusted_files.libgcc_s = file:/lib/x86_64-linux-gnu/libgcc_s.so.1

 sgx.allowed_files.tmp_dir = file:tmp/
+
+sgx.protected_files_key = ffeeddccbbaa99887766554433221100
+sgx.protected_files.input = file:tmp/pf_input


It would be nice to support multiple protected-directory paths, probably separated by a delimiter(like colon or semi-colon).

sudharkrish · 2020-02-20T02:25:45Z

a discussion (no related file):
@omeg , from looking at the code, your providing ->file swap protection (a PF can only be accessed when in a specific path), by the check in this api->/* Exact match of path in protected_file_list /
struct protected_file find_protected_file(const char* path). Is this understanding correct.

omeg

Reviewable status: 0 of 32 files reviewed, 9 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @sudharkrish)

a discussion (no related file):

Previously, sudharkrish (Sudha Krishnakumar) wrote…

@omeg, sgx-protect fs has the option where user can provide custom-key(like how you are providing through manifest file) or it can be automatically generated using platform's sealing key. Using automatically generated keys(using platform key) will be a good feature, for applications that do not have usages to move protected files from one platform to another. Wondering what was the reason, for removing that functionality.

I think that the application only needs to protect its files if it had some secrets provisioned to it. If there's no secret data, there's no point of using protected files IMO. And if secrets are provisioned (after remote attestation for example) then the PF key can also be provided.

a discussion (no related file):

Previously, sudharkrish (Sudha Krishnakumar) wrote…

@omeg , from looking at the code, your providing ->file swap protection (a PF can only be accessed when in a specific path), by the check in this api->/* Exact match of path in protected_file_list /
struct protected_file find_protected_file(const char* path). Is this understanding correct.

This check is to determine if a file should be treated like a protected file. The actual path check against PF's metadata is in ipf_init_existing_file().

LibOS/shim/test/fs/manifest.template, line 35 at r1 (raw file):

Previously, sudharkrish (Sudha Krishnakumar) wrote…

Trying to think option, to avoid having key in the manifest. There is plan to have a generic mechanism for secret provisioning in graphene after remote attestation. For example, after remote attestation, graphene library can output a sealed(using SGX sealing) secret to filesystem. And then the application that uses, protected-files feature, can unseal the blob, to retrieve the user-provided custom key. For the short-term, if we can add support for provisioning sealed custom key, that will be better than having the key in the manifest file. This would also mean that we add support for SGX seal and unseal in graphene.

Agreed, the current solution is only temporary until a proper remote attestation framework is available.

LibOS/shim/test/fs/manifest.template, line 36 at r1 (raw file):

Previously, sudharkrish (Sudha Krishnakumar) wrote…

It would be nice to support multiple protected-directory paths, probably separated by a delimiter(like colon or semi-colon).

You can have multiple entries in the manifest, like this:

sgx.protected_files.pf_1 = file:tmp/some_file
sgx.protected_files.pf_2 = file:tmp/some_dir
sgx.protected_files.pf_3 = file:tmp/another_dir/some_file

LibOS/shim/test/fs/test_pf.py, line 20 at r1 (raw file):

Previously, sudharkrish (Sudha Krishnakumar) wrote…

Just wondering, if it would be possible to add some tests that shows performance of protected file access, like file read, file write and other protected file operations, when compared to regular non-protected files, when running within graphene. It will be helpful to have this peformance data.

I'm working on that actually (and collecting FS usage patterns from some example real-world payloads for possible optimization).

Pal/src/host/Linux/pal_linux_error.h, line 12 at r1 (raw file):

Previously, sudharkrish (Sudha Krishnakumar) wrote…

In general 0 is returned for success. Do you need this check for 0 in this error-related function.

Without this change a 0 errno was converted to -PAL_ERROR_DENIED which complicated some code paths in the mbedtls crypto adapter.

Pal/src/host/Linux-SGX/db_files.c, line 131 at r1 (raw file):

Previously, sudharkrish (Sudha Krishnakumar) wrote…

Just wondering, if we would need a spinlock to protect increment or decrement of this reference count.

I need to properly review how concurrency is handled in Graphene FS handlers. I'm preparing some multithreaded/multiprocess FS tests for Graphene that should help find related bugs as well.

sudharkrish · 2020-02-20T23:15:29Z

a discussion (no related file):

Previously, omeg (Rafał Wojdyła) wrote…

I think that the application only needs to protect its files if it had some secrets provisioned to it. If there's no secret data, there's no point of using protected files IMO. And if secrets are provisioned (after remote attestation for example) then the PF key can also be provided.

@omeg, I am not sure, if we can assume, that an application needs to protect its files, only if it had some secrets provisioned to it. These 2 aspects can be independent. Maybe something we can discuss in the graphene meeting. During run-time application can generate content that needs protection. And even if secrets are provisioned, they can be something else, not necessarily a key to protect files. I think it will be good to give the option to the end-user, for generating files based on automatically generated platform key. The code to use the sealing key is already there in the updated version we provided, just needs to be enabled.

sudharkrish · 2020-02-20T23:44:40Z

a discussion (no related file):

Previously, omeg (Rafał Wojdyła) wrote…

This check is to determine if a file should be treated like a protected file. The actual path check against PF's metadata is in ipf_init_existing_file().

@omeg , looks like we have an issue here. In ipf_open, you have the check-> if (filename && strnlen(filename, FULLNAME_MAX_LEN) >= FULLNAME_MAX_LEN - 1), but then inside ipf_init_new_file, ipf_init_existing_file, you are using FILENAME_MAX_LEN, which is much less than FULLNAME_MAX_LEN. the current sgx-sdk implementation removes the path-prefix, and only maintains the actual filename in the meta-data, with 260 characters as max length for filename-> char clean_filename[FILENAME_MAX_LEN]; In linux systems, filename(including path-prefix) can be upto 4KB. Given that you are NOT removing the path-prefix in ipf_open, and storing the full-path and filename in clean_filename the length of this array needs to be bumped to be atleast equal to FULLNAME_MAX_LEN or as per lengths in linux systems.

sudharkrish · 2020-02-21T00:12:09Z

Pal/src/host/Linux-SGX/lru_cache.c, line 128 at r1 (raw file):

}

uint32_t lruc_size(lruc_context_t lruc) {

I have updated the list.h macros PR, to get the size in one call using LISTP_SIZE, instead of having to loop through. Can you try to use the changes in this PR>#1268

sudharkrish · 2020-02-21T00:23:31Z

Pal/src/host/Linux-SGX/enclave_pf.c, line 199 at r1 (raw file):

/* Exact match of path in protected_file_list */
struct protected_file* find_protected_file(const char* path) {

Given that this would get called first, for any PF related api, it can introduce latency, can we use a hashtable here to speed up protected file lookup.

sudharkrish · 2020-02-21T00:34:18Z

Pal/src/host/Linux/pal_linux_error.h, line 12 at r1 (raw file):

Previously, omeg (Rafał Wojdyła) wrote…

Without this change a 0 errno was converted to -PAL_ERROR_DENIED which complicated some code paths in the mbedtls crypto adapter.

In general, this unix_to_pal_error, should only get called if errno is non-zero. I noticed in enclave_pf.c::is_directory, you are calling without an extra check. Can you try modifying that. return IS_ERR(ret) ? unix_to_pal_error(ERRNO(ret)) : ret;

sudharkrish · 2020-02-21T00:35:31Z

Pal/src/host/Linux-SGX/enclave_pf.c

+        }
+    }
+
+    return unix_to_pal_error(ERRNO(ret));


Can you change to return IS_ERR(ret) ? unix_to_pal_error(ERRNO(ret)) : ret;

sudharkrish · 2020-02-21T00:39:46Z

LibOS/shim/test/fs/manifest.template, line 36 at r1 (raw file):

Previously, omeg (Rafał Wojdyła) wrote…

You can have multiple entries in the manifest, like this:

sgx.protected_files.pf_1 = file:tmp/some_file
sgx.protected_files.pf_2 = file:tmp/some_dir
sgx.protected_files.pf_3 = file:tmp/another_dir/some_file

That sounds great. Maybe the manifest file of the test application can illustrate that or in the README.

sudharkrish · 2020-02-21T00:40:37Z

LibOS/shim/test/fs/test_pf.py, line 20 at r1 (raw file):

Previously, omeg (Rafał Wojdyła) wrote…

I'm working on that actually (and collecting FS usage patterns from some example real-world payloads for possible optimization).

Okay sounds good.

sudharkrish · 2020-02-21T00:42:23Z

Pal/src/host/Linux-SGX/db_files.c, line 131 at r1 (raw file):

Previously, omeg (Rafał Wojdyła) wrote…

I need to properly review how concurrency is handled in Graphene FS handlers. I'm preparing some multithreaded/multiprocess FS tests for Graphene that should help find related bugs as well.

Okay sure, you can also ask people in the slack group regarding this. Maybe some folks already know the answer.

sudharkrish · 2020-02-21T01:50:47Z

Pal/src/host/Linux-SGX/protected_files.c, line 1937 at r1 (raw file):

                    pf_file_mode_t mode, bool create, bool enable_recovery, const pf_key_t* key,
                    pf_context_t* context) {
    if (!check_callbacks())

How about having a global variable like g_pf_lib_init_done, in init_protected_files api. And check if (g_pf_lib_init_done) instead of using check_callbacks in these pf_* apis.

sudharkrish · 2020-02-21T02:13:42Z

Pal/src/host/Linux-SGX/enclave_pf.c, line 39 at r1 (raw file):

/* Callbacks for protected files handling */
static void* cb_malloc(size_t size) {

Just thinking, if we really need this callback->cb_malloc, why cant you directly use the malloc or calloc api(to avoid memset after malloc) in protected_files.c source code.

sudharkrish · 2020-02-21T02:42:51Z

Pal/src/host/Linux-SGX/protected_files_internal.h, line 196 at r1 (raw file):

    meta_data_encrypted_t encrypted_part_plain; // encrypted part of meta data node, decrypted
    file_node_t root_mht; // the root of the mht is always needed (for files bigger than 3KB)
    pf_handle_t file;

Just wondering, is there a need for this pf_handle_t and whether it needs to be void * pointer. Since you are getting the OS file descriptor after call to open in db_files.c::db_open, we can just store this as ->int fd in struct pf_context. Not sure, if I am missing something else here.

sudharkrish · 2020-02-21T02:51:29Z

Pal/src/host/Linux-SGX/protected_files.c, line 91 at r1 (raw file):

#define PF_DEBUG_PRINT_SIZE_MAX 4096

/* Debug print without function name prefix. Implicit param: pf (context pointer). */

These macros should be NOOPs, unless #ifdef DEBUG is enabled. But I know, #ifdefs are frowned upon.

sudharkrish · 2020-02-21T02:52:25Z

Pal/src/host/Linux-SGX/protected_files_internal.h, line 209 at r1 (raw file):

    char recovery_filename[RECOVERY_FILE_MAX_LEN]; // might include full path to the file
    lruc_context_t cache;
    char* debug_buffer; // buffer for debug output

This debug_buffer needs to be there, only if #ifdef DEBUG is enabled. But I know, #ifdefs are frowned upon.

sudharkrish · 2020-02-21T02:59:09Z

Pal/src/host/Linux-SGX/uthash.h, line 1 at r1 (raw file):

/*

How about moving this file to Pal/include/lib, where we already have other related files such as list.h

sudharkrish · 2020-02-21T03:02:50Z

Pal/src/host/Linux-SGX/tools/pf_util.c, line 551 at r1 (raw file):

};

static int process_files(const char* input_dir, const char* output_dir, const char* prefix,

Nice work, with these utility apis in pf_util.c, will be helpful for people using this feature.

sudharkrish · 2020-02-21T03:09:27Z

a discussion (no related file):
Just curious, if an application tries to rename a protected file, how will it get handled. I am sure, it will be dis-allowed, but it will be good to have a test case for that. Not sure, if you already have one for protected file rename.

mkow

Reviewed 6 of 6 files at r14.
Reviewable status: all files reviewed, 32 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv, @omeg, and @sudharkrish)

Pal/src/host/Linux-SGX/db_files.c, line 159 at r9 (raw file):

Previously, omeg (Rafał Wojdyła) wrote…

Well, changing this (and other instances below) will require touching every PAL fs handler as the PAL callbacks' signature would change. Should I do it here or leave that for a separate PR?

Ok, let's fix this separately after this PR.

Pal/src/host/Linux-SGX/protected-files/lru_cache.h, line 65 at r11 (raw file):

Previously, omeg (Rafał Wojdyła) wrote…

assertion failed currently ;)
I'll probably change that when cleaning up the code.

Ok, please do :)

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 257 at r11 (raw file):

Previously, omeg (Rafał Wojdyła) wrote…

I don't really know :)

Let's drop it and just generate random keys with DkRandomBitsRead.

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 275 at r11 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto (why not random?)

ditto, DkRandomBitsRead should do the work equally well.

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 870 at r11 (raw file):

Previously, omeg (Rafał Wojdyła) wrote…

Converted to array with quicksort. It would be nice to have that in PAL for other uses, but that's probably for another PR.

Quicksort in its vanilla form is pretty nasty - it's O(n^2) worst case, in your implementation e.g. for an already sorted input. This could be fixed by using IntroSort (a total overkill for this case, don't do this :) ) or by just randomizing the pivot. Do we have any fast, non-crypto random to use? I'm afraid we don't... Any ideas?
Worst case we can just leave a comment here that this needs to be fixed (most likely by just migrating from our custom mini C library to musl).

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 1538 at r11 (raw file):

Previously, omeg (Rafał Wojdyła) wrote…

I'll remove them before merging.

ack.

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 156 at r13 (raw file):

Previously, omeg (Rafał Wojdyła) wrote…

thought I removed all strncpys... I hate C sometimes ;)

I hate C all the time :)

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 564 at r14 (raw file):

void swap_nodes(file_node_t** data, size_t idx1, size_t idx2) {
    if (idx1 == idx2)

no need for this if

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 596 at r14 (raw file):

    if (low < high) {
        size_t pi = partition(data, low, high);
        sort_nodes(data, low, pi - 1);

Can't pi equal low here? This sounds like a still-correctly-handled case, but when low == 0 this will underflow.

omeg

Reviewable status: 38 of 44 files reviewed, 32 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, @omeg, and @sudharkrish)

Pal/src/host/Linux-SGX/db_files.c, line 92 at r9 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Hmm, this seems to be checking something slightly different, depending on whether the writable handle was opened first - if so, one writable + a few readonly handles are allowed, but if we started with a readonly handles, then we can't open even a single writable one.

Done.

Pal/src/host/Linux-SGX/db_files.c, line 216 at r9 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto (size_t)

Leaving this for a separate PR.

Pal/src/host/Linux-SGX/db_files.c, line 308 at r9 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

so, it also has the same problem... Would fixing it here force you to also fix it in file_map and possibly other places? If not, then I'd fix it.

Leaving this for a separate PR.

Pal/src/host/Linux-SGX/enclave_pf.c, line 241 at r9 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

This is linear on the number of all directories registered (not only those explicitly in the manifest, but also their subdirs, from what I see, they also land on this list). This won't scale well.

Not sure how to make it faster, added a TODO for now.

Pal/src/host/Linux-SGX/protected-files/lru_cache.h, line 65 at r11 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Ok, please do :)

Done.

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 257 at r11 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Let's drop it and just generate random keys with DkRandomBitsRead.

Done. This eliminates the need for session_master_key.

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 275 at r11 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto, DkRandomBitsRead should do the work equally well.

Done.

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 870 at r11 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Quicksort in its vanilla form is pretty nasty - it's O(n^2) worst case, in your implementation e.g. for an already sorted input. This could be fixed by using IntroSort (a total overkill for this case, don't do this :) ) or by just randomizing the pivot. Do we have any fast, non-crypto random to use? I'm afraid we don't... Any ideas?
Worst case we can just leave a comment here that this needs to be fixed (most likely by just migrating from our custom mini C library to musl).

Changed to a median pivot (which at least isn't slow for sorted data) and added a TODO.

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 564 at r14 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

no need for this if

Done.

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 596 at r14 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Can't pi equal low here? This sounds like a still-correctly-handled case, but when low == 0 this will underflow.

Done.

mkow

Reviewed 6 of 6 files at r15.
Reviewable status: all files reviewed, 24 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @omeg, and @sudharkrish)

Pal/src/host/Linux-SGX/pal_linux.h, line 185 at r15 (raw file):

    pf_context_t* context; /* NULL until PF is opened */
    int64_t refcount; /* used for deciding when to call unload_protected_file() */
    int writable_fd; /* multiple writable handles to one PF are disallowed */

this comment doesn't really say what's held in this field - which is -1 in case of 0 writable handles and the writable fd otherwise.

Pal/src/host/Linux-SGX/protected-files/lru_cache.h, line 65 at r11 (raw file):

Previously, omeg (Rafał Wojdyła) wrote…

Done.

Please describe the semantics in a comment.

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 257 at r11 (raw file):

Previously, omeg (Rafał Wojdyła) wrote…

Done. This eliminates the need for session_master_key.

Yup, I think it was useless anyway ;)

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 870 at r11 (raw file):

Previously, omeg (Rafał Wojdyła) wrote…

Changed to a median pivot (which at least isn't slow for sorted data) and added a TODO.

Hmm, I think this isn't actually the median? It's just a more optimal (on average) version, but still O(n^2) pessimistically. Finding the median in O(n) is possible, but much more complicated (Median of Medians algorithm).

mkow

Reviewable status: all files reviewed, 24 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @omeg, and @sudharkrish)

Pal/src/host/Linux-SGX/protected-files/lru_cache.h, line 46 at r11 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

@intel folks (@dimakuv, @monavij)

Could we fold this copyright? In other files we just have a few copyright lines + LGPL license info. Actually, the LRU implementation here was completely rewritten if I see correctly, so why not just drop the copyright?

This file has more lawyer boilerplate than the code :)

btw: we migrated to SPDX annotations.
So, @dimakuv @monavij: Any updates on this huge banner removal? I believe it's already contained in the license we link to.

omeg

Reviewable status: all files reviewed, 24 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, @omeg, and @sudharkrish)

Pal/src/host/Linux-SGX/pal_linux.h, line 185 at r15 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

this comment doesn't really say what's held in this field - which is -1 in case of 0 writable handles and the writable fd otherwise.

made this more descriptive

Pal/src/host/Linux-SGX/protected-files/lru_cache.h, line 65 at r11 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Please describe the semantics in a comment.

Done.

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 870 at r11 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Hmm, I think this isn't actually the median? It's just a more optimal (on average) version, but still O(n^2) pessimistically. Finding the median in O(n) is possible, but much more complicated (Median of Medians algorithm).

Well, I don't have time to look at this more right now so let's fix it when we use a sane libc :)

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 901 at r11 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Why is cur_key saved in the context? Shouldn't it be used only here and discarded right away? Why ipf_derive_random_node_key() isn't just generate_random_key() which returns the generated key?

Removed the need for cur_key altogether.

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 1399 at r11 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

What's the reason for doing this?

Not sure, probably performance optimization?

mkow

Reviewed 4 of 4 files at r16.
Reviewable status: all files reviewed, 20 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @omeg, and @sudharkrish)

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 870 at r11 (raw file):

Previously, omeg (Rafał Wojdyła) wrote…

Well, I don't have time to look at this more right now so let's fix it when we use a sane libc :)

That's why I've unblocked this discussion already ;)

ravikumarbhattiprolu · 2020-07-06T17:15:59Z

With the latest mainline code this pull request is causing merge conflicts. Can this request be rehashed over current mainline so that these conflicts are removed? This will help till code reviews are complete and PR is merged with mainline.

dimakuv

Reviewed 12 of 25 files at r8, 2 of 3 files at r9, 1 of 2 files at r10, 1 of 2 files at r11, 12 of 20 files at r13, 3 of 6 files at r14, 3 of 6 files at r15, 4 of 4 files at r16.
Reviewable status: all files reviewed, 19 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @mkow, @monavij, @omeg, and @sudharkrish)

Pal/src/host/Linux-SGX/protected-files/lru_cache.h, line 46 at r11 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

btw: we migrated to SPDX annotations.
So, @dimakuv @monavij: Any updates on this huge banner removal? I believe it's already contained in the license we link to.

Yes, we can remove it and replace with SPDX.

Pal/src/host/Linux-SGX/protected-files/lru_cache.c, line 46 at r11 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto (copyright)

ditto, can be just replaced with a single SPDX

Pal/src/host/Linux-SGX/protected-files/protected_files.h, line 46 at r11 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto (licence, CC: @dimakuv @monavij)

ditto, replace with a single SPDX

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 48 at r11 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto (license)

ditto, replace with a single SPDX

Pal/src/host/Linux-SGX/protected-files/protected_files_internal.h, line 46 at r11 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ditto (license)

ditto, replace with a single SPDX

omeg

Reviewable status: all files reviewed, 19 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @mkow, @monavij, @omeg, and @sudharkrish)

Pal/src/host/Linux-SGX/db_files.c, line 72 at r9 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Already fixed on master (by #1446). After rebase, you should check here for either PAL_CREATE_TRY or PAL_CREATE_ALWAYS.

Done.

Pal/src/host/Linux-SGX/db_files.c, line 75 at r9 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

This will need an update after rebase (see #1446)

Done.

Pal/src/host/Linux-SGX/protected-files/lru_cache.h, line 46 at r11 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Yes, we can remove it and replace with SPDX.

Done.

Pal/src/host/Linux-SGX/protected-files/lru_cache.c, line 46 at r11 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

ditto, can be just replaced with a single SPDX

Done.

Pal/src/host/Linux-SGX/protected-files/protected_files.h, line 46 at r11 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

ditto, replace with a single SPDX

Done.

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 48 at r11 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

ditto, replace with a single SPDX

Done.

Pal/src/host/Linux-SGX/protected-files/protected_files_internal.h, line 46 at r11 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

ditto, replace with a single SPDX

Done.

dimakuv

Reviewed 35 of 35 files at r17.
Reviewable status: all files reviewed, 20 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @mkow, @monavij, @omeg, and @sudharkrish)

Jenkinsfiles/Linux-SGX, line 45 at r17 (raw file):

                        sh '''
                            cd Pal/src/host/Linux-SGX/tools
                            make install PREFIX=../../../../../../LibOS/shim/test/fs

This line produces the error on Jenkins-SGX pipelines:

make[1]: Entering directory '/home/jenkins/jenkins_slave/workspace/graphene-sgx/Pal/src/host/Linux-SGX/tools/ra-tls'
make[1]: *** No rule to make target 'install'.  Stop.

This is because I added RA-TLS tool to this directory, and its Makefile doesn't have an install target. Could you add a dummy install target to RA-TLS to alleviate this error?

omeg · 2020-07-08T08:19:12Z

Jenkins, retest Jenkins-Debug please (apps.LTP.kill01 failed)

mkow

Reviewed 35 of 35 files at r17, 1 of 1 files at r18.
Reviewable status: all files reviewed, 13 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @omeg and @sudharkrish)

dimakuv

Reviewable status: all files reviewed, 13 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @omeg and @sudharkrish)

a discussion (no related file):
@omeg Jenkins-SGX pipelines fail on LibOS regression tests:

test_libos.TC_30_Syscall.test_092_sighandler_sigpipe
test_libos.TC_80_Socket.test_022_poll_closed_fd
test_libos.TC_80_Socket.test_095_mkfifo

A bit surprising. Something in your Linux-SGX PAL changes introduced a regression.

mkow

Reviewable status: all files reviewed, 14 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @omeg and @sudharkrish)

Pal/src/host/Linux-SGX/db_files.c, line 59 at r18 (raw file):

(create & PAL_CREATE_ALWAYS) || (options & PAL_CREATE_TRY)

I see a problem here :) But these 3 tests are still failing even after fixing this.

mkow

Reviewed 3 of 3 files at r19.
Reviewable status: all files reviewed, 13 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @omeg and @sudharkrish)

Pal/src/host/Linux-SGX/db_files.c, line 59 at r18 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

(create & PAL_CREATE_ALWAYS) || (options & PAL_CREATE_TRY)
I see a problem here :) But these 3 tests are still failing even after fixing this.

Fixed.

dimakuv

Dismissed @sudharkrish from 11 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @omeg)

Protected files (PF) are a new type of file that can be specified in the manifest (SGX only). They are encrypted on disk and transparently decrypted when accessed by the Graphene payload. Other features: - data is integrity protected (tamper resistance) - file swap protection (a PF can only be accessed when in a specific path) - transparency (Graphene payload sees PFs as regular files, no need to modify the payload) See Linux-SGX/protected-files directory for implementation. PF format is based on protected files from the SGX SDK: https://github.com/intel/linux-sgx/tree/master/sdk/protected_fs The following new manifest elements are added: sgx.protected_files_key = <16-byte hex value> sgx.protected_files.<name> = file:<host path> sgx.protected_files_key specifies the encryption key and is only a temporary implementation. This key should be provisioned with local/remote attestation in the future. Paths specifying PF entries can be files or directories. If a directory is specified, all files/directories within are registered as protected recursively (and are expected to be encrypted in the PF format). Linux-SGX/tools directory contains the pf_crypt utility that converts files to/from the protected format.

omeg

Reviewable status: 44 of 48 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv and @mkow)

Pal/src/host/Linux-SGX/protected-files/protected_files.c, line 1538 at r11 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

ack.

Done.

mkow

Reviewed 4 of 4 files at r20.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from different teams (1 more required, approved so far: ITL)

dimakuv

Reviewable status: complete! all files reviewed, all discussions resolved

omeg commented Feb 17, 2020

View reviewed changes

sudharkrish reviewed Feb 19, 2020

View reviewed changes

sudharkrish reviewed Feb 20, 2020

View reviewed changes

omeg commented Feb 20, 2020

View reviewed changes

sudharkrish suggested changes Feb 21, 2020

View reviewed changes

mkow reviewed Jun 30, 2020

View reviewed changes

omeg commented Jun 30, 2020

View reviewed changes

mkow reviewed Jul 1, 2020

View reviewed changes

omeg commented Jul 3, 2020

View reviewed changes

mkow reviewed Jul 3, 2020

View reviewed changes

dimakuv reviewed Jul 6, 2020

View reviewed changes

omeg force-pushed the omeg/protected-files-new branch from 967968c to 864ceb1 Compare July 7, 2020 18:54

omeg commented Jul 7, 2020

View reviewed changes

dimakuv mentioned this pull request Jul 7, 2020

strstr returns const char *; POSIX says it returns char * #1610

Closed

dimakuv reviewed Jul 7, 2020

View reviewed changes

omeg marked this pull request as ready for review July 8, 2020 07:53

mkow reviewed Jul 8, 2020

View reviewed changes

dimakuv suggested changes Jul 8, 2020

View reviewed changes

mkow suggested changes Jul 11, 2020

View reviewed changes

mkow reviewed Jul 12, 2020

View reviewed changes

dimakuv previously approved these changes Jul 13, 2020

View reviewed changes

omeg dismissed dimakuv’s stale review via cf84489 July 13, 2020 18:20

omeg force-pushed the omeg/protected-files-new branch from 2596705 to cf84489 Compare July 13, 2020 18:20

omeg commented Jul 13, 2020

View reviewed changes

mkow approved these changes Jul 13, 2020

View reviewed changes

dimakuv approved these changes Jul 13, 2020

View reviewed changes

omeg merged commit cf84489 into master Jul 13, 2020

omeg deleted the omeg/protected-files-new branch July 13, 2020 19:36

dimakuv mentioned this pull request Jul 19, 2020

[Linux-SGX] Add protected files implementation #1090

Closed

6 tasks

0400H mentioned this pull request Mar 16, 2021

Confidentiality and integrity of protected file system. #2242

Closed

mkow mentioned this pull request Sep 9, 2021

Known issues for production deployment gramineproject/gramine#7

Open

14 tasks

[Linux-SGX] Add protected files implementation (SGX SDK file format) #1325

[Linux-SGX] Add protected files implementation (SGX SDK file format) #1325

Conversation

omeg commented Feb 12, 2020 • edited by dimakuv Loading

Affected components

Description of the changes

How to test this PR?

sudharkrish commented Feb 14, 2020

sudharkrish commented Feb 14, 2020

sudharkrish commented Feb 14, 2020

sudharkrish commented Feb 14, 2020

omeg left a comment

Choose a reason for hiding this comment

sudharkrish commented Feb 19, 2020

sudharkrish commented Feb 19, 2020

sudharkrish left a comment

Choose a reason for hiding this comment

sudharkrish commented Feb 20, 2020

sudharkrish commented Feb 20, 2020

sudharkrish Feb 20, 2020

Choose a reason for hiding this comment

sudharkrish commented Feb 20, 2020

omeg left a comment

Choose a reason for hiding this comment

sudharkrish commented Feb 20, 2020

sudharkrish commented Feb 20, 2020

sudharkrish commented Feb 21, 2020

sudharkrish commented Feb 21, 2020

sudharkrish commented Feb 21, 2020

sudharkrish Feb 21, 2020

Choose a reason for hiding this comment

sudharkrish commented Feb 21, 2020

sudharkrish commented Feb 21, 2020

sudharkrish commented Feb 21, 2020

sudharkrish commented Feb 21, 2020

sudharkrish commented Feb 21, 2020

sudharkrish commented Feb 21, 2020

sudharkrish commented Feb 21, 2020

sudharkrish commented Feb 21, 2020

sudharkrish commented Feb 21, 2020

sudharkrish commented Feb 21, 2020

sudharkrish commented Feb 21, 2020

mkow left a comment

Choose a reason for hiding this comment

omeg left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

omeg left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

ravikumarbhattiprolu commented Jul 6, 2020

dimakuv left a comment

Choose a reason for hiding this comment

omeg left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

omeg commented Jul 8, 2020

mkow left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

omeg left a comment

Choose a reason for hiding this comment

mkow left a comment

Choose a reason for hiding this comment

dimakuv left a comment

Choose a reason for hiding this comment

omeg commented Feb 12, 2020 •

edited by dimakuv

Loading