fix: ignore all NODE_ envs from foreign parent in node process (#40882)

filenames.gni

@@ -643,6 +643,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",

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();

shell/common/mac/codesign_util.cc

@@ -5,15 +5,59 @@
 
 #include "shell/common/mac/codesign_util.h"
 
+#include "base/mac/foundation_util.h"
 #include "base/mac/mac_logging.h"
 #include "base/mac/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::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::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::mac::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::ScopedCFTypeRef<SecCodeRef> self_code;
   OSStatus status =
@@ -22,6 +66,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::ScopedCFTypeRef<CFNumberRef> process_cf(
       CFNumberCreate(nullptr, kCFNumberIntType, &pid));
@@ -46,9 +99,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;
 }
 

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
 

shell/common/node_util.h

@@ -7,6 +7,7 @@
 
 #include <vector>
 
+#include "build/build_config.h"
 #include "v8/include/v8.h"
 
 namespace node {
@@ -27,6 +28,12 @@ v8::MaybeLocal<v8::Value> CompileAndCall(
     std::vector<v8::Local<v8::Value>>* arguments,
     node::Environment* optional_env);
 
+#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_

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

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.

spec/node-spec.ts

@@ -674,7 +674,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) => {