fix: use constant-time comparisons for admin auth checks#8389
Conversation
kleysc
force-pushed
the
fix/constant-time-auth-comparisons
branch
2 times, most recently
from
cec0b80 to
7a96a61
Compare
| // Option<ApiAuth>` | ||
| /// Authentication uses the hashed user password in PHC format | ||
| /// Authentication secret used to verify guardian admin API requests | ||
| #[derive(Clone, PartialEq, Eq, Serialize, Deserialize)] |
There was a problem hiding this comment.
I guess we should remove Eq then?
There was a problem hiding this comment.
Removed both PartialEq and Eq from the derive
| /// Authentication uses the hashed user password in PHC format | ||
| /// Authentication secret used to verify guardian admin API requests | ||
| #[derive(Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| pub struct ApiAuth(pub String); |
There was a problem hiding this comment.
We should not make a String pub, I guess.
There was a problem hiding this comment.
Made it private. All access now goes through as_str().
There was a problem hiding this comment.
Well, the point was to never even access it in any way given its sensitive. We should really only need to compare it with a password (in const time) and that's about it. If there's any other operation that needs to touch it should also be a method. The goal is to make introducing same problem accidentally hard via type system.
This should probably be accompanied with a docstring comment about this goal.
There was a problem hiding this comment.
Added a doc comment on ApiAuth covering this
| input: LoginInput, | ||
| ) -> impl IntoResponse { | ||
| if auth.0 == input.password { | ||
| if bool::from(auth.0.as_bytes().ct_eq(input.password.as_bytes())) { |
There was a problem hiding this comment.
Added ApiAuth::verify() — wraps subtle::ConstantTimeEq internally
dpc
left a comment
dpc
left a comment
There was a problem hiding this comment.
Good find. But we should make sure this will not regress and make the String private, I think.
kleysc
force-pushed
the
fix/constant-time-auth-comparisons
branch
from
7a96a61 to
b1edf5f
Compare
field is private and |
Make the ApiAuth field private and drop PartialEq/Eq so raw == on passwords can no longer compile. All authentication checks now go through ApiAuth::verify(), which uses constant-time equality internally. Adds new() and as_str() accessors for the remaining call sites.
kleysc
force-pushed
the
fix/constant-time-auth-comparisons
branch
from
b1edf5f to
717a5fd
Compare
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
3 participants
Summary
Admin auth in
consensus/api.rs(auth gate andchange_password),config/setup.rs, and the server UI (lib.rslogin handler anddashboard/mod.rspassword change) was using plain==/!=for password comparison, which is vulnerable to timing side-channel attacks.Replaced with
subtle::ConstantTimeEq— same pattern already used inhttp_auth.rsforFM_FORCE_API_SECRETS.Also updates two doc comments that referenced PHC format — passwords are stored as plaintext in
password.private.Partial fix for #8277 — Argon2 hashing and storage refactor in a follow-up
Testing
just checkpasses cleanly. The change is mechanical,ct_eqis a drop-in replacement for==with no behavioral difference