From: Max Carrara <m.carrara@proxmox.com>
To: pbs-devel@lists.proxmox.com
Subject: Re: [pbs-devel] [PATCH proxmox 06/12] sys: crypt: use constant time comparison for password verification
Date: Mon, 19 Feb 2024 17:11:09 +0100 [thread overview]
Message-ID: <29e05069-547e-46a5-9ed2-befbfbe0e4b5@proxmox.com> (raw)
In-Reply-To: <20240215152001.269490-7-s.sterz@proxmox.com>
On 2/15/24 16:19, Stefan Sterz wrote:
> by using `openssl::memcmp::eq()` we can avoid potential timing side
> channels as its runtime only depends on the length of the arrays, not
> the contents. this requires the two arrays to have the same length, but
> that should be a given since the hashes should always have the same
> length.
>
> Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
See my reply to patch 04 - the usage of `openssl::memcmp::eq()` in the legacy
code block there could be merged with this commit first before moving to / implementing
HMAC.
LGTM otherwise, but see the comment inline.
> ---
> proxmox-sys/Cargo.toml | 3 ++-
> proxmox-sys/src/crypt.rs | 8 +++++++-
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/proxmox-sys/Cargo.toml b/proxmox-sys/Cargo.toml
> index 5ddbe21..1a44702 100644
> --- a/proxmox-sys/Cargo.toml
> +++ b/proxmox-sys/Cargo.toml
> @@ -16,6 +16,7 @@ lazy_static.workspace = true
> libc.workspace = true
> log.workspace = true
> nix.workspace = true
> +openssl = { workspace = true, optional = true }
> regex.workspace = true
> serde_json.workspace = true
> serde = { workspace = true, features = [ "derive" ] }
> @@ -29,5 +30,5 @@ proxmox-time.workspace = true
> default = []
> logrotate = ["dep:zstd"]
> acl = []
> -crypt = []
> +crypt = ["dep:openssl"]
> timer = []
> diff --git a/proxmox-sys/src/crypt.rs b/proxmox-sys/src/crypt.rs
> index fa10911..3313f66 100644
> --- a/proxmox-sys/src/crypt.rs
> +++ b/proxmox-sys/src/crypt.rs
> @@ -155,9 +155,15 @@ pub fn encrypt_pw(password: &str) -> Result<String, Error> {
> /// Verify if an encrypted password matches
> pub fn verify_crypt_pw(password: &str, enc_password: &str) -> Result<(), Error> {
> let verify = crypt(password.as_bytes(), enc_password.as_bytes())?;
> - if verify != enc_password {
> +
> + // `openssl::memcmp::eq()`'s runtime does not depend on the content of the arrays only the
> + // length, this makes it harder to exploit timing side-channels.
> + if verify.len() != enc_password.len()
> + || !openssl::memcmp::eq(verify.as_bytes(), enc_password.as_bytes())
Like in my comment on patch 04, would it make sense here to split these checks into two
for more fine-grained error messaging? Or are there any reasons why they should be together?
> + {
> bail!("invalid credentials");
> }
> +
> Ok(())
> }
>
next prev parent reply other threads:[~2024-02-19 16:11 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-15 15:19 [pbs-devel] [PATCH proxmox{, -backup} 00/12] authentication cleanup and Stefan Sterz
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 01/12] auth-api: move signing into the private key Stefan Sterz
2024-02-26 20:22 ` Esi Y
2024-02-27 9:12 ` Stefan Sterz
2024-02-27 18:13 ` Esi Y
2024-02-29 16:07 ` Stefan Sterz
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 02/12] auth-api: move to Ed25519 signatures Stefan Sterz
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 03/12] auth-api: add ability to use hmac singing in keyring Stefan Sterz
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 04/12] auth-api: move to hmac signing for csrf tokens Stefan Sterz
2024-02-19 16:02 ` Max Carrara
2024-02-20 12:54 ` Max Carrara
2024-02-23 9:26 ` Stefan Sterz
2024-02-23 10:48 ` Thomas Lamprecht
2024-02-23 10:52 ` Stefan Sterz
2024-02-23 13:06 ` Wolfgang Bumiller
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 05/12] sys: crypt: move to yescrypt for password hashing Stefan Sterz
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 06/12] sys: crypt: use constant time comparison for password verification Stefan Sterz
2024-02-19 16:11 ` Max Carrara [this message]
2024-02-23 9:26 ` Stefan Sterz
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 07/12] sys: crypt: add helper to allow upgrading hashes Stefan Sterz
2024-02-19 18:50 ` Max Carrara
2024-02-23 9:26 ` Stefan Sterz
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 08/12] auth-api: fix types `compilefail` test Stefan Sterz
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox-backup 09/12] auth: move to hmac keys for csrf tokens Stefan Sterz
2024-02-19 18:55 ` Max Carrara
2024-02-23 9:26 ` Stefan Sterz
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox-backup 10/12] auth: upgrade hashes on user log in Stefan Sterz
2024-02-19 18:58 ` Max Carrara
2024-02-23 9:26 ` Stefan Sterz
2024-02-15 15:20 ` [pbs-devel] [PATCH proxmox-backup 11/12] auth/manager: add manager command to upgrade hashes Stefan Sterz
2024-02-19 19:06 ` Max Carrara
2024-02-23 9:26 ` Stefan Sterz
2024-02-15 15:20 ` [pbs-devel] [PATCH proxmox-backup 12/12] auth: us ec keys as auth keys Stefan Sterz
2024-02-19 19:10 ` Max Carrara
2024-02-23 9:26 ` Stefan Sterz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=29e05069-547e-46a5-9ed2-befbfbe0e4b5@proxmox.com \
--to=m.carrara@proxmox.com \
--cc=pbs-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox