CVE-2019-1010200: Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection') (CWE-78)
value: Voice Builder Prior to commit c145d4604df67e6fc625992412eef0bf9a85e26b and f6660e6d8f0d1d931359d591dbdec580fef36d36 is affected by: CWE-78: Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection'). The impact is: Remote code execution with the same privileges as the servers. The component is: Two web servers in the projects expose three vulnerable endpoints that can be accessed remotely. The endpoints are defined at: - /tts: https://github.com/google/voice-builder/blob/3a449a3e8d5100ff323161c89b897f6d5ccdb6f9/merlin_model_server/api.js#L34 - /alignment: https://github.com/google/voice-builder/blob/3a449a3e8d5100ff323161c89b897f6d5ccdb6f9/festival_model_server/api.js#L28 - /tts: https://github.com/google/voice-builder/blob/3a449a3e8d5100ff323161c89b897f6d5ccdb6f9/festival_model_server/api.js#L65. The attack vector is: Attacker sends a GET request to the vulnerable endpoint with a specially formatted query parameter. The fixed version is: After commit f6660e6d8f0d1d931359d591dbdec580fef36d36.
CWE description: The product constructs all or part of an OS command using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify the intended OS command when it is sent to a downstream component.
CVSS v3 base score: 9.8
Published: 2019-07-23T18:15Z
Repository: https://github.com/google/voice-builder
Language: JavaScript
Files changed: merlin_model_server/api.js, merlin_model_server/jasmine.json, merlin_model_server/settings.js, merlin_model_server/tests/UtilsTests.js, merlin_model_server/utils.js
Commit date: 2018-09-24 20:04:58 +0800
Commit message: Added function to replace characters with space and use it to sanitize text

Change-Id: I3d14230444fefa0ce739c498eee6d100bcfe276a

