fix(security): reserve integration secrets to admins, and remove the shell from the backup chain

front/src/routes/integration/all/telegram/actions.js

@@ -1,4 +1,5 @@
 import { RequestStatus } from '../../../../utils/consts';
+import { USER_ROLE } from '../../../../../../server/utils/constants';
 
 const actions = store => ({
   updateTelegramApiKey(state, e) {
@@ -11,10 +12,15 @@ const actions = store => ({
       telegramGetApiKeyStatus: RequestStatus.Getting
     });
     try {
-      const variable = await state.httpClient.get('/api/v1/service/telegram/variable/TELEGRAM_API_KEY');
-      store.setState({
-        telegramApiKey: variable.value
-      });
+      // the bot API key is a service-wide secret: only an admin can read it,
+      // and only an admin is shown the form to change it. Every other user
+      // comes to this page for their own linking link, which is per-user.
+      if (state.user && state.user.role === USER_ROLE.ADMIN) {
+        const variable = await state.httpClient.get('/api/v1/service/telegram/variable/TELEGRAM_API_KEY');
+        store.setState({
+          telegramApiKey: variable.value
+        });
+      }
       const { link } = await state.httpClient.get('/api/v1/service/telegram/link');
       store.setState({
         telegramCustomLink: link,

server/api/controllers/variable.controller.js

@@ -1,5 +1,27 @@
 const asyncMiddleware = require('../middlewares/asyncMiddleware');
-const { NotFoundError } = require('../../utils/coreErrors');
+const { NotFoundError, ForbiddenError } = require('../../utils/coreErrors');
+const { USER_ROLE } = require('../../utils/constants');
+
+/**
+ * @description Ensure only an admin can reach a service-wide variable.
+ * Service variables that are not scoped to a user hold the credentials of the
+ * integrations (API keys, broker passwords, OAuth tokens), so reading or
+ * writing them is an administration gesture. Variables scoped to the calling
+ * user are left alone: a user can only ever touch their own row.
+ * @param {object} req - The Express request.
+ * @param {string} [userId] - The user the variable is scoped to, or null when it is service-wide.
+ * @returns {void}
+ * @example
+ * ensureAdminOnServiceWideVariable(req, null);
+ */
+function ensureAdminOnServiceWideVariable(req, userId) {
+  if (userId) {
+    return;
+  }
+  if (!req.user || req.user.role !== USER_ROLE.ADMIN) {
+    throw new ForbiddenError('This route is only accessible to admin user.');
+  }
+}
 
 module.exports = function VariableController(gladys) {
   /**
@@ -10,6 +32,7 @@ module.exports = function VariableController(gladys) {
    */
   async function setForLocalService(req, res) {
     const userId = req.body.userRelated ? req.user.id : null;
+    ensureAdminOnServiceWideVariable(req, userId);
     const service = await gladys.service.getLocalServiceByName(req.params.service_name);
     const variable = await gladys.variable.setValue(req.params.variable_key, req.body.value, service.id, userId);
     res.json(variable);
@@ -22,6 +45,7 @@ module.exports = function VariableController(gladys) {
    */
   async function getByLocalService(req, res) {
     const userId = req.query.userRelated ? req.user.id : null;
+    ensureAdminOnServiceWideVariable(req, userId);
     const service = await gladys.service.getLocalServiceByName(req.params.service_name);
     const value = await gladys.variable.getValue(req.params.variable_key, service.id, userId);
     if (!value) {

server/api/routes.js

@@ -409,9 +409,13 @@ function getRoutes(gladys) {
       admin: true,
       controller: gatewayController.createBackup,
     },
+    // reachable without authentication while the instance has no user (signup
+    // restore flow), and it makes the server download and unpack a remote file:
+    // rate limited like the other pre-authentication routes
     'post /api/v1/gateway/backup/restore': {
       authenticatedOrNotConfigured: true,
       admin: true,
+      rateLimit: true,
       controller: gatewayController.restoreBackup,
     },
     'get /api/v1/gateway/backup/restore/status': {
@@ -825,12 +829,18 @@ function getRoutes(gladys) {
       authenticated: true,
       controller: variableController.getByLocalService,
     },
+    // global variables hold instance-wide secrets (Gladys Plus keys, backup
+    // keys...): reading and writing them is reserved to admins. Per-user
+    // settings go through /api/v1/user/variable below, which stays open to
+    // every authenticated user.
     'post /api/v1/variable/:variable_key': {
       authenticated: true,
+      admin: true,
       controller: variableController.setValue,
     },
     'get /api/v1/variable/:variable_key': {
       authenticated: true,
+      admin: true,
       controller: variableController.getValue,
     },
     'post /api/v1/user/variable/:variable_key': {

server/lib/gateway/gateway.backup.js

@@ -6,7 +6,8 @@ const fsPromise = require('fs').promises;
 const retry = require('async-retry');
 const db = require('../../models');
 const logger = require('../../utils/logger');
-const { exec } = require('../../utils/childProcess');
+const { execFile } = require('../../utils/childProcess');
+const { escapeSqlStringLiteral } = require('../../utils/backupSafety');
 const { readChunk } = require('../../utils/readChunk');
 const { NotFoundError } = require('../../utils/coreErrors');
 const { USER_ROLE } = require('../../utils/constants');
@@ -86,7 +87,7 @@ async function backup(jobId) {
         await fse.emptyDir(this.config.backupsFolder);
         // We backup database
         logger.info(`Starting Gateway backup in folder ${sqliteBackupFilePath}`);
-        await exec(`sqlite3 ${this.config.storage} ".backup '${sqliteBackupFilePath}'"`);
+        await execFile('sqlite3', [this.config.storage, `.backup '${sqliteBackupFilePath}'`]);
         logger.info(`Gateway backup: Unlocking Database`);
       });
     }, SQLITE_BACKUP_RETRY_OPTIONS);
@@ -101,7 +102,7 @@ async function backup(jobId) {
     try {
       // ZSTD compresses better than GZIP and needs less memory during the export
       await backupInstance.allAsync(
-        ` EXPORT DATABASE '${duckDbBackupFolderPath}' (
+        ` EXPORT DATABASE '${escapeSqlStringLiteral(duckDbBackupFolderPath)}' (
             FORMAT PARQUET,
             COMPRESSION ZSTD
         )`,
@@ -113,15 +114,24 @@ async function backup(jobId) {
     }
     // compress backup
     logger.info(`Gateway backup: Compressing backup`);
-    await exec(
-      `cd ${this.config.backupsFolder} && tar -czvf ${compressedBackupFileName} ${sqliteBackupFileName} ${duckDbBackupFolder}`,
-    );
+    await execFile('tar', ['-czvf', compressedBackupFileName, sqliteBackupFileName, duckDbBackupFolder], {
+      cwd: this.config.backupsFolder,
+    });
     await this.job.updateProgress(jobId, 20);
     // encrypt backup
     logger.info(`Gateway backup: Encrypting backup`);
-    await exec(
-      `openssl enc -aes-256-cbc -pass pass:${encryptKey} -in ${compressedBackupFilePath} -out ${encryptedBackupFilePath}`,
-    );
+    // the encryption key is a passphrase the user chose: passed through a shell it
+    // would be a command injection, so it goes to openssl as a plain argument
+    await execFile('openssl', [
+      'enc',
+      '-aes-256-cbc',
+      '-pass',
+      `pass:${encryptKey}`,
+      '-in',
+      compressedBackupFilePath,
+      '-out',
+      encryptedBackupFilePath,
+    ]);
     await this.job.updateProgress(jobId, 30);
     // Upload file to the Gladys Gateway
     const encryptedFileInfos = await fsPromise.stat(encryptedBackupFilePath);

server/lib/gateway/gateway.downloadBackup.js

@@ -3,10 +3,14 @@ const fse = require('fs-extra');
 const fs = require('fs');
 const logger = require('../../utils/logger');
 const { EVENTS, WEBSOCKET_MESSAGE_TYPES } = require('../../utils/constants');
-const { exec, execFile } = require('../../utils/childProcess');
+const { execFile, spawnToFile } = require('../../utils/childProcess');
 const { NotFoundError } = require('../../utils/coreErrors');
+const { assertSafeBackupName, isSafeArchiveEntry } = require('../../utils/backupSafety');
 
 const RESTORE_FOLDER = 'restore';
+// thrown by the checks that must never fall back to the old restore strategy:
+// a rejected archive is a rejected backup, not a backup to try another way
+const UNSAFE_BACKUP_ERRORS = ['BACKUP_CONTAINS_UNSAFE_PATHS', 'BACKUP_UNSAFE_FILE_NAME'];
 
 /**
  * @description Restore a backup.
@@ -28,7 +32,10 @@ async function downloadBackup(fileUrl) {
   // we empty the restore backup folder
   await fse.emptyDir(restoreFolderPath);
 
-  const encryptedBackupName = path.basename(fileWithoutSignedParams, '.enc');
+  // the name comes from a URL the caller chose: `path.basename` stops a path
+  // traversal but keeps every shell and SQL metacharacter, and this name ends up
+  // building the paths handed to gzip, sqlite3 and DuckDB below
+  const encryptedBackupName = assertSafeBackupName(path.basename(fileWithoutSignedParams, '.enc'));
   const encryptedBackupFilePath = path.join(restoreFolderPath, `${encryptedBackupName}.enc`);
   const compressedBackupFilePath = path.join(restoreFolderPath, `${encryptedBackupName}.gz`);
 
@@ -58,13 +65,13 @@ async function downloadBackup(fileUrl) {
     logger.info(`Trying to restore the backup new style (DuckDB)`);
     // Check archive for path traversal attempts and symlinks
     const tarEntries = await execFile('tar', ['-tzf', compressedBackupFilePath]);
+    // every entry must be a plain relative name: this rejects path traversal and
+    // absolute paths, and also the quotes, `$`, backticks and semicolons that
+    // would otherwise reach sqlite3 and DuckDB through the extracted file names
     const hasUnsafePath = tarEntries
       .split('\n')
       .filter(Boolean)
-      .some((entry) => {
-        const normalized = path.posix.normalize(entry);
-        return path.posix.isAbsolute(entry) || normalized === '..' || normalized.startsWith('../');
-      });
+      .some((entry) => !isSafeArchiveEntry(entry));
     const tarList = await execFile('tar', ['-tzvf', compressedBackupFilePath]);
     const hasSymlink = tarList.split('\n').some((line) => line.startsWith('l'));
     if (hasUnsafePath || hasSymlink) {
@@ -83,13 +90,15 @@ async function downloadBackup(fileUrl) {
     );
   } catch (e) {
     // Re-throw security errors - don't fall back to old strategy
-    if (e.message === 'BACKUP_CONTAINS_UNSAFE_PATHS') {
+    if (UNSAFE_BACKUP_ERRORS.includes(e.message)) {
       throw e;
     }
     logger.info(`Extracting failed using new strategy (Error: ${e})`);
     logger.info(`Restoring using old backup strategy (SQLite only)`);
     sqliteBackupFilePath = path.join(restoreFolderPath, `${encryptedBackupName}.db`);
-    await exec(`gzip -dc ${compressedBackupFilePath} > ${sqliteBackupFilePath}`);
+    // no shell here: the redirection is a write stream, so the backup name can
+    // never be read as a command
+    await spawnToFile('gzip', ['-dc', compressedBackupFilePath], sqliteBackupFilePath);
   }
   // done!
   logger.info(`Gladys backup downloaded with success.`);

server/lib/gateway/gateway.restoreBackup.js

@@ -7,8 +7,9 @@ const { promisify } = require('util');
 
 const db = require('../../models');
 const logger = require('../../utils/logger');
-const { exec } = require('../../utils/childProcess');
+const { execFile } = require('../../utils/childProcess');
 const { NotFoundError } = require('../../utils/coreErrors');
+const { escapeSqlStringLiteral } = require('../../utils/backupSafety');
 
 /**
  * @description Replace the local sqlite database with a backup.
@@ -39,8 +40,10 @@ async function restoreBackup(sqliteBackupFilePath, duckDbBackupFolderPath) {
   logger.info('Backup seems to be a valid file. Restoring.');
   // shutting down the current DB
   await this.sequelize.close();
-  // copy the backupFile to the new DB
-  await exec(`sqlite3 ${this.config.storage} ".restore '${sqliteBackupFilePath}'"`);
+  // copy the backupFile to the new DB. The dot command is passed as a single
+  // argument to sqlite3, with no shell in between: a backup file name can no
+  // longer be read as a command.
+  await execFile('sqlite3', [this.config.storage, `.restore '${sqliteBackupFilePath}'`]);
   // done!
   logger.info(`SQLite backup restored`);
   if (duckDbBackupFolderPath) {
@@ -62,7 +65,9 @@ async function restoreBackup(sqliteBackupFilePath, duckDbBackupFolderPath) {
       .replace('CREATE SCHEMA information_schema;', '')
       .replace('CREATE SCHEMA pg_catalog;', '');
     await fse.writeFile(schemaFilePath, schemaCleaned);
-    await duckDbWriteConnection.run(`IMPORT DATABASE '${duckDbBackupFolderPath}'`);
+    // the folder name comes from the restored archive: DuckDB can read and write
+    // files from SQL, so the path is escaped before being inlined in the statement
+    await duckDbWriteConnection.run(`IMPORT DATABASE '${escapeSqlStringLiteral(duckDbBackupFolderPath)}'`);
     logger.info(`DuckDB restored with success`);
     duckDbWriteConnection.disconnectSync();
     duckDbInstance.closeSync();

server/services/mqtt/api/mqtt.controller.js

@@ -78,8 +78,10 @@ module.exports = function MqttController(mqttManager) {
       admin: true,
       controller: asyncMiddleware(setDebugMode),
     },
+    // returns the broker URL, username and password in clear
     'get /api/v1/service/mqtt/config': {
       authenticated: true,
+      admin: true,
       controller: asyncMiddleware(getConfiguration),
     },
     'post /api/v1/service/mqtt/config/docker': {

server/services/netatmo/api/netatmo.controller.js

@@ -98,12 +98,15 @@ module.exports = function NetatmoController(netatmoHandler) {
   }
 
   return {
+    // returns the Netatmo client id and client secret in clear
     'get /api/v1/service/netatmo/configuration': {
       authenticated: true,
+      admin: true,
       controller: asyncMiddleware(getConfiguration),
     },
     'post /api/v1/service/netatmo/configuration': {
       authenticated: true,
+      admin: true,
       controller: asyncMiddleware(saveConfiguration),
     },
     'get /api/v1/service/netatmo/status': {

server/services/nuki/api/nuki.controller.js

@@ -73,12 +73,15 @@ module.exports = function NukiController(nukiHandler) {
       authenticated: true,
       controller: asyncMiddleware(connect),
     },
+    // returns the Nuki API key in clear
     'get /api/v1/service/nuki/config': {
       authenticated: true,
+      admin: true,
       controller: asyncMiddleware(getConfiguration),
     },
     'post /api/v1/service/nuki/config': {
       authenticated: true,
+      admin: true,
       controller: asyncMiddleware(saveConfiguration),
     },
     'get /api/v1/service/nuki/discover/:protocol': {

server/services/tuya/api/tuya.controller.js

@@ -101,6 +101,7 @@ module.exports = function TuyaController(tuyaManager) {
     },
     'post /api/v1/service/tuya/configuration': {
       authenticated: true,
+      admin: true,
       controller: asyncMiddleware(saveConfiguration),
     },
     'post /api/v1/service/tuya/disconnect': {

server/services/zigbee2mqtt/api/zigbee2mqtt.controller.js

@@ -162,12 +162,16 @@ module.exports = function Zigbee2mqttController(gladys, zigbee2mqttManager) {
       authenticated: true,
       controller: asyncMiddleware(status),
     },
+    // the setup holds the MQTT broker credentials Zigbee2mqtt connects with
+    // (GLADYS_MQTT_PASSWORD among them) and hands them straight to res.json()
     'get /api/v1/service/zigbee2mqtt/setup': {
       authenticated: true,
+      admin: true,
       controller: asyncMiddleware(getCurrentSetup),
     },
     'post /api/v1/service/zigbee2mqtt/setup': {
       authenticated: true,
+      admin: true,
       controller: asyncMiddleware(setup),
     },
     'post /api/v1/service/zigbee2mqtt/connect': {

server/services/zwavejs-ui/api/zwaveJSUI.controller.js

@@ -67,10 +67,13 @@ module.exports = function ZwaveJSUIController(zwaveJSUIHandler) {
     },
     'post /api/v1/service/zwavejs-ui/configuration': {
       authenticated: true,
+      admin: true,
       controller: asyncMiddleware(saveConfiguration),
     },
+    // returns the Z-Wave JS UI broker URL, username and password in clear
     'get /api/v1/service/zwavejs-ui/configuration': {
       authenticated: true,
+      admin: true,
       controller: asyncMiddleware(getConfiguration),
     },
     'post /api/v1/service/zwavejs-ui/connect': {

server/test/controllers/serviceSecretRoutes.test.js

@@ -0,0 +1,38 @@
+const { expect } = require('chai');
+
+const MqttController = require('../../services/mqtt/api/mqtt.controller');
+const NetatmoController = require('../../services/netatmo/api/netatmo.controller');
+const NukiController = require('../../services/nuki/api/nuki.controller');
+const TuyaController = require('../../services/tuya/api/tuya.controller');
+const ZwaveJSUIController = require('../../services/zwavejs-ui/api/zwaveJSUI.controller');
+const Zigbee2mqttController = require('../../services/zigbee2mqtt/api/zigbee2mqtt.controller');
+
+// Some services expose their stored credentials through a route of their own,
+// next to the generic /api/v1/service/:service_name/variable/:variable_key one:
+// they hand the configuration object straight to res.json(), broker password
+// and OAuth client secret included. Those routes are the same secrets by
+// another door, so they must stay admin-only. The controllers only build their
+// route map here, they never touch the handler, so a bare object is enough.
+const SECRET_BEARING_ROUTES = [
+  { name: 'mqtt', controller: MqttController, route: 'get /api/v1/service/mqtt/config' },
+  { name: 'netatmo', controller: NetatmoController, route: 'get /api/v1/service/netatmo/configuration' },
+  { name: 'netatmo', controller: NetatmoController, route: 'post /api/v1/service/netatmo/configuration' },
+  { name: 'nuki', controller: NukiController, route: 'get /api/v1/service/nuki/config' },
+  { name: 'nuki', controller: NukiController, route: 'post /api/v1/service/nuki/config' },
+  { name: 'tuya', controller: TuyaController, route: 'post /api/v1/service/tuya/configuration' },
+  { name: 'zwavejs-ui', controller: ZwaveJSUIController, route: 'get /api/v1/service/zwavejs-ui/configuration' },
+  { name: 'zwavejs-ui', controller: ZwaveJSUIController, route: 'post /api/v1/service/zwavejs-ui/configuration' },
+  { name: 'zigbee2mqtt', controller: Zigbee2mqttController, route: 'get /api/v1/service/zigbee2mqtt/setup' },
+  { name: 'zigbee2mqtt', controller: Zigbee2mqttController, route: 'post /api/v1/service/zigbee2mqtt/setup' },
+];
+
+describe('Service routes carrying integration secrets', () => {
+  SECRET_BEARING_ROUTES.forEach(({ name, controller, route }) => {
+    it(`should keep "${route}" reserved to admin users`, () => {
+      const routes = controller({});
+      expect(routes, `${name} does not declare ${route} anymore`).to.have.property(route);
+      expect(routes[route]).to.have.property('authenticated', true);
+      expect(routes[route]).to.have.property('admin', true);
+    });
+  });
+});