[pve-devel] [PATCH container] fix #3960: properly set owner in CT setup

[pve-devel] [PATCH container] fix #3960: properly set owner in CT setup

[Fabian Grünbichler]

there were two helpers that were not handling this correctly:

ct_make_path

since this never gets called with $opts, and there also is no 'owner'
and 'group' in $self, the previous logic could never work, sometimes
leaving nobody:nogroup files around for unprivileged containers.

since only the centos and suse plugins use this helper, the issue was
fairly limited.

ct_symlink

could create symlinks owned by nobody:nogroup. since symlinks are
created 777 by default, this just meant they were not modifiable inside
the container, but reading/dereferencing was no problem so it went
unnoticed so far.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    instead of POSIX::lchown we could also expose AT_SYMLINK_NOFOLLOW in PVE::Tools
    and call fchownat with that, but it would require a versioned dep bump..

 src/PVE/LXC/Setup/Base.pm | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/src/PVE/LXC/Setup/Base.pm b/src/PVE/LXC/Setup/Base.pm
index dafd69a..7c93dfb 100644
--- a/src/PVE/LXC/Setup/Base.pm
+++ b/src/PVE/LXC/Setup/Base.pm
@@ -12,6 +12,7 @@ use Fcntl;
 use File::Path;
 use File::Spec;
 use File::Basename;
+use POSIX ();
 
 use PVE::INotify;
 use PVE::Tools;
@@ -663,22 +664,26 @@ sub ct_open_file_write {
 
 sub ct_make_path {
     my $self = shift;
-    if ($self->{id_map}) {
-	my $opts = pop;
-	if (ref($opts) eq 'HASH') {
-	    $opts->{owner} = $self->{rootuid} if !defined($self->{owner});
-	    $opts->{group} = $self->{rootgid} if !defined($self->{group});
-	}
-	File::Path::make_path(@_, $opts);
-    } else {
-	File::Path::make_path(@_);
+
+    my $opts = {};
+    if (defined($self->{id_map})) {
+	$opts->{owner} = $self->{rootuid};
+	$opts->{group} = $self->{rootgid};
     }
+    File::Path::make_path(@_, $opts);
 }
 
 sub ct_symlink {
     my ($self, $old, $new) = @_;
     return if $self->ct_is_file_ignored($new);
-    return CORE::symlink($old, $new);
+    if (CORE::symlink($old, $new)) {
+	if (defined($self->{id_map})) {
+	    POSIX::lchown($self->{rootuid}, $self->{rootgid}, $new);
+	}
+	return 1;
+    } else {
+	return 0;
+    }
 }
 
 sub ct_readlink {
-- 
2.30.2

[pve-devel] applied: [PATCH container] fix #3960: properly set owner in CT setup

[Thomas Lamprecht]

On 28.03.22 10:06, Fabian Grünbichler wrote:
> there were two helpers that were not handling this correctly:
> 
> ct_make_path
> 
> since this never gets called with $opts, and there also is no 'owner'
> and 'group' in $self, the previous logic could never work, sometimes
> leaving nobody:nogroup files around for unprivileged containers.
> 
> since only the centos and suse plugins use this helper, the issue was
> fairly limited.
> 
> ct_symlink
> 
> could create symlinks owned by nobody:nogroup. since symlinks are
> created 777 by default, this just meant they were not modifiable inside
> the container, but reading/dereferencing was no problem so it went
> unnoticed so far.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> 
> Notes:
>     instead of POSIX::lchown we could also expose AT_SYMLINK_NOFOLLOW in PVE::Tools
>     and call fchownat with that, but it would require a versioned dep bump..

we can still do that in the future with this patch applied if its really deemed
an improved, and be it just to make it More Nice Code™.

> 
>  src/PVE/LXC/Setup/Base.pm | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
>

applied, thanks!