Skip to content

feat(probes): add unsafe-command #327

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ type WarningName = "parsing-error"
| "obfuscated-code"
| "weak-crypto"
| "unsafe-import"
| "unsafe-command"
| "shady-link";

declare const warnings: Record<WarningName, {
Expand Down Expand Up @@ -133,6 +134,7 @@ This section describe all the possible warnings returned by JSXRay. Click on the
| [unsafe-import](./docs/unsafe-import.md) | ❌ | Unable to follow an import (require, require.resolve) statement/expr. |
| [unsafe-regex](./docs/unsafe-regex.md) | ❌ | A RegEx as been detected as unsafe and may be used for a ReDoS Attack. |
| [unsafe-stmt](./docs//unsafe-stmt.md) | ❌ | Usage of dangerous statement like `eval()` or `Function("")`. |
| [unsafe-command](./docs/unsafe-command.md) | ❌ | Usage of suspicious commands in `spawn()` or `exec()`.|
| [encoded-literal](./docs/encoded-literal.md) | ❌ | An encoded literal has been detected (it can be an hexa value, unicode sequence or a base64 string) |
| [short-identifiers](./docs/short-identifiers.md) | ❌ | This mean that all identifiers has an average length below 1.5. |
| [suspicious-literal](./docs/suspicious-literal.md) | ❌ | A suspicious literal has been found in the source code. |
Expand Down
22 changes: 22 additions & 0 deletions docs/unsafe-command.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# Unsafe command

| Code | Severity | i18n | Experimental |
| --- | --- | --- | :-: |
| unsafe-command | `Warning` | `sast_warnings.unsafe_command` | ✅ |

## Introduction

This warning identifies potentially dangerous use of the `spawn()` or `exec()` function from the `child_process` module.
Spawning system-level commands can introduce security risks, especially if user-controlled input is involved or if the
command itself is sensitive (e.g., tools that query or change system configurations). This warning identifies also
commands passed to `spawnSync()` and `execSync()`.

> [!NOTE]
> This rule is experimental. The list of suspicious commands is not exhaustive and will evolve over time.

## Example

```js
const { spawn } = require("child_process");
spawn("csrutil", ["status"]);
```
4 changes: 3 additions & 1 deletion src/ProbeRunner.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import isBinaryExpression from "./probes/isBinaryExpression.js";
import isArrayExpression from "./probes/isArrayExpression.js";
import isESMExport from "./probes/isESMExport.js";
import isFetch from "./probes/isFetch.js";
import isUnsafeCommand from "./probes/isUnsafeCommand.js";

/**
* @typedef {import('./SourceFile.js').SourceFile} SourceFile
Expand Down Expand Up @@ -51,7 +52,8 @@ export class ProbeRunner {
isImportDeclaration,
isWeakCrypto,
isBinaryExpression,
isArrayExpression
isArrayExpression,
isUnsafeCommand
];

/**
Expand Down
111 changes: 111 additions & 0 deletions src/probes/isUnsafeCommand.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
// Import Internal Dependencies
import { ProbeSignals } from "../ProbeRunner.js";

// CONSTANTS
const kUnsafeCommands = ["csrutil"];

function isUnsafeCommand(command) {
return kUnsafeCommands.some((unsafeCommand) => command.includes(unsafeCommand));
}

function isSpwanOrExec(name) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function isSpwanOrExec(name) {
function isSpawnOrExec(name) {

return name === "spawn" ||
name === "exec" ||
name === "spawnSync" ||
name === "execSync";
}

/**
* @description Detect spawn or exec unsafe commands
* @example
* child_process.spawn("csrutil", ["status"]);
*
* require("child_process").spawn("csrutil", ["disable"]);
*
* const { exec } = require("child_process");
* exec("csrutil status");
*/
function validateNode(node) {
if (node.type !== "CallExpression" || node.arguments.length === 0) {
return [false];
}

// const { spawn } = require("child_process");
// spawn("...", ["..."]);
// or
// const { exec } = require("child_process");
// exec(...);
if (node.type === "CallExpression" &&
node.callee.type === "Identifier" &&
isSpwanOrExec(node.callee.name)
) {
return [true, node.callee.name];
}

// child_process.spawn(...) or require("child_process").spawn(...)
// child_process.exec(...) or require("child_process").exec(...)
if (
node.callee.type === "MemberExpression" &&
node.callee.property.type === "Identifier" &&
isSpwanOrExec(node.callee.property.name)
) {
// child_process.spawn(...)
// child_process.exec(...)
if (
node.callee.object.type === "Identifier" &&
node.callee.object.name === "child_process"
) {
return [true, node.callee.property.name];
}
// require("child_process").spawn(...)
// require("child_process").exec(...)
if (
node.callee.object.type === "CallExpression" &&
node.callee.object.callee.type === "Identifier" &&
node.callee.object.callee.name === "require" &&
node.callee.object.arguments.length === 1 &&
node.callee.object.arguments[0].type === "Literal" &&
node.callee.object.arguments[0].value === "child_process"
) {
return [true, node.callee.property.name];
}
}

return [false];
}

function main(node, options) {
const { sourceFile } = options;

const commandArg = node.arguments[0];
if (!commandArg || commandArg.type !== "Literal") {
return null;
}

let command = commandArg.value;
if (typeof command === "string" && isUnsafeCommand(command)) {
// Spwaned command arguments are filled into an Array
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Spwaned command arguments are filled into an Array
// Spawned command arguments are filled into an Array

// as second arguments. This is why we should add them
// manually to the command string.
if (options.data === "spawn" || options.data === "spawnSync") {
const args = node.arguments.at(1);
if (args && Array.isArray(args.elements)) {
args.elements.forEach((element) => {
command += ` ${element.value}`;
});
}
}

sourceFile.addWarning("unsafe-command", command, node.loc);

return ProbeSignals.Skip;
}

return null;
}

export default {
name: "isUnsafeCommand",
validateNode,
main
};
5 changes: 5 additions & 0 deletions src/warnings.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ export const warnings = Object.freeze({
i18n: "sast_warnings.shady_link",
severity: "Warning",
experimental: false
},
"unsafe-command": {
i18n: "sast_warnings.unsafe-command",
severity: "Warning",
experimental: true
}
});

Expand Down
Loading