fix: prevent node mode to be used as script runner by other apps

filenames.gni

@@ -199,6 +199,8 @@ filenames = {
     "shell/common/language_util_mac.mm",
     "shell/common/mac/main_application_bundle.h",
     "shell/common/mac/main_application_bundle.mm",
+    "shell/common/mac/codesign_util.cc",
+    "shell/common/mac/codesign_util.h",
     "shell/common/node_bindings_mac.cc",
     "shell/common/node_bindings_mac.h",
     "shell/common/platform_util_mac.mm",

shell/app/node_main.cc

@@ -13,13 +13,15 @@
 #include "base/base_switches.h"
 #include "base/command_line.h"
 #include "base/containers/fixed_flat_set.h"
+#include "base/environment.h"
 #include "base/feature_list.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/single_thread_task_runner.h"
 #include "base/task/thread_pool/thread_pool_instance.h"
 #include "content/public/common/content_switches.h"
 #include "electron/electron_version.h"
+#include "electron/fuses.h"
 #include "gin/array_buffer.h"
 #include "gin/public/isolate_holder.h"
 #include "gin/v8_initializer.h"
@@ -35,13 +37,16 @@
 #endif
 
 #if BUILDFLAG(IS_LINUX)
-#include "base/environment.h"
 #include "base/posix/global_descriptors.h"
 #include "base/strings/string_number_conversions.h"
 #include "components/crash/core/app/crash_switches.h"  // nogncheck
 #include "content/public/common/content_descriptors.h"
 #endif
 
+#if BUILDFLAG(IS_MAC)
+#include "shell/common/mac/codesign_util.h"
+#endif
+
 #if !IS_MAS_BUILD()
 #include "components/crash/core/app/crashpad.h"  // nogncheck
 #include "shell/app/electron_crash_reporter_client.h"
@@ -100,12 +105,36 @@ int NodeMain(int argc, char* argv[]) {
     exit(1);
   }
 
+  auto os_env = base::Environment::Create();
+  bool node_options_enabled = electron::fuses::IsNodeOptionsEnabled();
+#if BUILDFLAG(IS_MAC)
+  if (node_options_enabled && os_env->HasVar("NODE_OPTIONS")) {
+    // 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"])
+    // However it is possible to bypass the restriction by abusing the node mode
+    // of Electron apps:
+    //   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;
+    }
+  }
+#endif  // BUILDFLAG(IS_MAC)
+  if (!node_options_enabled) {
+    os_env->UnSetVar("NODE_OPTIONS");
+  }
+
 #if BUILDFLAG(IS_WIN)
   v8_crashpad_support::SetUp();
 #endif
 
 #if BUILDFLAG(IS_LINUX)
-  auto os_env = base::Environment::Create();
   std::string fd_string, pid_string;
   if (os_env->GetVar("CRASHDUMP_SIGNAL_FD", &fd_string) &&
       os_env->GetVar("CRASHPAD_HANDLER_PID", &pid_string)) {

shell/common/mac/codesign_util.cc

@@ -0,0 +1,55 @@
+// Copyright 2023 Microsoft, Inc.
+// Copyright 2013 The Chromium Authors
+// Use of this source code is governed by the MIT license that can be
+// found in the LICENSE file.
+
+#include "shell/common/mac/codesign_util.h"
+
+#include "base/apple/osstatus_logging.h"
+#include "base/apple/scoped_cftyperef.h"
+
+#include <CoreFoundation/CoreFoundation.h>
+#include <Security/Security.h>
+
+namespace electron {
+
+bool ProcessBelongToCurrentApp(pid_t pid) {
+  // Get and check the code signature of current app.
+  base::apple::ScopedCFTypeRef<SecCodeRef> self_code;
+  OSStatus status =
+      SecCodeCopySelf(kSecCSDefaultFlags, self_code.InitializeInto());
+  if (status != errSecSuccess) {
+    OSSTATUS_LOG(ERROR, status) << "SecCodeCopyGuestWithAttributes";
+    return false;
+  }
+  // Get the code signature of process.
+  base::apple::ScopedCFTypeRef<CFNumberRef> process_cf(
+      CFNumberCreate(nullptr, kCFNumberIntType, &pid));
+  const void* attribute_keys[] = {kSecGuestAttributePid};
+  const void* attribute_values[] = {process_cf.get()};
+  base::apple::ScopedCFTypeRef<CFDictionaryRef> attributes(CFDictionaryCreate(
+      nullptr, attribute_keys, attribute_values, std::size(attribute_keys),
+      &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));
+  base::apple::ScopedCFTypeRef<SecCodeRef> process_code;
+  status = SecCodeCopyGuestWithAttributes(nullptr, attributes.get(),
+                                          kSecCSDefaultFlags,
+                                          process_code.InitializeInto());
+  if (status != errSecSuccess) {
+    OSSTATUS_LOG(ERROR, status) << "SecCodeCopyGuestWithAttributes";
+    return false;
+  }
+  // Get the requirement of current app's code signature.
+  base::apple::ScopedCFTypeRef<SecRequirementRef> self_requirement;
+  status = SecCodeCopyDesignatedRequirement(self_code.get(), kSecCSDefaultFlags,
+                                            self_requirement.InitializeInto());
+  if (status != errSecSuccess) {
+    OSSTATUS_LOG(ERROR, status) << "SecCodeCopyDesignatedRequirement";
+    return false;
+  }
+  // Check whether the process meets the signature requirement of current app.
+  status = SecCodeCheckValidity(process_code.get(), kSecCSDefaultFlags,
+                                self_requirement.get());
+  return status == errSecSuccess;
+}
+
+}  // namespace electron

shell/common/mac/codesign_util.h

@@ -0,0 +1,19 @@
+// Copyright 2023 Microsoft, Inc.
+// Copyright 2013 The Chromium Authors
+// Use of this source code is governed by the MIT license that can be
+// found in the LICENSE file.
+
+#ifndef SHELL_COMMON_MAC_CODESIGN_UTIL_H_
+#define SHELL_COMMON_MAC_CODESIGN_UTIL_H_
+
+#include <unistd.h>
+
+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);
+
+}  // namespace electron
+
+#endif  // SHELL_COMMON_MAC_CODESIGN_UTIL_H_

