From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id E027D1FF13B for ; Wed, 25 Mar 2026 18:05:27 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 6E67931B2D; Wed, 25 Mar 2026 18:05:49 +0100 (CET) Message-ID: <21976801-5b8e-4690-b1a7-b8d769e445cf@proxmox.com> Date: Wed, 25 Mar 2026 18:05:13 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH proxmox-backup] fix #5247: relative paths in exclude patterns. To: Manuel Federanko , pbs-devel@lists.proxmox.com References: <20260323153202.76874-1-m.federanko@proxmox.com> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: <20260323153202.76874-1-m.federanko@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1774458265938 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.061 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: P2V4EW4XWZE422ZFGQTU5L3TQT7AI6SM X-Message-ID-Hash: P2V4EW4XWZE422ZFGQTU5L3TQT7AI6SM X-MailFrom: c.ebner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 > 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 + ?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))?, > ); > } >