On Fri, May 12, 2023 at 02:23:48PM +0200, Dominik Csapak wrote:
> only three small things were missing for it to work:
> * on the cli, we have to get the option as an array if the type is an array
> * the untainting must be done recursively, otherwise, the regex matching
> converts an array hash into the string 'ARRAY(0x123412341234)'
> * JSONSchema::parse_config did not handle array formats specially, but
> we want to allow to specify them multiple time
>
> Signed-off-by: Dominik Csapak <d.csa...@proxmox.com>
> ---
> src/PVE/JSONSchema.pm | 12 ++++++++++++
> src/PVE/RESTHandler.pm | 41 ++++++++++++++++++++++++++++++++++-------
> 2 files changed, 46 insertions(+), 7 deletions(-)
>
> diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
> index 527e409..526fc2b 100644
> --- a/src/PVE/JSONSchema.pm
> +++ b/src/PVE/JSONSchema.pm
> @@ -1709,6 +1709,8 @@ sub get_options {
> } else {
> if ($pd->{format} && $pd->{format} =~ m/-a?list/) {
> push @getopt, "$prop=s@";
> + } elsif ($pd->{type} eq 'array') {
> + push @getopt, "$prop=s@";
> } else {
> push @getopt, "$prop=s";
> }
> @@ -1869,6 +1871,16 @@ sub parse_config : prototype($$$;$) {
>
> $value = parse_boolean($value) // $value;
> }
> + if ($schema->{properties}->{$key} &&
> + $schema->{properties}->{$key}->{type} eq 'array') {
> +
> + if (defined($cfg->{$key})) {
> + push $cfg->{$key}->@*, $value;
> + } else {
> + $cfg->{$key} = [$value];
> + }
> + next;
> + }
> $cfg->{$key} = $value;
> } else {
> warn "ignore config line: $line\n"
> diff --git a/src/PVE/RESTHandler.pm b/src/PVE/RESTHandler.pm
> index 944a04b..20714a5 100644
> --- a/src/PVE/RESTHandler.pm
> +++ b/src/PVE/RESTHandler.pm
> @@ -426,6 +426,38 @@ sub find_handler {
> return ($handler_class, $method_info);
> }
>
> +my $untaint_recursive;
> +
> +$untaint_recursive = sub {
> + my ($param) = @_;
> +
> + my $ref = ref $param;
> + if ($ref eq 'HASH') {
> + while (my ($key, $val) = each %$param) {
> + my $newval = $untaint_recursive->($val);
> + if (defined($newval)) {
if defined, assign untainted value
> + ($param->{$key}) = $newval;
> + } else {
> + $param->{$key} = undef;
if ==undef, assign undef
> + }
> + }
So this entire case could just be:
$param->{$_} = $untaint_recursive->($param->{$_) for keys %$param;
can it not?
However - here, we're explicitly assigning `undef`s, but in the array
case below...
> + } elsif ($ref eq 'ARRAY') {
> + my $newparam = [];
> + for my $val (@$param) {
> + my $newval = $untaint_recursive->($val);
> + push @$newparam, $newval if defined($newval);
...here we're skipping undef.
I'd like comments explaining the cases in within this sub.
Further, the 'hash' case replaces the existing hash, while the array
case produces a _new_ array, which could make an actual difference for
the toplevel caller of this method.
The caller below expects a return value for a hash, discarding the
original, so this might be fine, but I'd prefer to either stay
consistent, or a have a big fat side effect warning comment on top of
the sub.
> + }
> + $param = $newparam;
> + } else {
> + if (defined($param)) {
> + my ($newval) = $param =~ /^(.*)$/s;
> + $param = $newval;
> + }
> + }
> +
> + return $param;
> +};
> +
> sub handle {
> my ($self, $info, $param, $result_verification) = @_;
>
> @@ -437,16 +469,11 @@ sub handle {
>
> if (my $schema = $info->{parameters}) {
> # warn "validate ". Dumper($param}) . "\n" . Dumper($schema);
> +
> PVE::JSONSchema::validate($param, $schema);
> # untaint data (already validated)
> my $extra = delete $param->{'extra-args'};
> - while (my ($key, $val) = each %$param) {
> - if (defined($val)) {
> - ($param->{$key}) = $val =~ /^(.*)$/s;
> - } else {
> - $param->{$key} = undef;
> - }
> - }
> + $param = $untaint_recursive->($param);
And after this, I think the line below can be dropped?
> $param->{'extra-args'} = [map { /^(.*)$/ } @$extra] if $extra;
> }
>
> --
> 2.30.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
