Skip to content

RB: add second-order-command-injection - #11236

Draft
erik-krogh wants to merge 4 commits into
github:mainfrom
erik-krogh:rbSndCmd
Draft

RB: add second-order-command-injection#11236
erik-krogh wants to merge 4 commits into
github:mainfrom
erik-krogh:rbSndCmd

Conversation

@erik-krogh

Copy link
Copy Markdown
Contributor

I also did some drive-by refactorizations in the first commit.
I noticed an override DataFlow::Node getAnArgument in classes that extends DataFlow::CallNode.
That is just a foot-gun waiting to happen if DataFlow::CallNode gets an getAnArgument predicate, so I changed those classes to instanceof DataFlow::CallNode.


@erik-krogh erik-krogh added WIP This is a work-in-progress, do not merge yet! Ruby labels Nov 11, 2022
@github-actions

Copy link
Copy Markdown
Contributor

QHelp previews:

ruby/ql/src/queries/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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Ruby WIP This is a work-in-progress, do not merge yet!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants