Skip to main content

max / makenotwork

synckit: close 2FA login oracle + serialize rotation-complete (ultra-fuzz Run 3 Security #4) Phase 2 of the SyncKit ultra-fuzz remediation (Security axis A- -> A). #4 SERIOUS — /api/sync/auth was a lockout-free password-confirmation oracle for 2FA accounts: a wrong password incremented the failed-login counter, but a correct password on a suspended/locked/2FA account returned 401 without touching it, so the counter distinguished "correct-but-blocked" from "wrong". sync_auth now accounts every correct-password-but-denied outcome identically to a wrong password (increment lockout + 401), collapsing the branches into one uniform deny path. Closes the oracle for the whole blocked-account class, not just 2FA. complete_key_rotation — the remaining-entries count that authorizes the key swap ran on the pool, outside the swap transaction, so it could go stale against a concurrent rotation_batch. The rotation fetch (now FOR UPDATE), the count, and the swap+delete are one transaction; the authorizing read is serialized with the write. Test: correct password on a 2FA account returns 401 AND increments failed_login_attempts (indistinguishable from a wrong password). 50/50 synckit tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-06-23 17:36 UTC
Commit: 2dd934eca0e394d850dc5d3d9554b10e0b2f7670
Parent: 3d162e9
3 files changed, +71 insertions, -28 deletions
M server/src/db/synckit/rotation.rs +11 -6
@@ -207,13 +207,20 @@ pub async fn complete_key_rotation(
207 207 user_id: UserId,
208 208 rotation_id: uuid::Uuid,
209 209 ) -> Result<std::result::Result<i32, i64>> {
210 + // Lock the rotation row and run the remaining-entries check, the key swap,
211 + // and the rotation delete in ONE transaction. The `FOR UPDATE` serializes
212 + // this against a concurrent `rotation_batch` for the same rotation, so the
213 + // count that authorizes the swap can't go stale between the read and the
214 + // write (the prior code counted on the pool, outside the swap tx).
215 + let mut tx = pool.begin().await?;
216 +
210 217 let rotation = sqlx::query_as::<_, crate::db::models::DbSyncKeyRotation>(
211 - "SELECT * FROM sync_key_rotations WHERE id = $1 AND app_id = $2 AND user_id = $3",
218 + "SELECT * FROM sync_key_rotations WHERE id = $1 AND app_id = $2 AND user_id = $3 FOR UPDATE",
212 219 )
213 220 .bind(rotation_id)
214 221 .bind(app_id)
215 222 .bind(user_id)
216 - .fetch_optional(pool)
223 + .fetch_optional(&mut *tx)
217 224 .await?;
218 225
219 226 let Some(rotation) = rotation else {
@@ -235,16 +242,14 @@ pub async fn complete_key_rotation(
235 242 .bind(user_id)
236 243 .bind(rotation.new_key_id)
237 244 .bind(rotation.target_seq)
238 - .fetch_one(pool)
245 + .fetch_one(&mut *tx)
239 246 .await?;
240 247
241 248 if remaining > 0 {
242 249 return Ok(Err(remaining));
243 250 }
244 251
245 - // Atomically swap key and delete rotation
246 - let mut tx = pool.begin().await?;
247 -
252 + // Swap key and delete rotation (same transaction as the authorizing count).
248 253 sqlx::query(
249 254 r#"
250 255 UPDATE sync_keys
M server/src/routes/synckit/auth.rs +17 -22
@@ -83,8 +83,23 @@ pub(super) async fn sync_auth(
83 83 };
84 84
85 85 let valid = crate::auth::verify_password(&req.password, &user.password_hash)?;
86 - if !valid {
87 - // Track failed attempts for lockout (atomic increment + lock)
86 +
87 + // Account-status checks run after verify_password to avoid timing oracles.
88 + // A correct password that still can't complete login here — suspended,
89 + // locked, or 2FA-gated (2FA users must use the OAuth flow) — is accounted
90 + // and answered EXACTLY like a wrong password: increment the failed-login
91 + // counter and return 401. Otherwise the counter is an oracle that confirms
92 + // the password of a 2FA/locked/suspended account (wrong guesses increment,
93 + // a correct-but-blocked guess would not). Returning 401 (not 400) also
94 + // avoids leaking 2FA status. (ultra-fuzz Run 3 SECURITY #4.)
95 + let denied = !valid
96 + || user.is_suspended()
97 + || user
98 + .locked_until
99 + .is_some_and(|locked_until| locked_until > chrono::Utc::now())
100 + || user.totp_enabled;
101 +
102 + if denied {
88 103 db::auth::increment_failed_login(
89 104 &state.db, user.id,
90 105 crate::constants::MAX_LOGIN_ATTEMPTS,
@@ -93,26 +108,6 @@ pub(super) async fn sync_auth(
93 108 return Err(AppError::Unauthorized);
94 109 }
95 110
96 - // Password is correct — now check account status.
97 - // These checks happen after verify_password to avoid timing oracles.
98 -
99 - if user.is_suspended() {
100 - return Err(AppError::Unauthorized);
101 - }
102 -
103 - if let Some(locked_until) = user.locked_until
104 - && locked_until > chrono::Utc::now()
105 - {
106 - return Err(AppError::Unauthorized);
107 - }
108 -
109 - // Reject accounts with 2FA enabled — they must use the OAuth flow.
110 - // Returns 401 (not 400) to avoid leaking 2FA status to attackers who
111 - // guessed the password.
112 - if user.totp_enabled {
113 - return Err(AppError::Unauthorized);
114 - }
115 -
116 111 // Successful auth — reset failed login counter
117 112 db::auth::reset_failed_login(&state.db, user.id).await?;
118 113
M server/tests/workflows/synckit.rs +43
@@ -373,3 +373,46 @@ async fn push_validation() {
373 373 .await;
374 374 assert_eq!(resp.status, 400, "Expected 400 for empty changes: {}", resp.text);
375 375 }
376 +
377 + /// A correct password on a 2FA-enabled account must be accounted exactly like a
378 + /// wrong password: 401 AND the failed-login counter increments. Otherwise the
379 + /// counter is an oracle that confirms the password of a 2FA account (a wrong
380 + /// guess increments, a correct-but-2FA guess would not). ultra-fuzz Run 3 #4.
381 + #[tokio::test]
382 + async fn sync_auth_2fa_account_increments_lockout_like_wrong_password() {
383 + let mut h = TestHarness::new().await;
384 + let user_id = h.signup("tfa_user", "tfa@example.com", "Password1!").await;
385 + let (_app_id, api_key) = create_sync_app_for_user(&h.db, user_id).await;
386 +
387 + // Enable 2FA on the account.
388 + sqlx::query("UPDATE users SET totp_enabled = TRUE WHERE id = $1")
389 + .bind(user_id)
390 + .execute(&h.db)
391 + .await
392 + .expect("enable totp");
393 +
394 + // Correct password, but 2FA-gated → 401, not a token.
395 + let resp = h
396 + .client
397 + .post_json(
398 + "/api/sync/auth",
399 + &json!({
400 + "email": "tfa@example.com",
401 + "password": "Password1!",
402 + "api_key": api_key,
403 + "key": "test-sdk-key",
404 + })
405 + .to_string(),
406 + )
407 + .await;
408 + assert_eq!(resp.status, 401, "2FA account must not get a token: {}", resp.text);
409 +
410 + // The lockout counter incremented, exactly as a wrong password would — no
411 + // oracle distinguishing "correct password + 2FA" from "wrong password".
412 + let attempts: i32 = sqlx::query_scalar("SELECT failed_login_attempts FROM users WHERE id = $1")
413 + .bind(user_id)
414 + .fetch_one(&h.db)
415 + .await
416 + .expect("read failed_login_attempts");
417 + assert_eq!(attempts, 1, "correct-password-on-2FA must increment the lockout counter");
418 + }