[Pal/Linux-SGX] Refactor file_open and load_trusted_file functions

This commit moves all possible checks (file must be in a set of
protected, trusted, allowed files from the manifest; trusted files
cannot be created; etc.) before actual opening of the file in
`file_open`. As a side effect, this fixes a bug when an app wants to
open an unknown file for write/append -- previously, Graphene would
open such file, possibly truncate it and only then return an error.
Also, LibOS regression tests were enhanced to check for this case.

Also, this commit splits `load_trusted_file` into
`get_trusted_or_allowed_file` and `load_trusted_or_allowed_file` and
also refactors a few other other auxilary functions.

Co-authored-by: Michał Kowalczyk <[email protected]>
Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
Signed-off-by: Michał Kowalczyk <[email protected]>

.pylintrc

@@ -54,21 +54,22 @@ confidence=
 
 enable=*
 disable=
-    bad-option-value,  # for compatibility between different pylint versions
     bad-continuation,
+    bad-option-value,  # for compatibility between different pylint versions
     c-extension-no-member,
+    fixme,
+    invalid-name,
     missing-docstring,
     missing-function-docstring,
     missing-module-docstring,
-    invalid-name,
-    fixme,
+    no-self-use,
     raise-missing-from,
-    too-many-instance-attributes,
-    too-many-branches,
-    too-many-statements,
     too-few-public-methods,
+    too-many-branches,
+    too-many-instance-attributes,
+    too-many-lines,
     too-many-public-methods,
-    no-self-use
+    too-many-statements
 
 
 [REPORTS]

LibOS/shim/test/regression/.gitignore

@@ -1,6 +1,9 @@
 /*.manifest
 /*.xml
 
+/nonexisting_testfile
+/testfile
+
 /.cache
 /abort
 /abort_multithread
@@ -102,7 +105,6 @@
 /sysfs_common
 /tcp_ipv6_v6only
 /tcp_msg_peek
-/testfile
 /tmp
 /udp
 /unix

LibOS/shim/test/regression/file_check_policy.c

@@ -1,23 +1,34 @@
+#include <err.h>
 #include <errno.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 
 int main(int argc, char** argv) {
     if (argc != 3) {
-        fprintf(stderr, "Usage: %s read|append <filename>\n", argv[0]);
+        fprintf(stderr, "Usage: %s read|write|append <filename>\n", argv[0]);
         return 1;
     }
 
     /* we use append instead of write simply to not overwrite the file */
-    FILE* fp = fopen(argv[2], argv[1][0] == 'r' ? "r" : "a");
+    const char* mode;
+    if (!strcmp(argv[1], "read")) {
+        mode = "r";
+    } else if (!strcmp(argv[1], "write")) {
+        mode = "r+"; // without file creation
+    } else if (!strcmp(argv[1], "append")) {
+        mode = "a";
+    } else {
+        errx(1, "invalid cmdline argument");
+    }
+    FILE* fp = fopen(argv[2], mode);
     if (!fp) {
         perror("fopen failed");
         return 2;
     }
 
