[pve-devel] [PATCH common] run_command: untaint end of buffer

[pve-devel] [PATCH common] run_command: untaint end of buffer

[Stoiko Ivanov]

The performance improvements added in
cb9db10c1a9855cf40ff13e81f9dd97d6a9b2698 changed the output handling
to not remove the taintedness (see perlsec (1)) of the complete output
anymore.

This results in a few bugs which show up every now and then, and are
usually quite tedious to hunt down - usually they only occur when
run via GUI (since pveproxy/pvedaemon run in taint-mode, whereas our
CLI utilities usually do not) - see for example pve-storage commit
609f117ff24d2cff6b155e1d4b1175ceebe5bd7b

or more recently the report in our community-forum:
https://forum.proxmox.com/threads/cannot-clone-vm-or-move-disk-with-more-than-13-snapshots.89628/
(the magic number of 13 snapshots is the point where the output of
`qemu-img info --output=json` grows to more than 4k)

Given the doubtful security benefit of having to untaint output, only
if it does not end in new-line or if it is longer than 4k (we read
the output in 4k chunks) simply untaint the output unconditionally.

Tested with the reproducer from the forum-thread

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
Huge thanks to Dominik who pointed me in the right direction, after I
spend far more time than I'm proud to admit in the JSON::XS source,
perlsec(1), perlguts(1) and the surprisingly unhelpful output of
debugperl -Du

 src/PVE/Tools.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index 8946e93..eb140ae 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -516,7 +516,7 @@ sub run_command {
 				&$outfunc($line) if $outfunc;
 				&$logfunc($line) if $logfunc;
 			    }
