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)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id EED1BB9B0 for ; Thu, 10 Aug 2023 09:20:49 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id CFB871D5B4 for ; Thu, 10 Aug 2023 09:20: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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Thu, 10 Aug 2023 09:20: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 916CE44746 for ; Thu, 10 Aug 2023 09:20:48 +0200 (CEST) Message-ID: Date: Thu, 10 Aug 2023 09:20:47 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.1 To: Wolfgang Bumiller Cc: pbs-devel@lists.proxmox.com References: <20230804100225.95770-1-g.goller@proxmox.com> Content-Language: en-US From: Gabriel Goller In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 1.862 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 NICE_REPLY_A -4.14 Looks like a legit reply (A) 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: Thu, 10 Aug 2023 07:20:50 -0000 Thanks for the feedback, submitted a new patch (v3). On 8/8/23 12:25, Wolfgang Bumiller wrote: > 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