[LibOS] Allow unlink() on corrupted encrypted files

Currently, LibOS unconditionally sets up the inode of the dentry on
first lookup, for example, querying the size of the underlying file and
saving it into inode fields. This setup of dentry's inode is not needed
in cases like file removal (`unlink()`), since the inode will be
immediately removed after it was set up.

This peculiarity made it impossible to remove (unlink) corrupted
encrypted files by the app itself: already the lookup of the file would
fail, disallowing to proceed to the removal operation. "Corrupted" may
mean an attacker-modified file or a more benign case of
encrypted-with-old-key file.

This commit allows to perform `unlink()` on such "corrupted" encrypted
files (by keeping a "shallow" file object). A LibOS test was added.

Signed-off-by: Dmitrii Kuvaiskii <[email protected]>

libos/src/fs/chroot/encrypted.c

@@ -12,9 +12,15 @@
  *   key for files. Multiple mounts can use the same key. The list of keys is managed in
  *   `libos_fs_encrypted.c`.
  *
- * - Inodes (`libos_inode`, for regular files) hold a `libos_encrypted_file` object. This object
- *   lives as long as the inode, but is kept *open* only as long as there are `libos_handle` objects
- *   corresponding to it. We use `encrypted_file_{get,put}` operations to maintain that invariant.
+ * - Inodes (`libos_inode`, for regular files) hold a `libos_encrypted_file` object in `inode->data`
+ *   field. This object lives as long as the inode, but is kept *open* only as long as there are
+ *   `libos_handle` objects corresponding to it. We use `encrypted_file_{get,put}` operations to
+ *   maintain that invariant.
+ *
+ *   It is possible that inode has no corresponding `libos_encrypted_file` object, i.e.
+ *   `inode->data` is NULL. This happens if the encrypted file is corrupted; in this case we want to
+ *   allow at least some operations on the file (currently only `unlink()`), but all other
+ *   operations return -EACCES.
  *
  *   An open `libos_encrypted_file` object keeps an open PAL handle and associated data
  *   (`pf_context_t`), so that operations (read, write, truncate...) can be performed on the file.
@@ -152,18 +158,25 @@ static int chroot_encrypted_lookup(struct libos_dentry* dent) {
         struct libos_encrypted_files_key* key = dent->mount->data;
         ret = encrypted_file_open(uri, key, &enc);
         if (ret < 0) {
-            goto out;
-        }
-
-        ret = encrypted_file_get_size(enc, &size);
-        encrypted_file_put(enc);
-
-        if (ret < 0) {
-            encrypted_file_destroy(enc);
-            goto out;
+            if (ret == -EACCES) {
+                /* allow the inode to be created even if the underlying encrypted file is corrupted;
+                 * this is useful for unlinking a corrupted file */
+                inode->data = NULL;
+            } else {
+                goto out;
+            }
+        } else {
+            ret = encrypted_file_get_size(enc, &size);
+            encrypted_file_put(enc);
+
+            if (ret < 0) {
+                encrypted_file_destroy(enc);
+                goto out;
+            }
+
+            inode->data = enc;
+            inode->size = size;
         }
-        inode->data = enc;
-        inode->size = size;
     }
     dent->inode = inode;
     get_inode(inode);
