diff --git a/src/routes/adminConfig.js b/src/routes/adminConfig.js index 23ddfb0b..40f1da29 100644 --- a/src/routes/adminConfig.js +++ b/src/routes/adminConfig.js @@ -805,16 +805,11 @@ var invites = require('../utils/registrationInvites'); router.get('/invites', async function(req, res) { try { - // The code itself, decrypted for display, so an invitation can be copied - // again rather than only at the moment it was made. The cipher never leaves + // list() returns the code already decrypted, so an invitation can be copied + // again rather than only at the moment it was made; the cipher never leaves // the server. A row created before codes were kept simply has no code, and // its four-character hint is all there is to show. - var rows = (await invites.list()).map(function (row) { - var code = invites.decryptCode(row.code_cipher); - delete row.code_cipher; - return Object.assign(row, { code: code }); - }); - res.json({ success: true, invites: rows, inviteOnly: await invites.inviteOnly(), lockdown: lockdown.state() }); + res.json({ success: true, invites: await invites.list(), inviteOnly: await invites.inviteOnly(), lockdown: lockdown.state() }); } catch (e) { return serverError(res, 'Invites list', e, 'Could not list invitations'); } }); diff --git a/src/routes/nextcloud.js b/src/routes/nextcloud.js index b71b4508..4105cad3 100644 --- a/src/routes/nextcloud.js +++ b/src/routes/nextcloud.js @@ -7,6 +7,9 @@ var logger = require('../utils/logger'); var cryptoUtil = require('../utils/crypto'); var { serverError } = require('../utils/errors'); var { assertSafeHttpsUrl } = require('../utils/urlSafety'); +// One definition of what an app password is bound to, shared with the module +// that reads tokens for file exports. +var { tokenContext } = require('../utils/nextcloudFiles'); router.use('/nextcloud', authMiddleware, require('../utils/policy').requireFeature('nextcloud')); @@ -126,7 +129,7 @@ router.post('/nextcloud/login-flow/poll', authMiddleware, async function (req, r } await db.run('UPDATE users SET nextcloud_url = ?, nextcloud_user = ?, nextcloud_token = ?, nextcloud_folder = ? WHERE id = ?', - [serverUrl, loginName, cryptoUtil.encryptString(appPassword), folder, req.user.id]); + [serverUrl, loginName, cryptoUtil.encryptString(appPassword, tokenContext(req.user.id)), folder, req.user.id]); loginFlows.delete(String(req.body.handle)); logger.audit(req.user.id, 'nextcloud_connect', 'Connected Nextcloud via login flow', req, { category: 'integration' }); @@ -160,7 +163,7 @@ router.post('/nextcloud/connect', authMiddleware, async function(req, res) { } await db.run('UPDATE users SET nextcloud_url = ?, nextcloud_user = ?, nextcloud_token = ?, nextcloud_folder = ? WHERE id = ?', - [cleanUrl, username, cryptoUtil.encryptString(appPassword), targetFolder, req.user.id]); + [cleanUrl, username, cryptoUtil.encryptString(appPassword, tokenContext(req.user.id)), targetFolder, req.user.id]); res.json({ success: true, message: 'Connected! Files saved to ' + targetFolder + '/' }); logger.audit(req.user.id, 'nextcloud_connect', 'Connected Nextcloud', req, { category: 'integration' }); @@ -179,7 +182,7 @@ router.post('/nextcloud/export', authMiddleware, async function(req, res) { var targetPath = baseFolder + '/' + today; var pw; - try { pw = cryptoUtil.decryptString(user.nextcloud_token); } + try { pw = cryptoUtil.decryptString(user.nextcloud_token, tokenContext(req.user.id)); } catch (decErr) { return res.status(400).json({ error: 'Nextcloud credentials invalid. Please reconnect.' }); } var parts = targetPath.split('/').filter(Boolean); @@ -195,9 +198,11 @@ router.post('/nextcloud/export', authMiddleware, async function(req, res) { await axios({ method: 'PUT', url: filePath, data: content, auth: { username: user.nextcloud_user, password: pw }, headers: { 'Content-Type': 'text/plain; charset=utf-8' }, timeout: 30000, maxRedirects: 0 }); - // Migrate legacy plaintext token to encrypted form on first successful use. - if (!cryptoUtil.isEncrypted(user.nextcloud_token)) { - db.run('UPDATE users SET nextcloud_token = ? WHERE id = ?', [cryptoUtil.encryptString(pw), req.user.id]).catch(function(){}); + // Upgrade the stored token on first successful use: a legacy plaintext one + // to encrypted, and an older encrypted one to the form bound to this row. + if (!cryptoUtil.isBound(user.nextcloud_token)) { + db.run('UPDATE users SET nextcloud_token = ? WHERE id = ?', + [cryptoUtil.encryptString(pw, tokenContext(req.user.id)), req.user.id]).catch(function(){}); } res.json({ success: true, message: 'Saved to ' + targetPath + '/' + safeName + '.txt' }); diff --git a/src/utils/crypto.js b/src/utils/crypto.js index 07aca563..cce0c3c4 100644 --- a/src/utils/crypto.js +++ b/src/utils/crypto.js @@ -23,32 +23,81 @@ if (!KEY && (process.env.NODE_ENV === 'production' || process.env.APP_URL)) { process.exit(1); } -// Ciphertext format: "enc1:" + base64( iv(12) || authTag(16) || ciphertext ) +// Ciphertext formats: +// "enc1:" + base64( iv(12) || authTag(16) || ciphertext ) +// "enc2:" + base64( iv(12) || authTag(16) || ciphertext ), with the context +// string bound in as AES-GCM additional authenticated data +// +// enc2 exists because enc1 said what a value is but not WHERE it belongs. A +// ciphertext could be moved between rows and would decrypt perfectly: copy one +// account's Nextcloud token onto another's row and that account exports into +// someone else's storage. Binding a context — 'users:nextcloud_token:41' — +// means a ciphertext only decrypts in the place it was written for. The AAD is +// authenticated, not encrypted: it is not a secret, it is a claim about +// location that the tag now covers. +// +// enc1 is still read, forever. Every row written before this is enc1, and a +// value with no meaningful location still writes enc1. var PREFIX = 'enc1:'; +var PREFIX_AAD = 'enc2:'; function isEncrypted(value) { - return typeof value === 'string' && value.indexOf(PREFIX) === 0; + return typeof value === 'string' && + (value.indexOf(PREFIX) === 0 || value.indexOf(PREFIX_AAD) === 0); } -function encryptString(plaintext) { +// Already bound to a location. Call sites use this to re-encrypt an older row +// in place the next time they successfully read it. +function isBound(value) { + return typeof value === 'string' && value.indexOf(PREFIX_AAD) === 0; +} + +/** + * Build the context string for a stored value: context('users', + * 'nextcloud_token', 41) -> 'users:nextcloud_token:41'. + * + * Use the column's real identity. The point is that two different rows produce + * two different strings, so a ciphertext lifted from one will not open in the + * other; a constant here binds nothing. + */ +function context(table, column, id) { + return String(table) + ':' + String(column) + ':' + String(id); +} + +/** + * Encrypt a string. Pass a context — 'table:column:id' — to bind the ciphertext + * to where it is stored; decryption then requires the same context. + */ +function encryptString(plaintext, context) { if (plaintext == null) return plaintext; if (!KEY) return plaintext; // dev mode: passthrough var iv = crypto.randomBytes(12); var cipher = crypto.createCipheriv('aes-256-gcm', KEY, iv); + if (context) cipher.setAAD(Buffer.from(String(context), 'utf8')); var ct = Buffer.concat([cipher.update(String(plaintext), 'utf8'), cipher.final()]); var tag = cipher.getAuthTag(); - return PREFIX + Buffer.concat([iv, tag, ct]).toString('base64'); + return (context ? PREFIX_AAD : PREFIX) + Buffer.concat([iv, tag, ct]).toString('base64'); } -function decryptString(value) { +/** + * Decrypt. A value written with a context needs that same context back; the + * auth tag covers it, so a wrong or missing one fails as tampering rather than + * returning the wrong plaintext. + */ +function decryptString(value, context) { if (value == null) return value; if (!isEncrypted(value)) return value; // plaintext (legacy row) if (!KEY) throw new Error('Cannot decrypt: DATA_ENCRYPTION_KEY not set'); + var bound = String(value).indexOf(PREFIX_AAD) === 0; var buf = Buffer.from(String(value).slice(PREFIX.length), 'base64'); var iv = buf.subarray(0, 12); var tag = buf.subarray(12, 28); var ct = buf.subarray(28); var decipher = crypto.createDecipheriv('aes-256-gcm', KEY, iv); + if (bound) { + if (!context) throw new Error('Cannot decrypt: this value is bound to a location and none was given'); + decipher.setAAD(Buffer.from(String(context), 'utf8')); + } decipher.setAuthTag(tag); return Buffer.concat([decipher.update(ct), decipher.final()]).toString('utf8'); } @@ -84,6 +133,8 @@ module.exports = { encryptString: encryptString, decryptString: decryptString, isEncrypted: isEncrypted, + isBound: isBound, + context: context, encryptBuffer: encryptBuffer, decryptBuffer: decryptBuffer, isEncryptedBuffer: isEncryptedBuffer, diff --git a/src/utils/nextcloudFiles.js b/src/utils/nextcloudFiles.js index 6e890db1..5b013303 100644 --- a/src/utils/nextcloudFiles.js +++ b/src/utils/nextcloudFiles.js @@ -15,6 +15,15 @@ var db = require('../db/database'); var cryptoUtil = require('./crypto'); var { assertSafeHttpsUrl } = require('./urlSafety'); +// The stored app password is bound to the row it belongs to. Without this, a +// ciphertext copied from one user's nextcloud_token into another's decrypts +// perfectly, and that account's exports go into someone else's storage. Both +// this module and the Nextcloud routes read the context from here so the two +// cannot drift — a mismatch would read as a corrupt token. +function tokenContext(userId) { + return cryptoUtil.context('users', 'nextcloud_token', userId); +} + function davRoot(url, username) { return String(url).replace(/\/+$/, '') + '/remote.php/dav/files/' + encodeURIComponent(username); } @@ -32,7 +41,7 @@ async function target(userId) { await assertSafeHttpsUrl(user.nextcloud_url, 'Nextcloud URL'); var password; - try { password = cryptoUtil.decryptString(user.nextcloud_token); } + try { password = cryptoUtil.decryptString(user.nextcloud_token, tokenContext(userId)); } catch (e) { throw refuse(400, 'Nextcloud credentials are invalid. Reconnect in Settings.'); } var base = user.nextcloud_folder || '/PediatricScribe'; @@ -71,12 +80,13 @@ async function send(userId, name, bytes, contentType) { maxBodyLength: Infinity }); - // Migrate a legacy plaintext token to encrypted form on first successful use. - if (!cryptoUtil.isEncrypted(place.user.nextcloud_token)) { + // Upgrade the stored token on first successful use: a legacy plaintext one to + // encrypted, an older encrypted one to the form bound to this row. + if (!cryptoUtil.isBound(place.user.nextcloud_token)) { db.run('UPDATE users SET nextcloud_token = ? WHERE id = ?', - [cryptoUtil.encryptString(place.password), userId]).catch(function () {}); + [cryptoUtil.encryptString(place.password, tokenContext(userId)), userId]).catch(function () {}); } return place.folder + '/' + safe; } -module.exports = { send, target, davRoot }; +module.exports = { send, target, davRoot, tokenContext }; diff --git a/src/utils/registrationInvites.js b/src/utils/registrationInvites.js index 5fcdada6..dc409d6e 100644 --- a/src/utils/registrationInvites.js +++ b/src/utils/registrationInvites.js @@ -47,14 +47,27 @@ function hash(code) { // Encrypted with DATA_ENCRYPTION_KEY, like every other recoverable secret here. // Without a key configured the code is simply not kept — the invitation still // works, it just cannot be shown again, which is exactly the old behaviour. -function encryptCode(code) { - try { return cryptoUtil.hasKey() ? cryptoUtil.encryptString(normalize(code)) : null; } - catch (e) { return null; } +// +// Bound to the row's own code_hash, so a cipher lifted onto another invitation +// will not open there. The hash is already stored beside the cipher, so using +// it as the binding reveals nothing new, and unlike the row id it exists at the +// moment of the INSERT. +function codeContext(codeHash) { + return cryptoUtil.context('registration_invites', 'code_cipher', codeHash); } -function decryptCode(cipher) { +function encryptCode(code) { + try { + if (!cryptoUtil.hasKey()) return null; + return cryptoUtil.encryptString(normalize(code), codeContext(hash(code))); + } catch (e) { return null; } +} + +// Codes made before binding have no context; decryptString ignores the one +// passed for those, so both forms read here. +function decryptCode(cipher, codeHash) { if (!cipher) return null; - try { return format(cryptoUtil.decryptString(cipher)); } + try { return format(cryptoUtil.decryptString(cipher, codeContext(codeHash))); } catch (e) { return null; } } @@ -90,9 +103,12 @@ async function create(adminUserId, options) { return { code: code, days: days, note: note }; } +// Rows for the admin screen, with the code already decrypted. The cipher and +// the hash stay in here: the caller has no use for either, and the hash is now +// half of what opens the cipher. async function list() { - return db().all( - "SELECT i.id, i.code_hint, i.code_cipher, i.note, i.created_at, i.expires_at, i.used_at, i.revoked_at, " + + var rows = await db().all( + "SELECT i.id, i.code_hint, i.code_cipher, i.code_hash, i.note, i.created_at, i.expires_at, i.used_at, i.revoked_at, " + " c.email AS created_by_email, u.email AS used_by_email, " + " CASE WHEN i.revoked_at IS NOT NULL THEN 'revoked' " + " WHEN i.used_at IS NOT NULL THEN 'used' " + @@ -103,6 +119,12 @@ async function list() { "LEFT JOIN users u ON u.id = i.used_by " + "ORDER BY i.created_at DESC LIMIT 200", [] ); + return (rows || []).map(function (row) { + var code = decryptCode(row.code_cipher, row.code_hash); + delete row.code_cipher; + delete row.code_hash; + return Object.assign(row, { code: code }); + }); } async function revoke(id, adminUserId) { @@ -174,7 +196,6 @@ async function inviteOnly() { } module.exports = { - decryptCode: decryptCode, formatCode: format, DEFAULT_TTL_DAYS, MAX_TTL_DAYS, diff --git a/test/crypto-context-binding.test.js b/test/crypto-context-binding.test.js new file mode 100644 index 00000000..27e23b35 --- /dev/null +++ b/test/crypto-context-binding.test.js @@ -0,0 +1,107 @@ +// A ciphertext used to say what a value was but not where it belonged. Copy the +// enc1 blob in one user's nextcloud_token onto another user's row and it +// decrypted perfectly — that account's exports then land in someone else's +// storage. AES-GCM's additional authenticated data closes that: the auth tag +// covers a context string naming the row, so a moved ciphertext fails to open. +// +// These tests exercise the real primitive, not a description of it. +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +// The module reads the key once at load, so it has to be set before requiring. +process.env.DATA_ENCRYPTION_KEY = require('node:crypto').randomBytes(32).toString('hex'); +const cryptoUtil = require('../src/utils/crypto'); + +const AAD = cryptoUtil.context('users', 'nextcloud_token', 41); + +test('a bound value reads back under its own context', () => { + const sealed = cryptoUtil.encryptString('app-password', AAD); + assert.equal(cryptoUtil.decryptString(sealed, AAD), 'app-password'); +}); + +test('the same value moved to another row does not open', () => { + const sealed = cryptoUtil.encryptString('app-password', AAD); + const otherRow = cryptoUtil.context('users', 'nextcloud_token', 99); + assert.throws(() => cryptoUtil.decryptString(sealed, otherRow)); +}); + +test('a bound value does not open with no context at all', () => { + // GCM would refuse this regardless — an absent AAD is a different AAD, so the + // tag fails. The explicit guard exists so the fault reads as "you forgot the + // context" rather than the generic unable-to-authenticate, which is what a + // call site that was never updated will actually hit. Assert the message, or + // losing the guard looks like a passing test. + const sealed = cryptoUtil.encryptString('app-password', AAD); + assert.throws(() => cryptoUtil.decryptString(sealed), /bound to a location/); +}); + +test('binding to a different column of the same row does not open', () => { + const sealed = cryptoUtil.encryptString('app-password', AAD); + assert.throws(() => + cryptoUtil.decryptString(sealed, cryptoUtil.context('users', 'other_secret', 41))); +}); + +test('enc1 rows written before binding still read, with or without a context', () => { + // Every row in the live database is enc1. If this breaks, every Nextcloud + // connection, note and saved chat becomes unreadable. + const legacy = cryptoUtil.encryptString('written-last-year'); + assert.equal(legacy.slice(0, 5), 'enc1:'); + assert.equal(cryptoUtil.decryptString(legacy), 'written-last-year'); + assert.equal(cryptoUtil.decryptString(legacy, AAD), 'written-last-year'); +}); + +test('the two formats are distinguishable, which is how upgrade-on-read works', () => { + assert.equal(cryptoUtil.isBound(cryptoUtil.encryptString('x', AAD)), true); + assert.equal(cryptoUtil.isBound(cryptoUtil.encryptString('x')), false); + assert.equal(cryptoUtil.isBound('plaintext'), false); + // Both are still encrypted, so nothing that asks that question changes. + assert.equal(cryptoUtil.isEncrypted(cryptoUtil.encryptString('x', AAD)), true); +}); + +test('two rows produce two different contexts', () => { + // A constant context would bind nothing at all. + assert.notEqual(cryptoUtil.context('users', 'nextcloud_token', 1), + cryptoUtil.context('users', 'nextcloud_token', 2)); +}); + +// ---- the call sites that matter ------------------------------------------- + +const read = (f) => fs.readFileSync(path.join(__dirname, '..', f), 'utf8'); + +test('both Nextcloud token readers and writers use one shared context', () => { + // A route defining its own copy is how the two drift into a mismatch that + // reads as a corrupt token. + const files = read('src/utils/nextcloudFiles.js'); + const route = read('src/routes/nextcloud.js'); + assert.match(files, /function tokenContext\(userId\)/); + assert.match(files, /module\.exports = \{[^}]*tokenContext/); + assert.match(route, /require\('\.\.\/utils\/nextcloudFiles'\)/); + assert.doesNotMatch(route, /function tokenContext/); + + for (const [name, src] of [['nextcloudFiles.js', files], ['nextcloud.js', route]]) { + for (const call of src.match(/crypt\w*\.(en|de)cryptString\([^;]*nextcloud_token[^;]*\)|crypt\w*\.(en|de)cryptString\((?:appPassword|pw|place\.password)[^)]*\)/g) || []) { + assert.match(call, /tokenContext/, name + ' has an unbound token call: ' + call); + } + } +}); + +test('an older token is rebound the next time it is used', () => { + // isEncrypted() was the old test; it is true for enc1, so upgrade-on-read + // would never fire for the rows that most need it. + for (const f of ['src/utils/nextcloudFiles.js', 'src/routes/nextcloud.js']) { + const src = read(f); + assert.match(src, /if \(!cryptoUtil\.isBound\(/, f + ' still gates on isEncrypted'); + } +}); + +test('an invite code is bound to its own row', () => { + const src = read('src/utils/registrationInvites.js'); + assert.match(src, /function codeContext\(codeHash\)/); + assert.match(src, /encryptString\(normalize\(code\), codeContext\(hash\(code\)\)\)/); + // The cipher and the hash that opens it must not leave the module together. + assert.match(src, /delete row\.code_cipher/); + assert.match(src, /delete row\.code_hash/); + assert.doesNotMatch(read('src/routes/adminConfig.js'), /decryptCode/); +}); diff --git a/test/nextcloud-login-flow.test.js b/test/nextcloud-login-flow.test.js index e834471f..26e567f1 100644 --- a/test/nextcloud-login-flow.test.js +++ b/test/nextcloud-login-flow.test.js @@ -53,8 +53,11 @@ test('the server Nextcloud reports is re-checked before it is stored', () => { assert.match(poll, /assertSafeHttpsUrl\(serverUrl, 'Nextcloud URL'\)/); }); -test('the app password is encrypted at rest, like every other credential here', () => { - assert.match(poll, /cryptoUtil\.encryptString\(appPassword\)/); +test('the app password is encrypted at rest, and bound to the row it is stored in', () => { + // Bound, not merely encrypted: an unbound ciphertext copied onto another + // user's row decrypts there, and that account's exports land in someone + // else's storage. See test/crypto-context-binding.test.js. + assert.match(poll, /cryptoUtil\.encryptString\(appPassword, tokenContext\(req\.user\.id\)\)/); }); test('a flow expires, and starting again replaces the old one', () => { diff --git a/test/resource-to-nextcloud.test.js b/test/resource-to-nextcloud.test.js index 5fde61e8..348a689d 100644 --- a/test/resource-to-nextcloud.test.js +++ b/test/resource-to-nextcloud.test.js @@ -61,7 +61,10 @@ test('the button appears only when there is a Nextcloud to send to', () => { assert.match(ui, /row\.kind === 'article' \? 'docx' : 'pptx'/); }); -test('a legacy plaintext token is encrypted on first successful use', () => { - assert.match(util, /if \(!cryptoUtil\.isEncrypted\(place\.user\.nextcloud_token\)\)/); - assert.match(util, /cryptoUtil\.encryptString\(place\.password\)/); +test('an unbound token is upgraded on first successful use', () => { + // Covers both older forms — plaintext, and encrypted before binding existed. + // isEncrypted() is true for the latter, so gating on it would leave exactly + // the rows that need rebinding untouched. + assert.match(util, /if \(!cryptoUtil\.isBound\(place\.user\.nextcloud_token\)\)/); + assert.match(util, /cryptoUtil\.encryptString\(place\.password, tokenContext\(userId\)\)/); });