Diff: diff --git a/merlin_model_server/api.js b/merlin_model_server/api.js
index 528ad53..93874e9 100644
--- a/merlin_model_server/api.js
+++ b/merlin_model_server/api.js
@@ -31,34 +31,31 @@ router.get('/healthz', (_req, res) => res.json('OK'));
 router.get('/alignment', (req, res) => res.status(200).send([]));
 
 /** GET synthesize voice based on a voice model */
 router.get('/tts', (req, res) => {
   const { text, type } = req.query;
-  // TODO(twattanavekin): Test Unicode text.
-  const escapedText = JSON.stringify(text).slice(1, -1);
+  const sanitizedText = utils.replaceCharactersWithSpaces(text);
 
-  // Example command:
-  //  echo "hello" | ${SYNTH_SCRIPT}
-
-  const cmd = `echo "${escapedText}" | ${SYNTH_SCRIPT}`;
-  console.log(`Running command - \n${cmd}`);
+  console.log(`Synthesizing ${sanitizedText}`);
 
   const options = {
     maxBuffer: 1024 * 1000, // 1 mb buffer
     cwd: '/tmp/',
     timeout: 60000, // 1 minute timeout
     encoding: 'buffer',
   };
 
-  exec(cmd, options, (err, stdout, stderr) => {
+  const child = exec(SYNTH_SCRIPT, options, (err, stdout, stderr) => {
     const errMsg = utils.getExecErrorMessage(err, stderr);
     if (errMsg) {
       return res.status(500).send(errMsg);
     }
     res.status(200);
     res.set('Content-Type', 'audio/wav');
     const data = type === 'base64' ? stdout.toString('base64') : stdout;
     return res.send(data);
   });
+  child.stdin.write(sanitizedText);
+  child.stdin.end();
 });
 
 module.exports = router;
diff --git a/merlin_model_server/jasmine.json b/merlin_model_server/jasmine.json
index 9f7f042..0c4fede 100644
--- a/merlin_model_server/jasmine.json
+++ b/merlin_model_server/jasmine.json
@@ -1,6 +1,8 @@
 {
   "spec_dir": "tests",
-  "spec_files": [],
+  "spec_files": [
+    "UtilsTests.js"
+  ],
   "stopSpecOnExpectationFailure": false,
   "random": false
 }
diff --git a/merlin_model_server/settings.js b/merlin_model_server/settings.js
new file mode 100644
index 0000000..ec53c6c
--- /dev/null
+++ b/merlin_model_server/settings.js
@@ -0,0 +1,24 @@
+// Copyright 2018 Google Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+/**
+ * Module containing settings for merlin server.
+ */
+const settings = {
+  // Merlin synthesis pipeline might not handle these characters as expected
+  // and also they are typically not needed during tts synthesis.
+  // Therefore we replace them with spaces.
+  CHARACTERS_TO_REPLACE_WITH_SPACES: '!"#$%&\'()*+,-./:;<=>?@[\\]^_`{|}~',
+};
+module.exports = settings;
diff --git a/merlin_model_server/tests/UtilsTests.js b/merlin_model_server/tests/UtilsTests.js
new file mode 100644
index 0000000..548c4dc
--- /dev/null
+++ b/merlin_model_server/tests/UtilsTests.js
@@ -0,0 +1,43 @@
+// Copyright 2018 Google Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+const utils = require('../utils');
+
+describe('Testing utils replaceCharactersWithSpaces method', () => {
+  it('The function should return characters replaced with spaces and trim', () => {
+    const testData = [
+      ['`<text>`', 'text'],
+      ['`text`', 'text'],
+      ['$(text)', 'text'],
+      ['"\$(text\)', 'text'],
+      ['\"\$(text\)', 'text'],
+      ['${{{text}', 'text'],
+      ['\$\{text\}', 'text'],
+      ['\${text"}', 'text'],
+      ['"${{text"}', 'text'],
+      ['"${text"}', 'text'],
+      ["\"ශ්රී ලංකාව'\"", 'ශ්රී ලංකාව'],
+      ['"$text"', 'text'],
+      ['$text', 'text'],
+      [",./;'[]*\-=", ''],
+      ['echo `<text>`', 'echo   text'],
+      ['echo $("text")', 'echo    text'],
+      ['echo \$\{text\}', 'echo   text'],
+    ];
+
+    testData.forEach(function(entry) {
+      expect(utils.replaceCharactersWithSpaces(entry[0])).toEqual(entry[1]);
+    });
+  });
+});
diff --git a/merlin_model_server/utils.js b/merlin_model_server/utils.js
index 8f26f41..208775f 100644
--- a/merlin_model_server/utils.js
+++ b/merlin_model_server/utils.js
@@ -10,10 +10,12 @@
 // distributed under the License is distributed on an "AS IS" BASIS,
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+const settings = require('./settings');
+
 /**
  * Returns a error message for the given parameters.
  * @param {String} err error from the exec call.
  * @param {String} stderr standard error from the exec call.
  */
@@ -25,9 +27,20 @@ const getExecErrorMessage = (err, stderr) => {
     errMsg = `stderr: ${stderr.toString()}`;
   }
   return errMsg;
 };
 
+/**
+ * Returns the original string with characters
+ * specified in settings replaced with spaces.
+ * @text {String} text to be replaced.
+ */
+const replaceCharactersWithSpaces = (text) => {
+  const re = new RegExp(`[${settings.CHARACTERS_TO_REPLACE_WITH_SPACES}]`, 'g');
+  const newText = text.replace(re, ' ').trim();
+  return newText;
+};
 
 module.exports = {
   getExecErrorMessage,
+  replaceCharactersWithSpaces,
 };

Vulnerable code:   // TODO(twattanavekin): Test Unicode text.
  const escapedText = JSON.stringify(text).slice(1, -1);
  // Example command:
  //  echo "hello" | ${SYNTH_SCRIPT}

  const cmd = `echo "${escapedText}" | ${SYNTH_SCRIPT}`;
  console.log(`Running command - \n${cmd}`);
  exec(cmd, options, (err, stdout, stderr) => {
  "spec_files": [],
Fixed code:   const sanitizedText = utils.replaceCharactersWithSpaces(text);
  console.log(`Synthesizing ${sanitizedText}`);
  const child = exec(SYNTH_SCRIPT, options, (err, stdout, stderr) => {
  child.stdin.write(sanitizedText);
  child.stdin.end();
  "spec_files": [
    "UtilsTests.js"
  ],
// Copyright 2018 Google Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//     https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

/**
 * Module containing settings for merlin server.
 */
const settings = {
  // Merlin synthesis pipeline might not handle these characters as expected
  // and also they are typically not needed during tts synthesis.
  // Therefore we replace them with spaces.
  CHARACTERS_TO_REPLACE_WITH_SPACES: '!"#$%&\'()*+,-./:;<=>?@[\\]^_`{|}~',
};
module.exports = settings;
// Copyright 2018 Google Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//     https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

const utils = require('../utils');

describe('Testing utils replaceCharactersWithSpaces method', () => {
  it('The function should return characters replaced with spaces and trim', () => {
    const testData = [
      ['`<text>`', 'text'],
      ['`text`', 'text'],
      ['$(text)', 'text'],
      ['"\$(text\)', 'text'],
      ['\"\$(text\)', 'text'],
      ['${{{text}', 'text'],
      ['\$\{text\}', 'text'],
      ['\${text"}', 'text'],
      ['"${{text"}', 'text'],
      ['"${text"}', 'text'],
      ["\"ශ්රී ලංකාව'\"", 'ශ්රී ලංකාව'],
      ['"$text"', 'text'],
      ['$text', 'text'],
      [",./;'[]*\-=", ''],
      ['echo `<text>`', 'echo   text'],
      ['echo $("text")', 'echo    text'],
      ['echo \$\{text\}', 'echo   text'],
    ];

    testData.forEach(function(entry) {
      expect(utils.replaceCharactersWithSpaces(entry[0])).toEqual(entry[1]);
    });
  });
});
const settings = require('./settings');

/**
 * Returns the original string with characters
 * specified in settings replaced with spaces.
 * @text {String} text to be replaced.
 */
const replaceCharactersWithSpaces = (text) => {
  const re = new RegExp(`[${settings.CHARACTERS_TO_REPLACE_WITH_SPACES}]`, 'g');
  const newText = text.replace(re, ' ').trim();
  return newText;
};
  replaceCharactersWithSpaces,