From dfce1a9eb4de3a5789536cd511e81c6dfd81e393 Mon Sep 17 00:00:00 2001
From: Cheng Zhao <zcbenz@gmail.com>
Date: Thu, 4 Jan 2024 16:34:08 +0900
Subject: [PATCH] fix: ignore all NODE_ envs from foreign parent in node
 process (#40770)

* fix: ignore all NODE_ envs from foreign parent

* fix: recognize ad-hoc signed binary
---
 filenames.gni                               |  1 +
 shell/app/node_main.cc                      | 21 +++----
 shell/common/mac/codesign_util.cc           | 61 ++++++++++++++++++++-
 shell/common/mac/codesign_util.h            | 11 +++-
 shell/common/node_util.h                    |  7 +++
 shell/common/node_util_mac.mm               | 23 ++++++++
 spec/fixtures/api/fork-with-node-options.js |  5 +-
 spec/node-spec.ts                           |  2 +-
 8 files changed, 115 insertions(+), 16 deletions(-)
 create mode 100644 shell/common/node_util_mac.mm

diff --git a/filenames.gni b/filenames.gni
index 382474966371..d142f312bb3b 100644
--- a/filenames.gni
+++ b/filenames.gni
@@ -656,6 +656,7 @@ filenames = {
     "shell/common/node_includes.h",
     "shell/common/node_util.cc",
     "shell/common/node_util.h",
+    "shell/common/node_util_mac.mm",
     "shell/common/options_switches.cc",
     "shell/common/options_switches.h",
     "shell/common/platform_util.cc",
diff --git a/shell/app/node_main.cc b/shell/app/node_main.cc
index adca7ff48381..469b7e31c80e 100644
--- a/shell/app/node_main.cc
+++ b/shell/app/node_main.cc
@@ -31,6 +31,7 @@
 #include "shell/common/gin_helper/dictionary.h"
 #include "shell/common/node_bindings.h"
 #include "shell/common/node_includes.h"
+#include "shell/common/node_util.h"
 
 #if BUILDFLAG(IS_WIN)
 #include "chrome/child/v8_crashpad_support_win.h"
@@ -107,8 +108,12 @@ int NodeMain(int argc, char* argv[]) {
 
   auto os_env = base::Environment::Create();
   bool node_options_enabled = electron::fuses::IsNodeOptionsEnabled();
+  if (!node_options_enabled) {
+    os_env->UnSetVar("NODE_OPTIONS");
+  }
+
 #if BUILDFLAG(IS_MAC)
-  if (node_options_enabled && os_env->HasVar("NODE_OPTIONS")) {
+  if (!ProcessSignatureIsSameWithCurrentApp(getppid())) {
     // On macOS, it is forbidden to run sandboxed app with custom arguments
     // from another app, i.e. args are discarded in following call:
     //   exec("Sandboxed.app", ["--custom-args-will-be-discarded"])
@@ -117,18 +122,14 @@ int NodeMain(int argc, char* argv[]) {
     //   exec("Electron.app", {env: {ELECTRON_RUN_AS_NODE: "1",
     //                               NODE_OPTIONS: "--require 'bad.js'"}})
     // To prevent Electron apps from being used to work around macOS security
-    // restrictions, when NODE_OPTIONS is passed it will be checked whether
-    // this process is invoked by its own app.
-    if (!ProcessBelongToCurrentApp(getppid())) {
-      LOG(ERROR) << "NODE_OPTIONS is disabled because this process is invoked "
-                    "by other apps.";
-      node_options_enabled = false;
+    // restrictions, when the parent process is not part of the app bundle, all
+    // environment variables starting with NODE_ will be removed.
+    if (util::UnsetAllNodeEnvs()) {
+      LOG(ERROR) << "Node.js environment variables are disabled because this "
+                    "process is invoked by other apps.";
     }
   }
 #endif  // BUILDFLAG(IS_MAC)
-  if (!node_options_enabled) {
-    os_env->UnSetVar("NODE_OPTIONS");
-  }
 
 #if BUILDFLAG(IS_WIN)
   v8_crashpad_support::SetUp();
diff --git a/shell/common/mac/codesign_util.cc b/shell/common/mac/codesign_util.cc
index 5171f90aac11..2abfb2eb32d2 100644
--- a/shell/common/mac/codesign_util.cc
+++ b/shell/common/mac/codesign_util.cc
@@ -5,15 +5,60 @@
 
 #include "shell/common/mac/codesign_util.h"
 
+#include "base/apple/foundation_util.h"
 #include "base/apple/osstatus_logging.h"
 #include "base/apple/scoped_cftyperef.h"
+#include "third_party/abseil-cpp/absl/types/optional.h"
 
 #include <CoreFoundation/CoreFoundation.h>
 #include <Security/Security.h>
 
 namespace electron {
 
-bool ProcessBelongToCurrentApp(pid_t pid) {
+absl::optional<bool> IsUnsignedOrAdHocSigned(SecCodeRef code) {
+  base::apple::ScopedCFTypeRef<SecStaticCodeRef> static_code;
+  OSStatus status = SecCodeCopyStaticCode(code, kSecCSDefaultFlags,
+                                          static_code.InitializeInto());
+  if (status == errSecCSUnsigned) {
+    return true;
+  }
+  if (status != errSecSuccess) {
+    OSSTATUS_LOG(ERROR, status) << "SecCodeCopyStaticCode";
+    return absl::optional<bool>();
+  }
+  // Copy the signing info from the SecStaticCodeRef.
+  base::apple::ScopedCFTypeRef<CFDictionaryRef> signing_info;
+  status =
+      SecCodeCopySigningInformation(static_code.get(), kSecCSSigningInformation,
+                                    signing_info.InitializeInto());
+  if (status != errSecSuccess) {
+    OSSTATUS_LOG(ERROR, status) << "SecCodeCopySigningInformation";
+    return absl::optional<bool>();
+  }
+  // Look up the code signing flags. If the flags are absent treat this as
+  // unsigned. This decision is consistent with the StaticCode source:
+  // https://github.com/apple-oss-distributions/Security/blob/Security-60157.40.30.0.1/OSX/libsecurity_codesigning/lib/StaticCode.cpp#L2270
+  CFNumberRef signing_info_flags =
+      base::apple::GetValueFromDictionary<CFNumberRef>(signing_info.get(),
+                                                       kSecCodeInfoFlags);
+  if (!signing_info_flags) {
+    return true;
+  }
+  // Using a long long to extract the value from the CFNumberRef to be
+  // consistent with how it was packed by Security.framework.
+  // https://github.com/apple-oss-distributions/Security/blob/Security-60157.40.30.0.1/OSX/libsecurity_utilities/lib/cfutilities.h#L262
+  long long flags;
+  if (!CFNumberGetValue(signing_info_flags, kCFNumberLongLongType, &flags)) {
+    LOG(ERROR) << "CFNumberGetValue";
+    return absl::optional<bool>();
+  }
+  if (static_cast<uint32_t>(flags) & kSecCodeSignatureAdhoc) {
+    return true;
+  }
+  return false;
+}
+
+bool ProcessSignatureIsSameWithCurrentApp(pid_t pid) {
   // Get and check the code signature of current app.
   base::apple::ScopedCFTypeRef<SecCodeRef> self_code;
   OSStatus status =
@@ -22,6 +67,15 @@ bool ProcessBelongToCurrentApp(pid_t pid) {
     OSSTATUS_LOG(ERROR, status) << "SecCodeCopyGuestWithAttributes";
     return false;
   }
+  absl::optional<bool> not_signed = IsUnsignedOrAdHocSigned(self_code.get());
+  if (!not_signed.has_value()) {
+    // Error happened.
+    return false;
+  }
+  if (not_signed.value()) {
+    // Current app is not signed.
+    return true;
+  }
   // Get the code signature of process.
   base::apple::ScopedCFTypeRef<CFNumberRef> process_cf(
       CFNumberCreate(nullptr, kCFNumberIntType, &pid));
@@ -46,9 +100,14 @@ bool ProcessBelongToCurrentApp(pid_t pid) {
     OSSTATUS_LOG(ERROR, status) << "SecCodeCopyDesignatedRequirement";
     return false;
   }
+  DCHECK(self_requirement.get());
   // Check whether the process meets the signature requirement of current app.
   status = SecCodeCheckValidity(process_code.get(), kSecCSDefaultFlags,
                                 self_requirement.get());
+  if (status != errSecSuccess && status != errSecCSReqFailed) {
+    OSSTATUS_LOG(ERROR, status) << "SecCodeCheckValidity";
+    return false;
+  }
   return status == errSecSuccess;
 }
 
diff --git a/shell/common/mac/codesign_util.h b/shell/common/mac/codesign_util.h
index 8a2fb94ee8e6..1fabc4fe42bd 100644
--- a/shell/common/mac/codesign_util.h
+++ b/shell/common/mac/codesign_util.h
@@ -10,9 +10,14 @@
 
 namespace electron {
 
-// Given a pid, check if the process belongs to current app by comparing its
-// code signature with current app.
-bool ProcessBelongToCurrentApp(pid_t pid);
+// Given a pid, return true if the process has the same code signature with
+// with current app.
+// This API returns true if current app is not signed or ad-hoc signed, because
+// checking code signature is meaningless in this case, and failing the
+// signature check would break some features with unsigned binary (for example,
+// process.send stops working in processes created by child_process.fork, due
+// to the NODE_CHANNEL_ID env getting removed).
+bool ProcessSignatureIsSameWithCurrentApp(pid_t pid);
 
 }  // namespace electron
 
diff --git a/shell/common/node_util.h b/shell/common/node_util.h
index 38f9d28b2b18..d4aa75a0db45 100644
--- a/shell/common/node_util.h
+++ b/shell/common/node_util.h
@@ -7,6 +7,7 @@
 
 #include <vector>
 
+#include "build/build_config.h"
 #include "v8/include/v8.h"
 
 namespace node {
@@ -26,6 +27,12 @@ v8::MaybeLocal<v8::Value> CompileAndCall(
     std::vector<v8::Local<v8::String>>* parameters,
     std::vector<v8::Local<v8::Value>>* arguments);
 
+#if BUILDFLAG(IS_MAC)
+// Unset all environment variables that start with NODE_. Return false if there
+// is no node env at all.
+bool UnsetAllNodeEnvs();
+#endif
+
 }  // namespace electron::util
 
 #endif  // ELECTRON_SHELL_COMMON_NODE_UTIL_H_
diff --git a/shell/common/node_util_mac.mm b/shell/common/node_util_mac.mm
new file mode 100644
index 000000000000..85c072509867
--- /dev/null
+++ b/shell/common/node_util_mac.mm
@@ -0,0 +1,23 @@
+// Copyright (c) 2023 Microsoft, Inc.
+// Use of this source code is governed by the MIT license that can be
+// found in the LICENSE file.
+
+#include "shell/common/node_util.h"
+
+#include <Foundation/Foundation.h>
+
+namespace electron::util {
+
+bool UnsetAllNodeEnvs() {
+  bool has_unset = false;
+  for (NSString* env in NSProcessInfo.processInfo.environment) {
+    if (![env hasPrefix:@"NODE_"])
+      continue;
+    const char* name = [[env componentsSeparatedByString:@"="][0] UTF8String];
+    unsetenv(name);
+    has_unset = true;
+  }
+  return has_unset;
+}
+
+}  // namespace electron::util
diff --git a/spec/fixtures/api/fork-with-node-options.js b/spec/fixtures/api/fork-with-node-options.js
index 49dad89944b6..97439598f4c7 100644
--- a/spec/fixtures/api/fork-with-node-options.js
+++ b/spec/fixtures/api/fork-with-node-options.js
@@ -2,11 +2,14 @@ const { execFileSync } = require('node:child_process');
 const path = require('node:path');
 
 const fixtures = path.resolve(__dirname, '..');
+const failJs = path.join(fixtures, 'module', 'fail.js');
 
 const env = {
   ELECTRON_RUN_AS_NODE: 'true',
   // Process will exit with 1 if NODE_OPTIONS is accepted.
-  NODE_OPTIONS: `--require "${path.join(fixtures, 'module', 'fail.js')}"`
+  NODE_OPTIONS: `--require "${failJs}"`,
+  // Try bypassing the check with NODE_REPL_EXTERNAL_MODULE.
+  NODE_REPL_EXTERNAL_MODULE: failJs
 };
 // Provide a lower cased NODE_OPTIONS in case some code ignores case sensitivity
 // when reading NODE_OPTIONS.
diff --git a/spec/node-spec.ts b/spec/node-spec.ts
index 2bb967055e92..e1d9c76d9e67 100644
--- a/spec/node-spec.ts
+++ b/spec/node-spec.ts
@@ -673,7 +673,7 @@ describe('node feature', () => {
     });
 
     const script = path.join(fixtures, 'api', 'fork-with-node-options.js');
-    const nodeOptionsWarning = 'NODE_OPTIONS is disabled because this process is invoked by other apps';
+    const nodeOptionsWarning = 'Node.js environment variables are disabled because this process is invoked by other apps';
 
     it('is disabled when invoked by other apps in ELECTRON_RUN_AS_NODE mode', async () => {
       await withTempDirectory(async (dir) => {