From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 19FBA69848 for ; Wed, 3 Mar 2021 08:07:19 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0F92C32F18 for ; Wed, 3 Mar 2021 08:07:19 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 5B4B532F0A for ; Wed, 3 Mar 2021 08:07:18 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 26A3641D72 for ; Wed, 3 Mar 2021 08:07:18 +0100 (CET) Message-ID: Date: Wed, 3 Mar 2021 08:07:17 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:87.0) Gecko/20100101 Thunderbird/87.0 Content-Language: en-US From: Thomas Lamprecht To: Proxmox Backup Server development discussion , Dominik Csapak Reply-To: Proxmox Backup Server development discussion References: <20210302153120.31213-1-d.csapak@proxmox.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.053 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [rest.rs] Subject: Re: [pbs-devel] [RFC PATCH proxmox-backup] server/rest: disallow non-protected api calls in privileged environment X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 Mar 2021 07:07:19 -0000 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 >> --- >> 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 >> }; >>