From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 7FFE0AE7C for ; Tue, 8 Aug 2023 12:26:19 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 61CA08333 for ; Tue, 8 Aug 2023 12:25:49 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 8 Aug 2023 12:25:48 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 6CA3F43711 for ; Tue, 8 Aug 2023 12:25:48 +0200 (CEST) Date: Tue, 8 Aug 2023 12:25:47 +0200 From: Wolfgang Bumiller To: Gabriel Goller Cc: pbs-devel@lists.proxmox.com Message-ID: References: <20230804100225.95770-1-g.goller@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230804100225.95770-1-g.goller@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.108 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [create.rs] Subject: Re: [pbs-devel] [PATCH proxmox-backup] fix #4380: check permissions before entering directory X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 08 Aug 2023 10:26:19 -0000 NAK. You can never assume that the permission bits apply to your user, and if they do, there can be ACLs and security-module rules (Apparmor, SELinux, ...) involved and a lot more. Permission checks are much more complicated, and checking them manually is *always* wrong. Handling an `EPERM`/`EACCESS` error is the only correct way. (The closest you'd get would be with the `eaccess()` syscall, but that's also unnecessarily racy, and an extra call for something you just don't need...) See my reply to the other patch about how I'd tackle this. Anyway, further code comments down below. On Fri, Aug 04, 2023 at 12:02:25PM +0200, Gabriel Goller wrote: > When creating a backup, we now check if we have the correct permissions > (r,x) before entering a directory. This is mainly to prevent stat() from > failing with EACCESS errors. We also check if the directory contains > non-excluded files and warn the user. > > Signed-off-by: Gabriel Goller > --- > pbs-client/src/pxar/create.rs | 47 +++++++++++++++++++++++++++++++++-- > 1 file changed, 45 insertions(+), 2 deletions(-) > > diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs > index 2577cf98..f2333284 100644 > --- a/pbs-client/src/pxar/create.rs > +++ b/pbs-client/src/pxar/create.rs > @@ -638,7 +638,7 @@ impl Archiver { > async fn add_directory( > &mut self, > encoder: &mut Encoder<'_, T>, > - dir: Dir, > + mut dir: Dir, > dir_name: &CStr, > metadata: &Metadata, > stat: &FileStat, > @@ -663,9 +663,52 @@ impl Archiver { > skip_contents = !set.contains(&stat.st_dev); > } > } > + if skip_contents { > + log::warn!("Skipping mount point: {:?}", self.path); > + } > + > + let mode = nix::sys::stat::Mode::from_bits_truncate(stat.st_mode); > + // if we have read and write permissions on the folder ^ wrong comment? > + if (!mode.contains(Mode::S_IRUSR) || !mode.contains(Mode::S_IXUSR)) ^ if you look at the bitflags docs' description of `.contains()` vs `.intersects()` you'll see that `.contains()` requires all the bits in question to be set. You're asking "(does not contain A) or (does not contain B)" => which means: "not (both are set)" => `!mode.contains(Mode::S_IRUSR | Mode::S_IXUSR)` should be a shorter version of the same, no? :-) > + && skip_contents == false > + { > + skip_contents = true; > + let mut contains_non_excluded_files = false; > + if mode.contains(Mode::S_IRUSR) { > + // check if all children are excluded > + for file in dir.iter() { > + let file = file?; > + > + let file_name = file.file_name().to_owned(); So this bit is copied - but the original could use some cleanup ;-) `to_owned()` allocates and is not necessary for the `.to_bytes()` call and we don't actually need it owned anywhere up until the end > + let file_name_bytes = file_name.to_bytes(); > + if file_name_bytes == b"." || file_name_bytes == b".." { > + continue; > + } > + let os_file_name = OsStr::from_bytes(file_name_bytes); > + assert_single_path_component(os_file_name)?; > + let full_path = self.path.join(os_file_name); > + let match_path = PathBuf::from("/").join(full_path.clone()); > + if self > + .patterns > + .matches(match_path.as_os_str().as_bytes(), Some(stat.st_mode)) > + != Some(MatchType::Exclude) > + { > + contains_non_excluded_files = true; > + break; > + } > + } > + } > + if contains_non_excluded_files { > + log::warn!( > + "Skipping directory: {:?}, access denied (contains non-excluded files)", ^ that is a weird error message which sounds like the existence of files is the reason the access is denied ;-) > + self.path > + ); > + } else { > + log::warn!("Skipping directory: {:?}, access denied", self.path); > + } > + } > > let result = if skip_contents { > - log::info!("skipping mount point: {:?}", self.path); > Ok(()) > } else { > self.archive_dir_contents(&mut encoder, dir, false).await > -- > 2.39.2