Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(fs): Escape paths in Scope methods #2070

Merged
merged 9 commits into from
Nov 21, 2024
Merged

fix(fs): Escape paths in Scope methods #2070

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b42064d
fix(fs): Escape paths in Scope methods
FabianLars Nov 18, 2024
caac44d
just testing some smaller workarounds before pulling out the big guns
FabianLars Nov 18, 2024
f323eb8
i am making it worse
FabianLars Nov 19, 2024
0f2e408
just breaking stuff, don't mind me
FabianLars Nov 20, 2024
e8fabf0
android
FabianLars Nov 20, 2024
6b344c8
Merge remote-tracking branch 'origin/v2' into fix/fs/scope-escape
FabianLars Nov 20, 2024
ad3156f
fmt
FabianLars Nov 21, 2024
00b3d16
propagate errors
FabianLars Nov 21, 2024
cc182fb
Merge branch 'v2' into fix/fs/scope-escape
amrbashir Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changes/fix-fs-scope-escape-paths.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
fs: minor
persisted-scope: minor
---

**Breaking Change:** Replaced the custom `tauri_plugin_fs::Scope` struct with `tauri::fs::Scope`.
10 changes: 5 additions & 5 deletions plugins/dialog/src/commands.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ pub(crate) async fn open<R: Runtime>(
for folder in folders {
if let Ok(path) = folder.clone().into_path() {
if let Some(s) = window.try_fs_scope() {
s.allow_directory(&path, options.recursive);
s.allow_directory(&path, options.recursive)?;
}
tauri_scope.allow_directory(&path, options.directory)?;
}
Expand All @@ -157,7 +157,7 @@ pub(crate) async fn open<R: Runtime>(
if let Some(folder) = &folder {
if let Ok(path) = folder.clone().into_path() {
if let Some(s) = window.try_fs_scope() {
s.allow_directory(&path, options.recursive);
s.allow_directory(&path, options.recursive)?;
}
tauri_scope.allow_directory(&path, options.directory)?;
}
Expand All @@ -175,7 +175,7 @@ pub(crate) async fn open<R: Runtime>(
for file in files {
if let Ok(path) = file.clone().into_path() {
if let Some(s) = window.try_fs_scope() {
s.allow_file(&path);
s.allow_file(&path)?;
}

tauri_scope.allow_file(&path)?;
Expand All @@ -190,7 +190,7 @@ pub(crate) async fn open<R: Runtime>(
if let Some(file) = &file {
if let Ok(path) = file.clone().into_path() {
if let Some(s) = window.try_fs_scope() {
s.allow_file(&path);
s.allow_file(&path)?;
}
tauri_scope.allow_file(&path)?;
}
Expand Down Expand Up @@ -232,7 +232,7 @@ pub(crate) async fn save<R: Runtime>(
if let Some(p) = &path {
if let Ok(path) = p.clone().into_path() {
if let Some(s) = window.try_fs_scope() {
s.allow_file(&path);
s.allow_file(&path)?;
}
tauri_scope.allow_file(&path)?;
}
Expand Down
2 changes: 1 addition & 1 deletion plugins/fs/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ rustc-args = ["--cfg", "docsrs"]
rustdoc-args = ["--cfg", "docsrs"]

[package.metadata.platforms.support]
windows = { level = "full", notes = "" }
windows = { level = "full", notes = "Apps installed via MSI or NSIS in `perMachine` and `both` mode require admin permissions for write acces in `$RESOURCES` folder" }
linux = { level = "full", notes = "No write access to `$RESOURCES` folder" }
macos = { level = "full", notes = "No write access to `$RESOURCES` folder" }
android = { level = "partial", notes = "Access is restricted to Application folder by default" }
Expand Down
83 changes: 63 additions & 20 deletions plugins/fs/src/commands.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,13 @@ use std::{
borrow::Cow,
fs::File,
io::{BufReader, Lines, Read, Write},
path::PathBuf,
path::{Path, PathBuf},
str::FromStr,
sync::Mutex,
time::{SystemTime, UNIX_EPOCH},
};

use crate::{scope::Entry, Error, FsExt, SafeFilePath};
use crate::{scope::Entry, Error, SafeFilePath};

#[derive(Debug, thiserror::Error)]
pub enum CommandError {
Expand Down Expand Up @@ -967,6 +967,8 @@ pub fn resolve_file<R: Runtime>(
path: SafeFilePath,
open_options: OpenOptions,
) -> CommandResult<(File, PathBuf)> {
use crate::FsExt;

match path {
SafeFilePath::Url(url) => {
let path = url.as_str().into();
Expand Down Expand Up @@ -999,40 +1001,81 @@ pub fn resolve_path<R: Runtime>(
path
};

let fs_scope = webview.state::<crate::Scope>();

let scope = tauri::scope::fs::Scope::new(
webview,
&FsScope::Scope {
allow: webview
.fs_scope()
.allowed
.lock()
.unwrap()
.clone()
.into_iter()
.chain(global_scope.allows().iter().filter_map(|e| e.path.clone()))
allow: global_scope
.allows()
.iter()
.filter_map(|e| e.path.clone())
.chain(command_scope.allows().iter().filter_map(|e| e.path.clone()))
.collect(),
deny: webview
.fs_scope()
.denied
.lock()
.unwrap()
.clone()
.into_iter()
.chain(global_scope.denies().iter().filter_map(|e| e.path.clone()))
deny: global_scope
.denies()
.iter()
.filter_map(|e| e.path.clone())
.chain(command_scope.denies().iter().filter_map(|e| e.path.clone()))
.collect(),
require_literal_leading_dot: webview.fs_scope().require_literal_leading_dot,
require_literal_leading_dot: fs_scope.require_literal_leading_dot,
},
)?;

if scope.is_allowed(&path) {
let require_literal_leading_dot = fs_scope.require_literal_leading_dot.unwrap_or(cfg!(unix));

if is_forbidden(&fs_scope.scope, &path, require_literal_leading_dot)
|| is_forbidden(&scope, &path, require_literal_leading_dot)
{
return Err(CommandError::Plugin(Error::PathForbidden(path)));
}

if fs_scope.scope.is_allowed(&path) || scope.is_allowed(&path) {
Comment on lines +1002 to +1008
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do really need to manually call is_forbidden, doesn't scope.is_allowed check for forbidden/denied patterns?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem is that is_allowed returns false for paths that are explictly denied and paths that are neither allowed nor denied.
When we're checking for 2 scopes at the same time we only care about the explicitly denied paths because those take precedence of explictly allowed paths.
A path that's neither allowed nor denied in the first scope could still be explicitly allowed by the second scope.

Alternatively a way to create a new Scope from 2 existing scopes would be cool too, then is_allowed would work for us here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can improve this a bit by adding the Scope::is_forbidden in tauri

Ok(path)
} else {
Err(CommandError::Plugin(Error::PathForbidden(path)))
}
}

fn is_forbidden<P: AsRef<Path>>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this implemented separately here?

Copy link
Member Author

@FabianLars FabianLars Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i can implement it in tauri::fs::Scope too and re-use that but wanted to keep the plugin compatible with all tauri v2 versions for now.

not too happy about the approach itself but merging instances of tauri::fs::Scopes isn't possible rn so this was the next best thing to make the draft work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's ship with this for now, but I think we should still add it in tauri and then migrate the fs plugin later to that.

Copy link
Member Author

@FabianLars FabianLars Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, will open a pr later if i don't forget

scope: &tauri::fs::Scope,
path: P,
require_literal_leading_dot: bool,
) -> bool {
let path = path.as_ref();
let path = if path.is_symlink() {
match std::fs::read_link(path) {
Ok(p) => p,
Err(_) => return false,
}
} else {
path.to_path_buf()
};
let path = if !path.exists() {
crate::Result::Ok(path)
} else {
std::fs::canonicalize(path).map_err(Into::into)
};

if let Ok(path) = path {
let path: PathBuf = path.components().collect();
scope.forbidden_patterns().iter().any(|p| {
p.matches_path_with(
&path,
glob::MatchOptions {
// this is needed so `/dir/*` doesn't match files within subdirectories such as `/dir/subdir/file.txt`
// see: <https://github.com/tauri-apps/tauri/security/advisories/GHSA-6mv3-wm7j-h4w5>
require_literal_separator: true,
require_literal_leading_dot,
..Default::default()
},
)
})
} else {
false
}
}

struct StdFileResource(Mutex<File>);

impl StdFileResource {
Expand Down
36 changes: 21 additions & 15 deletions plugins/fs/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use serde::Deserialize;
use tauri::{
ipc::ScopeObject,
plugin::{Builder as PluginBuilder, TauriPlugin},
utils::acl::Value,
utils::{acl::Value, config::FsScope},
AppHandle, DragDropEvent, Manager, RunEvent, Runtime, WindowEvent,
};

Expand All @@ -39,7 +39,6 @@ pub use desktop::Fs;
pub use mobile::Fs;

pub use error::Error;
pub use scope::{Event as ScopeEvent, Scope};

pub use file_path::FilePath;
pub use file_path::SafeFilePath;
Expand Down Expand Up @@ -365,21 +364,26 @@ impl ScopeObject for scope::Entry {
}
}

pub(crate) struct Scope {
pub(crate) scope: tauri::fs::Scope,
pub(crate) require_literal_leading_dot: Option<bool>,
}

pub trait FsExt<R: Runtime> {
fn fs_scope(&self) -> &Scope;
fn try_fs_scope(&self) -> Option<&Scope>;
fn fs_scope(&self) -> tauri::fs::Scope;
fn try_fs_scope(&self) -> Option<tauri::fs::Scope>;

/// Cross platform file system APIs that also support manipulating Android files.
fn fs(&self) -> &Fs<R>;
}

impl<R: Runtime, T: Manager<R>> FsExt<R> for T {
fn fs_scope(&self) -> &Scope {
self.state::<Scope>().inner()
fn fs_scope(&self) -> tauri::fs::Scope {
self.state::<Scope>().scope.clone()
}

fn try_fs_scope(&self) -> Option<&Scope> {
self.try_state::<Scope>().map(|s| s.inner())
fn try_fs_scope(&self) -> Option<tauri::fs::Scope> {
self.try_state::<Scope>().map(|s| s.scope.clone())
}

fn fs(&self) -> &Fs<R> {
Expand Down Expand Up @@ -419,11 +423,13 @@ pub fn init<R: Runtime>() -> TauriPlugin<R, Option<config::Config>> {
watcher::unwatch
])
.setup(|app, api| {
let mut scope = Scope::default();
scope.require_literal_leading_dot = api
.config()
.as_ref()
.and_then(|c| c.require_literal_leading_dot);
let scope = Scope {
require_literal_leading_dot: api
.config()
.as_ref()
.and_then(|c| c.require_literal_leading_dot),
scope: tauri::fs::Scope::new(app, &FsScope::default())?,
};

#[cfg(target_os = "android")]
{
Expand All @@ -446,9 +452,9 @@ pub fn init<R: Runtime>() -> TauriPlugin<R, Option<config::Config>> {
let scope = app.fs_scope();
for path in paths {
if path.is_file() {
scope.allow_file(path);
let _ = scope.allow_file(path);
} else {
scope.allow_directory(path, true);
let _ = scope.allow_directory(path, true);
}
}
}
Expand Down
Loading
Loading