* [pbs-devel] [RFC PATCH proxmox-backup] server/rest: disallow non-protected api calls in privileged environment @ 2021-03-02 15:31 Dominik Csapak 2021-03-02 17:02 ` Thomas Lamprecht 0 siblings, 1 reply; 5+ messages in thread From: Dominik Csapak @ 2021-03-02 15:31 UTC (permalink / raw) To: pbs-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pbs-devel] [RFC PATCH proxmox-backup] server/rest: disallow non-protected api calls in privileged environment 2021-03-02 15:31 [pbs-devel] [RFC PATCH proxmox-backup] server/rest: disallow non-protected api calls in privileged environment Dominik Csapak @ 2021-03-02 17:02 ` Thomas Lamprecht 2021-03-03 7:07 ` Thomas Lamprecht 0 siblings, 1 reply; 5+ messages in thread From: Thomas Lamprecht @ 2021-03-02 17:02 UTC (permalink / raw) To: Proxmox Backup Server development discussion, Dominik Csapak 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 > }; > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pbs-devel] [RFC PATCH proxmox-backup] server/rest: disallow non-protected api calls in privileged environment 2021-03-02 17:02 ` Thomas Lamprecht @ 2021-03-03 7:07 ` Thomas Lamprecht 2021-03-03 7:27 ` Dominik Csapak 0 siblings, 1 reply; 5+ messages in thread From: Thomas Lamprecht @ 2021-03-03 7:07 UTC (permalink / raw) To: Proxmox Backup Server development discussion, Dominik Csapak 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 >> }; >> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pbs-devel] [RFC PATCH proxmox-backup] server/rest: disallow non-protected api calls in privileged environment 2021-03-03 7:07 ` Thomas Lamprecht @ 2021-03-03 7:27 ` Dominik Csapak 2021-03-03 8:22 ` Thomas Lamprecht 0 siblings, 1 reply; 5+ messages in thread From: Dominik Csapak @ 2021-03-03 7:27 UTC (permalink / raw) To: Proxmox Backup Server development discussion 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 :) ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pbs-devel] [RFC PATCH proxmox-backup] server/rest: disallow non-protected api calls in privileged environment 2021-03-03 7:27 ` Dominik Csapak @ 2021-03-03 8:22 ` Thomas Lamprecht 0 siblings, 0 replies; 5+ messages in thread From: Thomas Lamprecht @ 2021-03-03 8:22 UTC (permalink / raw) To: Proxmox Backup Server development discussion, Dominik Csapak 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). ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-03-03 8:23 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-02 15:31 [pbs-devel] [RFC PATCH proxmox-backup] server/rest: disallow non-protected api calls in privileged environment Dominik Csapak 2021-03-02 17:02 ` Thomas Lamprecht 2021-03-03 7:07 ` Thomas Lamprecht 2021-03-03 7:27 ` Dominik Csapak 2021-03-03 8:22 ` Thomas Lamprecht
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox