Skip to content

Commit

Permalink
fix: do not allow command injection
Browse files Browse the repository at this point in the history 
Due to usage of execSync, callers may inject commands, like getPath("something & touch abc", "somethingElse & touch def").
To fix this, replace execSync with spawnSync, which is safe for command injection.
  • Loading branch information
@rosen-vladimirov
rosen-vladimirov committed Jan 8, 2023
1 parent 8c8ede0 commit edbdaff
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 51 deletions.
27 changes: 23 additions & 4 deletions lib/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,26 @@ const getNpmExecutable = (platform) => {
return npmExecutableName;
};

const spawnSyncWrapper = (command, commandArgs) => {
const result = childProcess.spawnSync(command, commandArgs);
if (!result) {
return null;
}

if (result.error) {
throw result.error;
}

if (result.stdout) {
return result.stdout.toString().trim();
}

return null;
};

const getNpmPrefix = (pathToNpm) => {
try {
const npmPrefixStdout = childProcess.execSync(`${pathToNpm} config get prefix`);
const npmPrefixStdout = spawnSyncWrapper(pathToNpm, ["config", "get", "prefix"]);
return npmPrefixStdout && npmPrefixStdout.toString().trim();
} catch (err) {
console.error(err.message);
Expand All @@ -49,6 +66,7 @@ const getPathFromNpmConfig = (platform, packageName) => {

const packagePath = path.join(nodeModulesPath, packageName);
const verifiedPath = getVerifiedPath(packagePath, packageName);

if (verifiedPath) {
return verifiedPath;
}
Expand Down Expand Up @@ -107,7 +125,8 @@ const getVerifiedPath = (suggestedPath, packageName) => {

const getPathFromExecutableNameOnWindows = (packageName, executableName) => {
try {
const whereResult = (childProcess.execSync(`where ${executableName}`) || "").toString().split("\n");
const whereResult = (spawnSyncWrapper("where", executableName) || "").split("\n");

for (const line of whereResult) {
const pathToExecutable = line && line.trim();

Expand Down Expand Up @@ -145,8 +164,8 @@ const getPathFromExecutableNameOnNonWindows = (packageName, executableName) => {

// whichResult: /usr/local/nvm/versions/node/v4.2.1/bin/mobile-cli-lib
// lsLResult: lrwxrwxrwx 1 rvladimirov rvladimirov 52 Oct 20 14:51 /usr/local/nvm/versions/node/v4.2.1/bin/mobile-cli-lib -> ../lib/node_modules/mobile-cli-lib/bin/common-lib.js
const whichResult = (childProcess.execSync(`which ${executableName}`) || "").toString().trim(),
lsLResult = (childProcess.execSync(`ls -l \`which ${executableName}\``) || "").toString().trim();
const whichResult = spawnSyncWrapper("which", [executableName]);
const lsLResult = spawnSyncWrapper("ls", ["-l", whichResult]);

if (whichResult && lsLResult) {
const regex = new RegExp(`${whichResult}\\s+->\\s+(.*?)$`),
Expand Down
95 changes: 48 additions & 47 deletions test/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ let processWrapper = require("../lib/process-wrapper");
let fs = require("fs"),
childProcess = require("child_process");

const originalExecSync = childProcess.execSync;
const originalspawnSync = childProcess.spawnSync;
const originalConsoleError = console.error;
const originalExistsSync = fs.existsSync;
const originalReadFileSync = fs.readFileSync;
Expand All @@ -21,7 +21,7 @@ describe("getPath", () => {

afterEach(() => {
processWrapper.getProcessPlatform = () => process.platform;
childProcess.execSync = originalExecSync;
childProcess.spawnSync = originalspawnSync;
console.error = originalConsoleError;
fs.existsSync = originalExistsSync;
fs.readFileSync = originalReadFileSync;
Expand All @@ -41,9 +41,9 @@ describe("getPath", () => {
const expectedErrorMessage = "npm throws";
let errors = [];
console.error = (message) => errors.push(message);
childProcess.execSync = (command) => {
if (command.indexOf("get prefix") !== -1) {
throw new Error(expectedErrorMessage);
childProcess.spawnSync = (command, commandArgs) => {
if (commandArgs.indexOf("get") !== -1 && commandArgs.indexOf("prefix") !== -1) {
return { error: new Error(expectedErrorMessage) };
}
};

Expand All @@ -59,7 +59,7 @@ describe("getPath", () => {

fs.existsSync = () => false;

childProcess.execSync = (command) => {
childProcess.spawnSync = (command) => {
if (command.indexOf("get prefix") !== -1) {
return npmConfigPrefix;
}
Expand All @@ -75,7 +75,7 @@ describe("getPath", () => {
const packageName = "test1",
executableName = "test1.js";

childProcess.execSync = () => null;
childProcess.spawnSync = () => null;

const result = index.getPath(packageName, executableName);
assert.deepEqual(result, null);
Expand All @@ -87,9 +87,10 @@ describe("getPath", () => {
const getCorrectResultFromNpmPrefix = (npmConfigPrefix, packageName) => {
fs.existsSync = () => true;
fs.readFileSync = () => JSON.stringify({ name: packageName });
childProcess.execSync = (command) => {
if (command.indexOf("get prefix") !== -1) {
return npmConfigPrefix;
fs.realpathSync = (p) => p;
childProcess.spawnSync = (command, commandArgs) => {
if (commandArgs.indexOf("get") !== -1 && commandArgs.indexOf("prefix") !== -1) {
return { stdout: npmConfigPrefix };
}
};

Expand All @@ -105,8 +106,8 @@ describe("getPath", () => {
describe("works correctly when npm prefix is used", () => {
it("uses npm.cmd for npm executable", () => {
let commands = [];
childProcess.execSync = (command) => {
commands.push(command);
childProcess.spawnSync = (command, commandArgs) => {
commands.push(`${command} ${(commandArgs || []).join(" ")}`);
};

index.getPath("test1");
Expand Down Expand Up @@ -141,9 +142,9 @@ describe("getPath", () => {

fs.existsSync = () => true;

childProcess.execSync = (command) => {
childProcess.spawnSync = (command) => {
if (command.indexOf("where") !== -1) {
throw new Error(expectedErrorMessage);
return { error: new Error(expectedErrorMessage) };
}

return null;
Expand All @@ -163,9 +164,9 @@ describe("getPath", () => {

fs.existsSync = () => true;

childProcess.execSync = (command) => {
childProcess.spawnSync = (command) => {
if (command.indexOf("where") !== -1) {
return whereResult;
return { stdout: whereResult };
}

return null;
Expand Down Expand Up @@ -194,9 +195,9 @@ describe("getPath", () => {

fs.existsSync = (pathToCheck) => pathToCheck !== path.join(executableDirName, "node_modules", packageName);

childProcess.execSync = (command) => {
childProcess.spawnSync = (command) => {
if (command.indexOf("where") !== -1) {
return whereResult;
return { stdout: whereResult };
}

return null;
Expand Down Expand Up @@ -233,9 +234,9 @@ describe("getPath", () => {

fs.existsSync = (pathToCheck) => pathToCheck !== path.join(executableDirName, "node_modules", packageName);

childProcess.execSync = (command) => {
childProcess.spawnSync = (command) => {
if (command.indexOf("where") !== -1) {
return whereResult;
return { stdout: whereResult };
}

return null;
Expand Down Expand Up @@ -284,9 +285,9 @@ describe("getPath", () => {
return "";
};

childProcess.execSync = (command) => {
childProcess.spawnSync = (command) => {
if (command.indexOf("where") !== -1) {
return whereResult;
return { stdout: whereResult };
}

return null;
Expand Down Expand Up @@ -316,9 +317,9 @@ describe("getPath", () => {
return "";
};

childProcess.execSync = (command) => {
childProcess.spawnSync = (command) => {
if (command.indexOf("where") !== -1) {
return whereResult;
return { stdout: whereResult };
}

return null;
Expand Down Expand Up @@ -354,9 +355,9 @@ describe("getPath", () => {
return "";
};

childProcess.execSync = (command) => {
childProcess.spawnSync = (command) => {
if (command.indexOf("where") !== -1) {
return whereResult;
return { stdout: whereResult };
}

return null;
Expand All @@ -380,8 +381,8 @@ describe("getPath", () => {

it("uses npm for npm executable", () => {
let commands = [];
childProcess.execSync = (command) => {
commands.push(command);
childProcess.spawnSync = (command, commandArgs) => {
commands.push(`${command} ${(commandArgs || []).join(" ")}`);
};

index.getPath("test1");
Expand Down Expand Up @@ -416,7 +417,7 @@ describe("getPath", () => {

fs.existsSync = () => true;

childProcess.execSync = (command) => {
childProcess.spawnSync = (command) => {
if (command.indexOf("which") !== -1) {
throw new Error(expectedErrorMessage);
}
Expand All @@ -440,9 +441,9 @@ describe("getPath", () => {

fs.existsSync = () => true;

childProcess.execSync = (command) => {
if (command.indexOf("ls -l") !== -1) {
throw new Error(expectedErrorMessage);
childProcess.spawnSync = (command) => {
if (command === "ls") {
return { error: new Error(expectedErrorMessage) };
}

return null;
Expand Down Expand Up @@ -479,14 +480,14 @@ describe("getPath", () => {
return filePath;
};

childProcess.execSync = (command) => {
childProcess.spawnSync = (command) => {

if (command.indexOf("ls -l") !== -1) {
return lsLResult;
if (command === "ls") {
return { stdout: lsLResult };
}

if (command.indexOf("which") !== -1) {
return whichResult;
if (command == "which") {
return { stdout: whichResult };
}

return null;
Expand Down Expand Up @@ -544,14 +545,14 @@ describe("getPath", () => {
return filePath.indexOf("package.json") !== -1;
};

childProcess.execSync = (command) => {
childProcess.spawnSync = (command) => {

if (command.indexOf("ls -l") !== -1) {
return lsLResult;
if (command === "ls") {
return { stdout: lsLResult };
}

if (command.indexOf("which") !== -1) {
return whichResult;
if (command == "which") {
return { stdout: whichResult };
}

return null;
Expand Down Expand Up @@ -582,14 +583,14 @@ describe("getPath", () => {
return filePath.indexOf("package.json") !== -1;
};

childProcess.execSync = (command) => {
childProcess.spawnSync = (command) => {

if (command.indexOf("ls -l") !== -1) {
return lsLResult;
if (command === "ls") {
return { stdout: lsLResult };
}

if (command.indexOf("which") !== -1) {
return whichResult;
if (command == "which") {
return { stdout: whichResult };
}

return null;
Expand Down

0 comments on commit edbdaff

Please sign in to comment.