From bbc5b30e26ab9af80deed744694bf79071d404dc Mon Sep 17 00:00:00 2001 From: Olivier 'reivilibre Date: Mon, 24 Jan 2022 21:57:50 +0000 Subject: [PATCH] Audit for symlink safety --- olivefsd/src/server/file_access.rs | 63 +++++++++++++++++++++++++++++- 1 file changed, 62 insertions(+), 1 deletion(-) diff --git a/olivefsd/src/server/file_access.rs b/olivefsd/src/server/file_access.rs index 8a3b986..de3057e 100644 --- a/olivefsd/src/server/file_access.rs +++ b/olivefsd/src/server/file_access.rs @@ -109,6 +109,11 @@ pub fn error_to_response(error: anyhow::Error) -> DataResponse { } impl FileAccess { + /// Looks up a Vnode to a Path. + /// + /// Symlink safety: + /// This function can return the path to a symlink and no effort has been made to dereference + /// safe symlinks. async fn resolve_vnode(&self, vnode: VnodeId) -> Result> { let inode_map = self.client_state.inode_map.read().await; if let Some(inode_info) = inode_map.get(vnode.0 as usize) { @@ -122,6 +127,14 @@ impl FileAccess { } } + /// Looks up a Vnode to a Path that has had all known symlinks checked. + /// + /// Symlink safety: + /// This function can return the path to a symlink (due to a TOCTOU race) but best effort has + /// been made to dereference safe symlinks. + /// All resolving was done without leaving the virtual root. + /// The caller should feel confident in failing if the destination path is still a symlink, + /// because that would be a race condition. async fn resolve_vnode_including_follow_symlinks_safely_best_effort( &self, vnode: VnodeId, @@ -138,6 +151,11 @@ impl FileAccess { /// on opening a symlink if it gets changed under their feet). /// The symlinks will stop resolving if they ever point outside of the root directory, even /// if they point to a symlink that points back into the restricted tree. + /// + /// Symlink safety: + /// All safe symlinks in the path will be safely dereferenced. + /// However, because of the potential for a TOCTOU race, the resultant path may still be + /// a symlink, potentially an unsafe one. async fn safely_resolve_symlinks_best_effort( &self, path: PathBuf, @@ -161,6 +179,10 @@ impl FileAccess { .map_err(|_| io::Error::from_raw_os_error(EFAULT))? } + /// Allocates a Vnode for a path + /// + /// Symlink safety: + /// No operations are carried out on the provided path. async fn allocate_vnode(&self, path: PathBuf) -> anyhow::Result { let paths_to_vnodes = self.client_state.existing_paths_to_inodes.read().await; if let Some(vi) = paths_to_vnodes.get(&path) { @@ -179,6 +201,12 @@ impl FileAccess { Ok(vnode_id) } + /// Reads the metadata for a path. + /// + /// Symlink safety: + /// The path is not dereferenced. The returned metadata concerns the symlink if the path + /// points to one. + /// The caller should resolve safe symlinks first if desired. async fn read_metadata(&self, path: &Path, vnode: VnodeId) -> std::io::Result { let metadata = tokio::fs::symlink_metadata(path).await?; let file_metadata = FileMetadata { @@ -199,6 +227,8 @@ impl FileAccess { Ok(file_metadata) } + /// Symlink safety: + /// No paths are dereferenced; see `read_metadata`. pub async fn getattr(&self, vnode: VnodeId) -> anyhow::Result> { let inode_map = self.client_state.inode_map.read().await; if let Some(inode_info) = inode_map.get(vnode.0 as usize) { @@ -217,6 +247,8 @@ impl FileAccess { } } + /// Symlink safety: + /// UNSAFE TODO SECURITY BUG pub async fn opendir(&self, vnode: VnodeId) -> anyhow::Result> { match self.resolve_vnode(vnode).await { Ok(path) => { @@ -238,6 +270,8 @@ impl FileAccess { } } + /// Symlink safety: + /// This operation does not handle any paths itself; see `opendir`. pub async fn readdir( &self, dir_handle: u32, @@ -337,6 +371,8 @@ impl FileAccess { } } + /// Symlink safety: + /// No paths are handled, pub async fn releasedir(&self, dir_handle: u32) -> anyhow::Result> { let dir_handle = dir_handle as usize; let mut dir_handles = self.client_state.dir_handles.write().await; @@ -344,12 +380,15 @@ impl FileAccess { Ok(DataResponse::Success(())) } + /// Symlink safety: + /// Safe — uses O_NOFOLLOW after dereferencing safe symlinks. pub async fn create( &self, dir_vnode: VnodeId, mode: OpenMode, name: String, ) -> anyhow::Result> { + // TODO use the better version of this then handle I/O errors in the callsite match self.resolve_vnode(dir_vnode).await { Ok(dir_path) => { let lookup_path = dir_path.join(name); @@ -412,11 +451,14 @@ impl FileAccess { } } + /// Symlink safety: + /// TODO CHECK pub async fn lookup( &self, dir_vnode: VnodeId, name: String, ) -> anyhow::Result> { + // TODO switch to the easier ones and then handle I/O errors in the callsite match self.resolve_vnode(dir_vnode).await { Ok(dir_path) => { let lookup_path = dir_path.join(name); @@ -453,10 +495,19 @@ impl FileAccess { } } + /// Symlink safety: + /// Safe — uses O_NOFOLLOW after dereferencing safe symlinks. pub async fn open(&self, vnode: VnodeId, mode: OpenMode) -> anyhow::Result> { - match self.resolve_vnode(vnode).await { + match self + .resolve_vnode_including_follow_symlinks_safely_best_effort(vnode) + .await + { Ok(path) => { let mut open_options = OpenOptions::new(); + + // IMPORTANT — Don't follow symlinks + open_options.custom_flags(O_NOFOLLOW); + if mode.read { open_options.read(true); } @@ -497,6 +548,8 @@ impl FileAccess { } } + /// Symlink safety: + /// No paths are touched during this operation. pub async fn release(&self, file_handle: u32) -> anyhow::Result> { let file_handle = file_handle as usize; let mut file_handles = self.client_state.file_handles.write().await; @@ -510,6 +563,8 @@ impl FileAccess { } } + /// Symlink safety: + /// No paths are touched during this operation. pub async fn read( &self, file_handle: u32, @@ -568,6 +623,8 @@ impl FileAccess { } } + /// Symlink safety: + /// No paths are touched in this operation. pub async fn write( &self, file_handle: u32, @@ -613,6 +670,8 @@ impl FileAccess { } } + /// Symlink safety: + /// No paths are touched in this operation. pub async fn flush(&self, file_handle: u32) -> anyhow::Result> { let file_handle = file_handle as usize; let file_handles = self.client_state.file_handles.read().await; @@ -629,6 +688,8 @@ impl FileAccess { } } + /// Symlink safety: + /// Safe — uses O_NOFOLLOW after dereferencing safe symlinks. pub async fn setattr( &self, vnode: VnodeId,