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

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

[Dominik Csapak]

to prevent potential abuse of non-protected api calls as root

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
allows code execution. this could then be abused to connect to the
daemon and reabuse the same api call, but with root permissions

also if we want this, maybe this would be good to have in pve too?

 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
                     };
-- 
2.20.1

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

[Thomas Lamprecht]

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.

> 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?

> 
> 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
>                      };
> 

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

[Thomas Lamprecht]

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
>>                      };
>>

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

[Dominik Csapak]

On 3/3/21 08:07, Thomas Lamprecht wrote:
> 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)
> 

yeah as i admitted, the vector is rather theoretical, but just maybe to 
explain better:

* i have access to an non-protected api call '/foo'
   (if thats unauthenticated or not does not matter)
* that api call has a code execution vuln
   (e.g. in perl system('foo $param')
* now i can execute code as backup user
* with that i can now connect to localhost:82 and reuse the same
   api call with the same vuln again -> exec as root

but yes, rather constructed scenario...
thanks anyway for looking and commenting :)

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

[Thomas Lamprecht]

On 03.03.21 08:27, Dominik Csapak wrote:
> On 3/3/21 08:07, Thomas Lamprecht wrote:
>> 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)
>>
> 
> yeah as i admitted, the vector is rather theoretical, but just maybe to 
explain better:
> 
> * i have access to an non-protected api call '/foo'
>  (if thats unauthenticated or not does not matter)
> * that api call has a code execution vuln
>  (e.g. in perl system('foo $param')
> * now i can execute code as backup user
> * with that i can now connect to localhost:82 and reuse the same
>  api call with the same vuln again -> exec as root

But your patch is not really a fix for that unlikely vector, it just reduces one
single further possibility, there a probably so many still leftm DOS and hijacking
wise, that this is just a drop in the bucket, IMO in such cases its better to ensure
we never allow an PBS user to get command injection.

So I rather would have (whish list to santa):
* tooling for registering any API call which runs commands and track those,
  it shouldn't be a big number, a few dozen at max, so allows auditing.
  Could be even combined with seccomp to query the list of OK commands when the
  unpriv. daemon does an exec.
* remove any perl call to system or `cmd` and ensure we use arrays for arguments
  for the run_command ones
* Adding the (not complete for demonstration purpose) snippet below to the
  respective service units

[Service]
ProtectSystem=true
ProtectHome=true
InaccessiblePaths=/bin /usr/bin /sbin /usr/sbin


What also would work is to set a new private root and only bind mount the 
datastore
directories and /etc/proxmox-backup.

Those all have also the potential for some subtle breakage, but at least they protect
against whole classes, e.g., disarming any command line injection (if I did not
miss anything, at least).