src: pass resource on permission checks for spawn

This commit enhances the permission model errors
when no --allow-child-process is used and the error
is emitted.

Signed-off-by: RafaelGSS <[email protected]>
PR-URL: #58758
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Edy Silva <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>

Author: RafaelGSS

src/node_process_methods.cc

@@ -518,9 +518,6 @@ static void Execve(const FunctionCallbackInfo<Value>& args) {
   Isolate* isolate = env->isolate();
   Local<Context> context = env->context();
 
-  THROW_IF_INSUFFICIENT_PERMISSIONS(
-      env, permission::PermissionScope::kChildProcess, "");
-
   CHECK(args[0]->IsString());
   CHECK(args[1]->IsArray());
   CHECK(args[2]->IsArray());
@@ -530,6 +527,11 @@ static void Execve(const FunctionCallbackInfo<Value>& args) {
 
   // Copy arguments and environment
   Utf8Value executable(isolate, args[0]);
+
+  THROW_IF_INSUFFICIENT_PERMISSIONS(env,
+                                    permission::PermissionScope::kChildProcess,
+                                    executable.ToStringView());
+
   std::vector<std::string> argv_strings(argv_array->Length());
   std::vector<std::string> envp_strings(envp_array->Length());
   std::vector<char*> argv(argv_array->Length() + 1);

src/process_wrap.cc

@@ -186,8 +186,6 @@ class ProcessWrap : public HandleWrap {
     Local<Context> context = env->context();
     ProcessWrap* wrap;
     ASSIGN_OR_RETURN_UNWRAP(&wrap, args.This());
-    THROW_IF_INSUFFICIENT_PERMISSIONS(
-        env, permission::PermissionScope::kChildProcess, "");
     int err = 0;
 
     if (!args[0]->IsObject()) {
@@ -201,6 +199,20 @@ class ProcessWrap : public HandleWrap {
 
     options.exit_cb = OnExit;
 
+    // TODO(bnoordhuis) is this possible to do without mallocing ?
+
+    // options.file
+    Local<Value> file_v;
+    if (!js_options->Get(context, env->file_string()).ToLocal(&file_v)) {
+      return;
+    }
+    CHECK(file_v->IsString());
+    node::Utf8Value file(env->isolate(), file_v);
+    options.file = *file;
+
+    THROW_IF_INSUFFICIENT_PERMISSIONS(
+        env, permission::PermissionScope::kChildProcess, file.ToStringView());
+
     // options.uid
     Local<Value> uid_v;
     if (!js_options->Get(context, env->uid_string()).ToLocal(&uid_v)) {
@@ -225,17 +237,6 @@ class ProcessWrap : public HandleWrap {
       options.gid = static_cast<uv_gid_t>(gid);
     }
 
-    // TODO(bnoordhuis) is this possible to do without mallocing ?
-
-    // options.file
-    Local<Value> file_v;
-    if (!js_options->Get(context, env->file_string()).ToLocal(&file_v)) {
-      return;
-    }
-    CHECK(file_v->IsString());
-    node::Utf8Value file(env->isolate(), file_v);
-    options.file = *file;
-
     // Undocumented feature of Win32 CreateProcess API allows spawning
     // batch files directly but is potentially insecure because arguments
     // are not escaped (and sometimes cannot be unambiguously escaped),

src/spawn_sync.cc

@@ -378,8 +378,24 @@ void SyncProcessRunner::RegisterExternalReferences(
 
 void SyncProcessRunner::Spawn(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
+  Local<Context> context = env->context();
+
+  std::string resource = "";
+  if (env->permission()->enabled() && args[0]->IsObject()) {
+    Local<Object> js_options = args[0].As<Object>();
+    Local<Value> js_file;
+    if (!js_options->Get(context, env->file_string()).ToLocal(&js_file)) {
+      return;
+    }
+
+    if (js_file->IsString()) {
+      node::Utf8Value executable(env->isolate(), js_file.As<String>());
+      resource = executable.ToString();
+    }
+  }
+
   THROW_IF_INSUFFICIENT_PERMISSIONS(
-      env, permission::PermissionScope::kChildProcess, "");
+      env, permission::PermissionScope::kChildProcess, resource);
   env->PrintSyncTrace();
   SyncProcessRunner p(env);
   Local<Value> result;

test/parallel/test-permission-child-process-cli.js

@@ -29,12 +29,14 @@ if (process.argv[2] === 'child') {
     message: 'Access to this API has been restricted. Use --allow-child-process to manage permissions.',
     code: 'ERR_ACCESS_DENIED',
     permission: 'ChildProcess',
+    resource: process.execPath,
   }));
   assert.throws(() => {
     childProcess.spawnSync(process.execPath, ['--version']);
   }, common.expectsError({
     code: 'ERR_ACCESS_DENIED',
     permission: 'ChildProcess',
+    resource: process.execPath,
   }));
   assert.throws(() => {
     childProcess.exec(...common.escapePOSIXShell`"${process.execPath}" --version`);
@@ -53,6 +55,7 @@ if (process.argv[2] === 'child') {
   }, common.expectsError({
     code: 'ERR_ACCESS_DENIED',
     permission: 'ChildProcess',
+    resource: process.execPath,
   }));
   assert.throws(() => {
     childProcess.execFile(...common.escapePOSIXShell`"${process.execPath}" --version`);

test/parallel/test-process-execve-permission-fail.js

@@ -17,5 +17,5 @@ if (process.argv[2] === 'replaced') {
 } else {
   throws(mustCall(() => {
     process.execve(process.execPath, [process.execPath, __filename, 'replaced'], process.env);
-  }), { code: 'ERR_ACCESS_DENIED', permission: 'ChildProcess' });
+  }), { code: 'ERR_ACCESS_DENIED', permission: 'ChildProcess', resource: process.execPath });
 }