self describing password hashes

lib/user/credentials.js

@@ -0,0 +1,148 @@
+import crypto from 'node:crypto'
+
+const DEFAULT_ITERATIONS = 220000
+const LEGACY_ITERATIONS = 5000
+const SELF_DESCRIBING_RE = /^(\d+)\$([0-9a-f]{64})$/
+const LEGACY_PBKDF2_RE = /^[0-9a-f]{64}$/
+const LEGACY_SHA1_RE = /^[0-9a-f]{40}$/
+
+// PBKDF2-SHA512 costs ~40ms at the 220k default. This ceiling bounds both a
+// misconfigured env var and a tampered DB row: a stored count is untrusted
+// input to a CPU bound function, so cap the amplification at roughly 4x.
+const MAX_ITERATIONS = 1_000_000
+
+function validIterations(n) {
+  return Number.isSafeInteger(n) && n >= 1 && n <= MAX_ITERATIONS
+}
+
+const PBKDF2_ITERATIONS = (() => {
+  const raw = process.env.PBKDF2_ITERATIONS
+  if (raw === undefined) return DEFAULT_ITERATIONS
+
+  // Number() rather than parseInt(), which would read "220000zzz" as 220000
+  const parsed = Number(raw)
+  if (!validIterations(parsed)) {
+    console.warn(
+      `PBKDF2_ITERATIONS="${raw}" is not an integer in 1..${MAX_ITERATIONS}, using ${DEFAULT_ITERATIONS}`,
+    )
+    return DEFAULT_ITERATIONS
+  }
+  return parsed
+})()
+
+function equalsConstantTime(a, b) {
+  const bufA = Buffer.from(a, 'hex')
+  const bufB = Buffer.from(b, 'hex')
+  if (bufA.length !== bufB.length) return false
+  return crypto.timingSafeEqual(bufA, bufB)
+}
+
+/**
+ * Password hashing and verification, independent of how users are stored.
+ *
+ * Hashes are stored self-describing as `iterations$hexHash`, so verification
+ * never has to guess a format and a wrong password costs exactly one PBKDF2
+ * computation no matter how many legacy formats have existed.
+ */
+class Credentials {
+  constructor(args = {}) {
+    this.debug = args?.debug ?? false
+  }
+
+  generateSalt(length = 16) {
+    const chars = Array.from({ length: 87 }, (_, i) => String.fromCharCode(i + 40)) // ASCII 40–126
+    let salt = ''
+    for (let i = 0; i < length; i++) {
+      salt += chars[Math.floor(Math.random() * 87)]
+    }
+    return salt
+  }
+
+  async hashAuthPbkdf2(pass, salt, iterations = PBKDF2_ITERATIONS) {
+    return new Promise((resolve, reject) => {
+      crypto.pbkdf2(pass, salt, iterations, 32, 'sha512', (err, derivedKey) => {
+        if (err) return reject(err)
+        resolve(derivedKey.toString('hex'))
+      })
+    })
+  }
+
+  async hashForStorage(pass, salt, iterations = PBKDF2_ITERATIONS) {
+    const hex = await this.hashAuthPbkdf2(pass, salt, iterations)
+    return `${iterations}$${hex}`
+  }
+
+  /**
+   * The password/pass_salt pair to persist for a plain text password.
+   *
+   * Omit `salt` to get a fresh one, which is what a password change and a
+   * hash upgrade both want. Pass an existing salt only to preserve a caller
+   * supplied one, as create() does for fixtures and imports.
+   */
+  async forStorage(pass, salt = this.generateSalt()) {
+    return { password: await this.hashForStorage(pass, salt), pass_salt: salt }
+  }
+
+  async validPassword(passTry, passDb, username, salt) {
+    const invalid = { valid: false, needsUpgrade: false }
+
+    if (!salt && passTry === passDb) {
+      return { valid: true, needsUpgrade: true }
+    }
+
+    if (salt) {
+      // Self-describing format: "iterations$hexHash" — single hash, no fallback
+      const m = SELF_DESCRIBING_RE.exec(passDb)
+      if (m) {
+        const storedIters = parseInt(m[1], 10)
+        const storedHashHex = m[2]
+        if (!validIterations(storedIters)) {
+          console.warn(`refusing stored PBKDF2 iteration count ${storedIters} for user ${username}`)
+          return invalid
+        }
+
+        let hashed
+        try {
+          hashed = await this.hashAuthPbkdf2(passTry, salt, storedIters)
+        } catch (err) {
+          console.error(`PBKDF2 failed for user ${username}`, err)
+          return invalid
+        }
+
+        if (this.debug) console.log(`self-describing: ${hashed}`)
+        if (equalsConstantTime(hashed, storedHashHex)) {
+          return { valid: true, needsUpgrade: storedIters < PBKDF2_ITERATIONS }
+        }
+        return invalid
+      }
+
+      // Raw hex (legacy NicTool 2 format, implicitly 5000 iterations). Reject
+      // on shape before hashing, so an unrecognized stored value still costs
+      // an attempt exactly zero PBKDF2 computations.
+      if (!LEGACY_PBKDF2_RE.test(passDb)) return invalid
+
+      const legacy = await this.hashAuthPbkdf2(passTry, salt, LEGACY_ITERATIONS)
+      if (this.debug) console.log(`legacy: ${legacy}`)
+      if (equalsConstantTime(legacy, passDb)) {
+        return { valid: true, needsUpgrade: true }
+      }
+      return invalid
+    }
+
+    // HMAC SHA-1 (NicTool 2). Verified, never written: a successful match
+    // returns needsUpgrade so the caller rewrites it as PBKDF2 immediately.
+    // CodeQL js/insufficient-password-hash flags this; dismissed as won't fix.
+    if (LEGACY_SHA1_RE.test(passDb)) {
+      const digest = crypto.createHmac('sha1', username.toLowerCase()).update(passTry).digest('hex')
+      if (this.debug) console.log(`digest: ${digest}`)
+      if (equalsConstantTime(digest, passDb)) {
+        return { valid: true, needsUpgrade: true }
+      }
+    }
+
+    return invalid
+  }
+}
+
+export default new Credentials()
+export { Credentials }

