Re: [pbs-devel] [RFC PATCH proxmox-backup] server/rest: disallow non-protected api calls in privileged environment

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 02.03.21 18:02, Thomas Lamprecht wrote:
> On 02.03.21 16:31, Dominik Csapak wrote:
>> to prevent potential abuse of non-protected api calls as root
>>
> 
> this breaks important CLI tools using client::connect_to_localhost
> i.e., proxmox-backup-manager and proxmox-tape and maybe others which
> connect still manually.
> 
 
Ok, this is not true, I had in mind that we directly connect to :82, like
we did for pvesh way in the past.

>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>> this is a rather theoretical security improvement, i am not sure if we
>> want this? it would only guard against an unprotected api call that somehow
> 
> no, such stuff only tends to break things while not providing any value...
> lets keep theoretical security improvements also theoretical..
> 
>> allows code execution. this could then be abused to connect to the
>> daemon and reabuse the same api call, but with root permissions
> 
> with magically generating a ticket and circumventing permission checks
> how exactly?
> 

Security wise I find this still nonsense, its way too constructed with no
single practical possible example state, and it effectively requires to have
a free-choose binary path or control of $PATH from the environment of that
process (if that is given you have other problems) plus local access to the
machine and a entry in PBS user config would be required.

But, one thing this could help with is the issue that we sometimes had that
doing creating a config file as privileged user got us the wrong permissions,
making it inaccessible for the unprivileged code, which was a bug but not
always immediately found, we have all cases covered with chown+checks, IIRC,
but if a new config came in this could help detection (albeit such things
are quite visible, normally)


>>
>> also if we want this, maybe this would be good to have in pve too?
> 
> no
> 
> 
>>
>>  src/server/rest.rs | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/src/server/rest.rs b/src/server/rest.rs
>> index 9bf494fd..6b170b7f 100644
>> --- a/src/server/rest.rs
>> +++ b/src/server/rest.rs
>> @@ -750,6 +750,9 @@ async fn handle_request(
>>  
>>                      let result = if api_method.protected && env_type == RpcEnvironmentType::PUBLIC {
>>                          proxy_protected_request(api_method, parts, body, peer).await
>> +                    } else if !api_method.protected && env_type == RpcEnvironmentType::PRIVILEGED {
>> +                        let err = http_err!(FORBIDDEN, "invalid server request");
>> +                        return Ok((formatter.format_error)(err));
>>                      } else {
>>                          handle_api_request(rpcenv, api_method, formatter, parts, body, uri_param).await
>>                      };
>>