From 68e02962d5af9b62a192240462dca4245fb98ba0 Mon Sep 17 00:00:00 2001 From: Shuang Liu Date: Sat, 12 Oct 2019 09:03:06 +0000 Subject: [PATCH] Harden SEFS with extra MAC and permission checks --- deps/sefs | 2 +- src/libos/src/fs/hostfs.rs | 15 ++- src/libos/src/fs/inode_file.rs | 28 ++++++ src/libos/src/fs/mod.rs | 11 ++- src/libos/src/fs/root_inode.rs | 13 ++- src/libos/src/fs/sgx_impl.rs | 29 ++++-- test/Makefile | 2 +- test/file/main.c | 2 +- test/fs_perms/Makefile | 5 + test/fs_perms/main.c | 161 +++++++++++++++++++++++++++++++++ test/link/main.c | 4 +- test/mkdir/main.c | 6 +- test/mmap/main.c | 6 +- test/truncate/main.c | 2 +- tools/occlum | 2 + 15 files changed, 258 insertions(+), 30 deletions(-) create mode 100644 test/fs_perms/Makefile create mode 100644 test/fs_perms/main.c diff --git a/deps/sefs b/deps/sefs index f095460e..96fbb3ed 160000 --- a/deps/sefs +++ b/deps/sefs @@ -1 +1 @@ -Subproject commit f095460eefa473eefaddf8723170289e16d4648e +Subproject commit 96fbb3ed48ec949dd3aa0d34b8a54205bd8e09d3 diff --git a/src/libos/src/fs/hostfs.rs b/src/libos/src/fs/hostfs.rs index 2bc7763c..4bbc5223 100644 --- a/src/libos/src/fs/hostfs.rs +++ b/src/libos/src/fs/hostfs.rs @@ -99,19 +99,23 @@ impl INode for HNode { } fn set_metadata(&self, metadata: &Metadata) -> Result<()> { - unimplemented!() + warn!("HostFS: set_metadata() is unimplemented"); + Ok(()) } fn sync_all(&self) -> Result<()> { - unimplemented!() + warn!("HostFS: sync_all() is unimplemented"); + Ok(()) } fn sync_data(&self) -> Result<()> { - unimplemented!() + warn!("HostFS: sync_data() is unimplemented"); + Ok(()) } fn resize(&self, len: usize) -> Result<()> { - unimplemented!() + warn!("HostFS: resize() is unimplemented"); + Ok(()) } fn create(&self, name: &str, type_: FileType, mode: u32) -> Result> { @@ -185,7 +189,8 @@ impl INode for HNode { } fn io_control(&self, cmd: u32, data: usize) -> Result<()> { - unimplemented!() + warn!("HostFS: io_control is unimplemented"); + Ok(()) } fn fs(&self) -> Arc { diff --git a/src/libos/src/fs/inode_file.rs b/src/libos/src/fs/inode_file.rs index 64be87d7..1db49291 100644 --- a/src/libos/src/fs/inode_file.rs +++ b/src/libos/src/fs/inode_file.rs @@ -1,4 +1,6 @@ use super::*; +use rcore_fs_sefs::dev::SefsMac; +use sgx_trts::libc::S_IRUSR; use std::fmt; pub struct INodeFile { @@ -148,6 +150,13 @@ impl File for INodeFile { impl INodeFile { pub fn open(inode: Arc, options: OpenOptions) -> Result { + if (options.read && !inode.allow_read()?) { + return errno!(EBADF, "File not readable"); + } + if (options.write && !inode.allow_write()?) { + return errno!(EBADF, "File not writable"); + } + Ok(INodeFile { inode, offset: SgxMutex::new(0), @@ -178,6 +187,9 @@ impl From for Error { FsError::IOCTLError => EINVAL, FsError::Again => EAGAIN, FsError::Busy => EBUSY, + FsError::WrProtected => EROFS, + FsError::NoIntegrity => EROFS, + FsError::PermError => EPERM, }; Error::new(errno, "") } @@ -196,6 +208,8 @@ impl Debug for INodeFile { pub trait INodeExt { fn read_as_vec(&self) -> Result, Error>; + fn allow_write(&self) -> Result; + fn allow_read(&self) -> Result; } impl INodeExt for INode { @@ -208,4 +222,18 @@ impl INodeExt for INode { self.read_at(0, buf.as_mut_slice())?; Ok(buf) } + + fn allow_write(&self) -> Result { + let info = self.metadata()?; + let perms = info.mode as u32; + let writable = (perms & S_IWUSR) == S_IWUSR; + Ok(writable) + } + + fn allow_read(&self) -> Result { + let info = self.metadata()?; + let perms = info.mode as u32; + let readable = (perms & S_IRUSR) == S_IRUSR; + Ok(readable) + } } diff --git a/src/libos/src/fs/mod.rs b/src/libos/src/fs/mod.rs index ad0fdce5..8753ff91 100644 --- a/src/libos/src/fs/mod.rs +++ b/src/libos/src/fs/mod.rs @@ -19,6 +19,7 @@ pub use self::pipe::Pipe; pub use self::root_inode::ROOT_INODE; pub use self::socket_file::{AsSocket, SocketFile}; pub use self::unix_socket::{AsUnixSocket, UnixSocketFile}; +use sgx_trts::libc::S_IWUSR; use std::any::Any; use std::mem::uninitialized; @@ -311,6 +312,9 @@ pub fn do_mkdir(path: &str, mode: usize) -> Result<(), Error> { if inode.find(file_name).is_ok() { return errno!(EEXIST, ""); } + if !inode.allow_write()? { + return errno!(EPERM, "dir cannot be written"); + } inode.create(file_name, FileType::Dir, mode as u32)?; Ok(()) } @@ -434,7 +438,12 @@ impl Process { } file_inode } - Err(FsError::EntryNotFound) => dir_inode.create(file_name, FileType::File, mode)?, + Err(FsError::EntryNotFound) => { + if !dir_inode.allow_write()? { + return errno!(EPERM, "file cannot be created"); + } + dir_inode.create(file_name, FileType::File, mode)? + } Err(e) => return Err(Error::from(e)), } } else { diff --git a/src/libos/src/fs/root_inode.rs b/src/libos/src/fs/root_inode.rs index 64a1433e..11f6fea9 100644 --- a/src/libos/src/fs/root_inode.rs +++ b/src/libos/src/fs/root_inode.rs @@ -27,7 +27,7 @@ lazy_static! { } fn open_root_fs_according_to(mount_config: &Vec) -> Result, Error> { - let root_sefs_source = { + let (root_sefs_mac, root_sefs_source) = { let root_mount_config = mount_config .iter() .find(|m| m.target == Path::new("/")) @@ -45,11 +45,14 @@ fn open_root_fs_according_to(mount_config: &Vec) -> Result SefsUuid { - let mut uuid: [u8; 16] = Default::default(); - let status = unsafe { sgx_read_rand(uuid.as_mut_ptr(), uuid.len()) }; - if status != sgx_status_t::SGX_SUCCESS { - panic!("sgx_read_rand failed"); - } - SefsUuid::new(uuid) + let mut uuid: [u8; 16] = [0u8; 16]; + let buf = uuid.as_mut_ptr(); + let size = 16; + let status = unsafe { sgx_read_rand(buf, size) }; + assert!(status == sgx_status_t::SGX_SUCCESS); + SefsUuid(uuid) } } @@ -42,13 +42,17 @@ pub struct SgxStorage { } impl SgxStorage { - pub fn new(path: impl AsRef, integrity_only: bool) -> Self { + pub fn new( + path: impl AsRef, + integrity_only: bool, + file_mac: Option, + ) -> Self { // assert!(path.as_ref().is_dir()); SgxStorage { path: path.as_ref().to_path_buf(), integrity_only: integrity_only, file_cache: Mutex::new(BTreeMap::new()), - root_mac: None, + root_mac: file_mac, } } /// Get file by `file_id`. @@ -174,6 +178,10 @@ impl Storage for SgxStorage { caches.remove(&key); Ok(()) } + + fn is_integrity_only(&self) -> bool { + self.integrity_only + } } #[derive(Clone)] @@ -232,4 +240,9 @@ impl File for LockedFile { file.flush().expect("failed to flush SgxFile"); Ok(()) } + + fn get_file_mac(&self) -> DevResult { + let file = self.0.lock().unwrap(); + Ok(SefsMac(file.get_mac().unwrap())) + } } diff --git a/test/Makefile b/test/Makefile index 8376966a..a80e4c78 100644 --- a/test/Makefile +++ b/test/Makefile @@ -5,7 +5,7 @@ BUILD_DIR := $(PROJECT_DIR)/build # Dependencies: need to be compiled but not to run by any Makefile target TEST_DEPS := dev_null client # Tests: need to be compiled and run by test-% target -TESTS := empty env hello_world malloc mmap file getpid spawn sched pipe time \ +TESTS := empty env hello_world malloc mmap file fs_perms getpid spawn sched pipe time \ truncate readdir mkdir link tls pthread uname rlimit server \ server_epoll unix_socket cout hostfs cpuid rdtsc device # Benchmarks: need to be compiled and run by bench-% target diff --git a/test/file/main.c b/test/file/main.c index 21d9a5b2..dc0ebb77 100644 --- a/test/file/main.c +++ b/test/file/main.c @@ -6,7 +6,7 @@ #include int main(int argc, const char* argv[]) { - const char* file_name = "tmp.txt"; + const char* file_name = "/root/test_filesystem_file_read_write.txt"; int fd, flags, mode, len; off_t offset; const char* write_msg = "Hello World\n"; diff --git a/test/fs_perms/Makefile b/test/fs_perms/Makefile new file mode 100644 index 00000000..9e1b6dec --- /dev/null +++ b/test/fs_perms/Makefile @@ -0,0 +1,5 @@ +include ../test_common.mk + +EXTRA_C_FLAGS := +EXTRA_LINK_FLAGS := +BIN_ARGS := diff --git a/test/fs_perms/main.c b/test/fs_perms/main.c new file mode 100644 index 00000000..3be86f3e --- /dev/null +++ b/test/fs_perms/main.c @@ -0,0 +1,161 @@ +#include +#include +#include +#include +#include +#include +#include "test.h" + +// ============================================================================ +// Helper macros +// ============================================================================ + +//#define PRINT_DBG(msg) printf("%s %s %d ", msg, __FUNCTION__, __LINE__); +#define PRINT_DBG(msg) +#define OK (0) +#define NG (-1) + +#define NUM_TEST_CASES 6 +#define NUM_TEST_FILES 5 + +// ============================================================================ +// Helper functions +// ============================================================================ + +static int open_file(const char *filename, int flags, int mode) { + int fd = -1; + if ((fd = open(filename, flags, mode)) < 0) { + PRINT_DBG("ERROR: failed to open a file\n"); + } + return fd; +} + +static const char* write_msg = "Hello SEFS 1234567890\n"; + +static int write_file(int fd) { + int len = strlen(write_msg); + if ((len = write(fd, write_msg, len)<= 0)) { + PRINT_DBG("ERROR: failed to write to the file\n"); + return -1; + } + fsync(fd); + close(fd); + return 0; +} + +static int read_file(int fd){ + int len; + char read_buf[128] = {0}; + if ((len = read(fd, read_buf, sizeof(read_buf) - 1)) <= 0) { + PRINT_DBG("ERROR: failed to read from the file\n"); + return -1; + } + close(fd); + + if (strcmp(write_msg, read_buf) != 0) { + PRINT_DBG("ERROR: the message read from the file is not as it was written\n"); + return -1; + } + return 0; +} + + +// for each file in test_filename +// open the file with the given flags +// do read or write according to do_write +// check the result of the read/write with the given expected_result +static int do_perm_tests( + const char** files, + size_t num_files, + int flags, int do_write, + int* expected_results) +{ + flags |= O_CREAT | O_TRUNC; + for(size_t i = 0; i < num_files; i++) { + const char* filename = files[i]; + int expected_result = expected_results[i]; + + int fd = open_file(filename, flags, 0666); + int result = do_write ? write_file(fd) : read_file(fd); + if (result != expected_result) { + return -1; + } + } + return 0; +} + +// ============================================================================ +// Test cases +// ============================================================================ + +// Test files +static const char* test_files[NUM_TEST_FILES] = { + "/test_fs_perms.txt", + "/bin/test_fs_perms.txt", + "/lib/test_fs_perms.txt", + "/root/test_fs_perms.txt", + "/host/test_fs_perms.txt", +}; + +// Test cases X Test files -> Test Results +static int test_expected_results[NUM_TEST_CASES][NUM_TEST_FILES]= { + // test_open_ro_then_write() + {NG, NG, NG, NG, NG}, + // test_open_wo_then_write() + {NG, NG, NG, OK, OK}, + // test_open_rw_then_write() + {NG, NG, NG, OK, OK}, + // test_open_ro_then_read() + {NG, NG, NG, OK, OK}, + // test_open_wo_then_read() + {NG, NG, NG, NG, NG}, + // test_open_rw_then_read() + {NG, NG, NG, OK, OK}, +}; + +int test_open_ro_then_write() { + return do_perm_tests(test_files, NUM_TEST_FILES, + O_RDONLY, 1, test_expected_results[0]); +} + +int test_open_wo_then_write() { + return do_perm_tests(test_files, NUM_TEST_FILES, + O_WRONLY, 1, test_expected_results[1]); +} + +int test_open_rw_then_write() { + return do_perm_tests(test_files, NUM_TEST_FILES, + O_RDWR, 1, test_expected_results[2]); +} + +int test_open_ro_then_read() { + return do_perm_tests(test_files, NUM_TEST_FILES, + O_RDONLY, 0, test_expected_results[3]); +} + +int test_open_wo_then_read(){ + return do_perm_tests(test_files, NUM_TEST_FILES, + O_WRONLY, 0, test_expected_results[4]); +} + +int test_open_rw_then_read() { + return do_perm_tests(test_files, NUM_TEST_FILES, + O_RDWR, 0, test_expected_results[5]); +} + +// ============================================================================ +// Test suite main +// ============================================================================ + +test_case_t test_cases[NUM_TEST_CASES] = { + TEST_CASE(test_open_ro_then_write), + TEST_CASE(test_open_wo_then_write), + TEST_CASE(test_open_rw_then_write), + TEST_CASE(test_open_ro_then_read), + TEST_CASE(test_open_wo_then_read), + TEST_CASE(test_open_rw_then_read) +}; + +int main(int argc, const char* argv[]) { + return test_suite_run(test_cases, NUM_TEST_CASES); +} diff --git a/test/link/main.c b/test/link/main.c index a959079a..e0d4ba70 100644 --- a/test/link/main.c +++ b/test/link/main.c @@ -7,8 +7,8 @@ #include int main(int argc, const char* argv[]) { - const char* file_name = "tmp.txt"; - const char* link_name = "link.txt"; + const char* file_name = "/root/test_filesystem_link.txt"; + const char* link_name = "/root/link.txt"; const char* write_msg = "Hello World\n"; char read_buf[128] = {0}; int ret; diff --git a/test/mkdir/main.c b/test/mkdir/main.c index 4ce33b3b..5cbf3eb4 100644 --- a/test/mkdir/main.c +++ b/test/mkdir/main.c @@ -23,8 +23,10 @@ int main(int argc, const char* argv[]) { return -1; } - const char DIR_NAME[] = "test_dir"; - const char DIR_PATH[] = "/test_dir"; + //const char DIR_NAME[] = "test_dir"; + //const char DIR_PATH[] = "/test_dir"; + const char DIR_NAME[] = "/root/test_dir"; + const char DIR_PATH[] = "/root/test_dir"; const int DIR_MODE = 0664; ret = mkdir(DIR_NAME, DIR_MODE); if(ret < 0) { diff --git a/test/mmap/main.c b/test/mmap/main.c index 2d67fa8b..b46f7d72 100644 --- a/test/mmap/main.c +++ b/test/mmap/main.c @@ -254,7 +254,7 @@ int test_anonymous_mmap_with_non_page_aligned_len() { // ============================================================================ int test_file_mmap() { - const char* file_path = "mmap_file.data"; + const char* file_path = "/root/mmap_file.data"; int fd = open(file_path, O_CREAT | O_TRUNC | O_WRONLY, 0644); if (fd < 0) { throw_error("file creation failed"); @@ -292,7 +292,7 @@ int test_file_mmap() { } int test_file_mmap_with_offset() { - const char* file_path = "mmap_file.data"; + const char* file_path = "/root/mmap_file.data"; int fd = open(file_path, O_CREAT | O_TRUNC | O_RDWR, 0644); if (fd < 0) { throw_error("file creation failed"); @@ -352,7 +352,7 @@ int test_file_mmap_with_invalid_fd() { } int test_file_mmap_with_non_page_aligned_offset() { - const char* file_path = "mmap_file.data"; + const char* file_path = "/root/mmap_file.data"; int fd = open(file_path, O_CREAT | O_TRUNC | O_RDWR, 0644); if (fd < 0) { throw_error("file creation failed"); diff --git a/test/truncate/main.c b/test/truncate/main.c index 36a3d45a..a783d934 100644 --- a/test/truncate/main.c +++ b/test/truncate/main.c @@ -6,7 +6,7 @@ #include int main(int argc, const char* argv[]) { - const char* FILE_NAME = "tmp.txt"; + const char* FILE_NAME = "root/test_filesystem_truncate.txt"; const int TRUNC_LEN = 256; const int TRUNC_LEN1 = 128; const int MODE_MASK = 0777; diff --git a/tools/occlum b/tools/occlum index d5125974..728b565b 100755 --- a/tools/occlum +++ b/tools/occlum @@ -95,6 +95,8 @@ cmd_build() { cp "$occlum_dir/build/lib/libcompiler-rt-patch.a" build/lib/ mkdir -p build/src/libos/src/builtin + chmod 531 -R $working_dir/image/bin + chmod 531 -R $working_dir/image/lib mkdir -p build/mount/ cd "$occlum_dir/deps/sefs/sefs-fuse/bin/" && \ ./app \