spec/api-autoupdater-darwin-spec.ts

@@ -3,96 +3,33 @@
 import * as http from 'node:http';
 import * as express from 'express';
 import * as fs from 'fs-extra';
-import * as os from 'node:os';
 import * as path from 'node:path';
 import * as psList from 'ps-list';
 import { AddressInfo } from 'node:net';
 import { ifdescribe, ifit } from './lib/spec-helpers';
+import { copyApp, getCodesignIdentity, shouldRunCodesignTests, signApp, spawn, withTempDirectory } from './lib/codesign-helpers';
 import * as uuid from 'uuid';
 import { autoUpdater, systemPreferences } from 'electron';
 
-const features = process._linkedBinding('electron_common_features');
-
-const fixturesPath = path.resolve(__dirname, 'fixtures');
-
 // We can only test the auto updater on darwin non-component builds
-ifdescribe(process.platform === 'darwin' && !(process.env.CI && process.arch === 'arm64') && !process.mas && !features.isComponentBuild())('autoUpdater behavior', function () {
+ifdescribe(shouldRunCodesignTests)('autoUpdater behavior', function () {
   this.timeout(120000);
 
   let identity = '';
 
   beforeEach(function () {
-    const result = cp.spawnSync(path.resolve(__dirname, '../script/codesign/get-trusted-identity.sh'));
-    if (result.status !== 0 || result.stdout.toString().trim().length === 0) {
-      // Per https://circleci.com/docs/2.0/env-vars:
-      // CIRCLE_PR_NUMBER is only present on forked PRs
-      if (process.env.CI && !process.env.CIRCLE_PR_NUMBER) {
-        throw new Error('No valid signing identity available to run autoUpdater specs');
-      }
-
+    const result = getCodesignIdentity();
+    if (result === null) {
       this.skip();
     } else {
-      identity = result.stdout.toString().trim();
+      identity = result;
     }
   });
 
   it('should have a valid code signing identity', () => {
     expect(identity).to.be.a('string').with.lengthOf.at.least(1);
   });
 
-  const copyApp = async (newDir: string, fixture = 'initial') => {
-    const appBundlePath = path.resolve(process.execPath, '../../..');
-    const newPath = path.resolve(newDir, 'Electron.app');
-    cp.spawnSync('cp', ['-R', appBundlePath, path.dirname(newPath)]);
-    const appDir = path.resolve(newPath, 'Contents/Resources/app');
-    await fs.mkdirp(appDir);
-    await fs.copy(path.resolve(fixturesPath, 'auto-update', fixture), appDir);
-    const plistPath = path.resolve(newPath, 'Contents', 'Info.plist');
-    await fs.writeFile(
-      plistPath,
-      (await fs.readFile(plistPath, 'utf8')).replace('<key>BuildMachineOSBuild</key>', `<key>NSAppTransportSecurity</key>
-      <dict>
-          <key>NSAllowsArbitraryLoads</key>
-          <true/>
-          <key>NSExceptionDomains</key>
-          <dict>
-              <key>localhost</key>
-              <dict>
-                  <key>NSExceptionAllowsInsecureHTTPLoads</key>
-                  <true/>
-                  <key>NSIncludesSubdomains</key>
-                  <true/>
-              </dict>
-          </dict>
-      </dict><key>BuildMachineOSBuild</key>`)
-    );
-    return newPath;
-  };
-
-  const spawn = (cmd: string, args: string[], opts: any = {}) => {
-    let out = '';
-    const child = cp.spawn(cmd, args, opts);
-    child.stdout.on('data', (chunk: Buffer) => {
-      out += chunk.toString();
-    });
-    child.stderr.on('data', (chunk: Buffer) => {
-      out += chunk.toString();
-    });
-    return new Promise<{ code: number, out: string }>((resolve) => {
-      child.on('exit', (code, signal) => {
-        expect(signal).to.equal(null);
-        resolve({
-          code: code!,
-          out
-        });
-      });
-    });
-  };
-
-  const signApp = (appPath: string) => {
-    return spawn('codesign', ['-s', identity, '--deep', '--force', appPath]);
-  };
-
   const launchApp = (appPath: string, args: string[] = []) => {
     return spawn(path.resolve(appPath, 'Contents/MacOS/Electron'), args);
   };
@@ -107,17 +44,6 @@
     return activeShipIts;
   };
 
-  const withTempDirectory = async (fn: (dir: string) => Promise<void>, autoCleanUp = true) => {
-    const dir = await fs.mkdtemp(path.resolve(os.tmpdir(), 'electron-update-spec-'));
-    try {
-      await fn(dir);
-    } finally {
-      if (autoCleanUp) {
-        cp.spawnSync('rm', ['-r', dir]);
-      }
-    }
-  };
-
   const logOnError = (what: any, fn: () => void) => {
     try {
       fn();
@@ -143,15 +69,15 @@
         const appPJPath = path.resolve(secondAppPath, 'Contents', 'Resources', 'app', 'package.json');
         await fs.writeFile(
           appPJPath,
           (await fs.readFile(appPJPath, 'utf8')).replace('1.0.0', version)
         );
         const infoPath = path.resolve(secondAppPath, 'Contents', 'Info.plist');
         await fs.writeFile(
           infoPath,
           (await fs.readFile(infoPath, 'utf8')).replace(/(<key>CFBundleShortVersionString<\/key>\s+<string>)[^<]+/g, `$1${version}`)
         );
         await mutateAppPreSign?.mutate(secondAppPath);
-        await signApp(secondAppPath);
+        await signApp(secondAppPath, identity);
         await mutateAppPostSign?.mutate(secondAppPath);
         updateZipPath = path.resolve(dir, 'update.zip');
         await spawn('zip', ['-0', '-r', '--symlinks', updateZipPath, './'], {
@@ -183,7 +109,7 @@
   it('should cleanly set the feed URL when the app is signed', async () => {
     await withTempDirectory(async (dir) => {
       const appPath = await copyApp(dir);
-      await signApp(appPath);
+      await signApp(appPath, identity);
       const launchResult = await launchApp(appPath, ['http://myupdate']);
       expect(launchResult.code).to.equal(0);
       expect(launchResult.out).to.include('Feed URL Set: http://myupdate');
@@ -224,7 +150,7 @@
     it('should hit the update endpoint when checkForUpdates is called', async () => {
       await withTempDirectory(async (dir) => {
         const appPath = await copyApp(dir, 'check');
-        await signApp(appPath);
+        await signApp(appPath, identity);
         server.get('/update-check', (req, res) => {
           res.status(204).send();
         });
@@ -241,7 +167,7 @@
     it('should hit the update endpoint with customer headers when checkForUpdates is called', async () => {
       await withTempDirectory(async (dir) => {
         const appPath = await copyApp(dir, 'check-with-headers');
-        await signApp(appPath);
+        await signApp(appPath, identity);
         server.get('/update-check', (req, res) => {
           res.status(204).send();
         });
@@ -258,7 +184,7 @@
     it('should hit the download endpoint when an update is available and error if the file is bad', async () => {
       await withTempDirectory(async (dir) => {
         const appPath = await copyApp(dir, 'update');
-        await signApp(appPath);
+        await signApp(appPath, identity);
         server.get('/update-file', (req, res) => {
           res.status(500).send('This is not a file');
         });
@@ -296,12 +222,12 @@
         const infoPath = path.resolve(appPath, 'Contents', 'Info.plist');
         await fs.writeFile(
           infoPath,
           (await fs.readFile(infoPath, 'utf8')).replace(/(<key>CFBundleShortVersionString<\/key>\s+<string>)[^<]+/g, '$11.0.0')
         );
-        await signApp(appPath);
+        await signApp(appPath, identity);
 
         const updateZipPath = await getOrCreateUpdateZipPath(opts.nextVersion, opts.endFixture, opts.mutateAppPreSign, opts.mutateAppPostSign);
 
         await fn(appPath, updateZipPath);
       });
     };

spec/fixtures/api/fork-with-node-options.js

@@ -0,0 +1,22 @@
+const { execFileSync } = require('node:child_process');
+const path = require('node:path');
+
+const fixtures = path.resolve(__dirname, '..');
+
+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')}"`
+};
+// Provide a lower cased NODE_OPTIONS in case some code ignores case sensitivity
+// when reading NODE_OPTIONS.
+env.node_options = env.NODE_OPTIONS;
+try {
+  execFileSync(process.argv[2],
+    ['--require', path.join(fixtures, 'module', 'noop.js')],
+    { env, stdio: 'inherit' });
+  process.exit(0);
+} catch (error) {
+  console.log('NODE_OPTIONS passed to child');
+  process.exit(1);
+}