From 74a1350d5950096ce09917f110b65e61ebd41378 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 4 Jan 2024 16:37:18 +0900 Subject: [PATCH] fix: ignore all NODE_ envs from foreign parent in node process --- filenames.gni | 1 + shell/app/node_main.cc | 21 +++---- shell/common/mac/codesign_util.cc | 62 ++++++++++++++++++++- 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(+), 17 deletions(-) create mode 100644 shell/common/node_util_mac.mm diff --git a/filenames.gni b/filenames.gni index 1611c42c770f3..1fa00ee9f6972 100644 --- a/filenames.gni +++ b/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", diff --git a/shell/app/node_main.cc b/shell/app/node_main.cc index d4456642757d5..8c12e7887d98b 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 e723b91908b5b..f64b177ec5e7d 100644 --- a/shell/common/mac/codesign_util.cc +++ b/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 #include namespace electron { -bool ProcessBelongToCurrentApp(pid_t pid) { +absl::optional IsUnsignedOrAdHocSigned(SecCodeRef code) { + base::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::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::mac::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::ScopedCFTypeRef self_code; OSStatus status = @@ -22,6 +66,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::ScopedCFTypeRef 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; } diff --git a/shell/common/mac/codesign_util.h b/shell/common/mac/codesign_util.h index 8a2fb94ee8e6f..1fabc4fe42bde 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 e8cc05370c9af..1acb04e2d888f 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 { @@ -27,6 +28,12 @@ v8::MaybeLocal CompileAndCall( std::vector>* 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_ diff --git a/shell/common/node_util_mac.mm b/shell/common/node_util_mac.mm new file mode 100644 index 0000000000000..85c0725098673 --- /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 49dad89944b69..97439598f4c7a 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 b42dbf9a627ac..2c7d43600e54a 100644 --- a/spec/node-spec.ts +++ b/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) => {