* [PATCH proxmox-backup] fix #5247: relative paths in exclude patterns.
@ 2026-03-23 15:32 Manuel Federanko
2026-03-25 17:05 ` Christian Ebner
0 siblings, 1 reply; 3+ messages in thread
From: Manuel Federanko @ 2026-03-23 15:32 UTC (permalink / raw)
To: pbs-devel
Patterns which start with ./ or for that matter contain /../ or similar
constructs will never match, since they get compared to sanitized paths
from directory traversal, sanitize those patterns too.
Tested by creating a .pxarexclude file and via the --exclude command
line option.
I'm unsure if we should accept those silently, currently I added a
warning for patterns supplied via the cli option, since that feels most
susceptible to wrong usage.
It might also make sense to base the patterns for the cli arg on the
cwd, but that would be a breaking change.
Signed-off-by: Manuel Federanko <m.federanko@proxmox.com>
Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5247
---
pbs-client/src/pxar/create.rs | 7 ++++++-
pbs-client/src/pxar/tools.rs | 32 +++++++++++++++++++++++++++++++
proxmox-backup-client/src/main.rs | 16 ++++++++++++++--
3 files changed, 52 insertions(+), 3 deletions(-)
diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
index e42bcc87..1c878cb7 100644
--- a/pbs-client/src/pxar/create.rs
+++ b/pbs-client/src/pxar/create.rs
@@ -560,7 +560,12 @@ impl Archiver {
(line, MatchType::Exclude, false)
};
- match MatchEntry::parse_pattern(line, PatternFlag::PATH_NAME, mode) {
+ let line_normalized = crate::pxar::tools::normalize_lexically(OsStr::from_bytes(line));
+ match MatchEntry::parse_pattern(
+ line_normalized.as_path().as_os_str().as_bytes(),
+ PatternFlag::PATH_NAME,
+ mode,
+ ) {
Ok(pattern) => {
if anchored {
self.patterns.push(pattern.add_flags(MatchFlag::ANCHORED));
diff --git a/pbs-client/src/pxar/tools.rs b/pbs-client/src/pxar/tools.rs
index 475c08ad..71a38b29 100644
--- a/pbs-client/src/pxar/tools.rs
+++ b/pbs-client/src/pxar/tools.rs
@@ -76,6 +76,38 @@ fn assert_single_path_component_do(path: &Path) -> Result<(), Error> {
Ok(())
}
+pub fn normalize_lexically<S: AsRef<OsStr> + ?Sized>(path: &S) -> PathBuf {
+ normalize_lexically_do(Path::new(path))
+}
+fn normalize_lexically_do(path: &Path) -> PathBuf {
+ // Once std::path::normalize_lexically is stabilized we can switch
+ // to that
+ use std::path::Component;
+ let has_trailing_slash = path
+ .as_os_str()
+ .as_encoded_bytes()
+ .last()
+ .copied()
+ .is_some_and(|c: u8| c == std::path::MAIN_SEPARATOR_STR.bytes().last().unwrap());
+ let mut new = PathBuf::new();
+ let iter = path.components();
+ for component in iter {
+ match component {
+ Component::RootDir => new.push(Component::RootDir),
+ Component::Prefix(p) => new.push(Component::Prefix(p)),
+ Component::CurDir => continue,
+ Component::ParentDir => {
+ new.pop();
+ }
+ Component::Normal(n) => new.push(n),
+ };
+ }
+ if has_trailing_slash {
+ new.push("");
+ }
+ new
+}
+
#[rustfmt::skip]
fn symbolic_mode(c: u64, special: bool, special_x: u8, special_no_x: u8) -> [u8; 3] {
[
diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index 2ada87bd..4ed23d6e 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -826,9 +826,21 @@ async fn create_backup(
let entry = entry
.as_str()
.ok_or_else(|| format_err!("Invalid pattern string slice"))?;
+ let entry_normalized = pbs_client::pxar::tools::normalize_lexically(entry);
+ if entry_normalized.as_os_str() != entry {
+ log::info!(
+ "Sanitized exclude pattern. Exclude patterns are relative to backup root, \
+ not the current working directory and should not contain '.' or '..' as path \
+ segments"
+ );
+ }
pattern_list.push(
- MatchEntry::parse_pattern(entry, PatternFlag::PATH_NAME, MatchType::Exclude)
- .map_err(|err| format_err!("invalid exclude pattern entry: {}", err))?,
+ MatchEntry::parse_pattern(
+ entry_normalized.as_os_str().as_encoded_bytes(),
+ PatternFlag::PATH_NAME,
+ MatchType::Exclude,
+ )
+ .map_err(|err| format_err!("invalid exclude pattern entry: {}", err))?,
);
}
--
2.47.3
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH proxmox-backup] fix #5247: relative paths in exclude patterns.
2026-03-23 15:32 [PATCH proxmox-backup] fix #5247: relative paths in exclude patterns Manuel Federanko
@ 2026-03-25 17:05 ` Christian Ebner
2026-03-26 8:50 ` Manuel Federanko
0 siblings, 1 reply; 3+ messages in thread
From: Christian Ebner @ 2026-03-25 17:05 UTC (permalink / raw)
To: Manuel Federanko, pbs-devel
Hi, thanks for the patch! A few high level comments, no full in detail
review yet.
On 3/23/26 4:31 PM, Manuel Federanko wrote:
> Patterns which start with ./ or for that matter contain /../ or similar
> constructs will never match, since they get compared to sanitized paths
> from directory traversal, sanitize those patterns too.
May also state that the user is now warned if such patterns are encountered.
> Tested by creating a .pxarexclude file and via the --exclude command
> line option.
Would not include explicitly that you tested this in the commit message,
it should rather state that this has effect for both cases. Don't get me
wrong, it can make sense to include such information as well, e.g. as
future references for more complex command invocations, just a bit to
much for this trivial test case IMHO.
>
your comment from here ...
> I'm unsure if we should accept those silently, currently I added a
> warning for patterns supplied via the cli option, since that feels most
> susceptible to wrong usage.
>
> It might also make sense to base the patterns for the cli arg on the
> cwd, but that would be a breaking change.
... to here should not be part of the commit message (which will be
applied), but rather be additional notes which go below the `---`
separator (where it does not get applied). See marking below.
>
> Signed-off-by: Manuel Federanko <m.federanko@proxmox.com>
> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5247
> ---
-> additional notes and comments here
> pbs-client/src/pxar/create.rs | 7 ++++++-
> pbs-client/src/pxar/tools.rs | 32 +++++++++++++++++++++++++++++++
> proxmox-backup-client/src/main.rs | 16 ++++++++++++++--
> 3 files changed, 52 insertions(+), 3 deletions(-)
>
> diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
> index e42bcc87..1c878cb7 100644
> --- a/pbs-client/src/pxar/create.rs
> +++ b/pbs-client/src/pxar/create.rs
> @@ -560,7 +560,12 @@ impl Archiver {
> (line, MatchType::Exclude, false)
> };
>
> - match MatchEntry::parse_pattern(line, PatternFlag::PATH_NAME, mode) {
> + let line_normalized = crate::pxar::tools::normalize_lexically(OsStr::from_bytes(line));
> + match MatchEntry::parse_pattern(
> + line_normalized.as_path().as_os_str().as_bytes(),
> + PatternFlag::PATH_NAME,
> + mode,
> + ) {
> Ok(pattern) => {
> if anchored {
> self.patterns.push(pattern.add_flags(MatchFlag::ANCHORED));
> diff --git a/pbs-client/src/pxar/tools.rs b/pbs-client/src/pxar/tools.rs
> index 475c08ad..71a38b29 100644
> --- a/pbs-client/src/pxar/tools.rs
> +++ b/pbs-client/src/pxar/tools.rs
> @@ -76,6 +76,38 @@ fn assert_single_path_component_do(path: &Path) -> Result<(), Error> {
> Ok(())
> }
>
> +pub fn normalize_lexically<S: AsRef<OsStr> + ?Sized>(path: &S) -> PathBuf {
> + normalize_lexically_do(Path::new(path))
comment: `path` could be wrapped here and the function in-lined:
let path = Path::new(path);
[...]
> +}
> +fn normalize_lexically_do(path: &Path) -> PathBuf {
> + // Once std::path::normalize_lexically is stabilized we can switch
> + // to that
comment: This comment warrants a FIXME: prefix.
> + use std::path::Component;
> + let has_trailing_slash = path
> + .as_os_str()
> + .as_encoded_bytes()
> + .last()
> + .copied()
> + .is_some_and(|c: u8| c == std::path::MAIN_SEPARATOR_STR.bytes().last().unwrap());
> + let mut new = PathBuf::new();
> + let iter = path.components();
> + for component in iter {
> + match component {
> + Component::RootDir => new.push(Component::RootDir),
> + Component::Prefix(p) => new.push(Component::Prefix(p)),
> + Component::CurDir => continue,
> + Component::ParentDir => {
> + new.pop();
> + }
> + Component::Normal(n) => new.push(n),
> + };
> + }
> + if has_trailing_slash {
> + new.push("");
> + }
> + new
> +}
> +
> #[rustfmt::skip]
> fn symbolic_mode(c: u64, special: bool, special_x: u8, special_no_x: u8) -> [u8; 3] {
> [
> diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
> index 2ada87bd..4ed23d6e 100644
> --- a/proxmox-backup-client/src/main.rs
> +++ b/proxmox-backup-client/src/main.rs
> @@ -826,9 +826,21 @@ async fn create_backup(
> let entry = entry
> .as_str()
> .ok_or_else(|| format_err!("Invalid pattern string slice"))?;
> + let entry_normalized = pbs_client::pxar::tools::normalize_lexically(entry);
> + if entry_normalized.as_os_str() != entry {
> + log::info!(
nit: better escalate this to a log::warn!
After all it has proven to behave unexpectedly to some users.
> + "Sanitized exclude pattern. Exclude patterns are relative to backup root, \
> + not the current working directory and should not contain '.' or '..' as path \
> + segments"
> + );
> + }
> pattern_list.push(
> - MatchEntry::parse_pattern(entry, PatternFlag::PATH_NAME, MatchType::Exclude)
> - .map_err(|err| format_err!("invalid exclude pattern entry: {}", err))?,
> + MatchEntry::parse_pattern(
> + entry_normalized.as_os_str().as_encoded_bytes(),
> + PatternFlag::PATH_NAME,
> + MatchType::Exclude,
> + )
> + .map_err(|err| format_err!("invalid exclude pattern entry: {}", err))?,
> );
> }
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH proxmox-backup] fix #5247: relative paths in exclude patterns.
2026-03-25 17:05 ` Christian Ebner
@ 2026-03-26 8:50 ` Manuel Federanko
0 siblings, 0 replies; 3+ messages in thread
From: Manuel Federanko @ 2026-03-26 8:50 UTC (permalink / raw)
To: Christian Ebner, pbs-devel
Hi!
thanks for your input, I will incorporate the changes in v2.
On 2026-03-25 6:04 PM, Christian Ebner wrote:
> Hi, thanks for the patch! A few high level comments, no full in detail
> review yet.
>
> On 3/23/26 4:31 PM, Manuel Federanko wrote:
>> Patterns which start with ./ or for that matter contain /../ or similar
>> constructs will never match, since they get compared to sanitized paths
>> from directory traversal, sanitize those patterns too.
>
> May also state that the user is now warned if such patterns are encountered.
>
>> Tested by creating a .pxarexclude file and via the --exclude command
>> line option.
>
> Would not include explicitly that you tested this in the commit message,
> it should rather state that this has effect for both cases. Don't get me
> wrong, it can make sense to include such information as well, e.g. as
> future references for more complex command invocations, just a bit to
> much for this trivial test case IMHO.
>
>>
>
> your comment from here ...
>
>> I'm unsure if we should accept those silently, currently I added a
>> warning for patterns supplied via the cli option, since that feels most
>> susceptible to wrong usage.
>>
>> It might also make sense to base the patterns for the cli arg on the
>> cwd, but that would be a breaking change.
>
> ... to here should not be part of the commit message (which will be
> applied), but rather be additional notes which go below the `---`
> separator (where it does not get applied). See marking below.
>
>>
>> Signed-off-by: Manuel Federanko <m.federanko@proxmox.com>
>> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5247
>> ---
>
> -> additional notes and comments here
>
>> pbs-client/src/pxar/create.rs | 7 ++++++-
>> pbs-client/src/pxar/tools.rs | 32 +++++++++++++++++++++++++++++++
>> proxmox-backup-client/src/main.rs | 16 ++++++++++++++--
>> 3 files changed, 52 insertions(+), 3 deletions(-)
>>
>> diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
>> index e42bcc87..1c878cb7 100644
>> --- a/pbs-client/src/pxar/create.rs
>> +++ b/pbs-client/src/pxar/create.rs
>> @@ -560,7 +560,12 @@ impl Archiver {
>> (line, MatchType::Exclude, false)
>> };
>>
>> - match MatchEntry::parse_pattern(line, PatternFlag::PATH_NAME, mode) {
>> + let line_normalized = crate::pxar::tools::normalize_lexically(OsStr::from_bytes(line));
>> + match MatchEntry::parse_pattern(
>> + line_normalized.as_path().as_os_str().as_bytes(),
>> + PatternFlag::PATH_NAME,
>> + mode,
>> + ) {
>> Ok(pattern) => {
>> if anchored {
>> self.patterns.push(pattern.add_flags(MatchFlag::ANCHORED));
>> diff --git a/pbs-client/src/pxar/tools.rs b/pbs-client/src/pxar/tools.rs
>> index 475c08ad..71a38b29 100644
>> --- a/pbs-client/src/pxar/tools.rs
>> +++ b/pbs-client/src/pxar/tools.rs
>> @@ -76,6 +76,38 @@ fn assert_single_path_component_do(path: &Path) -> Result<(), Error> {
>> Ok(())
>> }
>>
>> +pub fn normalize_lexically<S: AsRef<OsStr> + ?Sized>(path: &S) -> PathBuf {
>> + normalize_lexically_do(Path::new(path))
>
> comment: `path` could be wrapped here and the function in-lined:
>
> let path = Path::new(path);
> [...]
>
>
>> +}
>> +fn normalize_lexically_do(path: &Path) -> PathBuf {
>> + // Once std::path::normalize_lexically is stabilized we can switch
>> + // to that
>
> comment: This comment warrants a FIXME: prefix.
>
>> + use std::path::Component;
>> + let has_trailing_slash = path
>> + .as_os_str()
>> + .as_encoded_bytes()
>> + .last()
>> + .copied()
>> + .is_some_and(|c: u8| c == std::path::MAIN_SEPARATOR_STR.bytes().last().unwrap());
>> + let mut new = PathBuf::new();
>> + let iter = path.components();
>> + for component in iter {
>> + match component {
>> + Component::RootDir => new.push(Component::RootDir),
>> + Component::Prefix(p) => new.push(Component::Prefix(p)),
>> + Component::CurDir => continue,
>> + Component::ParentDir => {
>> + new.pop();
>> + }
>> + Component::Normal(n) => new.push(n),
>> + };
>> + }
>> + if has_trailing_slash {
>> + new.push("");
>> + }
>> + new
>> +}
>> +
>> #[rustfmt::skip]
>> fn symbolic_mode(c: u64, special: bool, special_x: u8, special_no_x: u8) -> [u8; 3] {
>> [
>> diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
>> index 2ada87bd..4ed23d6e 100644
>> --- a/proxmox-backup-client/src/main.rs
>> +++ b/proxmox-backup-client/src/main.rs
>> @@ -826,9 +826,21 @@ async fn create_backup(
>> let entry = entry
>> .as_str()
>> .ok_or_else(|| format_err!("Invalid pattern string slice"))?;
>> + let entry_normalized = pbs_client::pxar::tools::normalize_lexically(entry);
>> + if entry_normalized.as_os_str() != entry {
>> + log::info!(
>
> nit: better escalate this to a log::warn!
> After all it has proven to behave unexpectedly to some users.
>
>> + "Sanitized exclude pattern. Exclude patterns are relative to backup root, \
>> + not the current working directory and should not contain '.' or '..' as path \
>> + segments"
>> + );
>> + }
>> pattern_list.push(
>> - MatchEntry::parse_pattern(entry, PatternFlag::PATH_NAME, MatchType::Exclude)
>> - .map_err(|err| format_err!("invalid exclude pattern entry: {}", err))?,
>> + MatchEntry::parse_pattern(
>> + entry_normalized.as_os_str().as_encoded_bytes(),
>> + PatternFlag::PATH_NAME,
>> + MatchType::Exclude,
>> + )
>> + .map_err(|err| format_err!("invalid exclude pattern entry: {}", err))?,
>> );
>> }
>>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-03-26 8:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-03-23 15:32 [PATCH proxmox-backup] fix #5247: relative paths in exclude patterns Manuel Federanko
2026-03-25 17:05 ` Christian Ebner
2026-03-26 8:50 ` Manuel Federanko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox