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

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

[Gabriel Goller]

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
+        if (!mode.contains(Mode::S_IRUSR) || !mode.contains(Mode::S_IXUSR))
+            && 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();
+                    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)",
+                    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

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

[Thomas Lamprecht]

just some higher-level process review:

please use a v2 tag if it's the second version, both git format-patch and
git send-email support adding that conveniently using -v2 (or -v3 and so
one if a later revision).

Am 04/08/2023 um 12:02 schrieb Gabriel Goller:
> 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>
> ---

please add a series changelog here, i.e., outside of what gets added to git
but easily discoverable. E.g., like:

changes v1:
- ...


See https://pve.proxmox.com/wiki/Developer_Documentation#Preparing_Patches

and btw., as I observed it in a reply to Fiona in the v1 of this fix: please
ensure you use "reply all" when replying, so that the mailing list stays
included.

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

[Wolfgang Bumiller]

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

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

[Gabriel Goller]

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 <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