feat(fs): use aes256-gcm encryption algorithm (#6447)

Fixes zammad#9788

@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/",

@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 info = crypto.getCipherInfo(algorithm, { keyLength: key.length })
-  const ivLength = 16
+  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) {
+    return pipeline(
+      input,
+      async function* (source) {
         const iv = crypto.randomBytes(ivLength)
         const cipher = crypto.createCipheriv(algorithm, Buffer.from(key), iv)
-
+        yield iv
-    const encrypted = pumpify(input, cipher)
+        for await (const data of source) {
-    encrypted.unshift(iv)
+          yield cipher.update(data)
-    return encrypted
+        }
+        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) {
+  function decryptStream(encryptedStream) {
-    const iv = await readChunk(encryptedStream, ivLength)
+    return pipeline(
-    const cipher = crypto.createDecipheriv(algorithm, Buffer.from(key), iv)
+      encryptedStream,
+      async function* (source) {
         /**
          * WARNING
          *
-     * the crytped size has an initializtion vector + a padding at the end
+         * 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
          *
          */
-    return pumpify(encryptedStream, cipher)
+
+        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()])
   }

@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)
+    })
+  })
+})

@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(
+      this._writeFile(normalizePath(ENCRYPTION_DESC_FILENAME), JSON.stringify({ algorithm: encryptionAlgorithm }), {
-        normalizePath(ENCRYPTION_DESC_FILENAME),
-        JSON.stringify({ algorithm: this._encryptor.algorithm }),
-        {
         flags: 'w',
-        }
+      }), // not encrypted
-      ), // 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))
+      data = await this._readFile(normalizePath(ENCRYPTION_DESC_FILENAME), 'utf-8')
-      JSON.parse(data)
+      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()
       }

@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()
+  })
+})

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