From 242aaa0caaf0c15109067b598d58fdeae603c5fd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= <tobias.niessen@tuwien.ac.at>
Date: Sun, 16 Apr 2023 22:26:47 +0200
Subject: [PATCH] policy: disable process.binding() when enabled

process.binding() can be used to trivially bypass restrictions imposed
through a policy. Since the function is deprecated already, simply
replace it with a stub when a policy is being enabled.

Fixes: https://hackerone.com/bugs?report_id=1946470
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
CVE-ID: CVE-2023-32559
PR-URL: https://github.com/nodejs-private/node-private/pull/459
---
 doc/api/deprecations.md                       |  4 +++
 doc/api/errors.md                             |  8 ++++++
 lib/internal/errors.js                        |  3 ++
 lib/internal/process/policy.js                | 10 +++++++
 test/fixtures/policy/process-binding/app.js   | 10 +++++++
 .../policy/process-binding/policy.json        | 10 +++++++
 test/parallel/test-policy-process-binding.js  | 28 +++++++++++++++++++
 7 files changed, 73 insertions(+)
 create mode 100644 test/fixtures/policy/process-binding/app.js
 create mode 100644 test/fixtures/policy/process-binding/policy.json
 create mode 100644 test/parallel/test-policy-process-binding.js

diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md
index b107ebefbf5586..7db32e766ef2d3 100644
--- a/doc/api/deprecations.md
+++ b/doc/api/deprecations.md
@@ -2215,6 +2215,9 @@ Type: Documentation-only (supports [`--pending-deprecation`][])
 
 `process.binding()` is for use by Node.js internal code only.
 
+While `process.binding()` has not reached End-of-Life status in general, it is
+unavailable when [policies][] are enabled.
+
 ### DEP0112: `dgram` private APIs
 
 <!-- YAML
@@ -3340,6 +3343,7 @@ Node-API callbacks.
 [from_arraybuffer]: buffer.md#static-method-bufferfromarraybuffer-byteoffset-length
 [from_string_encoding]: buffer.md#static-method-bufferfromstring-encoding
 [legacy `urlObject`]: url.md#legacy-urlobject
+[policies]: permissions.md#policies
 [static methods of `crypto.Certificate()`]: crypto.md#class-certificate
 [subpath exports]: packages.md#subpath-exports
 [subpath folder mappings]: packages.md#subpath-folder-mappings
diff --git a/doc/api/errors.md b/doc/api/errors.md
index a4f6902be32d63..dcf8744d8b8b7a 100644
--- a/doc/api/errors.md
+++ b/doc/api/errors.md
@@ -679,6 +679,14 @@ APIs _not_ using `AbortSignal`s typically do not raise an error with this code.
 This code does not use the regular `ERR_*` convention Node.js errors use in
 order to be compatible with the web platform's `AbortError`.
 
+<a id="ERR_ACCESS_DENIED"></a>
+
+### `ERR_ACCESS_DENIED`
+
+A special type of error that is triggered whenever Node.js tries to get access
+to a resource restricted by the [policy][] manifest.
+For example, `process.binding`.
+
 <a id="ERR_AMBIGUOUS_ARGUMENT"></a>
 
 ### `ERR_AMBIGUOUS_ARGUMENT`
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
index a75e7ccd056889..fc22776a0bcf37 100644
--- a/lib/internal/errors.js
+++ b/lib/internal/errors.js
@@ -934,6 +934,9 @@ module.exports = {
 //
 // Note: Node.js specific errors must begin with the prefix ERR_
 
+E('ERR_ACCESS_DENIED',
+  'Access to this API has been restricted. Permission: %s',
+  Error);
 E('ERR_AMBIGUOUS_ARGUMENT', 'The "%s" argument is ambiguous. %s', TypeError);
 E('ERR_ARG_NOT_ITERABLE', '%s must be iterable', TypeError);
 E('ERR_ASSERTION', '%s', Error);
diff --git a/lib/internal/process/policy.js b/lib/internal/process/policy.js
index ea283a449742fc..8d01b657e07e49 100644
--- a/lib/internal/process/policy.js
+++ b/lib/internal/process/policy.js
@@ -7,6 +7,7 @@ const {
 } = primordials;
 
 const {
+  ERR_ACCESS_DENIED,
   ERR_MANIFEST_TDZ,
 } = require('internal/errors').codes;
 const { Manifest } = require('internal/policy/manifest');
@@ -32,6 +33,15 @@ module.exports = ObjectFreeze({
       return o;
     });
     manifest = new Manifest(json, url);
+
+    // process.binding() is deprecated (DEP0111) and trivially allows bypassing
+    // policies, so if policies are enabled, make this API unavailable.
+    process.binding = function binding(_module) {
+      throw new ERR_ACCESS_DENIED('process.binding');
+    };
+    process._linkedBinding = function _linkedBinding(_module) {
+      throw new ERR_ACCESS_DENIED('process._linkedBinding');
+    };
   },
 
   get manifest() {
diff --git a/test/fixtures/policy/process-binding/app.js b/test/fixtures/policy/process-binding/app.js
new file mode 100644
index 00000000000000..16e26d12a16028
--- /dev/null
+++ b/test/fixtures/policy/process-binding/app.js
@@ -0,0 +1,10 @@
+'use strict';
+
+const assert = require('assert');
+
+assert.throws(() => { process.binding(); }, {
+  code: 'ERR_ACCESS_DENIED'
+});
+assert.throws(() => { process._linkedBinding(); }, {
+  code: 'ERR_ACCESS_DENIED'
+});
diff --git a/test/fixtures/policy/process-binding/policy.json b/test/fixtures/policy/process-binding/policy.json
new file mode 100644
index 00000000000000..81f8b40c90a069
--- /dev/null
+++ b/test/fixtures/policy/process-binding/policy.json
@@ -0,0 +1,10 @@
+{
+  "resources": {
+    "./app.js": {
+      "integrity": true,
+      "dependencies": {
+        "assert": true
+      }
+    }
+  }
+}
diff --git a/test/parallel/test-policy-process-binding.js b/test/parallel/test-policy-process-binding.js
new file mode 100644
index 00000000000000..34a6954e6c47c8
--- /dev/null
+++ b/test/parallel/test-policy-process-binding.js
@@ -0,0 +1,28 @@
+'use strict';
+
+const common = require('../common');
+common.requireNoPackageJSONAbove();
+
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const fixtures = require('../common/fixtures');
+
+const assert = require('node:assert');
+const { spawnSync } = require('node:child_process');
+
+const dep = fixtures.path('policy', 'process-binding', 'app.js');
+const depPolicy = fixtures.path(
+  'policy',
+  'process-binding',
+  'policy.json');
+const { status } = spawnSync(
+  process.execPath,
+  [
+    '--experimental-policy', depPolicy, dep,
+  ],
+  {
+    stdio: 'inherit'
+  },
+);
+assert.strictEqual(status, 0);