JS: Env Injection query

[am0o0]

Detect user-controllable environment variable injection that can lead to security issues.

[ghsecuritylab]

Hello am0o0 👋
You have submitted this pull request as a bug bounty report in the github/securitylab repository and therefore this pull request has been put into draft state to give time for the GitHub Security Lab to assess the PR. When GitHub Security Lab has finished assessing your pull request, it will be marked automatically as Ready for review. Until then, please don't change the draft state.

In the meantime, feel free to make changes to the pull request. If you'd like to maximize payout for your this and future submissions, here are a few general guidelines, that we might take into consideration when reviewing a submission.

• the submission models widely-used frameworks/libraries
• the vulnerability modeled in the submission is impactful
• the submission finds new true positive vulnerabilities
• the submission finds very few false positives
• code in the submission is easy to read and will be easy to maintain
• documentation is written clearly, highlighting the impact of the issue it finds and is written without grammatical or other errors. The code samples clearly show the vulnerability
• the submission includes tests, change note etc.

Please note that these are guidelines, not rules. Since we have a lot of different types of submissions, the guidelines might vary for each submission.

Happy hacking!

[am0o0]

hi, sorry if I updated this query after changing the state from draft, according my latest review I learned mannythings and I applied my learnings in here too.

[github-actions]

QHelp previews:

javascript/ql/src/experimental/Security/CWE-099/EnvValueAndKeyInjection.qhelp

errors/warnings:

/home/runner/work/codeql/codeql/javascript/ql/src/experimental/Security/CWE-099/EnvValueAndKeyInjection.qhelp:33:78: element "a" not allowed here; expected the element end-tag or element "li"
A fatal error occurred: 1 qhelp files could not be processed.

javascript/ql/src/experimental/Security/CWE-099/EnvValueInjection.qhelp

errors/warnings:

/home/runner/work/codeql/codeql/javascript/ql/src/experimental/Security/CWE-099/EnvValueInjection.qhelp:33:78: element "a" not allowed here; expected the element end-tag or element "li"
A fatal error occurred: 1 qhelp files could not be processed.

[erik-krogh]

You're restricting your query to only find results where the same source flows to both the value and key of environment variable write.
The sources should not be shared between the two dataflow configurations.

But you're not restricting that it's the same write to process.env.
So you can have one write like: process.env[tainted] = "constant", and another like: process.env.constant = tainted, and the second write would get flagged because the first exist somewhere else.

You should make a predicate that relate the key and value of the write to process.env, and then use that to make sure it's the same write to process.env you're finding.
Something like:

private predicate readToProcessEnv(DataFlow::Node key, DataFlow::Node value) {
    exists(DataFlow::PropWrite env | env = NodeJSLib::process().getAPropertyWrite("env") |
      key = env.getPropertyNameExpr().flow() and
      value = env.getRhs()
    )
  }

---

Also, there are some errors with the QHelp.
I havne't look at those yet, but this is the errors from the CI:

/home/runner/work/codeql/codeql/javascript/ql/src/experimental/Security/CWE-099/EnvValueAndKeyInjection.qhelp:33:78: element "a" not allowed here; expected the element end-tag or element "li"
/home/runner/work/codeql/codeql/javascript/ql/src/experimental/Security/CWE-099/EnvValueInjection.qhelp:33:78: element "a" not allowed here; expected the element end-tag or element "li"

[github-actions]

QHelp previews:

javascript/ql/src/experimental/Security/CWE-099/EnvValueAndKeyInjection.qhelp

errors/warnings:

/home/runner/work/codeql/codeql/javascript/ql/src/experimental/Security/CWE-099/EnvValueAndKeyInjection.qhelp:33:78: element "a" not allowed here; expected the element end-tag or element "li"
A fatal error occurred: 1 qhelp files could not be processed.

javascript/ql/src/experimental/Security/CWE-099/EnvValueInjection.qhelp

errors/warnings:

/home/runner/work/codeql/codeql/javascript/ql/src/experimental/Security/CWE-099/EnvValueInjection.qhelp:33:78: element "a" not allowed here; expected the element end-tag or element "li"
A fatal error occurred: 1 qhelp files could not be processed.

[github-actions]

QHelp previews:

javascript/ql/src/experimental/Security/CWE-099/EnvValueAndKeyInjection.qhelp

User controlled arbitrary environment variable injection

Controlling the value of arbitrary environment variables from user-controllable data is not safe.

Recommendation

Restrict this operation only to privileged users or only for some not important environment variables.

Example

The following example allows unauthorized users to assign a value to any environment variable.

const http = require('node:http');

http.createServer((req, res) => {
  const { EnvValue, EnvKey } = req.body;
  process.env[EnvKey] = EnvValue; // NOT OK

  res.end('env has been injected!');
});

References

• Admin account TakeOver in mintplex-labs/anything-llm
• Common Weakness Enumeration: CWE-89.

javascript/ql/src/experimental/Security/CWE-099/EnvValueInjection.qhelp

User controlled environment variable value injection

Assigning Value to environment variables from user-controllable data is not safe.

Recommendation

Restrict this operation only to privileged users or only for some not important environment variables.

Example

The following example allows unauthorized users to assign a value to a critical environment variable.

const http = require('node:http');

http.createServer((req, res) => {
  const { EnvValue } = req.body;
  process.env["A_Critical_Env"] = EnvValue; // NOT OK

  res.end('env has been injected!');
});

References

• Admin account TakeOver in mintplex-labs/anything-llm
• Common Weakness Enumeration: CWE-89.