SECURITY: CommandBuilder fixes

Closes #599 This was an issue privately reported by @cruatta in #593 who also helped in resolving the issue - huge thanks! I also updated some other unit tests to use `assert.match` for better error reporting when (if) tests are broken.
sbs20 · May 7, 2023 · 8314599 · 8314599
1 parent 0023f82
commit 8314599
Show file tree

Hide file tree

Showing 5 changed files with 98 additions and 16 deletions.
diff --git a/packages/server/src/classes/command-builder.js b/packages/server/src/classes/command-builder.js
@@ -15,14 +15,17 @@ module.exports = class CommandBuilder {
    * @returns {string}
    */
   _format(value) {
-    if (typeof value === 'string') {
+    if (['boolean', 'number'].includes(typeof value)) {
+      return `${value}`;
+    } else if ('string' === typeof value) {
       if (value.includes('\'')) {
         throw Error('Argument must not contain single quote "\'"');
-      } else if (['$', ' ', '#', '\\', ';'].some(c => value.includes(c))) {
-        return `'${value}'`;
+      } else if (/^[0-9a-z-=/~.:]+$/i.test(value)) {
+        return `${value}`;
       }
+      return `'${value}'`;
     }
-    return `${value}`;
+    throw Error(`Invalid argument type: '${typeof value}'`);
   }
 
   /**
@@ -36,6 +39,18 @@ module.exports = class CommandBuilder {
     return this;
   }
 
+  /**
+   * @param {string} operator
+   * @returns {CmdBuilder}
+   */
+  redirect(operator) {
+    if (typeof operator !== 'string' || !/^[&<>|]+$/.test(operator)) {
+      throw Error(`Invalid argument: '${operator}'`);
+    }
+    this.args.push(operator);
+    return this;
+  }
+
   /**
    * @param {boolean} [ignoreStderr]
    * @returns {string}

diff --git a/packages/server/src/classes/scanimage-command.js b/packages/server/src/classes/scanimage-command.js
@@ -132,7 +132,7 @@ module.exports = class ScanimageCommand {
       if (this.scanimage.supportsOutputFlag) {
         cmdBuilder.arg('-o', outputFile);
       } else {
-        cmdBuilder.arg('>', outputFile);
+        cmdBuilder.redirect('>').arg(outputFile);
       }
     }
     return cmdBuilder.build();

diff --git a/packages/server/test/command-builder.test.js b/packages/server/test/command-builder.test.js
@@ -9,12 +9,36 @@ describe('CommandBuilder', () => {
       'echo');
   });
 
-  it('command-arg', async () => {
+  it('command-arg-number', async () => {
+    assert.strictEqual(
+      new CommandBuilder('echo').arg(1).build(),
+      'echo 1');
+  });
+
+  it('command-arg-boolean', async () => {
+    assert.strictEqual(
+      new CommandBuilder('echo').arg(true).build(),
+      'echo true');
+  });
+
+  it('command-arg-string-no-space', async () => {
+    assert.strictEqual(
+      new CommandBuilder('echo').arg('hello-world').build(),
+      'echo hello-world');
+  });
+
+  it('command-arg-string-space', async () => {
     assert.strictEqual(
       new CommandBuilder('echo').arg('hello world').build(),
       'echo \'hello world\'');
   });
 
+  it('command-arg-string-tab-backtick', async () => {
+    assert.strictEqual(
+      new CommandBuilder('echo').arg('`cat\t/etc/os-release\t1>&2`').build(),
+      'echo \'`cat\t/etc/os-release\t1>&2`\'');
+  });
+
   it('command-arg-hash', async () => {
     assert.strictEqual(
       new CommandBuilder('echo').arg('-n', 'hello#world').build(),
@@ -27,6 +51,12 @@ describe('CommandBuilder', () => {
       'echo -n \'hello;world\'');
   });
 
+  it('command-arg-redirect', async () => {
+    assert.strictEqual(
+      new CommandBuilder('echo').arg('> thing').build(),
+      'echo \'> thing\'');
+  });
+
   it('command-security-1', async () => {
     assert.strictEqual(
       new CommandBuilder('echo').arg('-n', 'hello" && ls -al;# world').build(),
@@ -36,13 +66,48 @@ describe('CommandBuilder', () => {
   it('command-security-2', async () => {
     assert.throws(
       () => new CommandBuilder('echo').arg('-n', 'hello\' && echo break shell').build(),
-      Error,
-      'Broke shell');
+      /Error: Argument.*single quote.*/);
+  });
+
+  it('command-security-array', async () => {
+    assert.throws(
+      () => new CommandBuilder('echo').arg(['`cat /etc/os-release 1>&2`']).build(),
+      /Error: Invalid argument.*object.*/);
+  });
+
+  it('command-security-object', async () => {
+    assert.throws(
+      () => new CommandBuilder('echo').arg({arg: '`cat /etc/os-release 1>&2`'}).build(),
+      /Error: Invalid argument.*object.*/);
   });
 
   it('command-quotes"', async () => {
     assert.strictEqual(
       new CommandBuilder('echo').arg('"1\n2\n3"').build(),
-      'echo "1\n2\n3"');
+      'echo \'"1\n2\n3"\'');
+  });
+
+  it('command-redirect-good"', async () => {
+    assert.strictEqual(
+      new CommandBuilder('echo').arg('"hello"').redirect('>').arg('output').build(),
+      'echo \'"hello"\' > output');
+  });
+
+  it('command-redirect-bad-string', async () => {
+    assert.throws(
+      () => new CommandBuilder('echo').redirect('a').build(),
+      /Error: Invalid argument.*/);
+  });
+
+  it('command-redirect-bad-number', async () => {
+    assert.throws(
+      () => new CommandBuilder('echo').redirect(1).build(),
+      /Error: Invalid argument.*/);
+  });
+
+  it('command-redirect-bad-array', async () => {
+    assert.throws(
+      () => new CommandBuilder('echo').redirect(['invalid']).build(),
+      /Error: Invalid argument.*/);
   });
 });
