fix: prevent node mode to be used as script runner by other apps (#40737)

filenames.gni

@@ -167,6 +167,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,6 +13,7 @@
 #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"
@@ -20,6 +21,7 @@
 #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"
@@ -101,12 +106,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,36 +3,26 @@ import * as cp from 'node:child_process';
 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 { 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;
     }
   });
 
@@ -40,59 +30,6 @@ ifdescribe(process.platform === 'darwin' && !(process.env.CI && process.arch ===
     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 @@ ifdescribe(process.platform === 'darwin' && !(process.env.CI && process.arch ===
     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,7 +69,7 @@ ifdescribe(process.platform === 'darwin' && !(process.env.CI && process.arch ===
           appPJPath,
           (await fs.readFile(appPJPath, 'utf8')).replace('1.0.0', version)
         );
-        await signApp(secondAppPath);
+        await signApp(secondAppPath, identity);
         await mutateAppPostSign?.mutate(secondAppPath);
         updateZipPath = path.resolve(dir, 'update.zip');
         await spawn('zip', ['-0', '-r', '--symlinks', updateZipPath, './'], {
@@ -175,7 +101,7 @@ ifdescribe(process.platform === 'darwin' && !(process.env.CI && process.arch ===
   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');
@@ -216,7 +142,7 @@ ifdescribe(process.platform === 'darwin' && !(process.env.CI && process.arch ===
     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();
         });
@@ -233,7 +159,7 @@ ifdescribe(process.platform === 'darwin' && !(process.env.CI && process.arch ===
     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();
         });
@@ -250,7 +176,7 @@ ifdescribe(process.platform === 'darwin' && !(process.env.CI && process.arch ===
     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');
         });
@@ -286,7 +212,7 @@ ifdescribe(process.platform === 'darwin' && !(process.env.CI && process.arch ===
     }, fn: (appPath: string, zipPath: string) => Promise<void>) => {
       await withTempDirectory(async (dir) => {
         const appPath = await copyApp(dir, opts.startFixture);
-        await signApp(appPath);
+        await signApp(appPath, identity);
 
         const updateZipPath = await getOrCreateUpdateZipPath(opts.nextVersion, opts.endFixture, opts.mutateAppPostSign);
 

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);
+}

spec/lib/codesign-helpers.ts

@@ -0,0 +1,99 @@
+import * as cp from 'node:child_process';
+import * as fs from 'fs-extra';
+import * as os from 'node:os';
+import * as path from 'node:path';
+import { expect } from 'chai';
+
+const features = process._linkedBinding('electron_common_features');
+const fixturesPath = path.resolve(__dirname, '..', 'fixtures');
+
+export const shouldRunCodesignTests =
+    process.platform === 'darwin' &&
+    !(process.env.CI && process.arch === 'arm64') &&
+    !process.mas &&
+    !features.isComponentBuild();
+
+let identity: string | null;
+
+export function getCodesignIdentity () {
+  if (identity === undefined) {
+    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');
+      }
+      identity = null;
+    } else {
+      identity = result.stdout.toString().trim();
+    }
+  }
+  return identity;
+}
+
+export async function copyApp (newDir: string, fixture: string | null = 'initial') {
+  const appBundlePath = path.resolve(process.execPath, '../../..');
+  const newPath = path.resolve(newDir, 'Electron.app');
+  cp.spawnSync('cp', ['-R', appBundlePath, path.dirname(newPath)]);
+  if (fixture) {
+    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;
+};
+
+export function 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
+      });
+    });
+  });
+};
+
+export function signApp (appPath: string, identity: string) {
+  return spawn('codesign', ['-s', identity, '--deep', '--force', appPath]);
+};
+
+export async function withTempDirectory (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]);
+    }
+  }
+};

spec/node-spec.ts

@@ -1,9 +1,10 @@
 import { expect } from 'chai';
 import * as childProcess from 'node:child_process';
-import * as fs from 'node:fs';
+import * as fs from 'fs-extra';
 import * as path from 'node:path';
 import * as util from 'node:util';
 import { getRemoteContext, ifdescribe, ifit, itremote, useRemoteContext } from './lib/spec-helpers';
+import { copyApp, getCodesignIdentity, shouldRunCodesignTests, signApp, spawn, withTempDirectory } from './lib/codesign-helpers';
 import { webContents } from 'electron/main';
 import { EventEmitter } from 'node:stream';
 import { once } from 'node:events';
@@ -659,6 +660,66 @@ describe('node feature', () => {
     });
   });
 
+  ifdescribe(shouldRunCodesignTests)('NODE_OPTIONS in signed app', function () {
+    let identity = '';
+
+    beforeEach(function () {
+      const result = getCodesignIdentity();
+      if (result === null) {
+        this.skip();
+      } else {
+        identity = result;
+      }
+    });
+
+    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';
+
+    it('is disabled when invoked by other apps in ELECTRON_RUN_AS_NODE mode', async () => {
+      await withTempDirectory(async (dir) => {
+        const appPath = await copyApp(dir);
+        await signApp(appPath, identity);
+        // Invoke Electron by using the system node binary as middle layer, so
+        // the check of NODE_OPTIONS will think the process is started by other
+        // apps.
+        const { code, out } = await spawn('node', [script, path.join(appPath, 'Contents/MacOS/Electron')]);
+        expect(code).to.equal(0);
+        expect(out).to.include(nodeOptionsWarning);
+      });
+    });
+
+    it('is disabled when invoked by alien binary in app bundle in ELECTRON_RUN_AS_NODE mode', async function () {
+      await withTempDirectory(async (dir) => {
+        const appPath = await copyApp(dir);
+        await signApp(appPath, identity);
+        // Find system node and copy it to app bundle.
+        const nodePath = process.env.PATH?.split(path.delimiter).find(dir => fs.existsSync(path.join(dir, 'node')));
+        if (!nodePath) {
+          this.skip();
+          return;
+        }
+        const alienBinary = path.join(appPath, 'Contents/MacOS/node');
+        await fs.copy(path.join(nodePath, 'node'), alienBinary);
+        // Try to execute electron app from the alien node in app bundle.
+        const { code, out } = await spawn(alienBinary, [script, path.join(appPath, 'Contents/MacOS/Electron')]);
+        expect(code).to.equal(0);
+        expect(out).to.include(nodeOptionsWarning);
+      });
+    });
+
+    it('is respected when invoked from self', async () => {
+      await withTempDirectory(async (dir) => {
+        const appPath = await copyApp(dir, null);
+        await signApp(appPath, identity);
+        const appExePath = path.join(appPath, 'Contents/MacOS/Electron');
+        const { code, out } = await spawn(appExePath, [script, appExePath]);
+        expect(code).to.equal(1);
+        expect(out).to.not.include(nodeOptionsWarning);
+        expect(out).to.include('NODE_OPTIONS passed to child');
+      });
+    });
+  });
+
   describe('Node.js cli flags', () => {
     let child: childProcess.ChildProcessWithoutNullStreams;
     let exitPromise: Promise<any[]>;