JS: second-order-command-injection

[erik-krogh]

CVE-2022-25900: FN (dynamically chosen git cmd).
CVE-2022-24437: TP/TN
CVE-2022-25865: TP/TN
CVE-2022-24066: FN (we don't recognize the source due to very dynamic initialization, and we don't recognize the sink due to some dynamic command construction).
CVE-2022-1440: FN (this is more for js/shell-command-constructed-from-input, but they roll their own shell sanitization, which it doens't track through).
CVE-2022-25766: FN (dynamic construction of the arguments, so we don't recognize the git invocation).
CVE-2022-24433: FN (same project as CVE-2022-24066).

---

This blog post is a nice introduction to the vulnerability: https://justi.cz/security/2021/04/20/cocoapods-rce.html

The sinks for this query are arguments to e.g. cp.execFile("git", ["clone", <sink>]).

But also functions that just forwards their arguments to a shell executions, because I've seen that pattern in a few CVEs.
E.g.

function indirect(cmd, args) { cp.execFile(cmd, args) }
indirect("git", ["clone", <sink>])

---

Evaluation was uneventful.

[github-actions]

QHelp previews:

javascript/ql/src/Security/CWE-078/SecondOrderCommandInjection.qhelp

Second order command injection

Some shell commands, like git ls-remote, can execute arbitrary commands if a user provides a malicious URL that starts with --upload-pack. This can be used to execute arbitrary code on the server.

Recommendation

Sanitize user input before passing it to the shell command. For example, ensure that URLs are valid and do not contain malicious commands.

Example

The following example shows code that executes git ls-remote on a URL that can be controlled by a malicious user.

const express = require("express");
const app = express();

const cp = require("child_process");

app.get("/ls-remote", (req, res) => {
  const remote = req.query.remote;
  cp.execFile("git", ["ls-remote", remote]); // NOT OK
});

The problem has been fixed in the snippet below, where the URL is validated before being passed to the shell command.

const express = require("express");
const app = express();

const cp = require("child_process");

app.get("/ls-remote", (req, res) => {
  const remote = req.query.remote;
  if (!(remote.startsWith("git@") || remote.startsWith("https://"))) {
    throw new Error("Invalid remote: " + remote);
  }
  cp.execFile("git", ["ls-remote", remote]); // OK
});

References

• Max Justicz: Hacking 3,000,000 apps at once through CocoaPods.
• Git: Git - git-ls-remote Documentation.
• OWASP: Command Injection.
• Common Weakness Enumeration: CWE-78.
• Common Weakness Enumeration: CWE-88.

[asgerf]

Looks great! Just a minor comment in the change note

[asgerf]

I'd add something about which command names are currently recognized by the query, for example

This currently flags up unsafe invocations of git and hg.

[asgerf]

Sorry, just a few more comments.

[asgerf]

Great! LGTM

[erik-krogh]

Lets get some eyes from the doc team.

[lucascosti]

👋 Docs first responder here! I've put this on our review board for a writer to review

[mchammer01]

@erik-krogh - this LGTM from a Docs perspective ✨
I've made a comment about improving the query description and adding a missing whitespace in the References section. The rest is very minor. Feel free to ignore any comment you don't agree with.

[mchammer01]

Could we rewrite this so it follows the preferred syntax for a query description which is "Syntax X causes behavior Y." ?
I think it would be clearer to users if we went straight to the point.

[erik-krogh]

I've rewritten it to: Using user controlled data as arguments to some commands, such as git clone, can allow arbitrary commands to be executed.

[mchammer01]

Missing witespace

Suggested change

	<li>OWASP:<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.</li>
	<li>OWASP: <a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.</li>

[mchammer01]

Should we simplify here, or isn't it accurate to say this:

Suggested change

	arbitrary commands if a user provides a malicious URL that can start with
	arbitrary commands if a user provides a malicious URL that starts with

[mchammer01]

LGTM ✨
Thanks for the docs updates!

[mchammer01]