diff --git a/packages/server/test/process.test.js b/packages/server/test/process.test.js
@@ -17,13 +17,15 @@ describe('Process', () => {
     result = await Process.execute(new CmdBuilder('echo').arg('-n', '`ls -al`').build());
     assert.strictEqual(result, '`ls -al`');
 
+    result = await Process.execute(new CmdBuilder('echo').arg('-n', '`ls\t-al`').build());
+    assert.strictEqual(result, '`ls\t-al`');
+
     result = await Process.execute(new CmdBuilder('echo').arg('-n', '$(date)').build());
     assert.strictEqual(result, '$(date)');
   });
 
   it('echo "1\\n2\\n3" | wc -l', async () => {
     const cmd = new CmdBuilder('echo').arg('"1\n2\n3"').build();
-    assert.strictEqual(cmd, 'echo "1\n2\n3"');
     const ls = await Process.spawn(cmd);
     const result = await Process.spawn('wc -l', ls);
     assert.strictEqual(result.toString(), '3\n');

diff --git a/packages/server/test/scanimage-command.test.js b/packages/server/test/scanimage-command.test.js
@@ -38,32 +38,32 @@ function commandFor(version, request) {
 describe('ScanimageCommand', () => {
   it('scanimageVersion:1.0.27:scan', () => {
     const command = commandFor('1.0.27', requestScan);
-    assert.ok(command.match(/.*scanimage.* > data\/temp\/~tmp-scan-0-ined.tif/));
+    assert.match(command, /.*scanimage.* > data\/temp\/~tmp-scan-0-ined.tif/);
   });
 
   it('scanimageVersion:1.0.27:preview', () => {
     const command = commandFor('1.0.27', requestPreview);
-    assert.ok(command.match(/.*scanimage.* > data\/preview\/preview.tif/));
+    assert.match(command, /.*scanimage.* > data\/preview\/preview.tif/);
   });
 
   it('scanimageVersion:1.0.28:scan', () => {
     const command = commandFor('1.0.28', requestScan);
-    assert.ok(command.match(/.*scanimage.* -o data\/temp\/~tmp-scan-0-ined.tif/));
+    assert.match(command, /.*scanimage.* -o data\/temp\/~tmp-scan-0-ined.tif/);
   });
 
   it('scanimageVersion:1.0.28:preview', () => {
     const command = commandFor('1.0.28', requestPreview);
-    assert.ok(command.match(/.*scanimage.* -o data\/preview\/preview.tif/));
+    assert.match(command, /.*scanimage.* -o data\/preview\/preview.tif/);
   });
 
   it('scanimageVersion:1.0.31:scan', () => {
     const command = commandFor('1.0.31', requestScan);
-    assert.ok(command.match(/.*scanimage.* -o data\/temp\/~tmp-scan-0-ined.tif/));
+    assert.match(command, /.*scanimage.* -o data\/temp\/~tmp-scan-0-ined.tif/);
   });
 
   it('scanimageVersion:1.0.31:preview', () => {
     const command = commandFor('1.0.31', requestPreview);
-    assert.ok(command.match(/.*scanimage.* -o data\/preview\/preview.tif/));
+    assert.match(command, /.*scanimage.* -o data\/preview\/preview.tif/);
   });
 
   it('scanimage-a10.txt', () => {