Add parameter escaping and binary renaming to ensure we don't get rac… (

#14)

* Add parameter escaping and binary renaming to ensure we don't get race conditions if there are multiple inflight requests. Further tests on the controller to come

* Linting fix

* Version bump

* Add in other examples to openapi type

* Additional unit tests

* Fix linting

* Debug CI failure

* Fix test to incorporate env

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@digicatapult/morello-api",
-  "version": "0.4.1",
+  "version": "0.5.0",
   "description": "An interface for executing binaries on it's self and morello host.",
   "main": "src/index.ts",
   "scripts": {

src/controllers/scenario/__tests__/index.test.js

@@ -1,12 +1,17 @@
+const util = require('../../../utils/params')
 const { scenario } = require('../index')
 const { stub } = require('sinon')
 const { expect } = require('chai')
 const child = require('child_process')
+const config = require('config')
+
+const address = `${config.get('morello.username')}@${config.get('morello.address')}`
+const port = config.get('morello.port')
 
 const execute = async () => {
   try {
     const controller = new scenario()
-    return await controller.get()
+    return await controller.get('out-of-bounds-read-cheri', ['test'])
   } catch (err) {
     return err
   }
@@ -17,13 +22,16 @@ describe('/scenario/{example} endpoint', () => {
   let stubs = {}
 
   beforeEach(async () => {
+    stubs.getRandomProcessName = stub(util, 'getRandomProcessName')
+    stubs.getRandomProcessName.returns('out-of-bounds-read-cheri_foo')
     stubs.exec = stub(child, 'exec')
     stubs.exec.onCall(0).yields(null, 'stdout - success')
     stubs.exec.onCall(1)
     res = await execute()
   })
 
   afterEach(() => {
+    stubs.getRandomProcessName.restore()
     stubs.exec.restore()
   })
 
@@ -36,6 +44,17 @@ describe('/scenario/{example} endpoint', () => {
       res = await execute()
     })
 
+    it('calls exec correctly', () => {
+      const firstCallArg = stubs.exec.firstCall.args[0]
+      const expectation = `scp -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -q -P ${port} bin/out-of-bounds-read-cheri ${address}:/tmp/out-of-bounds-read-cheri_foo; ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -q -p ${port} ${address} -t << 'EOF'
+chmod +x /tmp/out-of-bounds-read-cheri;
+/tmp/out-of-bounds-read-cheri_foo 'test' 2>&1;
+exit;
+EOF`
+
+      expect(firstCallArg).to.equal(expectation)
+    })
+
     it('returns correct state along with exceptions if both binaries fail', () => {
       expect(res).to.deep.equal({
         status: 'error',

src/controllers/scenario/index.ts

@@ -9,24 +9,35 @@ import config from 'config'
 import { exec } from 'child_process'
 import { IScenario, HostResponse, Executables } from '../../../types'
 import Logger from '../../utils/Logger'
+import * as util from '../../utils/params'
 
 @Route('scenario')
 export class scenario extends Controller implements IScenario {
   address: string
   port: number
   log: typeof Logger
-  
+
   constructor() {
     super()
     this.address = `${config.get('morello.username')}@${config.get('morello.address')}`
     this.port = config.get('morello.port')
     this.log = Logger.child({ controller: '/scenario', ...config.get('morello') })
   }
 
-  execute(bin: string): Promise<HostResponse> {
-    const scp = `scp -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -q -P ${this.port} bin/${bin} ${this.address}:/tmp`
-    const ssh = `ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -q -p ${this.port} ${this.address} -tt 'chmod +x /tmp/${bin}; /tmp/${bin} with args' 2>&1`
-    const rm = `ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -q -p ${this.port} ${this.address} rm -v /tmp/${bin} &> /dev/null`
+  execute(bin: string, params: string[] = []): Promise<HostResponse> {
+    params = params.map(p => util.escapeParam(p))
+    const destBin = util.getRandomProcessName(bin)
+    const eof = util.getValidHeredocEOF(bin, params)
+
+    const scp = `scp -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -q -P ${this.port} bin/${bin} ${this.address}:/tmp/${destBin}`
+    const ssh =
+`ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -q -p ${this.port} ${this.address} -t << '${eof}'
+chmod +x /tmp/${bin};
+/tmp/${destBin} ${params.join(' ')} 2>&1;
+exit;
+${eof}`
+
+    const rm = `ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -q -p ${this.port} ${this.address} rm -v /tmp/${destBin} &> /dev/null`
     this.log.debug({ msg: `executing ${bin} on ${this.address} host`, scp, ssh })
 
     return new Promise((resolve) => {
@@ -40,11 +51,11 @@ export class scenario extends Controller implements IScenario {
       })
     })
   }
-  
+
   @Get('{executable}')
-  public async get(@Path() executable: Executables , @Query() params: string[]): Promise<HostResponse> {
+  public async get(@Path() executable: Executables , @Query() params?: string[]): Promise<HostResponse> {
     this.log.debug(`attempting to execute ${executable} scenario with [${params}] arguments`)
-    
-    return this.execute(`${executable} ${params}`)
+
+    return this.execute(executable, params)
   }
 }

src/utils/__tests__/params.test.js

@@ -0,0 +1,65 @@
+const { expect } = require('chai')
+const { escapeParam, getValidHeredocEOF, getRandomProcessName } = require('../params')
+
+describe('getRandomProcessName', () => {
+  it('should append a length 10 string of lowercase alpha chars', () => {
+    expect(getRandomProcessName('test')).to.match(/^test_[a-z]{10}$/)
+  })
+
+  it('should always return a different string', () => {
+    const exampleSet = new Set(
+      Array(1000)
+        .fill(null)
+        .map(() => getRandomProcessName('test'))
+    )
+    expect(exampleSet.size).to.equal(1000)
+  })
+})
+
+describe('getValidHeredocEOF', () => {
+  it('should by default returns EOF', () => {
+    expect(getValidHeredocEOF('test', [])).to.equal('EOF')
+  })
+
+  it('should append _ if bin contains EOF', () => {
+    expect(getValidHeredocEOF('test_EOF', [])).to.equal('EOF_')
+  })
+
+  it('should append _ if parameter contains EOF', () => {
+    expect(getValidHeredocEOF('test', ['param_EOF'])).to.equal('EOF_')
+  })
+
+  it('should append __ if bin contains EOF_', () => {
+    expect(getValidHeredocEOF('test_EOF_', [])).to.equal('EOF__')
+  })
+
+  it('should append __ if parameter contains EOF_', () => {
+    expect(getValidHeredocEOF('test_EOF', ['param_EOF_'])).to.equal('EOF__')
+  })
+
+  it(`should append _____ if bin contains EOF____`, () => {
+    expect(getValidHeredocEOF('test_EOF____test', [])).to.equal('EOF_____')
+  })
+})
+
+describe('escapeParams', () => {
+  it('returns input wrapped in quotes', () => {
+    expect(escapeParam('arg1')).to.equal(`'arg1'`)
+  })
+
+  it('replaces single quotes with an escaped quote', () => {
+    expect(escapeParam("arg1'test")).to.equal(`'arg1'\\''test'`)
+  })
+
+  it('replaces all single quotes with an escaped quote', () => {
+    expect(escapeParam("arg1'test'test")).to.equal(`'arg1'\\''test'\\''test'`)
+  })
+
+  it('replaces initial quote correctly', () => {
+    expect(escapeParam("'arg1")).to.equal(`''\\''arg1'`)
+  })
+
+  it('replaces trailing quote correctly', () => {
+    expect(escapeParam("arg1'")).to.equal(`'arg1'\\'''`)
+  })
+})

src/utils/params.ts

@@ -0,0 +1,17 @@
+const processChars: string = 'abcdefghijklmnopqrstuvwxyz'
+export const getRandomProcessName = (bin: string): string => {
+  return `${bin}_${Array(10).fill(null).map(_ => processChars[Math.floor(Math.random() * processChars.length)]).join('')}`
+}
+
+export const getValidHeredocEOF = (bin: string, params: string[]): string => {
+  const fullCmd: string = `/tmp/${bin} ${params.join(' ')}`
+  let eof: string = 'EOF'
+  while (fullCmd.includes(eof)) {
+    eof = `${eof}_`
+  }
+  return eof
+}
+
+export const escapeParam = (param: string): string => {
+  return `'${param.replace(/'/g, "'\\''")}'`
+}

types/models/scenario.ts

@@ -1,11 +1,21 @@
 import { ExecException } from 'child_process'
 import Logger from '../../src/utils/Logger'
 
-export type Executables = 'out-of-bounds-read' | 'out-of-bounds-write'
+export type Executables =
+  'out-of-bounds-access-aarch64' |
+  'out-of-bounds-access-cheri' |
+  'out-of-bounds-read-aarch64' |
+  'out-of-bounds-read-cheri' |
+  'out-of-bounds-readV2-aarch64' |
+  'out-of-bounds-readV2-cheri' |
+  'out-of-bounds-write-aarch64' |
+  'out-of-bounds-write-cheri' |
+  'use-after-free-aarch64' |
+  'use-after-free-cheri'
 
 export type HostResponse = {
   status: 'success' | 'error' | 'exception',
-  output: string, 
+  output: string,
   exception?: ExecException,
 }
 
@@ -14,5 +24,5 @@ export interface IScenario {
   readonly port: number
   log: typeof Logger
   get: (executable: Executables, params: string[]) => Promise<HostResponse>
-  execute: (cmd: string) => Promise<HostResponse>
+  execute: (cmd: string, params: string[]) => Promise<HostResponse>
 }