@@ -185,6 +198,8 @@ static int chroot_encrypted_open(struct libos_handle* hdl, struct libos_dentry*
 
     if (dent->inode->type == S_IFREG) {
         struct libos_encrypted_file* enc = dent->inode->data;
+        if (!enc)
+            return -EACCES;
 
         lock(&dent->inode->lock);
         ret = encrypted_file_get(enc);
@@ -325,6 +340,10 @@ static int chroot_encrypted_rename(struct libos_dentry* old, struct libos_dentry
     lock(&old->inode->lock);
 
     struct libos_encrypted_file* enc = old->inode->data;
+    if (!enc) {
+        ret = -EACCES;
+        goto out;
+    }
 
     ret = encrypted_file_get(enc);
     if (ret < 0)
@@ -342,6 +361,9 @@ static int chroot_encrypted_chmod(struct libos_dentry* dent, mode_t perm) {
     assert(locked(&g_dcache_lock));
     assert(dent->inode);
 
+    if (!dent->inode->data)
+        return -EACCES;
+
     char* uri = NULL;
 
     int ret = chroot_dentry_uri(dent, dent->inode->type, &uri);
@@ -374,6 +396,8 @@ static int chroot_encrypted_fchmod(struct libos_handle* hdl, mode_t perm) {
     assert(hdl->inode);
 
     struct libos_encrypted_file* enc = hdl->inode->data;
+    assert(enc);
+
     mode_t host_perm = HOST_PERM(perm);
     PAL_STREAM_ATTR attr = {.share_flags = host_perm};
     int ret = PalStreamAttributesSetByHandle(enc->pal_handle, &attr);
@@ -389,6 +413,7 @@ static int chroot_encrypted_flush(struct libos_handle* hdl) {
         return 0;
 
     struct libos_encrypted_file* enc = hdl->inode->data;
+    assert(enc);
 
     /* If there are any MAP_SHARED mappings for the file, this will write data to `enc` */
     int ret = msync_handle(hdl);
@@ -408,6 +433,7 @@ static int chroot_encrypted_close(struct libos_handle* hdl) {
         return 0;
 
     struct libos_encrypted_file* enc = hdl->inode->data;
+    assert(enc);
 
     lock(&hdl->inode->lock);
     encrypted_file_put(enc);
@@ -425,6 +451,8 @@ static ssize_t chroot_encrypted_read(struct libos_handle* hdl, void* buf, size_t
     }
 
     struct libos_encrypted_file* enc = hdl->inode->data;
+    assert(enc);
+
     size_t actual_count;
 
     lock(&hdl->inode->lock);
@@ -447,6 +475,8 @@ static ssize_t chroot_encrypted_write(struct libos_handle* hdl, const void* buf,
     }
 
     struct libos_encrypted_file* enc = hdl->inode->data;
+    assert(enc);
+
     size_t actual_count;
 
     lock(&hdl->inode->lock);
@@ -485,6 +515,7 @@ static int chroot_encrypted_truncate(struct libos_handle* hdl, file_off_t size)
 
     int ret;
     struct libos_encrypted_file* enc = hdl->inode->data;
+    assert(enc);
 
     lock(&hdl->inode->lock);
     ret = encrypted_file_set_size(enc, size);
@@ -495,6 +526,16 @@ static int chroot_encrypted_truncate(struct libos_handle* hdl, file_off_t size)
     return ret;
 }
 
+static int chroot_encrypted_stat(struct libos_dentry* dent, struct stat* buf) {
+    assert(locked(&g_dcache_lock));
+    assert(dent->inode);
+
+    if (!dent->inode->data)
+        return -EACCES;
+
+    return generic_inode_stat(dent, buf);
+}
+
 struct libos_fs_ops chroot_encrypted_fs_ops = {
     .mount      = &chroot_encrypted_mount,
     .flush      = &chroot_encrypted_flush,
@@ -517,7 +558,7 @@ struct libos_d_ops chroot_encrypted_d_ops = {
     .lookup        = &chroot_encrypted_lookup,
     .creat         = &chroot_encrypted_creat,
     .mkdir         = &chroot_encrypted_mkdir,
-    .stat          = &generic_inode_stat,
+    .stat          = &chroot_encrypted_stat,
     .readdir       = &chroot_readdir, /* same as in `chroot` filesystem */
     .unlink        = &chroot_encrypted_unlink,
     .rename        = &chroot_encrypted_rename,

libos/test/regression/.gitignore

@@ -3,6 +3,6 @@
 /trusted_testfile
 
 /tmp/*
-!/tmp/.dummy
+!/tmp/.gitkeep
 /tmp_enc/*
-!/tmp_enc/.dummy
+!/tmp_enc/.gitkeep

libos/test/regression/Makefile

@@ -17,5 +17,5 @@ clean:
 		testfile \
 		trusted_testfile \
 		asm/x86_64/__pycache__ \
-		tmp/* \
-		tmp_enc/*
+		tmp/*
+	find tmp_enc/ -not -path '*/.*' -type f -delete

libos/test/regression/manifest.template

@@ -20,8 +20,8 @@ fs.mounts = [
 
   { type = "tmpfs", path = "/mnt/tmpfs" },
   { type = "encrypted", path = "/tmp_enc", uri = "file:tmp_enc", key_name = "my_custom_key" },
-  { type = "encrypted", path = "/encrypted_file_mrenclave.dat", uri = "file:encrypted_file_mrenclave.dat", key_name = "_sgx_mrenclave" },
-  { type = "encrypted", path = "/encrypted_file_mrsigner.dat", uri = "file:encrypted_file_mrsigner.dat", key_name = "_sgx_mrsigner" },
+  { type = "encrypted", path = "/tmp_enc/mrenclaves", uri = "file:tmp_enc/mrenclaves", key_name = "_sgx_mrenclave" },
+  { type = "encrypted", path = "/tmp_enc/mrsigners", uri = "file:tmp_enc/mrsigners", key_name = "_sgx_mrsigner" },
 ]
 
 sgx.max_threads = {{ '1' if env.get('EDMM', '0') == '1' else '16' }}

libos/test/regression/sealed_file.c

@@ -19,8 +19,10 @@ int main(int argc, char** argv) {
     int ret;
     ssize_t bytes;
 
-    if (argc != 2)
-        errx(EXIT_FAILURE, "Usage: %s <protected file to create/validate>", argv[0]);
+    if (argc != 3 || (strcmp(argv[2], "unlink") && strcmp(argv[2], "nounlink"))) {
+        errx(EXIT_FAILURE, "Usage: %s <protected file to create/validate> unlink|nounlink",
+             argv[0]);
+    }
 
     ret = access(argv[1], F_OK);
     if (ret < 0) {
@@ -35,10 +37,26 @@ int main(int argc, char** argv) {
             if (bytes != SECRETSTRING_LEN)
                 errx(EXIT_FAILURE, "Wrote %ld instead of expected %ld", bytes, SECRETSTRING_LEN);
 
-            printf("CREATION OK\n");
+            puts("CREATION OK");
             return 0;
         }
-        err(EXIT_FAILURE, "access failed");
+        if (errno != EACCES || strcmp(argv[2], "unlink") != 0) {
+            /* access() can legitimately return EACCES if we're testing the "modified-MRENCLAVE
+             * app wants to delete the previous-MRENCLAVE-sealed file" corner case */
+            err(EXIT_FAILURE, "access failed");
+        }
+    }
+
+    /* at this point, the file exists (either created by above or already existed on storage) */
+
+    if (strcmp(argv[2], "unlink") == 0) {
+        /* verify that removing the file always works, even with a mismatching MRENCLAVE */
+        ret = unlink(argv[1]);
+        if (ret < 0)
+            err(EXIT_FAILURE, "unlink failed");
+
+        puts("UNLINK OK");
+        return 0;
     }
 
     char buf[SECRETSTRING_LEN];
@@ -58,9 +76,9 @@ int main(int argc, char** argv) {
     /* The build system adds MODIFE_MRENCLAVE macro to produce a slightly different executable (due
      * to the below different string), which in turn produces a different MRENCLAVE SGX measurement.
      * This trick is to test `_sgx_mrsigner` functionality. */
-    printf("READING FROM MODIFIED ENCLAVE OK\n");
+    puts("READING FROM MODIFIED ENCLAVE OK");
 #else
-    printf("READING OK\n");
+    puts("READING OK");
 #endif
     return 0;
 }

libos/test/regression/test_libos.py

@@ -807,10 +807,10 @@ def test_052_mmap_file_backed_trusted(self):
     @unittest.skipUnless(HAS_SGX, 'Sealed (protected) files are only available with SGX')
     def test_053_mmap_file_backed_protected(self):
         # create the protected file
-        pf_path = 'encrypted_file_mrsigner.dat'
+        pf_path = 'tmp_enc/mrsigners/mmap_file_backed.dat'
         if os.path.exists(pf_path):
             os.remove(pf_path)
-        stdout, _ = self.run_binary(['sealed_file', pf_path])
+        stdout, _ = self.run_binary(['sealed_file', pf_path, 'nounlink'])
         self.assertIn('CREATION OK', stdout)
 
         try:
@@ -1214,46 +1214,59 @@ def test_040_sysfs(self):
 
     @unittest.skipUnless(HAS_SGX, 'Sealed (protected) files are only available with SGX')
     def test_050_sealed_file_mrenclave(self):
-        pf_path = 'encrypted_file_mrenclave.dat'
+        pf_path = 'tmp_enc/mrenclaves/test_050.dat'
         if os.path.exists(pf_path):
             os.remove(pf_path)
 
-        stdout, _ = self.run_binary(['sealed_file', pf_path])
+        stdout, _ = self.run_binary(['sealed_file', pf_path, 'nounlink'])
         self.assertIn('CREATION OK', stdout)
-        stdout, _ = self.run_binary(['sealed_file', pf_path])
+        stdout, _ = self.run_binary(['sealed_file', pf_path, 'nounlink'])
         self.assertIn('READING OK', stdout)
 
     @unittest.skipUnless(HAS_SGX, 'Sealed (protected) files are only available with SGX')
     def test_051_sealed_file_mrsigner(self):
-        pf_path = 'encrypted_file_mrsigner.dat'
+        pf_path = 'tmp_enc/mrsigners/test_051.dat'
         if os.path.exists(pf_path):
             os.remove(pf_path)
 
-        stdout, _ = self.run_binary(['sealed_file', pf_path])
+        stdout, _ = self.run_binary(['sealed_file', pf_path, 'nounlink'])
         self.assertIn('CREATION OK', stdout)
-        stdout, _ = self.run_binary(['sealed_file_mod', pf_path])
+        stdout, _ = self.run_binary(['sealed_file_mod', pf_path, 'nounlink'])
         self.assertIn('READING FROM MODIFIED ENCLAVE OK', stdout)
 
     @unittest.skipUnless(HAS_SGX, 'Sealed (protected) files are only available with SGX')
     def test_052_sealed_file_mrenclave_bad(self):
-        pf_path = 'encrypted_file_mrenclave.dat'
+        pf_path = 'tmp_enc/mrenclaves/test_052.dat'
         # Negative test: Seal MRENCLAVE-bound file in one enclave -> opening this file in
         # another enclave (with different MRENCLAVE) should fail
         if os.path.exists(pf_path):
             os.remove(pf_path)
 
-        stdout, _ = self.run_binary(['sealed_file', pf_path])
+        stdout, _ = self.run_binary(['sealed_file', pf_path, 'nounlink'])
         self.assertIn('CREATION OK', stdout)
 
         try:
-            self.run_binary(['sealed_file_mod', pf_path])
+            self.run_binary(['sealed_file_mod', pf_path, 'nounlink'])
             self.fail('expected to return nonzero')
         except subprocess.CalledProcessError as e:
             self.assertEqual(e.returncode, 1)
             stdout = e.stdout.decode()
             self.assertNotIn('READING FROM MODIFIED ENCLAVE OK', stdout)
             self.assertIn('Permission denied', stdout)
 
+    @unittest.skipUnless(HAS_SGX, 'Sealed (protected) files are only available with SGX')
+    def test_053_sealed_file_mrenclave_unlink(self):
+        pf_path = 'tmp_enc/mrenclaves/test_053.dat'
+        # Unlinking test: Seal MRENCLAVE-bound file in one enclave -> unlinking this file in
+        # another enclave (with different MRENCLAVE) should still work
+        if os.path.exists(pf_path):
+            os.remove(pf_path)
+
+        stdout, _ = self.run_binary(['sealed_file', pf_path, 'nounlink'])
+        self.assertIn('CREATION OK', stdout)
+        stdout, _ = self.run_binary(['sealed_file_mod', pf_path, 'unlink'])
+        self.assertIn('UNLINK OK', stdout)
+
     def test_060_synthetic(self):
         stdout, _ = self.run_binary(['synthetic'])
         self.assertIn("TEST OK", stdout)