-    int reti = fclose(fp);
-    if (reti) {
+    int ret = fclose(fp);
+    if (ret) {
         perror("fclose failed");
         return 3;
     }

LibOS/shim/test/regression/test_libos.py

@@ -272,32 +272,78 @@ def test_000_strict_success(self):
         self.assertIn('file_check_policy succeeded', stdout)
 
     def test_001_strict_fail(self):
-        with self.expect_returncode(2):
+        try:
             self.run_binary(['file_check_policy_strict', 'read', 'unknown_testfile'])
+            self.fail('expected to return nonzero')
+        except subprocess.CalledProcessError as e:
+            self.assertEqual(e.returncode, 2)
+            stderr = e.stderr.decode()
+            self.assertIn('Disallowing access to file \'unknown_testfile\'', stderr)
 
     def test_002_strict_fail_create(self):
-        with self.expect_returncode(2):
+        if os.path.exists('nonexisting_testfile'):
+            os.remove('nonexisting_testfile')
+        try:
             # this tests a previous bug in Graphene that allowed creating unknown files
-            self.run_binary(['file_check_policy_strict', 'append', 'unknown_testfile'])
+            self.run_binary(['file_check_policy_strict', 'append', 'nonexisting_testfile'])
+            self.fail('expected to return nonzero')
+        except subprocess.CalledProcessError as e:
+            self.assertEqual(e.returncode, 2)
+            stderr = e.stderr.decode()
+            self.assertIn('Disallowing access to file \'nonexisting_testfile\'', stderr)
+            if os.path.exists('nonexisting_testfile'):
+                self.fail('test created a file unexpectedly')
+
+    def test_003_strict_fail_write(self):
+        try:
+            # writing to trusted files should not be possible
+            self.run_binary(['file_check_policy_strict', 'write', 'trusted_testfile'])
+            self.fail('expected to return nonzero')
+        except subprocess.CalledProcessError as e:
+            self.assertEqual(e.returncode, 2)
+            stderr = e.stderr.decode()
+            self.assertIn('Disallowing create/write/append to a trusted file \'trusted_testfile\'',
+                          stderr)
 
-    def test_003_allow_all_but_log_unknown(self):
+    def test_004_allow_all_but_log_unknown(self):
         stdout, stderr = self.run_binary(['file_check_policy_allow_all_but_log', 'read',
                                           'unknown_testfile'])
-        self.assertIn('Allowing access to an unknown file due to file_check_policy settings: '
-                      'file:unknown_testfile', stderr)
+        self.assertIn('Allowing access to unknown file \'unknown_testfile\' due to '
+                      'file_check_policy settings.', stderr)
         self.assertIn('file_check_policy succeeded', stdout)
 
-    def test_004_allow_all_but_log_trusted(self):
+    def test_005_allow_all_but_log_trusted(self):
         stdout, stderr = self.run_binary(['file_check_policy_allow_all_but_log', 'read',
                                           'trusted_testfile'])
-        self.assertNotIn('Allowing access to an unknown file due to file_check_policy settings: '
-                         'file:trusted_testfile', stderr)
+        self.assertNotIn('Allowing access to unknown file \'trusted_testfile\' due to '
+                         'file_check_policy settings.', stderr)
         self.assertIn('file_check_policy succeeded', stdout)
 
-    def test_005_allow_all_but_log_trusted_create_fail(self):
-        with self.expect_returncode(2):
+    def test_006_allow_all_but_log_trusted_create_fail(self):
+        try:
             # this fails because modifying trusted files is prohibited
             self.run_binary(['file_check_policy_allow_all_but_log', 'append', 'trusted_testfile'])
+            self.fail('expected to return nonzero')
+        except subprocess.CalledProcessError as e:
+            self.assertEqual(e.returncode, 2)
+            stderr = e.stderr.decode()
+            self.assertIn('Disallowing create/write/append to a trusted file \'trusted_testfile\'',
+                          stderr)
+
+    def test_007_allow_all_but_log_unknown_create(self):
+        if os.path.exists('nonexisting_testfile'):
+            os.remove('nonexisting_testfile')
+        try:
+            stdout, stderr = self.run_binary(['file_check_policy_allow_all_but_log', 'append',
+                                              'nonexisting_testfile'])
+            self.assertIn('Allowing access to unknown file \'nonexisting_testfile\' due to '
+                          'file_check_policy settings.', stderr)
+            self.assertIn('file_check_policy succeeded', stdout)
+            if not os.path.exists('nonexisting_testfile'):
+                self.fail('test did not create a file')
+        finally:
+            os.remove('nonexisting_testfile')
+
 
 @unittest.skipUnless(HAS_SGX,
     'These tests are only meaningful on SGX PAL because only SGX supports attestation.')

Pal/regression/File.manifest

@@ -6,7 +6,7 @@ fs.mount.root.uri = "file:"
 
 sgx.nonpie_binary = true
 
-sgx.trusted_files.tmp1 = "file:File"
-sgx.trusted_files.tmp2 = "file:../regression/File"
+sgx.allowed_files.tmp1 = "file:File"
+sgx.allowed_files.tmp2 = "file:../regression/File"
 sgx.allowed_files.tmp3 = "file:file_nonexist.tmp"
 sgx.allowed_files.tmp4 = "file:file_delete.tmp"

Pal/regression/normalize_path.c

@@ -54,8 +54,8 @@ static int run_test(void) {
             return 1;
         }
 
-        if (strlen(buf) != size) {
-            print_err(func_name, i, "returned wrong size: %zu\n", size);
+        if (size != strlen(buf) + 1) {
+            print_err(func_name, i, "returned wrong size: %lu\n", size);
             return 1;
         }