-			    $outlog .= $buf;
+			    ($outlog) .= ($buf =~ /(.*)/); #untaint
 			};
 			my $err = $@;
 			if ($err) {
@@ -537,7 +537,7 @@ sub run_command {
 				&$errfunc($line) if $errfunc;
 				&$logfunc($line) if $logfunc;
 			    }
-			    $errlog .= $buf;
+			    ($errlog) .= ($buf =~ /(.*)/); #untaint
 			};
 			my $err = $@;
 			if ($err) {
-- 
2.20.1

Re: [pve-devel] [PATCH common] run_command: untaint end of buffer

[Thomas Lamprecht]

On 22.06.21 16:28, Stoiko Ivanov wrote:
> The performance improvements added in
> cb9db10c1a9855cf40ff13e81f9dd97d6a9b2698 changed the output handling
> to not remove the taintedness (see perlsec (1)) of the complete output
> anymore.
> 
> This results in a few bugs which show up every now and then, and are
> usually quite tedious to hunt down - usually they only occur when
> run via GUI (since pveproxy/pvedaemon run in taint-mode, whereas our
> CLI utilities usually do not) - see for example pve-storage commit
> 609f117ff24d2cff6b155e1d4b1175ceebe5bd7b
> 
> or more recently the report in our community-forum:
> https://forum.proxmox.com/threads/cannot-clone-vm-or-move-disk-with-more-than-13-snapshots.89628/
> (the magic number of 13 snapshots is the point where the output of
> `qemu-img info --output=json` grows to more than 4k)
> 
> Given the doubtful security benefit of having to untaint output, only
> if it does not end in new-line or if it is longer than 4k (we read
> the output in 4k chunks) simply untaint the output unconditionally.
> 
> Tested with the reproducer from the forum-thread
> 

this long message nowhere talks about what the actual issue is, can we rather
fix that in the clone call?

> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
> Huge thanks to Dominik who pointed me in the right direction, after I
> spend far more time than I'm proud to admit in the JSON::XS source,
> perlsec(1), perlguts(1) and the surprisingly unhelpful output of
> debugperl -Du
> 
>  src/PVE/Tools.pm | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
> index 8946e93..eb140ae 100644
> --- a/src/PVE/Tools.pm
> +++ b/src/PVE/Tools.pm
> @@ -516,7 +516,7 @@ sub run_command {
>  				&$outfunc($line) if $outfunc;
>  				&$logfunc($line) if $logfunc;
>  			    }
> -			    $outlog .= $buf;
> +			    ($outlog) .= ($buf =~ /(.*)/); #untaint
>  			};
>  			my $err = $@;
>  			if ($err) {
> @@ -537,7 +537,7 @@ sub run_command {
>  				&$errfunc($line) if $errfunc;
>  				&$logfunc($line) if $logfunc;
>  			    }
> -			    $errlog .= $buf;
> +			    ($errlog) .= ($buf =~ /(.*)/); #untaint
>  			};
>  			my $err = $@;
>  			if ($err) {
> 

Re: [pve-devel] [PATCH common] run_command: untaint end of buffer

[Stoiko Ivanov]

On Tue, 22 Jun 2021 16:41:50 +0200
Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:

> On 22.06.21 16:28, Stoiko Ivanov wrote:
> > The performance improvements added in
> > cb9db10c1a9855cf40ff13e81f9dd97d6a9b2698 changed the output handling
> > to not remove the taintedness (see perlsec (1)) of the complete output
> > anymore.
> > 
> > This results in a few bugs which show up every now and then, and are
> > usually quite tedious to hunt down - usually they only occur when
> > run via GUI (since pveproxy/pvedaemon run in taint-mode, whereas our
> > CLI utilities usually do not) - see for example pve-storage commit
> > 609f117ff24d2cff6b155e1d4b1175ceebe5bd7b
> > 
> > or more recently the report in our community-forum:
> > https://forum.proxmox.com/threads/cannot-clone-vm-or-move-disk-with-more-than-13-snapshots.89628/
> > (the magic number of 13 snapshots is the point where the output of
> > `qemu-img info --output=json` grows to more than 4k)
> > 
> > Given the doubtful security benefit of having to untaint output, only
> > if it does not end in new-line or if it is longer than 4k (we read
> > the output in 4k chunks) simply untaint the output unconditionally.
> > 
> > Tested with the reproducer from the forum-thread
> >   
> 
> this long message nowhere talks about what the actual issue is, can we rather
> fix that in the clone call?
You're right - sorry- the call chain leading to the error is
move_vm_disk (PVE::API2::Qemu) -> PVE::QemuServer::clone_disk ->
PVE::Storage::volume_size_info -> PVE::Storage::Plugin::volume_size_info
(since it's a qcow2 image on a directory storage) -> file_size_info,
which calls qemu-img info via run_command. The return of file_size_info
is then used for PVE::Storage::vdisk_alloc ->
PVE::Storage::Plugin::alloc_image -> qemu-img create via run_command.

I had a patch for untainting the individual values in
PVE::Storage::Plugin::volume_size_info but then went with this patch,
since I expect the issue of output not ending in newline or being longer
than 4k to linger in a few places in our code.

For the volume_size_info calls of our storage plugins - a quick check says
only PBSPlugin.pm and Plugin.pm could cause this issue (LVMPlugin returns
a single value terminated with newline, RBDPlugin untaints the complete
json before decode_json, ZFSPoolPlugin and ISCSIDirectPlugin untaint the
return as well).



> 
> > Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> > ---
> > Huge thanks to Dominik who pointed me in the right direction, after I
> > spend far more time than I'm proud to admit in the JSON::XS source,
> > perlsec(1), perlguts(1) and the surprisingly unhelpful output of
> > debugperl -Du
> > 
> >  src/PVE/Tools.pm | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
> > index 8946e93..eb140ae 100644
> > --- a/src/PVE/Tools.pm
> > +++ b/src/PVE/Tools.pm
> > @@ -516,7 +516,7 @@ sub run_command {
> >  				&$outfunc($line) if $outfunc;
> >  				&$logfunc($line) if $logfunc;
> >  			    }
> > -			    $outlog .= $buf;
> > +			    ($outlog) .= ($buf =~ /(.*)/); #untaint
> >  			};
> >  			my $err = $@;
> >  			if ($err) {
> > @@ -537,7 +537,7 @@ sub run_command {
> >  				&$errfunc($line) if $errfunc;
> >  				&$logfunc($line) if $logfunc;
> >  			    }
> > -			    $errlog .= $buf;
> > +			    ($errlog) .= ($buf =~ /(.*)/); #untaint
> >  			};
> >  			my $err = $@;
> >  			if ($err) {
> >   
> 

Re: [pve-devel] [PATCH common] run_command: untaint end of buffer

[Thomas Lamprecht]

On 22.06.21 17:10, Stoiko Ivanov wrote:
> I had a patch for untainting the individual values in
> PVE::Storage::Plugin::volume_size_info but then went with this patch,

I'd rather have that patch, especially for back-porting to stable.
I mean, else we can probably just turn of the taint mode completely, what's the
point then.

> since I expect the issue of output not ending in newline or being longer
> than 4k to linger in a few places in our code.
> 
> For the volume_size_info calls of our storage plugins - a quick check says
> only PBSPlugin.pm and Plugin.pm could cause this issue 

can we patch it there then too?

Re: [pve-devel] [PATCH common] run_command: untaint end of buffer

[Stoiko Ivanov]

On Tue, 22 Jun 2021 17:15:08 +0200
Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:

> On 22.06.21 17:10, Stoiko Ivanov wrote:
> > I had a patch for untainting the individual values in
> > PVE::Storage::Plugin::volume_size_info but then went with this patch,  
> 
> I'd rather have that patch, especially for back-porting to stable.
Makes sense - sent the patch for pve-storage

> I mean, else we can probably just turn of the taint mode completely, what's the
> point then.
I'm always a bit (too) cautious when it comes to turning of 'security'
related 'features' (even if mostly doubting that taint-mode fits either of
those 2 categories) - so not sure about disabling it in general

the taint of the some of the run_command output on the other hand was
introduced as a side-effect with the changes last year afaict, and has
caused at least 2 glitches since then...



> 
> > since I expect the issue of output not ending in newline or being longer
> > than 4k to linger in a few places in our code.
> > 
> > For the volume_size_info calls of our storage plugins - a quick check says
> > only PBSPlugin.pm and Plugin.pm could cause this issue   
> 
> can we patch it there then too?

Re: [pve-devel] [PATCH common] run_command: untaint end of buffer

[Thomas Lamprecht]

On 22.06.21 18:52, Stoiko Ivanov wrote:
> On Tue, 22 Jun 2021 17:15:08 +0200
> Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:
> 
>> On 22.06.21 17:10, Stoiko Ivanov wrote:
>>> I had a patch for untainting the individual values in
>>> PVE::Storage::Plugin::volume_size_info but then went with this patch,  
>>
>> I'd rather have that patch, especially for back-porting to stable.
> Makes sense - sent the patch for pve-storage
> 
>> I mean, else we can probably just turn of the taint mode completely, what's the
>> point then.
> I'm always a bit (too) cautious when it comes to turning of 'security'
> related 'features' (even if mostly doubting that taint-mode fits either of
> those 2 categories) - so not sure about disabling it in general
> 
> the taint of the some of the run_command output on the other hand was
> introduced as a side-effect with the changes last year afaict, and has

it really wasn't, it gave no guarantees and some callers did not checked for
it, some floated up then, if we just blindly untainted anything it just has
no benefit to run under taint mode, especially as we want to move over as much
as possible to run_command anyway.

Rather than just band-aiding it somewhere in the middle with a catch all regex that
*completely* defeats the purpose of the concept of tainting, it can be better to
either just disable or fix the few places where it's actual wrong with a local
decision about how closely we can restrict the untainting, sometimes a match-all is
all it can realistically be there, but not always.

> caused at least 2 glitches since then...
> 

which is really not much, and the whole "fool me once, ..." should make it easier
to spot any remaining one ;-P