feat: sanitize file checks

packages/utils/commands.js

@@ -5,12 +5,12 @@ const fileTypes = {
   socket: '-S',
 }
 
+function sanitizePath(path) {
+  return path.replace(/[^a-zA-Z0-9._/-]/g, '')
+}
+
 // eslint-disable-next-line import/prefer-default-export
-export function buildFileCheckCmd(files) {
-  return files
-    .map(
-      ({ path, type, exists }) =>
-        `${exists ? '' : '! '}test ${fileTypes[type] || '-e'} ${path}`
-    )
-    .join(' && ')
+export function buildFileCheckCmd(file) {
+  const { path, type } = file
+  return ['test', fileTypes[type] || '-e', sanitizePath(path)]
 }

packages/utils/commands.test.js

@@ -4,35 +4,38 @@ describe('buildFileCheckCmd function', () => {
   it('builds the file check command correctly', () => {
     const testcases = [
       {
-        input: [{ path: '/etc/passwd', type: 'file', exists: true }],
-        output: 'test -f /etc/passwd',
+        input: { path: '/etc/passwd', type: 'file' },
+        output: ['test', '-f', '/etc/passwd'],
       },
       {
-        input: [{ path: '/etc', type: 'folder', exists: false }],
-        output: '! test -d /etc',
+        input: { path: '/etc', type: 'folder' },
+        output: ['test', '-d', '/etc'],
       },
       {
-        input: [
-          { path: '/etc/passwd', type: 'file', exists: true },
-          { path: '/etc', type: 'folder', exists: false },
-        ],
-        output: 'test -f /etc/passwd && ! test -d /etc',
+        input: { path: '/tmp/socket', type: 'socket' },
+        output: ['test', '-S', '/tmp/socket'],
       },
       {
-        input: [
-          { path: '/tmp/socket', type: 'socket', exists: true },
-          { path: '/tmp/link', type: 'link', exists: false },
-        ],
-        output: 'test -S /tmp/socket && ! test -L /tmp/link',
+        input: { path: '/tmp/link', type: 'link' },
+        output: ['test', '-L', '/tmp/link'],
       },
       {
-        input: [{ path: '/unknown', type: 'unknown', exists: true }],
-        output: 'test -e /unknown',
+        input: { path: '/unknown', type: 'unknown' },
+        output: ['test', '-e', '/unknown'],
+      },
+      // Add some injection tests
+      {
+        input: { path: '/etc/passwd;ls', type: 'file' },
+        output: ['test', '-f', '/etc/passwdls'],
+      },
+      {
+        input: { path: '/etc/passwd && ls', type: 'file' },
+        output: ['test', '-f', '/etc/passwdls'],
       },
     ]
 
     testcases.forEach(({ input, output }) => {
-      expect(buildFileCheckCmd(input)).toBe(output)
+      expect(buildFileCheckCmd(input)).toStrictEqual(output)
     })
   })
 })

quests/helloworld/game.yml

@@ -149,3 +149,18 @@ stages:
       - type: narrative
         content:
           - There is a jail
+
+  - id: test-file-not-exist
+    task: Remove the forgotten_scroll.txt
+    repeatable: true
+    condition:
+      command:
+        - ls -a
+      $not:
+        files:
+          - path: /home/commander/forgotten_scroll.txt
+            type: file
+    responses:
+      - type: narrative
+        content:
+          - There is no forgotten_scroll.txt

server/Dockerfile

@@ -30,5 +30,6 @@ CMD [ "yarn", "start" ]
 FROM build as dev
 
 RUN yarn install --frozen-lockfile
+COPY server/nodemon.json ./server/nodemon.json
 ENV NODE_ENV=development
 CMD [ "yarn", "server" ]

server/nodemon.json

@@ -1,4 +1,4 @@
 {
-  "watch": ["src", "swagger.yaml", "../quests"],
+  "watch": ["src", "swagger.yaml", "../quests", "../packages"],
   "ext": "js,yaml,yml"
 }

server/src/containers/cli.js

@@ -1,16 +1,32 @@
-import { exec } from 'child_process'
+import { execFile } from 'child_process'
 
 import { buildFileCheckCmd } from '@linux-odyssey/utils'
 
+function sanitizeId(id) {
+  return id.replace(/[^a-zA-Z0-9]/g, '')
+}
+
 // eslint-disable-next-line import/prefer-default-export
-export function checkFiles(id, files) {
-  return new Promise((resolve) => {
-    const cmd = buildFileCheckCmd(files)
-    // WARNING: This is not secure!
-    exec(`docker exec ${id} ${cmd}`, (error, stdout, stderr) => {
-      console.log('stdout', stdout)
-      console.log('stderr', stderr)
-      resolve(!error)
-    })
+export function checkFile(id, file) {
+  return new Promise((resolve, reject) => {
+    const cmd = buildFileCheckCmd(file)
+    console.log(cmd)
+    // WARNING: This is vulnerable to command injection
+    execFile(
+      'docker',
+      ['exec', sanitizeId(id), ...cmd],
+      { timeout: 1000 },
+      (error, stdout, stderr) => {
+        if (stderr) console.log('error', error)
+        if (stdout) console.log('stdout', stdout)
+        if (error) console.log('error', error)
+        const exists = !error
+        if (exists === file.exists) {
+          resolve(true)
+        } else {
+          reject(new Error('File check failed'))
+        }
+      }
+    )
   })
 }

server/src/game/commandHandler.js

@@ -3,7 +3,7 @@ import { FileGraph } from '@linux-odyssey/file-graph'
 import { Quest } from '@linux-odyssey/models'
 import { pushToSession } from '../api/socket.js'
 import SessionHandler from './sessionHandler.js'
-import { checkFiles } from '../containers/cli.js'
+import { checkFile } from '../containers/cli.js'
 
 // 檢查 pattern 是否符合 input
 const checkMatch = (pattern, input) => {
@@ -55,30 +55,47 @@ export default class CommandHandler extends SessionHandler {
   }
 
   async isMatch(condition) {
-    const keys = ['command', 'output', 'error', 'pwd']
-    if (keys.some((k) => !checkMatch(condition[k], this.commandInput[k]))) {
-      return false
-    }
-    if (condition.files.length > 0) {
-      const match = await this.checkFiles(condition.files)
-      if (!match) return false
-    }
-    if (condition.$not) {
-      const match = await this.isMatch(condition.$not)
-      if (match) return false
-    }
-    if (condition.$or.length > 0) {
-      const matches = await Promise.all(
-        condition.$or.map((c) => this.isMatch(c))
-      )
-      if (matches.every((m) => m === false)) return false
-    }
-    return true
+    return (
+      this.checkKeys(condition) &&
+      (await this.checkFiles(condition.files)) &&
+      (await this.checkNot(condition.$not)) &&
+      (await this.checkOr(condition.$or)) === true
+    )
   }
 
-  checkFiles(files) {
+  checkKeys(condition) {
+    const keys = ['command', 'output', 'error', 'pwd']
+    return keys.every((k) => checkMatch(condition[k], this.commandInput[k]))
+  }
+
+  async checkFiles(files) {
+    if (files.length === 0) {
+      return true
+    }
     console.log('checkFiles', files)
-    return checkFiles(this.session.containerId, files)
+    try {
+      const checks = await Promise.all(
+        files.map((f) => checkFile(this.session.containerId, f))
+      )
+      return checks.every((c) => c === true)
+    } catch (e) {
+      return false
+    }
+  }
+
+  async checkNot(condition) {
+    if (!condition) {
+      return true
+    }
+    return !(await this.isMatch(condition))
+  }
+
+  async checkOr(conditions) {
+    if (conditions.length === 0) {
+      return true
+    }
+    const matches = await Promise.all(conditions.map((c) => this.isMatch(c)))
+    return matches.some((m) => m === true)
   }
 
   async run() {
@@ -98,12 +115,12 @@ export default class CommandHandler extends SessionHandler {
 
     console.log('matches', matches)
 
-    const filteredStages = stages.filter((_, i) => matches[i])
-    console.log('filteredStages', filteredStages)
-    const response = filteredStages.reduce(
-      (r, s) => ({ ...r, ...this.execute(s) }),
-      {}
-    )
+    const stage = stages.find((_, i) => matches[i])
+    console.log('stage', stage)
+    if (!stage) {
+      return {}
+    }
+    const response = this.execute(stage)
 
     return {
       end: this.session.status === 'finished',