Re: [pbs-devel] [PATCH proxmox-backup] fix #4380: check permissions before entering directory

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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 <g.goller@proxmox.com>
> ---
>  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<T: SeqWrite + Send>(
>          &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