Audit for symlink safety

olivefsd/src/server/file_access.rs

@@ -109,6 +109,11 @@ pub fn error_to_response<A>(error: anyhow::Error) -> DataResponse<A> {
 }
 
 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<R>(&self, vnode: VnodeId) -> Result<PathBuf, DataResponse<R>> {
         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<VnodeId> {
         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<FileMetadata> {
         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<DataResponse<FileMetadata>> {
         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<DataResponse<u32>> {
         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<DataResponse<()>> {
         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<DataResponse<(u32, FileMetadata)>> {
+        // 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<DataResponse<FileMetadata>> {
+        // 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<DataResponse<u32>> {
-        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<DataResponse<()>> {
         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<DataResponse<()>> {
         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,