[pve-devel] [PATCH manager 2/4] api: new parameter for each shell to pass custom command string

[Tim Marx]

> Fabian Grünbichler <f.gruenbichler at proxmox.com> hat am 15. Januar 2019 um 13:53 geschrieben:
> 
> 
> On Thu, Jan 10, 2019 at 01:54:31PM +0100, Tim Marx wrote:
> > Signed-off-by: Tim Marx <t.marx at proxmox.com>
> > ---
> >  PVE/API2/Nodes.pm | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
> > index dd5471f8..c1f73cd0 100644
> > --- a/PVE/API2/Nodes.pm
> > +++ b/PVE/API2/Nodes.pm
> > @@ -658,6 +658,11 @@ __PACKAGE__->register_method ({
> >  		optional => 1,
> >  		default => 0,
> >  	    },
> > +	    cmd => {
> > +		type => 'string',
> > +		description => "Run command instead of normal shell.",
> > +		optional => 1,
> 
> I'd prefer an enum/whitelist approach here, which would also allow us to
> deprecate the 'upgrade' parameter in favor of this.
> 
> e.g.
> 
> enum => ['upgrade', 'ceph']
> 
> this makes the interface way less confusing (it's clear that all these
> are mutually exclusive, even if we add seven additional commands) and
> does not open up the possibility of directly executing arbitrary user
> provided commands via a forged request in case we ever have a bug of
> that sort.
> 

I agree, that's a cleaner way, wasn't very happy with that anyway.
Regarding executing a user provided command, yeah makes it probably easier to attack, but without it the point remains the same it's just a little bit harder to accomplish IMHO, but as I like the enum approach better we do not need to consider that anymore.


> > +	    },
> >  	    websocket => {
> >  		optional => 1,
> >  		type => 'boolean',
> > @@ -731,6 +736,8 @@ __PACKAGE__->register_method ({
> >  		my $upgradecmd = "pveupgrade --shell";
> >  		$upgradecmd = PVE::Tools::shellquote($upgradecmd) if $remip;
> >  		$shcmd = [ '/bin/bash', '-c', $upgradecmd ];
> > +	    } elsif ($param->{cmd}) {
> > +		push(@$shcmd, split(' ',$param->{cmd}));
> 
> this is wrong - what about spaces inside quoted strings? the whitelist
> approach avoids this altogether since we control the commands that each
> whitelist entry gets mapped to.

you are right, this only works for my use case and not as generic as someone would assume. Won't consider it anymore, because of the new approach.

> 
> >  	    } else {
> >  		$shcmd = [ '/bin/login', '-f', 'root' ];
> >  	    }
> > @@ -817,6 +824,11 @@ __PACKAGE__->register_method ({
> >  		optional => 1,
> >  		default => 0,
> >  	    },
> > +	    cmd => {
> > +		type => 'string',
> > +		description => "Run command instead of normal shell.",
> > +		optional => 1,
> 
> as above - and since this parameter gets used more than once, it
> probably makes sense to define a 'shellcmd' standard option somewhere
> once and reuse it for all these API paths.
> 
> (same would also apply to the 'upgrade' parameter that already exists)
> 

Makes sense.

> > +	    },
> >  	},
> >      },
> >      returns => {
> > @@ -861,6 +873,8 @@ __PACKAGE__->register_method ({
> >  	if ($user eq 'root at pam') {
> >  	    if ($param->{upgrade}) {
> >  		$concmd = [ '/usr/bin/pveupgrade', '--shell' ];
> > +	    } elsif ($param->{cmd}) {
> > +		@$concmd = split(' ',$param->{cmd});
> 
> as above
> 
> >  	    } else {
> >  		$concmd = [ '/bin/login', '-f', 'root' ];
> >  	    }
> > @@ -964,6 +978,11 @@ __PACKAGE__->register_method ({
> >  		optional => 1,
> >  		default => 0,
> >  	    },
> > +	    cmd => {
> > +		type => 'string',
> > +		description => "Run command instead of normal shell.",
> > +		optional => 1,
> 
> as above
> 
> > +	    },
> >  	},
> >      },
> >      returns => get_standard_option('remote-viewer-config'),
> > @@ -991,6 +1010,8 @@ __PACKAGE__->register_method ({
> >  	    if ($param->{upgrade}) {
> >  		my $upgradecmd = "pveupgrade --shell";
> >  		$shcmd = [ '/bin/bash', '-c', $upgradecmd ];
> > +	    } elsif ($param->{cmd}) {
> > +		@$shcmd = split(' ',$param->{cmd});
> 
> as above
> 
> >  	    } else {
> >  		$shcmd = [ '/bin/login', '-f', 'root' ];
> >  	    }
> > -- 
> > 2.11.0
> > 
> > _______________________________________________
> > pve-devel mailing list
> > pve-devel at pve.proxmox.com
> > https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


Thanks for the feedback!