On Wed, Jun 23, 2021 at 02:14:48PM +0200, Fabian Grünbichler wrote:
> On June 22, 2021 2:18 pm, Wolfgang Bumiller wrote:
> > Signed-off-by: Wolfgang Bumiller <[email protected]>
(...)
> > +sub volume_export {
> > + my (
> > + $class,
> > + $scfg,
> > + $storeid,
> > + $fh,
> > + $volname,
> > + $format,
> > + $snapshot,
> > + $base_snapshot,
> > + $with_snapshots,
> > + ) = @_;
> > +
> > + if ($format ne 'btrfs') {
> > + return PVE::Storage::Plugin::volume_export(@_);
> > + }
> > +
> > + die "format 'btrfs' only works on snapshots\n"
> > + if !defined $snapshot;
> > +
> > + die "'btrfs' format in incremental mode requires snapshots to be
> > listed explicitly\n"
> > + if defined($base_snapshot) && $with_snapshots && ref($with_snapshots)
> > ne 'ARRAY';
> > +
> > + my $volume_format = ($class->parse_volname($volname))[6];
> > +
> > + die "btrfs-sending volumes of type $volume_format ('$volname') is not
> > supported\n"
> > + if $volume_format ne 'raw' && $volume_format ne 'subvol';
>
> the three checks here are the same as in volume_export, maybe they could
> go into a single export-helper?
we get nicer errors this way, though (and more easily placed in the code
when a user runs into them, since we don't use Carp ;-) )
(...)
> > +
> > + # Now go through the remaining snapshots (if any)
> > + foreach my $snap (@snapshots) {
> > + $class->btrfs_cmd(['property', 'set', "$tmppath/$diskname\@$snap",
> > 'ro', 'false']);
> > + PVE::Tools::renameat2(
> > + -1,
> > + "$tmppath/$diskname\@$snap",
> > + -1,
> > + "$destination\@$snap",
> > + &PVE::Tools::RENAME_NOREPLACE,
> > + ) or die "failed to move received snapshot
> > '$tmppath/$diskname\@$snap'"
> > + . " into place at '$destination\@$snap' - $!\n";
> > + eval { $class->btrfs_cmd(['property', 'set', "$destination\@$snap",
> > 'ro', 'true']) };
> > + warn "failed to make $destination\@$snap read-only - $!\n" if $@;
> > + }
> > + };
> > + my $err = $@;
> > +
> > + eval {
> > + # Cleanup all the received snapshots we did not move into place, so we
> > can remove the temp
> > + # directory.
> > + if ($dh) {
> > + $dh->rewind;
> > + while (defined(my $entry = $dh->read)) {
> > + next if $entry eq '.' || $entry eq '..';
> > + eval { $class->btrfs_cmd(['subvolume', 'delete', '--',
> > "$tmppath/$entry"]) };
> > + warn $@ if $@;
> > + }
> > + $dh->close; undef $dh;
> > + }
> > + if (!rmdir($tmppath)) {
> > + warn "failed to remove temporary directory '$tmppath' - $!\n"
> > + }
> > + };
> > + warn $@ if $@;
> > + if ($err) {
> > + # clean up if the directory ended up being empty after an error
> > + rmdir($tmppath);
> > + die $err;
> > + }
>
> I am a bit wary about this (also w.r.t. future btrfs features) - a
> privileged container can do quite a lot of stuff with btrfs
>
> - create snapshots from within container
> - set default subvolume
> - set properties
> - ... (the above is what I found in a few minutes without much BTRFS
> experience)
>
> unprivileged containers can do the following
> - create new subvols
> - create new snapshots
> - set ro property
>
> (same caveat of not looking too much at what else is possible, lots of
> stuff does not work)
>
> how do we guarantee that the send stream is not affected in a dangerous
> fashion by an attacker inside the container?
Shouldn't be dangerous sine it's not recursive anyway.
However, we could just by default disable the corresponding ioctls via
seccomp for unprivileged containers...
_______________________________________________
pve-devel mailing list
[email protected]
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
