Re: [pbs-devel] [PATCH proxmox 06/12] sys: crypt: use constant time comparison for password verification

["Stefan Sterz" <s.sterz@proxmox.com>]

On Mon Feb 19, 2024 at 5:11 PM CET, Max Carrara wrote:
> 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.
>

i'd like to keep the `proxmox-sys` and `proxmox-auth-api` commits
seperate. imo this makes the git history a bit "cleaner".

> LGTM otherwise, but see the comment inline.
>

-- >8 snip 8< --

> > +    // `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?
>

see my response to your comment in patch 04. we don't want to give an
attacker more info than we have to imo.

-- >8 snip 8< --