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

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

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).