fix: Changed order of escape to prevent RCE

[Yasha-ops]

Summary

Nginx Proxy Manager is vulnerable to authenticated remote code execution due to a shell injection in setupCertbotPlugins() (backend/setup.js).

The user-controlled field dns_provider_credentials is interpolated into a shell command executed via child_process.exec() without proper escaping.
An attacker with certificates:manage permission can inject arbitrary commands, executed on backend restart (typically as root in Docker deployments).

Affected Versions

v2.9.14   v2.9.15   v2.9.16   v2.9.17   v2.9.18   v2.9.19   v2.9.20
v2.9.21   v2.9.22
v2.10.0   v2.10.1   v2.10.2   v2.10.3   v2.10.4
v2.11.0   v2.11.1   v2.11.2   v2.11.3
v2.12.0   v2.12.1   v2.12.2   v2.12.3   v2.12.4   v2.12.5   v2.12.6
v2.13.0   v2.13.1   v2.13.2   v2.13.3   v2.13.4   v2.13.5   v2.13.6   v2.13.7
v2.14.0   (latest, still vulnerable)

Root Cause

Incorrect escaping order:

const escapedCredentials = certificate.meta.dns_provider_credentials
    .replaceAll("'", "\\'")
    .replaceAll("\\", "\\\\");

Backslashes are escaped after single quotes, breaking the protection and allowing injection.
Example input:

x' ; cmd ; #

Results in command execution:

echo 'x\\' ; cmd ; #'

Exploitation

1. Store payload in dns_provider_credentials (DB or race condition)
2. Restart backend
3. Payload executes via /bin/sh -c inside exec()

Reliable in Docker since /etc/letsencrypt/credentials/ is not persisted.
Example payload:

fake' > /dev/null; CMD ; echo '

Impact

• Full RCE
• Access to secrets, TLS keys, database
• Service disruption and persistence

Fix

.replaceAll("\\", "\\\\")
.replaceAll("'", "\\'");

[jc21]

Code Review — Fix is Incomplete, Vulnerability Remains Exploitable

Thanks for the detailed write-up and for identifying this vulnerability. The PR correctly diagnoses the escaping-order bug, but the proposed fix does not fully close the RCE.

---

Why the Fix Is Still Vulnerable

The fundamental issue is that \' cannot escape a single quote inside a bash/sh single-quoted string. In POSIX shell, within '...', backslash has no special meaning — the first ' always closes the string unconditionally.

For the classic payload x' ; cmd ; # (no backslashes):

• Step 1 (escape \ first): no \ in input → unchanged: x' ; cmd ; #
• Step 2 (escape ' → \'): x\' ; cmd ; #
• Shell sees: echo 'x\' ; cmd ; #'

Shell parsing:

1. ' opens single-quoted string
2. x → literal x
3. \ → literal backslash (no special meaning inside single quotes)
4. ' → closes the single-quoted string (arg so far: x\)
5. ; cmd ; #' → ; is a command separator → cmd executes → RCE still works

The escaping-order fix only helps for inputs containing \ — it doesn't address the fundamental quoting flaw.

---

Correct Fixes

Option A — Use '\'' for single-quote escaping (proper shell idiom):

const escapedCredentials = certificate.meta.dns_provider_credentials
    .replaceAll("\\", "\\\\")
    .replaceAll("'", "'\\''"); // end-quote, escaped-quote, re-open-quote

For input x' ; cmd ; # → escaped: x'\'' ; cmd ; #
Shell sees: echo 'x'\'' ; cmd ; #' → parses as the literal string x' ; cmd ; #. No injection.

Option B — Avoid the shell entirely (strongly preferred):

Replace the exec() call with direct Node.js file operations. utils.execFile is already available in the codebase, and fs.writeFile/fs.chmod eliminate the shell command entirely:

import fs from "node:fs/promises";

const credentials_loc = `/etc/letsencrypt/credentials/credentials-${certificate.id}`;
if (typeof certificate.meta.dns_provider_credentials === "string") {
    promises.push(
        fs.mkdir("/etc/letsencrypt/credentials", { recursive: true })
            .then(() => fs.writeFile(credentials_loc, certificate.meta.dns_provider_credentials, { mode: 0o600, flag: "wx" }))
    );
}

This removes the attack surface entirely rather than trying to sanitize around shell injection.

---

Summary

Issue	Severity
Original escaping order bug	High — PR correctly identifies and fixes this
\' doesn't escape inside bash single-quoted strings	Critical — vulnerability remains
Shell command construction with user-controlled data	High — should use direct Node.js fs calls

Option B is the right long-term fix as it eliminates the entire class of shell-injection risk in this code path.

[Yasha-ops]

@jc21 Done !

[jc21]

Code Review

The vulnerability is real and well-described — the original escaping order (' before \) allows shell injection, and replacing exec() entirely with native fs operations is the correct fix class. However, there's a critical runtime bug in the implementation.

---

Critical Bug: Wrong fs Module

The PR imports the callback-based fs module but calls it as if it returns Promises:

import fs from "fs";   // callback-based — .mkdir() returns undefined, not a Promise
...
promises.push(fs.mkdir(...).then(...))  // TypeError: Cannot read properties of undefined (reading 'then')

This will throw at runtime on any backend startup where a DNS certificate exists, making the fix non-functional. The import should be:

import fs from "fs/promises";

---

Secondary Issue: EEXIST Not Handled

The original shell guard [ -f ... ] || ... silently skips if the credentials file already exists. Using flag: "wx" replicates the intent but rejects the promise with an EEXIST error instead of silently succeeding. This propagates into Promise.all(promises) and would break startup for any certificate that already has a credentials file on disk.

Suggested fix:

promises.push(
    fs.mkdir("/etc/letsencrypt/credentials", { recursive: true })
        .then(() => fs.writeFile(credentials_loc, certificate.meta.dns_provider_credentials, { mode: 0o600, flag: "wx" }))
        .catch((err) => { if (err.code !== "EEXIST") throw err; })
);

---

Summary

Vulnerability fix approach	Correct — eliminate shell execution entirely
Critical implementation bug	Wrong fs import — fix will not work at runtime
Behavior regression	EEXIST on pre-existing credential files will crash Promise.all

The approach is right, but it needs import fs from "fs/promises" and EEXIST handling before it's merge-ready.

[Yasha-ops]

Ok now it's fixed (hopefully) :D

[nginxproxymanagerci]

Docker Image for build 4 is available on DockerHub:

nginxproxymanager/nginx-proxy-manager-dev:pr-5498

Note

Ensure you backup your NPM instance before testing this image! Especially if there are database changes.
This is a different docker image namespace than the official image.

Warning

Changes and additions to DNS Providers require verification by at least 2 members of the community!