[pve-devel] [PATCH access-control 2/2] fix #1499: check ACL path validity

[Thomas Lamprecht]

On 10/10/2017 03:58 PM, Philip Abernethy wrote:
> On Tue, Oct 10, 2017 at 03:53:45PM +0200, Thomas Lamprecht wrote:
>> On 10/10/2017 03:44 PM, Philip Abernethy wrote:
>>> Checks ACL paths for logical validity before application. Checks of
>>> the various IDs are done by the existing format checkers to avoid code
>>> duplication.
>>> Also introduces a distinction between malformed (syntactically
>>> incorrect) and invalid (syntactically correct, but contextually wrong)
>>> paths.
>>
>> Just for the record, this fixes 1500 not 1499:
>> https://bugzilla.proxmox.com/show_bug.cgi?id=1500
>>
>> 1500 is intended for the backend check (i.e., this fix) where 1499
>> was opened for the frontend UI.
> 
> Oops, true. I can submit a v2 if desired. However this probably fixes
> both bugs, as the resulting exception is displayed on the WebUI and CLI
> as well.

I'd only do so if other comments arise regarding the changes, else this
could be fixed up on applying.

To some extend, it fixes both. But for 1499 it had a usability improvement
in mind. There the user does not directly sees the options for possible
object paths. As you see in the RFE description I made proposals/wishes
about how it could be handled - but as its located in pve-manager and
would be ExtJS I separated it into two bugs, as for the one not used to
ExtJS (if that can be achieved at all) 1500 is much easier than 1499 -
and as it enforces valid paths in the API it's arguably the more important
fix.

>>
>> Thanks for tackling this, though!
>>
>>> ---
>>>  PVE/API2/ACL.pm      |  4 +++-
>>>  PVE/AccessControl.pm | 13 +++++++++++++
>>>  2 files changed, 16 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/PVE/API2/ACL.pm b/PVE/API2/ACL.pm
>>> index d37771b..20d3d2a 100644
>>> --- a/PVE/API2/ACL.pm
>>> +++ b/PVE/API2/ACL.pm
>>> @@ -132,7 +132,9 @@ __PACKAGE__->register_method ({
>>>  	}
>>>  
>>>  	my $path = PVE::AccessControl::normalize_path($param->{path});
>>> -	raise_param_exc({ path => "invalid ACL path '$param->{path}'" }) if !$path;
>>> +	raise_param_exc({ path => "malformed ACL path '$param->{path}'" }) if !$path;
>>> +	raise_param_exc({ path => "invalid ACL path '$param->{path}'" })
>>> +	    if !PVE::AccessControl::validate_path($path);
>>>  
>>>  	PVE::AccessControl::lock_user_config(
>>>  	    sub {
>>> diff --git a/PVE/AccessControl.pm b/PVE/AccessControl.pm
>>> index f0fb7dc..183bf21 100644
>>> --- a/PVE/AccessControl.pm
>>> +++ b/PVE/AccessControl.pm
>>> @@ -591,6 +591,19 @@ sub verify_privname {
>>>      return $priv;
>>>  }
>>>  
>>> +sub validate_path {
>>> +    my $path = shift;
>>> +    return 0 if $path !~ m'^/(vms|nodes|storage|pool|access/(?:groups|realms))(?:/([[:alnum:]\.\-\_]+))?$';
>>> +
>>> +    if ($1 eq 'vms') {PVE::JSONSchema::pve_verify_vmid($2) if $2;}
>>> +    elsif ($1 eq 'nodes') {PVE::JSONSchema::pve_verify_node_name($2) if $2;}
>>> +    elsif ($1 eq 'storage') {PVE::JSONSchema::parse_storage_id($2) if $2;}
>>> +    elsif ($1 eq 'pool') {verify_poolname($2) if $2;}
>>> +    elsif ($1 eq 'access/realms') {PVE::Auth::Plugin::pve_verify_realm($2) if $2;}
>>> +
>>> +    return 1;
>>> +}
>>> +
>>>  sub userconfig_force_defaults {
>>>      my ($cfg) = @_;
>>>  
>>>
>>
>>
>> _______________________________________________
>> 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
>