diff --git a/filenames.gni b/filenames.gni index 38247496637..d142f312bb3 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 adca7ff4838..469b7e31c80 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 5171f90aac1..2abfb2eb32d 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 #include namespace electron { -bool ProcessBelongToCurrentApp(pid_t pid) { +absl::optional IsUnsignedOrAdHocSigned(SecCodeRef code) { + base::apple::ScopedCFTypeRef 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(); + } + // Copy the signing info from the SecStaticCodeRef. + base::apple::ScopedCFTypeRef signing_info; + status = + SecCodeCopySigningInformation(static_code.get(), kSecCSSigningInformation, + signing_info.InitializeInto()); + if (status != errSecSuccess) { + OSSTATUS_LOG(ERROR, status) << "SecCodeCopySigningInformation"; + return absl::optional(); + } + // 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(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(); + } + if (static_cast(flags) & kSecCodeSignatureAdhoc) { + return true; + } + return false; +} + +bool ProcessSignatureIsSameWithCurrentApp(pid_t pid) { // Get and check the code signature of current app. base::apple::ScopedCFTypeRef self_code; OSStatus status = @@ -22,6 +67,15 @@ bool ProcessBelongToCurrentApp(pid_t pid) { OSSTATUS_LOG(ERROR, status) << "SecCodeCopyGuestWithAttributes"; return false; } + absl::optional 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 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 8a2fb94ee8e..1fabc4fe42b 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 38f9d28b2b1..d4aa75a0db4 100644 --- a/shell/common/node_util.h +++ b/shell/common/node_util.h @@ -7,6 +7,7 @@ #include +#include "build/build_config.h" #include "v8/include/v8.h" namespace node { @@ -26,6 +27,12 @@ v8::MaybeLocal CompileAndCall( std::vector>* parameters, std::vector>* 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 00000000000..85c07250986 --- /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 + +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 49dad89944b..97439598f4c 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 2bb967055e9..e1d9c76d9e6 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) => {