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,