lib/user/store/base.js

@@ -1,10 +1,9 @@
-import crypto from 'node:crypto'
-
 /**
- * User domain class – pure attributes and password business logic.
+ * Base user repository – the persistence contract
  *
  * Has zero knowledge of how users are persisted. All user repository classes
- * must extend this class and implement the repo contract methods.
+ * must extend this class and implement the repo contract methods. Password
+ * hashing and verification live in ../credentials.js.
  *
  * Repo contract:
  *   authenticate(authTry)  → { user, group } | undefined
@@ -50,48 +49,6 @@ class UserBase {
   async destroy(_args) {
     throw new Error('destroy() not implemented by this repo')
   }
-
-  // -------------------------------------------------------------------------
-  // Password business logic
-  // -------------------------------------------------------------------------
-
-  generateSalt(length = 16) {
-    const chars = Array.from({ length: 87 }, (_, i) => String.fromCharCode(i + 40)) // ASCII 40–126
-    let salt = ''
-    for (let i = 0; i < length; i++) {
-      salt += chars[Math.floor(Math.random() * 87)]
-    }
-    return salt
-  }
-
-  async hashAuthPbkdf2(pass, salt) {
-    return new Promise((resolve, reject) => {
-      // match the defaults for NicTool 2.x
-      crypto.pbkdf2(pass, salt, 5000, 32, 'sha512', (err, derivedKey) => {
-        if (err) return reject(err)
-        resolve(derivedKey.toString('hex'))
-      })
-    })
-  }
-
-  async validPassword(passTry, passDb, username, salt) {
-    if (!salt && passTry === passDb) return true // plain pass, TODO, encrypt!
-
-    if (salt) {
-      const hashed = await this.hashAuthPbkdf2(passTry, salt)
-      if (this.debug) console.log(`hashed: (${hashed === passDb}) ${hashed}`)
-      return hashed === passDb
-    }
-
-    // Check for HMAC SHA-1 password
-    if (/^[0-9a-f]{40}$/.test(passDb)) {
-      const digest = crypto.createHmac('sha1', username.toLowerCase()).update(passTry).digest('hex')
-      if (this.debug) console.log(`digest: (${digest === passDb}) ${digest}`)
-      return digest === passDb
-    }
-
-    return false
-  }
 }
 
 export default UserBase

lib/user/store/mysql.js

@@ -1,5 +1,6 @@
 import Mysql from '../../mysql.js'
 import Config from '../../config.js'
+import Credentials from '../credentials.js'
 import UserBase from './base.js'
 import Permission from '../../permission/index.js'
 import { mapToDbColumn } from '../../util.js'
@@ -36,7 +37,23 @@ class UserRepoMySQL extends UserBase {
       AND g.name = ?`
 
     for (const u of await Mysql.execute(query, [username, groupName])) {
-      if (await this.validPassword(authTry.password, u.password, authTry.username, u.pass_salt)) {
+      const { valid, needsUpgrade } = await Credentials.validPassword(
+        authTry.password,
+        u.password,
+        authTry.username,
+        u.pass_salt,
+      )
+      if (valid) {
+        // best effort: a failed rewrite must not cost the user their login,
+        // they simply get upgraded on a later attempt
+        if (needsUpgrade) {
+          try {
+            const creds = await Credentials.forStorage(authTry.password)
+            await Mysql.execute(...Mysql.update('nt_user', `nt_user_id=${u.id}`, creds))
+          } catch (err) {
+            console.warn(`password hash upgrade failed for user ${authTry.username}`, err)
+          }
+        }
         for (const f of ['password', 'pass_salt']) {
           delete u[f] // SECURITY: no longer needed
         }
@@ -64,8 +81,7 @@ class UserRepoMySQL extends UserBase {
     delete args.inherit_group_permissions
 
     if (args.password) {
-      if (!args.pass_salt) args.pass_salt = this.generateSalt()
-      args.password = await this.hashAuthPbkdf2(args.password, args.pass_salt)
+      Object.assign(args, await Credentials.forStorage(args.password, args.pass_salt))
     }
 
     const userId = await Mysql.execute(...Mysql.insert(`nt_user`, mapToDbColumn(args, userDbMap)))

lib/user/store/toml.js

@@ -5,6 +5,7 @@ import { fileURLToPath } from 'node:url'
 import { parse, stringify } from 'smol-toml'
 
 import Config from '../../config.js'
+import Credentials from '../credentials.js'
 import UserBase from './base.js'
 
 const __dirname = path.dirname(fileURLToPath(import.meta.url))
@@ -86,7 +87,24 @@ class UserRepoTOML extends UserBase {
       if (u.username !== username) continue
       if (u.deleted) continue
 
-      if (await this.validPassword(authTry.password, u.password, authTry.username, u.pass_salt)) {
+      const { valid, needsUpgrade } = await Credentials.validPassword(
+        authTry.password,
+        u.password,
+        authTry.username,
+        u.pass_salt,
+      )
+      if (valid) {
+        // best effort, as in the MySQL repo: a failed rewrite must not cost
+        // the user their login
+        if (needsUpgrade) {
+          try {
+            Object.assign(u, await Credentials.forStorage(authTry.password))
+            await this._save(users)
+          } catch (err) {
+            console.warn(`password hash upgrade failed for user ${authTry.username}`, err)
+          }
+        }
+
         const result = { ...u }
         for (const f of ['password', 'pass_salt', 'permissions']) delete result[f]
         const g = { id: result.gid, name: groupName }
@@ -151,8 +169,7 @@ class UserRepoTOML extends UserBase {
     delete args.inherit_group_permissions
 
     if (args.password) {
-      if (!args.pass_salt) args.pass_salt = this.generateSalt()
-      args.password = await this.hashAuthPbkdf2(args.password, args.pass_salt)
+      Object.assign(args, await Credentials.forStorage(args.password, args.pass_salt))
     }
 
     if (inherit === false) {

lib/user/test/index.js

@@ -3,6 +3,7 @@ import { describe, it, after, before } from 'node:test'
 
 import User from '../index.js'
 import Group from '../../group/index.js'
+import Credentials from '../credentials.js'
 
 import userJson from './user.json' with { type: 'json' }
 import groupJson from '../../group/test/group.json' with { type: 'json' }
@@ -90,46 +91,54 @@ describe('user', function () {
 
   describe('validPassword', function () {
     it('auths user with plain text password', async () => {
-      const r = await User.validPassword('test', 'test', 'demo', '')
-      assert.equal(r, true)
+      const r = await Credentials.validPassword('test', 'test', 'demo', '')
+      assert.deepEqual(r, { valid: true, needsUpgrade: true })
     })
 
-    it('auths valid pbkdb2 password', async () => {
-      const r = await User.validPassword(
-        'YouGuessedIt!',
-        '050cfa70c3582be0d5bfae25138a8486dc2e6790f39bc0c4e111223ba6034432',
-        'unit-test',
-        '(ICzAm2.QfCa6.MN',
-      )
-      assert.equal(r, true)
+    it('auths valid self-describing PBKDF2 password', async () => {
+      const salt = '(ICzAm2.QfCa6.MN'
+      const hash = await Credentials.hashForStorage('YouGuessedIt!', salt)
+      const r = await Credentials.validPassword('YouGuessedIt!', hash, 'unit-test', salt)
+      assert.deepEqual(r, { valid: true, needsUpgrade: false })
     })
 
-    it('rejects invalid pbkdb2 password', async () => {
-      const r = await User.validPassword(
-        'YouMissedIt!',
-        '050cfa70c3582be0d5bfae25138a8486dc2e6790f39bc0c4e111223ba6034432',
-        'unit-test',
-        '(ICzAm2.QfCa6.MN',
-      )
-      assert.equal(r, false)
+    it('rejects invalid self-describing PBKDF2 password', async () => {
+      const salt = '(ICzAm2.QfCa6.MN'
+      const hash = await Credentials.hashForStorage('YouGuessedIt!', salt)
+      const r = await Credentials.validPassword('YouMissedIt!', hash, 'unit-test', salt)
+      assert.deepEqual(r, { valid: false, needsUpgrade: false })
+    })
+
+    it('auths valid legacy PBKDF2-5000 password', async () => {
+      const salt = '(ICzAm2.QfCa6.MN'
+      const hash = await Credentials.hashAuthPbkdf2('YouGuessedIt!', salt, 5000)
+      const r = await Credentials.validPassword('YouGuessedIt!', hash, 'unit-test', salt)
+      assert.deepEqual(r, { valid: true, needsUpgrade: true })
+    })
+
+    it('rejects invalid legacy PBKDF2-5000 password', async () => {
+      const salt = '(ICzAm2.QfCa6.MN'
+      const hash = await Credentials.hashAuthPbkdf2('YouGuessedIt!', salt, 5000)
+      const r = await Credentials.validPassword('YouMissedIt!', hash, 'unit-test', salt)
+      assert.deepEqual(r, { valid: false, needsUpgrade: false })
     })
 
     it('auths valid SHA1 password', async () => {
-      const r = await User.validPassword(
+      const r = await Credentials.validPassword(
         'OhNoYouDont',
         '083007777a5241d01abba70c938c60d80be60027',
         'unit-test',
       )
-      assert.equal(r, true)
+      assert.deepEqual(r, { valid: true, needsUpgrade: true })
     })
 
     it('rejects invalid SHA1 password', async () => {
-      const r = await User.validPassword(
+      const r = await Credentials.validPassword(
         'OhNoYouDont',
         '083007777a5241d01abba7Oc938c60d80be60027',
         'unit-test',
       )
-      assert.equal(r, false)
+      assert.deepEqual(r, { valid: false, needsUpgrade: false })
     })
   })