Re: [pbs-devel] [PATCH proxmox-backup 2/2] fix #3853: tape cli: add force flag to key change-passphrase

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Mon, Feb 07, 2022 at 01:48:25PM +0100, Stefan Sterz wrote:
> Adds the '--force' flag to the proxmox-tape command allowing users
> with root privileges to overwrite the passphrase of a given key.
> 
> Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
> ---
>  src/bin/proxmox_tape/encryption_key.rs | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/src/bin/proxmox_tape/encryption_key.rs b/src/bin/proxmox_tape/encryption_key.rs
> index 156295fd..3dd9b58b 100644
> --- a/src/bin/proxmox_tape/encryption_key.rs
> +++ b/src/bin/proxmox_tape/encryption_key.rs
> @@ -146,11 +146,18 @@ fn show_key(
>              hint: {
>                  schema: PASSWORD_HINT_SCHEMA,
>              },
> +            force: {
> +                optional: true,
> +                type: bool,
> +                description: "Don't ask for the old passphrase and overwrite it. Root only.",
> +                default: false,
> +            },
>          },
>      },
>  )]
>  /// Change the encryption key's password.
>  fn change_passphrase(
> +    force: bool,
>      mut param: Value,
>      rpcenv: &mut dyn RpcEnvironment,
>  ) -> Result<(), Error> {
> @@ -159,11 +166,23 @@ fn change_passphrase(
>          bail!("unable to change passphrase - no tty");
>      }
>  
> -    let password = tty::read_password("Current Tape Encryption Key Password: ")?;
> +    let effective_uid = nix::unistd::Uid::effective();
> +    let running_uid = nix::unistd::Uid::current();
> +
> +    // if the `--force` flag is provided check if we are root
> +    if force && !effective_uid.is_root() && !running_uid.is_root() {
> +        bail!("the force flag requires root privileges");
> +    }

^ Wrong place for this.

Also, as a general note, checks such as this one should simply not be
done by CLI tools unless they're actually installed as `setuid-root`
since it's the OS' job to do permission checks.

I may be running an unprivileged root (dropped capabilities, entered
some restrictive AppArmor label), or I may actually be a *privileged*
non-zero-uid for some reason as well...