Potential race condition in file permission enforcement
The methods get_key and delete_key in FilePasswordManager call ensure_file_permissions to verify and set file permissions before accessing or deleting the keyfile. However, there is a potential race condition between checking the permissions and performing the file operation.
Consider using fs::OpenOptions with appropriate flags to open the file securely, minimizing the window for race conditions.
Example adjustment in get_key:
async fn get_key(&self) -> Result<Zeroizing<Vec<u8>>> {
- let path = &self.path;
-
- ensure_file_permissions(path, 0o400)?;
-
- let bytes = fs::read(&self.path).context("Failed to access keyfile")?;
+ let mut file = OpenOptions::new()
+ .read(true)
+ .custom_flags(libc::O_NOFOLLOW)
+ .mode(0o400)
+ .open(&self.path)
+ .context("Failed to securely open keyfile")?;
+
+ let mut bytes = Vec::new();
+ file.read_to_end(&mut bytes).context("Failed to read keyfile")?;
Ok(Zeroizing::new(bytes))
}
Note: The use of custom_flags(libc::O_NOFOLLOW) prevents symbolic link attacks. Ensure libc is imported and available.
Potential race condition in file permission enforcement
The methods
get_key
anddelete_key
inFilePasswordManager
callensure_file_permissions
to verify and set file permissions before accessing or deleting the keyfile. However, there is a potential race condition between checking the permissions and performing the file operation.Consider using
fs::OpenOptions
with appropriate flags to open the file securely, minimizing the window for race conditions.Example adjustment in
get_key
:Note: The use of
custom_flags(libc::O_NOFOLLOW)
prevents symbolic link attacks. Ensurelibc
is imported and available.Also applies to: 105-106
_Originally posted by @coderabbitai[bot] in https://github.com/gnosisguild/enclave/pull/156#discussion_r1818435683_