Fix self-review regressions: throttle SASL SCRAM, key game target by UID, exclude keycard from password lockout
Some checks failed
CI / check (push) Has been cancelled

This commit is contained in:
Jean Chevronnet 2026-07-19 14:38:16 +00:00
parent 4599c4834d
commit c36341668c
No known key found for this signature in database
4 changed files with 28 additions and 10 deletions

View file

@ -243,8 +243,10 @@ pub enum AuthThen {
// NickServ IDENTIFY: `uid` logs in, `agent` (NickServ uid) sends the notices, // NickServ IDENTIFY: `uid` logs in, `agent` (NickServ uid) sends the notices,
// `name` is what the user typed (for lockout/note_auth), `account` is canonical. // `name` is what the user typed (for lockout/note_auth), `account` is canonical.
Identify { uid: String, agent: String, name: String, account: String }, Identify { uid: String, agent: String, name: String, account: String },
// SASL PLAIN: finish the SASL exchange for `client`, sourced from `agent`. // SASL: finish the exchange for `client`, sourced from `agent`. `password` is
Sasl { agent: String, client: String, account: String }, // whether this was a password verify (feeds the brute-force throttle) vs a
// one-time keycard redemption (which must not touch the password lockout).
Sasl { agent: String, client: String, account: String, password: bool },
} }
/// The ircd link layer. The engine only ever sees [`NetEvent`] / [`NetAction`]; /// The ircd link layer. The engine only ever sees [`NetEvent`] / [`NetAction`];

View file

@ -165,7 +165,9 @@ impl GameServ {
gt, gt,
meid, meid,
from.account.is_some(), from.account.is_some(),
target_acc.clone().unwrap_or_else(|| target.to_string()), // Key the target by the same identity ident() uses (account, else UID),
// or a guest target's acc_b (their nick) never matches their later meid.
target_acc.clone().unwrap_or_else(|| tuid.clone()),
target_acc.is_some(), target_acc.is_some(),
from.nick.to_string(), from.nick.to_string(),
target.to_string(), target.to_string(),

View file

@ -340,10 +340,13 @@ impl Engine {
actions.extend(feed); actions.extend(feed);
actions actions
} }
AuthThen::Sasl { agent, client, account } => { AuthThen::Sasl { agent, client, account, password } => {
// Feed the same brute-force throttle IDENTIFY uses: success clears it, // Feed the same brute-force throttle IDENTIFY uses (success clears it,
// a failure grows the backoff — so SASL isn't a throttle-free door. // failure grows the backoff) — but ONLY for a password verify; a failed
// one-time keycard redemption must not lock out the account's password.
if password {
self.db.note_auth(&account, ok); self.db.note_auth(&account, ok);
}
if ok { if ok {
self.sasl_login("SASL PLAIN", &agent, &client, account) self.sasl_login("SASL PLAIN", &agent, &client, account)
} else { } else {

View file

@ -74,7 +74,7 @@ impl Engine {
vec![NetAction::DeferKeycard { vec![NetAction::DeferKeycard {
token: passwd, token: passwd,
account: account.clone(), account: account.clone(),
then: AuthThen::Sasl { agent: agent.clone(), client: client.clone(), account }, then: AuthThen::Sasl { agent: agent.clone(), client: client.clone(), account, password: false },
}] }]
} }
Some((authcid, passwd)) => match self.scram_verifier(&authcid) { Some((authcid, passwd)) => match self.scram_verifier(&authcid) {
@ -88,7 +88,7 @@ impl Engine {
Some((account, verifier)) => vec![NetAction::DeferAuthenticate { Some((account, verifier)) => vec![NetAction::DeferAuthenticate {
verifier, verifier,
password: passwd, password: passwd,
then: AuthThen::Sasl { agent: agent.clone(), client: client.clone(), account }, then: AuthThen::Sasl { agent: agent.clone(), client: client.clone(), account, password: true },
}], }],
None => { None => {
let mut out = mk("D", vec!["F".to_string()]); let mut out = mk("D", vec!["F".to_string()]);
@ -134,6 +134,11 @@ impl Engine {
let (account, Some(verifier)) = (account.to_string(), scram::parse_verifier(verifier)) else { let (account, Some(verifier)) = (account.to_string(), scram::parse_verifier(verifier)) else {
return fail(); return fail();
}; };
// Refuse while throttled, so SCRAM (Orbit's default mech) can't be used
// to brute-force past the IDENTIFY/PLAIN lockout.
if self.db.auth_lockout(&account).is_some() {
return fail();
}
let (server_first, nonce) = scram::server_first(&cf.cnonce, &verifier); let (server_first, nonce) = scram::server_first(&cf.cnonce, &verifier);
let out = challenge(server_first.clone()); let out = challenge(server_first.clone());
self.stash_sasl(client.to_string(), SaslSession::Scram { self.stash_sasl(client.to_string(), SaslSession::Scram {
@ -153,11 +158,17 @@ impl Engine {
let Some(msg) = decode(chunk) else { return fail() }; let Some(msg) = decode(chunk) else { return fail() };
match scram::verify_final(hash, &verifier, &client_first_bare, &server_first, &gs2_header, &nonce, &msg) { match scram::verify_final(hash, &verifier, &client_first_bare, &server_first, &gs2_header, &nonce, &msg) {
Some(server_final) => { Some(server_final) => {
// Feed the same brute-force throttle as IDENTIFY/PLAIN: clear it
// on success, grow the backoff on a wrong password.
self.db.note_auth(&account, true);
let out = challenge(server_final); let out = challenge(server_final);
self.stash_sasl(client.to_string(), SaslSession::Scram { hash, step: ScramStep::Ack { account } }); self.stash_sasl(client.to_string(), SaslSession::Scram { hash, step: ScramStep::Ack { account } });
out out
} }
None => fail(), None => {
self.db.note_auth(&account, false);
fail()
}
} }
} }
// Client acknowledged our server-final ("+"); apply the login. // Client acknowledged our server-final ("+"); apply the login.