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 958ED69923 for ; Wed, 3 Mar 2021 09:23:10 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 7C286338BC for ; Wed, 3 Mar 2021 09:22:40 +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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 37DE7338B0 for ; Wed, 3 Mar 2021 09:22:39 +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 ED79D41D53 for ; Wed, 3 Mar 2021 09:22:38 +0100 (CET) Message-ID: Date: Wed, 3 Mar 2021 09:22:36 +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 To: Proxmox Backup Server development discussion , Dominik Csapak References: <20210302153120.31213-1-d.csapak@proxmox.com> From: Thomas Lamprecht In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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 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 08:23:10 -0000 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. >>> >> =C2=A0 Ok, this is not true, I had in mind that we directly connect to=20 :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 val= ue... >>> 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 check= s >>> how exactly? >>> >> >> Security wise I find this still nonsense, its way too constructed with=20 no >> single practical possible example state, and it effectively requires t= o 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 t= o 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=20 that >> doing creating a config file as privileged user got us the wrong permi= ssions, >> making it inaccessible for the unprivileged code, which was a bug but = not >> always immediately found, we have all cases covered with chown+checks,=20 IIRC, >> but if a new config came in this could help detection (albeit such thi= ngs >> are quite visible, normally) >> >=20 > yeah as i admitted, the vector is rather theoretical, but just maybe to=20 explain better: >=20 > * i have access to an non-protected api call '/foo' > =C2=A0(if thats unauthenticated or not does not matter) > * that api call has a code execution vuln > =C2=A0(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 > =C2=A0api call with the same vuln again -> exec as root But your patch is not really a fix for that unlikely vector, it just redu= ces 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 bette= r 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 thos= e, 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 wh= en the unpriv. daemon does an exec. * remove any perl call to system or `cmd` and ensure we use arrays for ar= guments for the run_command ones * Adding the (not complete for demonstration purpose) snippet below to th= e respective service units [Service] ProtectSystem=3Dtrue ProtectHome=3Dtrue InaccessiblePaths=3D/bin /usr/bin /sbin /usr/sbin What also would work is to set a new private root and only bind mount the=20 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 d= id not miss anything, at least).