[pve-devel] NACK: [RFC container] mountpoints: create parent dirs with correct owner
Fabian Grünbichler
f.gruenbichler at proxmox.com
Wed Jul 24 13:54:28 CEST 2019
On Wed, Jul 24, 2019 at 01:46:42PM +0200, Fabian Grünbichler wrote:
> disregard this, will send a v2 shortly that fixes an edge case.
seems like that edge case behaviour was not caused by this patch, sorry
for the noise.
> On Wed, Jul 24, 2019 at 01:37:13PM +0200, Fabian Grünbichler wrote:
> > otherwise unprivileged containers might end up with directories that
> > they cannot modify since they are owned by the user root in the host
> > namespace, instead of root inside the container.
> >
> > note: the problematic behaviour is only exhibited when an intermediate
> > directory needs to be created, e.g. a mountpoint /test/mp gets mounted,
> > and /test does not yet exist.
> >
> > Signed-off-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
> > ---
> > Notes:
> > requires fchownat support in PVE::Tools - see other patch and bump
> > build-depends + depends accordingly after applying!
> >
> > I am not sure whether this is 100% correct w.r.t. error edge cases, since we
> > potentially die after mkdirat without calling fchownat. it is for sure better
> > than the status quo though ;)
> >
> > thank you Dietmar for noticing the buggy behaviour!
> >
> > src/PVE/LXC.pm | 27 ++++++++++++++++-----------
> > src/PVE/VZDump/LXC.pm | 7 +++++--
> > src/lxc-pve-prestart-hook | 4 +++-
> > 3 files changed, 24 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> > index 2252685..0a1d95a 100644
> > --- a/src/PVE/LXC.pm
> > +++ b/src/PVE/LXC.pm
> > @@ -1177,13 +1177,15 @@ sub mount_all {
> > my $volid_list = PVE::LXC::Config->get_vm_volumes($conf);
> > PVE::Storage::activate_volumes($storage_cfg, $volid_list);
> >
> > + my (undef, $rootuid, $rootgid) = parse_id_maps($conf);
> > +
> > eval {
> > PVE::LXC::Config->foreach_mountpoint($conf, sub {
> > my ($ms, $mountpoint) = @_;
> >
> > $mountpoint->{ro} = 0 if $ignore_ro;
> >
> > - mountpoint_mount($mountpoint, $rootdir, $storage_cfg);
> > + mountpoint_mount($mountpoint, $rootdir, $storage_cfg, undef, $rootuid, $rootgid);
> > });
> > };
> > if (my $err = $@) {
> > @@ -1258,8 +1260,8 @@ sub run_with_loopdev {
> > # * the 2nd deepest directory (parent of the above)
> > # * directory name of the last directory
> > # So that the path $2/$3 should lead to $1 afterwards.
> > -sub walk_tree_nofollow($$$) {
> > - my ($start, $subdir, $mkdir) = @_;
> > +sub walk_tree_nofollow($$$;$$) {
> > + my ($start, $subdir, $mkdir, $rootuid, $rootgid) = @_;
> >
> > # splitdir() returns '' for empty components including the leading /
> > my @comps = grep { length($_)>0 } File::Spec->splitdir($subdir);
> > @@ -1288,6 +1290,9 @@ sub walk_tree_nofollow($$$) {
> >
> > $next = PVE::Tools::openat(fileno($fd), $component, O_NOFOLLOW | O_DIRECTORY);
> > die "failed to create path: $dir: $!\n" if !$next;
> > +
> > + PVE::Tools::fchownat(fileno($next), '', $rootuid, $rootgid, PVE::Tools::AT_EMPTY_PATH)
> > + if defined($rootuid) && defined($rootgid);
> > }
> >
> > close $second if defined($last_component);
> > @@ -1381,17 +1386,17 @@ sub bindmount {
> > # from $rootdir and $mount and walk the path from $rootdir to the final
> > # directory to check for symlinks.
> > sub __mount_prepare_rootdir {
> > - my ($rootdir, $mount) = @_;
> > + my ($rootdir, $mount, $rootuid, $rootgid) = @_;
> > $rootdir =~ s!/+!/!g;
> > $rootdir =~ s!/+$!!;
> > my $mount_path = "$rootdir/$mount";
> > - my ($mpfd, $parentfd, $last_dir) = walk_tree_nofollow($rootdir, $mount, 1);
> > + my ($mpfd, $parentfd, $last_dir) = walk_tree_nofollow($rootdir, $mount, 1, $rootuid, $rootgid);
> > return ($rootdir, $mount_path, $mpfd, $parentfd, $last_dir);
> > }
> >
> > # use $rootdir = undef to just return the corresponding mount path
> > sub mountpoint_mount {
> > - my ($mountpoint, $rootdir, $storage_cfg, $snapname) = @_;
> > + my ($mountpoint, $rootdir, $storage_cfg, $snapname, $rootuid, $rootgid) = @_;
> >
> > my $volid = $mountpoint->{volume};
> > my $mount = $mountpoint->{mp};
> > @@ -1408,7 +1413,7 @@ sub mountpoint_mount {
> >
> > if (defined($rootdir)) {
> > ($rootdir, $mount_path, $mpfd, $parentfd, $last_dir) =
> > - __mount_prepare_rootdir($rootdir, $mount);
> > + __mount_prepare_rootdir($rootdir, $mount, $rootuid, $rootgid);
> > }
> >
> > my ($storage, $volname) = PVE::Storage::parse_volume_id($volid, 1);
> > @@ -2007,7 +2012,7 @@ sub run_unshared {
> > }
> >
> > my $copy_volume = sub {
> > - my ($src_volid, $src, $dst_volid, $dest, $storage_cfg, $snapname, $bwlimit) = @_;
> > + my ($src_volid, $src, $dst_volid, $dest, $storage_cfg, $snapname, $bwlimit, $rootuid, $rootgid) = @_;
> >
> > my $src_mp = { volume => $src_volid, mp => '/', ro => 1 };
> > $src_mp->{type} = PVE::LXC::Config->classify_mountpoint($src_volid);
> > @@ -2019,10 +2024,10 @@ my $copy_volume = sub {
> > eval {
> > # mount and copy
> > mkdir $src;
> > - mountpoint_mount($src_mp, $src, $storage_cfg, $snapname);
> > + mountpoint_mount($src_mp, $src, $storage_cfg, $snapname, $rootuid, $rootgid);
> > push @mounted, $src;
> > mkdir $dest;
> > - mountpoint_mount($dst_mp, $dest, $storage_cfg);
> > + mountpoint_mount($dst_mp, $dest, $storage_cfg, undef, $rootuid, $rootgid);
> > push @mounted, $dest;
> >
> > $bwlimit //= 0;
> > @@ -2078,7 +2083,7 @@ sub copy_volume {
> > }
> >
> > run_unshared(sub {
> > - $copy_volume->($mp->{volume}, $src, $new_volid, $dest, $storage_cfg, $snapname, $bwlimit);
> > + $copy_volume->($mp->{volume}, $src, $new_volid, $dest, $storage_cfg, $snapname, $bwlimit, $rootuid, $rootgid);
> > });
> > };
> > if (my $err = $@) {
> > diff --git a/src/PVE/VZDump/LXC.pm b/src/PVE/VZDump/LXC.pm
> > index ae793dc..befb370 100644
> > --- a/src/PVE/VZDump/LXC.pm
> > +++ b/src/PVE/VZDump/LXC.pm
> > @@ -114,6 +114,9 @@ sub prepare {
> >
> > my ($id_map, $rootuid, $rootgid) = PVE::LXC::parse_id_maps($conf);
> > $task->{userns_cmd} = PVE::LXC::userns_command($id_map);
> > + $task->{rootuid} = $rootuid;
> > + $task->{rootgid} = $rootgid;
> > +
> >
> > my $volids = $task->{volids} = [];
> > PVE::LXC::Config->foreach_mountpoint($conf, sub {
> > @@ -219,7 +222,7 @@ sub snapshot {
> > PVE::Storage::activate_volumes($storage_cfg, $volids, 'vzdump');
> > foreach my $disk (@$disks) {
> > $disk->{dir} = "${rootdir}$disk->{mp}";
> > - PVE::LXC::mountpoint_mount($disk, $rootdir, $storage_cfg, 'vzdump');
> > + PVE::LXC::mountpoint_mount($disk, $rootdir, $storage_cfg, 'vzdump', $task->{rootuid}, $task->{rootgid});
> > }
> >
> > $task->{snapdir} = $rootdir;
> > @@ -306,7 +309,7 @@ sub archive {
> > my $storage_cfg = $self->{storecfg};
> > foreach my $disk (@$disks) {
> > $disk->{dir} = "${rootdir}$disk->{mp}";
> > - PVE::LXC::mountpoint_mount($disk, $rootdir, $storage_cfg);
> > + PVE::LXC::mountpoint_mount($disk, $rootdir, $storage_cfg, undef, $task->{rootuid}, $task->{rootgid});
> > # add every enabled mountpoint (since we use --one-file-system)
> > # mp already starts with a / so we only need to add the dot
> > push @sources, ".$disk->{mp}";
> > diff --git a/src/lxc-pve-prestart-hook b/src/lxc-pve-prestart-hook
> > index 3ff8d79..18b60cf 100755
> > --- a/src/lxc-pve-prestart-hook
> > +++ b/src/lxc-pve-prestart-hook
> > @@ -85,11 +85,13 @@ __PACKAGE__->register_method ({
> > unlink $devlist_file;
> > my $devices = [];
> >
> > + my (undef, $rootuid, $rootgid) = PVE::LXC::parse_id_maps($conf);
> > +
> > my $setup_mountpoint = sub {
> > my ($ms, $mountpoint) = @_;
> >
> > #return if $ms eq 'rootfs';
> > - my (undef, undef, $dev) = PVE::LXC::mountpoint_mount($mountpoint, $rootdir, $storage_cfg);
> > + my (undef, undef, $dev) = PVE::LXC::mountpoint_mount($mountpoint, $rootdir, $storage_cfg, undef, $rootuid, $rootgid);
> > push @$devices, $dev if $dev && $mountpoint->{quota};
> > };
> >
> > --
> > 2.20.1
> >
> >
> > _______________________________________________
> > pve-devel mailing list
> > pve-devel at pve.proxmox.com
> > https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
More information about the pve-devel
mailing list