diff --git a/lib/user/credentials.js b/lib/user/credentials.js new file mode 100644 index 0000000..0c570f6 --- /dev/null +++ b/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 } diff --git a/lib/user/store/base.js b/lib/user/store/base.js index 0cc77ed..2190215 100644 --- a/lib/user/store/base.js +++ b/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 diff --git a/lib/user/store/mysql.js b/lib/user/store/mysql.js index 352dbc1..85b55e6 100644 --- a/lib/user/store/mysql.js +++ b/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))) diff --git a/lib/user/store/toml.js b/lib/user/store/toml.js index dd37043..3ec4715 100644 --- a/lib/user/store/toml.js +++ b/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) { diff --git a/lib/user/test/index.js b/lib/user/test/index.js index b24872b..4a25f52 100644 --- a/lib/user/test/index.js +++ b/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 }) }) }) diff --git a/lib/user/test/mysql.js b/lib/user/test/mysql.js new file mode 100644 index 0000000..a52cc75 --- /dev/null +++ b/lib/user/test/mysql.js @@ -0,0 +1,128 @@ +import assert from 'node:assert/strict' +import crypto from 'node:crypto' +import { describe, it, before, after } from 'node:test' + +import User from '../index.js' +import Group from '../../group/index.js' +import Credentials from '../credentials.js' + +import groupJson from '../../group/test/group.json' with { type: 'json' } + +// Distinct ids so this never races with index.js, which runs concurrently +const groupCase = { ...groupJson, id: 9002, name: 'usertest-mysql.example.com' } +const upgradeUserId = 9002 +const upgradeUser = { + nt_user_id: upgradeUserId, + nt_group_id: groupCase.id, + username: 'upgrade-test', + email: 'upgrade-test@example.com', + first_name: 'Upgrade', + last_name: 'Test', +} +const testPass = 'UpgradeMe!123' +const authCreds = { + username: `${upgradeUser.username}@${groupCase.name}`, + password: testPass, +} + +const SELF_DESCRIBING = /^\d+\$[0-9a-f]{64}$/ + +before(async () => { + await Group.create(groupCase) +}) + +after(async () => { + await User.destroy({ id: upgradeUserId }) + await Group.destroy({ id: groupCase.id }) + await User.disconnect() +}) + +async function storedCredentials() { + const rows = await User.mysql.execute('SELECT password, pass_salt FROM nt_user WHERE nt_user_id = ?', [ + upgradeUserId, + ]) + return rows[0] +} + +// Raw SQL so we can plant the legacy password formats (plain text, SHA-1, +// PBKDF2-5000) that User.create() would hash on the way in. +async function seedUser(password, passSalt) { + await User.destroy({ id: upgradeUserId }) + await User.mysql.execute( + 'INSERT INTO nt_user (nt_user_id, nt_group_id, username, email, first_name, last_name, password, pass_salt) VALUES (?, ?, ?, ?, ?, ?, ?, ?)', + [ + upgradeUserId, + upgradeUser.nt_group_id, + upgradeUser.username, + upgradeUser.email, + upgradeUser.first_name, + upgradeUser.last_name, + password, + passSalt, + ], + ) +} + +describe('user (mysql)', function () { + describe('password upgrade on login', function () { + after(async () => { + await User.destroy({ id: upgradeUserId }) + }) + + it('upgrades plain text password to self-describing PBKDF2 on login', async () => { + await seedUser(testPass, null) + + assert.ok(await User.authenticate(authCreds), 'login should succeed') + + const creds = await storedCredentials() + assert.ok(creds.pass_salt, 'pass_salt should be set after upgrade') + assert.match(creds.password, SELF_DESCRIBING) + + assert.ok(await User.authenticate(authCreds), 'login should succeed after upgrade') + }) + + it('upgrades SHA1 password to self-describing PBKDF2 on login', async () => { + // authenticate() passes the full authTry.username (including @group) to + // validPassword(), so the HMAC key must match that full string + const sha1Hash = crypto + .createHmac('sha1', authCreds.username.toLowerCase()) + .update(testPass) + .digest('hex') + await seedUser(sha1Hash, null) + + assert.ok(await User.authenticate(authCreds), 'login should succeed with SHA1 hash') + + const creds = await storedCredentials() + assert.ok(creds.pass_salt, 'pass_salt should be set after upgrade') + assert.match(creds.password, SELF_DESCRIBING) + + assert.ok(await User.authenticate(authCreds), 'login should succeed after upgrade') + }) + + it('upgrades PBKDF2-5000 to self-describing format on login', async () => { + const legacySalt = Credentials.generateSalt() + const legacyHash = await Credentials.hashAuthPbkdf2(testPass, legacySalt, 5000) + await seedUser(legacyHash, legacySalt) + + assert.ok(await User.authenticate(authCreds), 'login should succeed with legacy PBKDF2') + + const creds = await storedCredentials() + assert.match(creds.password, SELF_DESCRIBING) + assert.notEqual(creds.pass_salt, legacySalt, 'salt should be regenerated') + + assert.ok(await User.authenticate(authCreds), 'login should succeed after upgrade') + }) + + it('does not re-hash password already in self-describing format', async () => { + const salt = Credentials.generateSalt() + const hash = await Credentials.hashForStorage(testPass, salt) + await seedUser(hash, salt) + + await User.authenticate(authCreds) + + const creds = await storedCredentials() + assert.equal(creds.password, hash, 'password should be unchanged') + assert.equal(creds.pass_salt, salt, 'salt should be unchanged') + }) + }) +}) diff --git a/routes/user.js b/routes/user.js index 85f7b6e..eeef463 100644 --- a/routes/user.js +++ b/routes/user.js @@ -1,6 +1,7 @@ import validate from '@nictool/validate' import User from '../lib/user/index.js' +import Credentials from '../lib/user/credentials.js' import { meta } from '../lib/util.js' function UserRoutes(server) { @@ -134,9 +135,9 @@ function UserRoutes(server) { const id = parseInt(request.params.id, 10) const args = { ...request.payload, id } + // no salt passed: a password change always gets a fresh one if (args.password) { - args.pass_salt = User.generateSalt() - args.password = await User.hashAuthPbkdf2(args.password, args.pass_salt) + Object.assign(args, await Credentials.forStorage(args.password)) } await User.put(args)