diff --git a/@xen-orchestra/fs/package.json b/@xen-orchestra/fs/package.json index 4fc7765ce..9b478e115 100644 --- a/@xen-orchestra/fs/package.json +++ b/@xen-orchestra/fs/package.json @@ -40,7 +40,6 @@ "lodash": "^4.17.4", "promise-toolbox": "^0.21.0", "proper-lockfile": "^4.1.2", - "pumpify": "^2.0.1", "readable-stream": "^4.1.0", "through2": "^4.0.2", "xo-remote-parser": "^0.9.1" @@ -54,7 +53,8 @@ "babel-plugin-lodash": "^3.3.2", "cross-env": "^7.0.2", "dotenv": "^16.0.0", - "rimraf": "^3.0.0" + "rimraf": "^3.0.0", + "tmp": "^0.2.1" }, "scripts": { "build": "cross-env NODE_ENV=production babel --source-maps --out-dir=dist/ src/", diff --git a/@xen-orchestra/fs/src/_encryptor.js b/@xen-orchestra/fs/src/_encryptor.js index 3361038e4..2125bcb60 100644 --- a/@xen-orchestra/fs/src/_encryptor.js +++ b/@xen-orchestra/fs/src/_encryptor.js @@ -1,8 +1,10 @@ +const { pipeline } = require('node:stream') const { readChunk } = require('@vates/read-chunk') const crypto = require('crypto') -const pumpify = require('pumpify') -function getEncryptor(key) { +export const DEFAULT_ENCRYPTION_ALGORITHM = 'aes-256-gcm' + +function getEncryptor(algorithm = DEFAULT_ENCRYPTION_ALGORITHM, key) { if (key === undefined) { return { id: 'NULL_ENCRYPTOR', @@ -15,43 +17,100 @@ function getEncryptor(key) { decryptStream: stream => stream, } } - const algorithm = 'aes-256-cbc' - const ivLength = 16 + const info = crypto.getCipherInfo(algorithm, { keyLength: key.length }) + if (info === undefined) { + const error = new Error( + `Either the algorithm ${algorithm} is not available, or the key length ${ + key.length + } is incorrect. Supported algorithm are ${crypto.getCiphers()}` + ) + error.code = 'BAD_ALGORITHM' + throw error + } + const { ivLength, mode } = info + const authTagLength = ['gcm', 'ccm', 'ocb'].includes(mode) ? 16 : 0 function encryptStream(input) { - const iv = crypto.randomBytes(ivLength) - const cipher = crypto.createCipheriv(algorithm, Buffer.from(key), iv) - - const encrypted = pumpify(input, cipher) - encrypted.unshift(iv) - return encrypted + return pipeline( + input, + async function* (source) { + const iv = crypto.randomBytes(ivLength) + const cipher = crypto.createCipheriv(algorithm, Buffer.from(key), iv) + yield iv + for await (const data of source) { + yield cipher.update(data) + } + yield cipher.final() + // must write the auth tag at the end of the encryption stream + if (authTagLength > 0) { + yield cipher.getAuthTag() + } + }, + () => {} + ) } - async function decryptStream(encryptedStream) { - const iv = await readChunk(encryptedStream, ivLength) - const cipher = crypto.createDecipheriv(algorithm, Buffer.from(key), iv) - /** - * WARNING - * - * the crytped size has an initializtion vector + a padding at the end - * whe can't predict the decrypted size from the start of the encrypted size - * thus, we can't set decrypted.length reliably - * - */ - return pumpify(encryptedStream, cipher) + function decryptStream(encryptedStream) { + return pipeline( + encryptedStream, + async function* (source) { + /** + * WARNING + * + * the crypted size has an initializtion vector + eventually an auth tag + a padding at the end + * whe can't predict the decrypted size from the start of the encrypted size + * thus, we can't set decrypted.length reliably + * + */ + + const iv = await readChunk(source, ivLength) + const cipher = crypto.createDecipheriv(algorithm, Buffer.from(key), iv) + let authTag = Buffer.alloc(0) + for await (const data of source) { + if (data.length >= authTagLength) { + // fast path, no buffer concat + yield cipher.update(authTag) + authTag = data.slice(data.length - authTagLength) + yield cipher.update(data.slice(0, data.length - authTagLength)) + } else { + // slower since there is a concat + const fullData = Buffer.concat([authTag, data]) + const fullDataLength = fullData.length + if (fullDataLength > authTagLength) { + authTag = fullData.slice(fullDataLength - authTagLength) + yield cipher.update(fullData.slice(0, fullDataLength - authTagLength)) + } else { + authTag = fullData + } + } + } + if (authTagLength > 0) { + cipher.setAuthTag(authTag) + } + yield cipher.final() + }, + () => {} + ) } function encryptData(buffer) { const iv = crypto.randomBytes(ivLength) const cipher = crypto.createCipheriv(algorithm, Buffer.from(key), iv) const encrypted = cipher.update(buffer) - return Buffer.concat([iv, encrypted, cipher.final()]) + return Buffer.concat([iv, encrypted, cipher.final(), authTagLength > 0 ? cipher.getAuthTag() : Buffer.alloc(0)]) } function decryptData(buffer) { const iv = buffer.slice(0, ivLength) - const encrypted = buffer.slice(ivLength) const decipher = crypto.createDecipheriv(algorithm, Buffer.from(key), iv) + let encrypted + if (authTagLength > 0) { + const authTag = buffer.slice(buffer.length - authTagLength) + decipher.setAuthTag(authTag) + encrypted = buffer.slice(ivLength, buffer.length - authTagLength) + } else { + encrypted = buffer.slice(ivLength) + } const decrypted = decipher.update(encrypted) return Buffer.concat([decrypted, decipher.final()]) } diff --git a/@xen-orchestra/fs/src/_encryptor.spec.js b/@xen-orchestra/fs/src/_encryptor.spec.js new file mode 100644 index 000000000..9b25fac65 --- /dev/null +++ b/@xen-orchestra/fs/src/_encryptor.spec.js @@ -0,0 +1,50 @@ +/* eslint-env jest */ +import { Readable } from 'node:stream' +import { _getEncryptor } from './_encryptor' +import crypto from 'crypto' + +const algorithms = ['none', 'aes-256-cbc', 'aes-256-gcm'] + +function streamToBuffer(stream) { + return new Promise(resolve => { + const bufs = [] + stream.on('data', function (d) { + bufs.push(d) + }) + stream.on('end', function () { + resolve(Buffer.concat(bufs)) + }) + }) +} + +algorithms.forEach(algorithm => { + describe(`test algorithm ${algorithm}`, () => { + const key = algorithm === 'none' ? undefined : '73c1838d7d8a6088ca2317fb5f29cd91' + const encryptor = _getEncryptor(algorithm, key) + const buffer = crypto.randomBytes(1024 * 1024 + 1) + it('handle buffer', () => { + const encrypted = encryptor.encryptData(buffer) + if (algorithm !== 'none') { + expect(encrypted.equals(buffer)).toEqual(false) // encrypted should be different + // ivlength, auth tag, padding + expect(encrypted.length).not.toEqual(buffer.length) + } + + const decrypted = encryptor.decryptData(encrypted) + expect(decrypted.equals(buffer)).toEqual(true) + }) + + it('handle stream', async () => { + const stream = Readable.from(buffer) + stream.length = buffer.length + const encrypted = encryptor.encryptStream(stream) + if (algorithm !== 'none') { + expect(encrypted.length).toEqual(undefined) + } + + const decrypted = encryptor.decryptStream(encrypted) + const decryptedBuffer = await streamToBuffer(decrypted) + expect(decryptedBuffer.equals(buffer)).toEqual(true) + }) + }) +}) diff --git a/@xen-orchestra/fs/src/abstract.js b/@xen-orchestra/fs/src/abstract.js index 0a2cfd13c..8b4d1446e 100644 --- a/@xen-orchestra/fs/src/abstract.js +++ b/@xen-orchestra/fs/src/abstract.js @@ -12,7 +12,7 @@ import { synchronized } from 'decorator-synchronized' import { basename, dirname, normalize as normalizePath } from './path' import { createChecksumStream, validChecksumOfReadStream } from './checksum' -import { _getEncryptor } from './_encryptor' +import { DEFAULT_ENCRYPTION_ALGORITHM, _getEncryptor } from './_encryptor' const { info, warn } = createLogger('@xen-orchestra:fs') @@ -68,7 +68,15 @@ class PrefixWrapper { } export default class RemoteHandlerAbstract { - _encryptor + #encryptor + + get _encryptor() { + if (this.#encryptor === undefined) { + throw new Error(`Can't access to encryptor before remote synchronization`) + } + return this.#encryptor + } + constructor(remote, options = {}) { if (remote.url === 'test://') { this._remote = remote @@ -79,7 +87,6 @@ export default class RemoteHandlerAbstract { } } ;({ highWaterMark: this._highWaterMark, timeout: this._timeout = DEFAULT_TIMEOUT } = options) - this._encryptor = _getEncryptor(this._remote.encryptionKey) const sharedLimit = limitConcurrency(options.maxParallelOperations ?? DEFAULT_MAX_PARALLEL_OPERATIONS) this.closeFile = sharedLimit(this.closeFile) @@ -330,35 +337,43 @@ export default class RemoteHandlerAbstract { } async _createMetadata() { + const encryptionAlgorithm = this._remote.encryptionKey === undefined ? 'none' : DEFAULT_ENCRYPTION_ALGORITHM + this.#encryptor = _getEncryptor(encryptionAlgorithm, this._remote.encryptionKey) + await Promise.all([ - this._writeFile( - normalizePath(ENCRYPTION_DESC_FILENAME), - JSON.stringify({ algorithm: this._encryptor.algorithm }), - { - flags: 'w', - } - ), // not encrypted + this._writeFile(normalizePath(ENCRYPTION_DESC_FILENAME), JSON.stringify({ algorithm: encryptionAlgorithm }), { + flags: 'w', + }), // not encrypted this.writeFile(ENCRYPTION_METADATA_FILENAME, `{"random":"${randomUUID()}"}`, { flags: 'w' }), // encrypted ]) } async _checkMetadata() { + let encryptionAlgorithm = 'none' + let data try { // this file is not encrypted - const data = await this._readFile(normalizePath(ENCRYPTION_DESC_FILENAME)) - JSON.parse(data) + data = await this._readFile(normalizePath(ENCRYPTION_DESC_FILENAME), 'utf-8') + const json = JSON.parse(data) + encryptionAlgorithm = json.algorithm } catch (error) { if (error.code !== 'ENOENT') { throw error } + encryptionAlgorithm = this._remote.encryptionKey === undefined ? 'none' : DEFAULT_ENCRYPTION_ALGORITHM } try { + this.#encryptor = _getEncryptor(encryptionAlgorithm, this._remote.encryptionKey) // this file is encrypted - const data = await this.readFile(ENCRYPTION_METADATA_FILENAME) + const data = await this.readFile(ENCRYPTION_METADATA_FILENAME, 'utf-8') JSON.parse(data) } catch (error) { - if (error.code === 'ENOENT' || (await this._canWriteMetadata())) { + // can be enoent, bad algorithm, or broeken json ( bad key or algorithm) + if ( + error.code === 'ENOENT' || // no encryption on non empty remote + (await this._canWriteMetadata()) // any other error , but on empty remote + ) { info('will update metadata of this remote') return this._createMetadata() } diff --git a/@xen-orchestra/fs/src/abstract.spec.js b/@xen-orchestra/fs/src/abstract.spec.js index 0b2a5dea2..21f9984b1 100644 --- a/@xen-orchestra/fs/src/abstract.spec.js +++ b/@xen-orchestra/fs/src/abstract.spec.js @@ -1,15 +1,21 @@ /* eslint-env jest */ -import { TimeoutError } from 'promise-toolbox' - +import { DEFAULT_ENCRYPTION_ALGORITHM, _getEncryptor } from './_encryptor' +import { getHandler } from '.' +import { pFromCallback, TimeoutError } from 'promise-toolbox' import AbstractHandler from './abstract' +import fs from 'fs-extra' +import rimraf from 'rimraf' +import tmp from 'tmp' const TIMEOUT = 10e3 class TestHandler extends AbstractHandler { constructor(impl) { super({ url: 'test://' }, { timeout: TIMEOUT }) - + Object.defineProperty(this, 'isEncrypted', { + get: () => false, // encryption is tested separatly + }) Object.keys(impl).forEach(method => { this[`_${method}`] = impl[method] }) @@ -101,3 +107,112 @@ describe('rmdir()', () => { await expect(promise).rejects.toThrowError(TimeoutError) }) }) + +describe('encryption', () => { + let handler, dir + + beforeEach(async () => { + dir = await pFromCallback(cb => tmp.dir(cb)) + }) + afterAll(async () => { + await handler?.forget() + handler = undefined + + await pFromCallback(cb => rimraf(dir, cb)) + }) + it('sync should create metadata if missing (not encrypted)', async () => { + handler = getHandler({ url: `file://${dir}` }) + await handler._checkMetadata() + + expect(await fs.readdir(dir)).toEqual(['encryption.json', 'metadata.json']) + + const encryption = JSON.parse(await fs.readFile(`${dir}/encryption.json`, 'utf-8')) + expect(encryption.algorithm).toEqual('none') + expect(async () => JSON.parse(await fs.readFile(`${dir}/metadata.json`))).not.toThrowError() + }) + + it('sync should create metadata if missing (encrypted)', async () => { + handler = getHandler({ url: `file://${dir}?encryptionKey="73c1838d7d8a6088ca2317fb5f29cd00"` }) + await handler._checkMetadata() + + expect(await fs.readdir(dir)).toEqual(['encryption.json', 'metadata.json']) + + const encryption = JSON.parse(await fs.readFile(`${dir}/encryption.json`, 'utf-8')) + expect(encryption.algorithm).toEqual(DEFAULT_ENCRYPTION_ALGORITHM) + // encrypted , should not be parsable + expect(async () => JSON.parse(await fs.readFile(`${dir}/metadata.json`))).rejects.toThrowError() + }) + + it('sync should not modify existing metadata', async () => { + handler = getHandler({ url: `file://${dir}` }) + await fs.writeFile(`${dir}/encryption.json`, `{"algorithm": "none"}`) + await fs.writeFile(`${dir}/metadata.json`, `{"random": "NOTSORANDOM"}`) + + await handler._checkMetadata() + + const encryption = JSON.parse(await fs.readFile(`${dir}/encryption.json`, 'utf-8')) + expect(encryption.algorithm).toEqual('none') + const metadata = JSON.parse(await fs.readFile(`${dir}/metadata.json`, 'utf-8')) + expect(metadata.random).toEqual('NOTSORANDOM') + }) + + it('should modify metadata if empty', async () => { + handler = getHandler({ url: `file://${dir}` }) + await handler._checkMetadata() + await handler.forget() + handler = getHandler({ url: `file://${dir}?encryptionKey="73c1838d7d8a6088ca2317fb5f29cd00"` }) + await handler._checkMetadata() + let encryption = JSON.parse(await fs.readFile(`${dir}/encryption.json`, 'utf-8')) + expect(encryption.algorithm).toEqual(DEFAULT_ENCRYPTION_ALGORITHM) + await handler.forget() + handler = getHandler({ url: `file://${dir}` }) + await handler._checkMetadata() + encryption = JSON.parse(await fs.readFile(`${dir}/encryption.json`, 'utf-8')) + expect(encryption.algorithm).toEqual('none') + }) + + it('sync should work with encrypted', async () => { + const encryptor = _getEncryptor(DEFAULT_ENCRYPTION_ALGORITHM, '73c1838d7d8a6088ca2317fb5f29cd91') + + await fs.writeFile(`${dir}/encryption.json`, `{"algorithm": "${DEFAULT_ENCRYPTION_ALGORITHM}"}`) + await fs.writeFile(`${dir}/metadata.json`, encryptor.encryptData(`{"random": "NOTSORANDOM"}`)) + + handler = getHandler({ url: `file://${dir}?encryptionKey="73c1838d7d8a6088ca2317fb5f29cd91"` }) + await handler._checkMetadata() + + const encryption = JSON.parse(await fs.readFile(`${dir}/encryption.json`, 'utf-8')) + expect(encryption.algorithm).toEqual(DEFAULT_ENCRYPTION_ALGORITHM) + const metadata = JSON.parse(await handler.readFile(`./metadata.json`)) + expect(metadata.random).toEqual('NOTSORANDOM') + }) + + it('sync should fail when changing key on non empty remote ', async () => { + const encryptor = _getEncryptor(DEFAULT_ENCRYPTION_ALGORITHM, '73c1838d7d8a6088ca2317fb5f29cd91') + + await fs.writeFile(`${dir}/encryption.json`, `{"algorithm": "${DEFAULT_ENCRYPTION_ALGORITHM}"}`) + await fs.writeFile(`${dir}/metadata.json`, encryptor.encryptData(`{"random": "NOTSORANDOM"}`)) + + // different key but empty remote => ok + handler = getHandler({ url: `file://${dir}?encryptionKey="73c1838d7d8a6088ca2317fb5f29cd00"` }) + await expect(handler._checkMetadata()).resolves.not.toThrowError() + + // rmote is now non empty : can't modify key anymore + await fs.writeFile(`${dir}/nonempty.json`, 'content') + handler = getHandler({ url: `file://${dir}?encryptionKey="73c1838d7d8a6088ca2317fb5f29cd10"` }) + await expect(handler._checkMetadata()).rejects.toThrowError() + }) + + it('sync should fail when changing algorithm', async () => { + // encrypt with a non default algorithm + const encryptor = _getEncryptor('aes-256-cbc', '73c1838d7d8a6088ca2317fb5f29cd91') + + await fs.writeFile(`${dir}/encryption.json`, `{"algorithm": "aes-256-gmc"}`) + await fs.writeFile(`${dir}/metadata.json`, encryptor.encryptData(`{"random": "NOTSORANDOM"}`)) + + // remote is now non empty : can't modify key anymore + await fs.writeFile(`${dir}/nonempty.json`, 'content') + + handler = getHandler({ url: `file://${dir}?encryptionKey="73c1838d7d8a6088ca2317fb5f29cd91"` }) + await expect(handler._checkMetadata()).rejects.toThrowError() + }) +}) diff --git a/CHANGELOG.unreleased.md b/CHANGELOG.unreleased.md index 04a43385d..d3216266d 100644 --- a/CHANGELOG.unreleased.md +++ b/CHANGELOG.unreleased.md @@ -4,9 +4,8 @@ > understandable by them. ### Enhancements - > Users must be able to say: “Nice enhancement, I'm eager to test it” - +- [Backup/Encryption] Use `aes-256-gcm` instead of `aes-256-ccm` to mitigate [padding oracle attacks](https://en.wikipedia.org/wiki/Padding_oracle_attack) (PR [#6447](https://github.com/vatesfr/xen-orchestra/pull/6447)) ### Bug fixes > Users must be able to say: “I had this issue, happy to know it's fixed” @@ -30,6 +29,7 @@ - @vates/nbd-client major - @vates/otp major - @vates/read-chunk patch +- @xen-orchestra/fs minor - @xen-orchestra/log minor - xo-remote-parser patch - xo-server-transport-nagios patch